JUnit Extension is loaded with wrong classloader inside @QuarkusTest

[tkalmar]

Describe the bug
(Describe the problem clearly and concisely.)
When using an custom JUnit extension the classloader for the extension is different then the classloader for the actual test.
This is only inside @QuarkusTest annotated tests.

Expected behavior
(Describe the expected behavior clearly and concisely.)
Same classloader as test.

Actual behavior
(Describe the actual behavior clearly and concisely.)
Different classloader (AppClassLoader vs. Base Runtime Classloader)

To Reproduce
Steps to reproduce the behavior:

https://github.com/tkalmar/bug-with-quarkus

org.acme.ExampleResourceTest fails because of the class loading issue
org.acme.StaticProviderTest shows the expected and in my opinion correct behaviour.

Configuration

# Add your application.properties here, if applicable.

Screenshots
(If applicable, add screenshots to help explain your problem.)

Environment (please complete the following information):

• Output of uname -a or ver: MINGW64_NT-10.0-18363 XXX 3.1.4-340.x86_64 2020-05-19 12:55 UTC x86_64 Msys
• Output of java -version: openjdk 11.0.7 2020-04-14 LTS
• GraalVM version (if different from Java):
• Quarkus version or git rev: 1.6.0.Final
• Build tool (ie. output of mvnw --version or gradlew --version): Apache Maven 3.6.3 (cecedd343002696d0abb50b32b541b8a6ba2883f)

Additional context
(Add any other context about the problem here.)

[tkalmar]

Perhaps related or identical to #10435

[famod]

Some more context:
Initially, I was trying to trouble-shoot this together with @tkalmar and we even tried QuarkusTestBeforeEachCallback, but with that approach we couldn't even get the annotation (here: @Unfriendly) from the test method because it was loaded in two classloaders. We could see the annotation in the debugger, but context.getTestMethod().getAnnotation(Unfriendly.class) returned null.
Very confusing...

/cc @stuartwdouglas & @geoand

@tkalmar Shouldn't this be @Order(2) instead? https://github.com/tkalmar/bug-with-quarkus/blob/master/src/test/java/org/acme/StaticProviderTest.java#L21

[tkalmar]

@tkalmar Shouldn't this be @Order(2) instead? https://github.com/tkalmar/bug-with-quarkus/blob/master/src/test/java/org/acme/StaticProviderTest.java#L21

You are right, but this does not change the result. ;) Thanks for reviewing was a little late.

[tkalmar]

For anyone hitting this problem our current workaround is an JunitExtension/Interceptor hybrid. For @QuarkusTest the interceptor kicks in and saves the day.

[geoand]

The fact that the Quarkus JUnit extension uses it's own Classloader is fundamental and not something that can be changed.

So let's focus on what you are trying to achieve so we can see how things can be improved (perhaps with more/better hooks in the extension itself).

[famod]

So let's focus on what you are trying to achieve

We have a ClockProvider that (statically) provides the java.time.Clock that shall be used by the business code.
In tests we want to shift time so there is an alternative TestClockProvider with a setter and TestClockProvider has to be reset after each method (automatically).
We also want to use annotation driven time shifts, so that we can clearly see whether a test method uses a specific time.
So we need to (in a extension/callback):

• properly load the annotation to get the desired time (which fails via Quarkus callback because the annotation cannot be retrieved from the test method)
• set the time so that the code under test "sees" it (fails via JUnit5 callback because the TestClockProvider in that CL is not the one seen by the code under test)
• reset the the time (also fails via JUnit5 callback, see above)

Our ClockProvider is just an example. There are certainly more cases for which you may want to implement annotation driven test extensions that control something in a way that becomes visible to the code under test.

[geoand]

Our ClockProvider is just an example. There are certainly more cases for which you may want to implement annotation driven test extensions that control something in a way that becomes visible to the code under test.

+1

Thanks for the explanation. So currently what is missing is the ability to easily get the annotations of the test methods and / or classes?

[tkalmar]

