Ensure that TestResourceManager is only closed once #10146
Conversation
test-framework/junit5/src/main/java/io/quarkus/test/junit/QuarkusTestExtension.java
Show resolved
Hide resolved
test-framework/junit5/src/main/java/io/quarkus/test/junit/QuarkusTestExtension.java
Show resolved
Hide resolved
|
@pedroigor @sberyozkin @stuartwdouglas This PR although seems reasonable to me, seems to be failing on the Keycloack tests... What happens is that it seems that for some reason the Keycloak related FWIW, all other |
|
The easiest way to see the problem is to run |
|
@geoand Looks like a CL issue. When invoking the When using the launcher class loader, you get a That is also why that method runs fine when you get a failure (the stop method is invoked on the runtime class loader) so that a test only works after either the Keycloak server is not yet with any data or only after a failure. |
|
@geoand Not sure if this is the best fix, but here is https://github.com/pedroigor/quarkus/tree/testresource-close-classloader. With that change I'm able to run |
|
Excellent information @pedroigor! I'll take another look tomorrow |
Without this PR the TestResourceManager gets closed 3 times, which is causing some issues with new surefire versions Relates to: quarkusio#10081
|
PR should be good now, but I'll leave it in Draft to make sure that the tests pass on my fork |
| } | ||
|
|
||
| class ExtensionState implements ExtensionContext.Store.CloseableResource { | ||
| class ExtensionState { |
There was a problem hiding this comment.
By removing the interface, we ensure that JUnit does not attempt to close this, because we are closing it ourselved
There was a problem hiding this comment.
How is this being shutdown then? By the shutdown hook?
There was a problem hiding this comment.
Yes. When we let Junit 5 shut it down, there are a few problems:
- It gets shutdown multiple times (which can make
QuarkusTestResourceLifecycleManagerfail - I saw this when looking at the Surefire M5 bump PR) - JUnit 5 shuts it down from the wrong ClassLoader. This is what @pedroigor found yesterday when debugging the mysterious failures I was seeing for the Keycloak tests
|
I don't think this is correct, if the surefire fork count is zero then the tests will be run in the main Maven JVM, and Quarkus will not be shut down until Maven exits, which would be problematic for multi module projects. Not sure if there are similar problematic scenarios with Gradle. The shutdown hook was supposed to be a 'last resort' form of termination, that only is called in abnormal circumstances. |
|
Right. I also thought that it would probably be problematic for the test profile. |
4 participants
Without this PR the TestResourceManager gets closed 3 times,
which is causing some issues with new surefire versions
Relates to: #10081