-
Notifications
You must be signed in to change notification settings - Fork 3k
QuarkusComponentTest: class loading refactoring #50164
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
d3a8aa5
to
e84c5c4
Compare
e84c5c4
to
44f090b
Compare
...unit5-component/src/main/java/io/quarkus/test/component/QuarkusComponentTestClassLoader.java
Show resolved
Hide resolved
Not an expert, but could we maybe move the whole infrastructure to Also, I'm not sure I'm able to do a proper review here, certainly not without playing with this locally for a while :-) |
I'm not an expert either but I don't feel confident enough to do such a bold move. Also
Right now, I think that we don't need any cleanup... which doesn't mean we won't need it in the future but I'd like to keep the SPI minimal. If possible, it should be considered internal. But AFAIK the only thing we could do is to add a sentence in the javadoc. Speaking of the name - do you have a better idea? Because I'm out of ideas...
No problem. Your feeedback is really appreciated! 🙏 |
Makes sense.
👍
Nothing extraordinary. I'd just call it |
44f090b
to
2ba3a7d
Compare
I am going through the code now but in the meantime - I was thinking whether it would be worth adding a note to the documentation stating that component tests now support transformations? |
This comment has been minimized.
This comment has been minimized.
Once we merge this pull request I'd like to write a blog post that would mention all recent improvements (incl. |
That's great idea! 👍
I know, but fair enough. |
BTW I think that this would open the door to other possibilities. For example, we could try to add support Panache mocking for the active record pattern. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few comments and questions.
Take them with a pinch of salt as while I understand what the PR does, I certainly do have hard time understanding how exactly with all the class loader juggling :)
test-framework/junit5/src/main/java/io/quarkus/test/junit/classloading/FacadeClassLoader.java
Show resolved
Hide resolved
test-framework/common/src/main/java/io/quarkus/test/common/FacadeClassLoaderProvider.java
Show resolved
Hide resolved
test-framework/common/src/main/java/io/quarkus/test/common/FacadeClassLoaderProvider.java
Show resolved
Hide resolved
test-framework/junit5-component/src/main/java/io/quarkus/test/component/ComponentContainer.java
Show resolved
Hide resolved
...nit5-component/src/main/java/io/quarkus/test/component/ComponentLauncherSessionListener.java
Show resolved
Hide resolved
...nit5-component/src/main/java/io/quarkus/test/component/ComponentLauncherSessionListener.java
Outdated
Show resolved
Hide resolved
Totally understand, it took me a few days to understand at least part of the class loading code... |
2ba3a7d
to
e88c55f
Compare
Status for workflow
|
@Ladicek @manovotn @holly-cummins I'd like to merge this pull request today. It's not perfect but we could address potential issues in the follow ups. I already have a POC for extension points called |
core/deployment/src/main/java/io/quarkus/deployment/dev/testing/JunitTestRunner.java
Show resolved
Hide resolved
private static boolean mustDelegateToParent(String name) { | ||
return name.startsWith("java.") | ||
|| name.startsWith("jdk.") | ||
|| name.startsWith("javax.") | ||
|| name.startsWith("jakarta.") | ||
|| name.startsWith("sun.") | ||
|| name.startsWith("com.sun.") | ||
|| name.startsWith("org.w3c.") | ||
|| name.startsWith("org.xml.") | ||
|| name.startsWith("org.junit.") | ||
|| name.startsWith("org.mockito.") | ||
|| PARENT_CL_CLASSES.contains(name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way to get this code more common with the QuarkusClassLoader? I seem to recall doing a few hacky overrides to what gets loaded parent-first over there to support dev services work. It might be (a) less code and (b) fewer confusing bugs if we had a common service.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure TBH. But I admit that this does not look very smart (it's more or less a result of experimentation) and we should get back to that sometime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The list here looks quite close to what I had in #35473 and that wasn't a result of experimentation -- that was a list of packages provided by the JDK. There's a few packages added here (jakarta.*
, org.junit.*
, org.mockito.*
) which make perfect sense, but still -- it's a list of packages we know are never provided by the application and make no sense to be loaded by the application class loader.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a list of packages we know are never provided by the application and make no sense to be loaded by the application class loader.
On the other hand, I observed weird errors while adding other packages which are also never provided by the app 🤷.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, that sounds strange then.
The goal of this pull request is to refactor the class loading used for
QuarkusComponentTest
, i .e. use a transformation-aware class loader to load a test class annotated with@QuarkusComponentTest
. This way we can support things like simplified constructor injection.Implementation-wise - we hook into the
FacaClassLoader
logic (if present) and similarly to how@QuarkusTest
is handled, we build the container (ArC in case ofQuarkusComponentTest
) when the test classes are loaded (note that there must be a special handling for continuous testing whereio.quarkus.deployment.dev.testing.JunitTestRunner
is used to load the test classes).When I discussed this PR with Ladislav and Matej, we had an idea that
quarkus-junit5
andquarkus-junit5-component
could be merged in one artifact. However, it turns out we would have to add a bunch of dependencies toquarkus-junit5
(including ArC, but what's worse also Mockito + bytebuddy - this combo has ~ 4MB of jars). So I decided to introduce theio.quarkus.test.common.FacadeClassLoaders
SPI instead. It lives in thequarkus-test-common
and the name is terrible, but it works soo 🤷.There's one use case where the new approach fails. It's a
QuarkusDevModeTest
iff quarkus-junit5 is not present, something along the lines of https://github.com/quarkusio/quarkus/blob/main/integration-tests/devmode/src/test/java/io/quarkus/test/component/ComponentContinuousTestingTest.java. I found thatquarkus-junit5
providesQuarkusTestConfigProviderResolver
that somehow overridesio.quarkus.test.config.TestConfigProviderResolver
to avoid Context ClassLoader mismatch. But I have no idea why/how and whyTestConfigProviderResolver
exists for the first place.This change should not break any existing apps but the test coverage is limited..