Fix #668 External DTD failure for uber-jar

[shumonsharif]

No description provided.

[ppalaga]

@shumonsharif would you mind adding a test that fails before this change and succeeds after it? Otherwise it is hard to make sure that we won't break it in the future.

[shumonsharif]

@shumonsharif would you mind adding a test that fails before this change and succeeds after it? Otherwise it is hard to make sure that we won't break it in the future.

@ppalaga The only way I can think of to test the @BuildStep would be to execute a uber jar build and look through the console output. Is this an agreeable approach? I don't believe we have any tests for a @BuildStep currently, right? Suggestions welcome in case you can think of a better approach!

[shumonsharif]

@ppalaga Added a unit test, and addressed the code review comments, but the build appears to be failing in a seemingly unrelated unit test: io.quarkiverse.cxf.deployment.test.ServerDevModeTest

https://github.com/quarkiverse/quarkus-cxf/actions/runs/3838074101/jobs/6534191935#step:5:622

Any ideas on what's causing this?

[ppalaga]

the build appears to be failing in a seemingly unrelated unit test: io.quarkiverse.cxf.deployment.test.ServerDevModeTest

I could not reproduce it locally, so I just rerun the failing tests.

[ppalaga]

I suspect this is not doing what we'd hope for. These @RegisterExtension tests IIRC always work with an in-memory class-loader and I think .overrideConfigKey("quarkus.package.type", "uber-jar") has no effect. This can be proven by a simple experiment: when all changes in this PR except the test are reverted, the test should fail, but in reality it does not (I checked). I think we need a proper integration test.

For reading the log from an integration test, you need two things:

1. quarkus.log.file.enable = true in application properties
2. A piece of Awaitility code repeatedly reading the whole log file: https://github.com/apache/camel-quarkus/blob/main/integration-test-groups/foundation/direct/src/test/java/org/apache/camel/quarkus/component/direct/it/DirectTest.java#L63-L66

[shumonsharif]

@ppalaga Reverting the PR (except for the unit test) failed the test for me when I had tried it locally? Not sure why you're seeing something different - but I'll look into it later this week and keep you posted.

[shumonsharif]

Aaah, I think I found the culprit just by looking at the code. I had fixed some typos in the code, so please include the changes below and the test should fail fine with everything else reverted. I think the test is fine and working as it should be. @ppalaga please keep me posted.

https://github.com/quarkiverse/quarkus-cxf/pull/669/files#diff-75da9f7e1c3e1384976a49d0ad67baf36638a284b5c6d1beafa5846d6ff55fdfL198-R198

https://github.com/quarkiverse/quarkus-cxf/pull/669/files#diff-75da9f7e1c3e1384976a49d0ad67baf36638a284b5c6d1beafa5846d6ff55fdfL255-R255

[ppalaga]

Yes, thanks for explaining, indeed it is failing as expected with those log messages modified.

[ppalaga]

Could you please squash your commits and rebase on top of the current main?

[shumonsharif]

@ppalaga cc: @famod I just squashed my commits and rebased on top of current main. However, the build is still oddly failing in that same ServerDevModeTest unit test. I'm unable to recreate the build failure locally, it completes successfully on my laptop. Any thoughts or suggestions on how to troubleshoot this would be welcome.

[famod]

Without touching anything, it doesn't fail on my box either (WSL2 Ubuntu 22.04).

But I can make it fail via -Dsurefire.runOrder=reversealphabetical so it's a test ordering / test interferrence issue.
I'm having a closer look...

[famod]

Passes:

 mvn clean install -f extensions/core/deployment/ -Dsurefire.runOrder=alphabetical -Dtest=UberJarMergedResourcesTest,ServerDevModeTest

Fails:

 mvn clean install -f extensions/core/deployment/ -Dsurefire.runOrder=reversealphabetical -Dtest=UberJarMergedResourcesTest,ServerDevModeTest

[famod]

RestAssured in ServerDevModeTest somehow picks up the port of the previous prod mode test (UberJarMergedResourcesTest) which is 8081 but it should use 8080 instead.
Digging further...

[famod]

I think this is a bug in Quarkus: It seems that the @QuarkusMain class/method terminates so quickly after start() in QuarkusProdModeTest.beforeAll() that .beforeEach() is not seeing the process anymore and is starting it again.
This is problematic because in start() there is also a RestAssured setup call and so you have two setup calls and the original port before that test is lost.

I'll try to work around that and will at least report it upstream (or maybe even send a PR).

Edit: It's more complicated as QuarkusProdModeTest waits for the app to stop if expectedExit is enabled.

[famod]

Green! 🎉

@shumonsharif @ppalaga I'll go ahead and merge this. Please do raise concerns via separate issues or comments in case you disagree with my workaround.
If I find some time I'll release 1.7.3 on sunday evening.

[shumonsharif]

Green! 🎉

@shumonsharif @ppalaga I'll go ahead and merge this. Please do raise concerns via separate issues or comments in case you disagree with my workaround. If I find some time I'll release 1.7.3 on sunday evening.

Nice work Falko, and thank you for helping out with this. The updates look good to me!

[ppalaga]

Great troubleshooting, thanks a lot @famod !

[famod]

Thanks guys! We should be able to get rid of that workaround once we have updated to 2.16.0.Final (see linked PR).