The main problem here are the different class loaders. You can`t expect every single junit extension out there to be aware of quarkus. The extension in the reproducer is working fine with junit but not inside a quarkus test. Should we rewrite all extensions, which rely on class path to be aware of quarkus tests? What about reusing such extensions in quarkus and non quarkus projects? What about third party extensions? This is clearly not a 'no go' but a 'goes very hard' ... So the question at all is why do we need a work around in the extension at all? Especially if it is a non quarkus related/aware extension?

[famod]

@geoand basically what @tkalmar just said plus:

Yes, properly getting a custom annotation from a test method or class via a QuarkusTestCallback should "just work".

The ideal DRY solution though, would be to just use the very same Junit5 extension that is needed anyway for simpler, non-Quarkus tests.

I think we should at least document this limitation in case such a change is against the current architecture.
Secondly, I might be wrong but the QuarkusTestCallbacks are not documented at all, are they?

[geoand]

I understand where you are coming from, but unfortunately using the same classloader is not an option. We did that up until Quarkus 1.2 and it had some pretty bad consequences.

+10 for the documentation parts. Indeed I don't think we explicitly mention either the CL limitation or the Quarkus specific callbacks

[famod]

I'll see what I can do for the documentation parts.

Regarding that annotation lookup problem in the QuarkusTestCallback: Couldn't we maybe add a method to the context that is working around the problem?

@tkalmar WDYT, given the need to register a QuarkusTestCallback for all tests (via ServiceLoader mechanism), would we still keep our interceptor workaround even in case annotations could be properly read in a QuarkusTestCallback?

[geoand]

I'll see what I can do for the documentation parts.

Thanks!

Regarding that annotation lookup problem in the QuarkusTestCallback: Couldn't we maybe add a method to the context that is working around the problem?

We don't have that already? I'd need to check. But if we don't, then yeah, it makes perfect sense.

[tkalmar]

@tkalmar WDYT, given the need to register a QuarkusTestCallback for all tests (via ServiceLoader mechanism), would we still keep our interceptor workaround even in case annotations could be properly read in a QuarkusTestCallback?

@famod @geoand The reading of the annotation is only one aspect. We somehow would need access to the classloader of the Test, so we could change the static member of the StaticProvider. I understand why the QuarkusExtension need an independent classloader but why is such an clasloader forced on all other extensions?

[geoand]

Depends on what you mean by forced.
If another extension needs to probe/interact with the actual test class (the one loaded by the Quarkus ClassLoader, not the original one loaded by JUnit), then that extension needs to implicitly used the Quarkus ClassLoader. That isn't a Quarkus specific limitation, it's just how classloading works in general.

Because remember, that all types in the method signatures and all field types will be loaded by the ClassLoader that loaded the class, in this case the Quarkus ClassLoader.
Should you need to do anything with those types, you need to use the proper ClassLoader.

[tkalmar]

Depends on what you mean by forced.
If another extension needs to probe/interact with the actual test class (the one loaded by the Quarkus ClassLoader, not the original one loaded by JUnit), then that extension needs to implicitly used the Quarkus ClassLoader. That isn't a Quarkus specific limitation, it's just how classloading works in general.

Why should an "normal" non quarkus aware extension need to know that there is another classloader? If i have a quarkus specific extension i'm totaly on your side but in this case it does not meet my expectations about JUnit extensions. I expect an extension to have the same classloader as the actual JUnit test execution.

[geoand]

I understand your expectation, believe me I do. But as I said before, that's how Quarkus 1.2- worked, and unfortunately it caused a lot of headaches in more cases than the current model. So we are unlikely to go back to that model.
The ideal solution for both Quarkus and users would be for JUnit to allow the use of a custom ClassLoader for tests - that would make things totally transparent (see issue no. 201 in the JUnit 5 github repo).

[tkalmar]

The point i don't understand is why our extension is loaded inside the quarkus classloader? Shouldn't stay quarkus out of the way in this case and leave the extension to JUnit execution?

[geoand]

I haven't seen (or much less ran) your code, but I think the problem is exactly the opposite of what you describe. Your extensions is being loaded by the system classloader (since that is what JUnit 5 uses to load extensions), but because Quarkus is loading the actual test class from its own classloader, you are getting unexpected results.

[tkalmar]

You are right. Extension is loaded inside system CL and the test runs under quarkus CL :/

[tkalmar]

So can we somehow detect this case or is our interceptor/JUnit extension hybrid the way to go until JUnit allows for custom class loaders?

[famod]

@tkalmar

The reading of the annotation is only one aspect. We somehow would need access to the classloader of the Test, so we could change the static member of the StaticProvider.

Hm, I was under the impression that if we got somehow past that annotation problem we would actually be running in the proper CL (in the QuarkusTestCallback).
Must have remembered that wrong, darn late hour debugging sessions. 😉

[geoand]

So can we somehow detect this case or is our interceptor/JUnit extension hybrid the way to go until JUnit allows for custom class loaders?

I would need to look at / run your code to be able to answer this.

[tkalmar]

I would need to look at / run your code to be able to answer this.
Ok thanks for the explanation. The reproducer is near to the goal, i can extend it to include our workaround if that helps.

[tkalmar]

Ok, i extended the reproducer to showcase our workaround. This turns all tests green. This will work for our own extensions for the project but not for extensions we share between projects or which are third-party. Perhaps it is something which can be done by the quarkus magic fairy automatically, disable the JUnit extension and turn it in an Interceptor ... So the extension doesn't need to be aware of quarkus and cdi at all and runs inside the right classloader ...

[knutwannheden]

FWIW: In a related problem I had when struggling with Quarkus' classloading in the context of JUnit 5, I ended up defining my JUnit extension inside a Quarkus extension, so that I could use <parentFirstArtifact> to get Quarkus to load specific classes using the parent classloader. This may not be applicable to your use case, however.

[geoand]

I just looked at the reproducer and indeed in this case the Quarkus test callbacks would be the way to go (although obviously not perfect for the reasons we have already mentioned).

I didn't see any code however showcasing the annotation retrieval problem. Do you have that anywhere?

[geoand]

Also, as of Quarkus 1.6, QuarkusTestAfterEachCallback and QuarkusTestAfterEachCallback take QuarkusTestMethodContext as an argument, which gives access to the test method

[tkalmar]

Also, as of Quarkus 1.6, QuarkusTestAfterEachCallback and QuarkusTestAfterEachCallback take QuarkusTestMethodContext as an argument, which gives access to the test method

Exactly what i tried at first, but the annotation is always null (i can see it in the debugger but it is not loadable/retrievable). I enhanced the reproducer to showcase this.

[geoand]

OK thanks, I'll have a look soon

[geoand]

#10675 fixes the annotation lookup issue - essentially was what happening was that QuarkusTestMethodContext contained the method from the original class - not the one loaded from the Quarkus ClassLoader.

[famod]

Thanks @geoand!

Do you see a chance to register QuarkusTestCallbacks also via annotations (similar to what is possible for Junit5 extensions which is also used in the reproducer)?
This would certainly be a separate enhancement issue though.

[geoand]

Yeah, we could do that as well. Feel free to open an issue and ping me and Stuart. I'm +1 for it, but Stuart might have some good reason for not adding it.

[loicmathieu]

The main problem here are the different class loaders. You can`t expect every single junit extension out there to be aware of quarkus.

To add more concern about this choice, I too have a JUnit extension that didn't work anymore since Quarkus 1.3. I didn't create an issue because I couldn't create a simple reproducer (and didn't have time to work on a more complex reproducer).

This is a complex extension so I will try to give a high level explaination of what it is doing: we have a custom JDBC driver that register query execution information inside a ThreadLocal, then at the end of the method execution (the extension uses a JUnit 5 InvocationInterceptor) it looks at this ThreadLocal to analyse query executions.
Starting with Quarkus 1.3 no query execution information can be retrieved from the ThreadLocal that appears to be empty.

As Quarkus JUnit extension uses it's own class loader, this explain why it didn't works.

I too agree that we cannot ask for all JUnit extensions to be aware of Quarkus ...

[geoand]

Like I have said above, I completely agree that in an ideal world all extensions would just work.
However with the current state of JUnit, that just isn't possible... Also undoing the classloader change to QuarkusTest is not an option either as it would break testing of a host of features.

[stuartwdouglas]

Maybe master...stuartwdouglas:test-parent-first is a solution we could consider. Basically if an artifact is "test" scoped then we will always load it parent first, so there will only every be one copy of the extension.

[geoand]

That's indeed an interesting approach!

[loicmathieu]

This will only works if the Quarkus extension is loaded before the other one right ?

[geoand]

Not sure TBH... We'd need to check it out