Draft: [obsolete code, needs refresh] Load test classes with runtime classloader #34681

holly-cummins · 2023-07-11T14:12:56Z

We see a lot of problems caused by the fact that we load test classes with the deployment classloader, and then intercept the execution and reload the classes with the runtime classloader. Although the new test is loaded with the runtime classloader, its arguments are still loaded with the deployment classloader. To work around that we sometimes need to clone the arguments by serializing and de-serializing. This was always brittle and no longer worked at all on Java 17+ (until #40601 fixed that). We also see issues because parts of the test infrastructure see the 'wrong' instance of the class. See, for example, quarkiverse/quarkus-pact#73 and #22611.

We have several feature raised against the JUnit team to allow us more control over classloading. The first of this features was introduced in JUnit 5.10, and allows an interceptor to be registered before any tests are launched. This interceptor can set a thread context classloader, which is then used by JUnit to load tests.

My experiments with this feature were thoroughly disappointing. It turns out, setting a TCCL early in the test lifecycle doesn't really help us, because we overwrite our 'early' TCCL with other TCCLs later in the test lifecycle. The following diagram shows some of the places we set the TCCL.

Source: https://excalidraw.com/#json=HFPHIKx8wv0iiyXgNhAzw,8IlEmPcMRvm9pfCGShdClQ

What if we just used one of the existing interception points to set the 'right' classloader, before tests are loaded? If the tests were loaded with our preferred classloader, we wouldn’t need to intercept the factory. Loading the tests with the runtime classloader needs us to move some of our app initialisation earlier in the lifecycle, but I don't think there's any fundamental barrier to this. (We would have had to do this with a solution based on the new JUnit Launcher Interceptor anyway.)

The logic for starting Quarkus needs to be in the test discovery phase, rather than in the extension. This allows us to create the runtime classloader before the test is loaded. The JUnitTest runner already knows about the Quarkus Extension, so it’s only a small extra bit of knowledge to do some of the startup actions.

This does move a bit of the brains of the test-framework module into core. That’s not ideal, but I think it’s more than made up for by the big improvement in simplicity.

@stuartwdouglas raised the point that if we have to set only a single classloader, that's not very flexible, because we have a runtime classloader for each test profile. Doing test discovery and classloader creation at around the same time gives a neat solution to this. We already tear down the quarkus app and create a new one if we see a new test profile. Now, we can batch execution by profile, which gets rid of the line in the docs saying people should name their classes to group things in the same profile close together.

Thoughts on serialization

A big initial goal of this PR was to get rid of the xstream serialization, since it didn't work on Java 17+. #40601 fixes this issue by switching to use the JBoss marshaller for serialization. Does that mean this work item isn't needed any more? No, although it does mean its benefits are smaller. Here's why it's still useful:

Even with the JBoss serializer, higher-level test infrastructure (such as @TestTemplate) does not see Quarkus bytecode transformations done by extensions
Although the JBoss serializer works a lot better than xstream with the Java 17 access restrictions (as in, it works), serialization may continue to be a challenge going forward. See https://bugs.openjdk.org/browse/JDK-8164908 for some context. Most serializers use sun.misc.unsafe, but unsafe is shrinking. It seems certain the JDK team will have to come up with some solution and API to open up access for serializers, but the final design could have security implications (perhaps opening up access in a blanket way), or performance implications (reflection fun), or user experience implications (a need to manually set flags such as --enable-serialization?). If we can avoid serialization, we avoid all that.

Why this PR is a draft/ the big TODO list

What’s not done yet on this PR:

Gradle in IDE support (not sure if we have automated tests for this)
TestProfile support (it will be easy and nice for continuous testing, but icky for normal mode)
Support for invocation of tests from within tests; at the moment it OOMs (!) … this is maybe exposing an existing resolution inefficiency, but still investigating
Updates to IntegrationTest and ComponentTest and perhaps also UnitTest
Removal of all dead code
Consolidation of duplicated code
More automated tests (will come in a PR that goes in first)
Tests for interaction with QuarkusProdModeTest, particularly for the tests in Add tests which exercise more complex JUnit extensions #35124
Remove the zillions of printlns
Warnings about BasicLoggingEnabler failed to retrieve config: java.util.ServiceConfigurationError: org.eclipse.microprofile.config.spi.ConfigSourceProvider: io.quarkus.test.common.http.TestHTTPConfigSourceProvider not a subtype - are these new?

Scary bits that need careful review

A few changes I want to call out specifically because they’re extra-scary.

Config lookup

ConfigProviderResolver.setInstance(new RunningAppConfigResolver(runningQuarkusApplication));

This was introduced in #7897. With my new control flow, it causes an infinite loop. What I don’t totally understand is how it ever did not cause an infinite loop. The RunningAppConfigResolver asks the runningQuarkusApplication for config:

  @Override
public <T> Optional<T> getOptionalValue(String propertyName, Class<T> propertyType) {
    return runningQuarkusApplication.getConfigValue(propertyName, propertyType);
}

Where does the RunningQuarkusApplicationImpl get its config? It’s a bit obfuscated because it does everything by reflection, but what it’s doing is asking the ConfigProvider for the configured config. Which, in this case, is the RunningAppConfigResolver, which asks the runningQuarkusApplication, which asks the ConfigProvider, which asks the RunningAppConfigResolver, which asks the runningQuarkusApplication, which asks the ConfigProvider … and disaster ensues.

I think the reason this didn’t cause an infinite loop before is that the classloader in which the ConfigProvider was set to the RunningAppConfigResolver was not same one that the running application ended up doing its query in. So the RunningQuarkusApplicationImpl didn’t end up asking the RunningAppConfigResolver. I can achieve this same ‘don’t actually use the RunningAppConfigResolver’ effect by just … not doing anything. The good news is that simplifies the code a lot. The bad news is that it might be introducing a regression somewhere I didn’t spot.

The-Funk · 2023-09-14T18:15:32Z

Still watching this one. I've created a minimal not-working-example to see if this fixes it. :)

holly-cummins · 2023-09-15T19:32:55Z

Still watching this one. I've created a minimal not-working-example to see if this fixes it. :)

Still going on it ... :)

The profile support in normal mode turned out to be a bit thorny, so looking at that now. I haven't checked for a while, but at one point I had reproducers for three of the test-classloading-related defects. One was passing, but two (annoyingly) were failing. If your reproducer is shareable, I'm happy to take it and include it in what I'm checking, or to add it into the test suites, if it's a gap in what we're testing now. (It'd be worth checking what I've added in #35124 to see if one of those covers your scenario, too.)

What’s not done yet on this commit: - Gradle support - TestProfile support (but it’s easy and nice) - Support for invocation of tests from within tests; at the moment it OOMs (!) … maybe a memory leak? - Removal of all dead code, including (hurray!) all the cloning logic - Consolidation of duplicated code - More automated tests (will come in an earlier PR) - Remove the zillions of printlns

…-set-case

…rking)

…testing mode

…cocli

quarkus-bot · 2024-05-22T23:25:52Z

Status for workflow `Quarkus CI`

This is the status report for running Quarkus CI on commit 014e3f0.

⚠️ Unable to include the stracktraces as the report was too long. See annotations below for the details.
⚠️ Unable to include the failure links as the report was too long. See annotations below for the details.

Failing Jobs

Status	Name	Step	Failures	Logs	Raw logs	Build scan
✖	JVM Tests - JDK 17	`Build`	Failures	Logs	Raw logs	🔍
✖	JVM Tests - JDK 21	`Build`	Failures	Logs	Raw logs	🔍
✖	JVM Tests - JDK 17 Windows	`Build`	Failures	Logs	Raw logs	🔍
✖	Maven Tests - JDK 17	`Build`	Failures	Logs	Raw logs	🔍
✖	Maven Tests - JDK 17 Windows	`Build`	Failures	Logs	Raw logs	🔍
✖	Devtools Tests - JDK 17	`Build`	Failures	Logs	Raw logs	🔍
✖	Devtools Tests - JDK 21	`Build`	Failures	Logs	Raw logs	🔍
✖	Devtools Tests - JDK 17 Windows	`Build`	Failures	Logs	Raw logs	🔍
✖	Native Tests - Amazon	`Build`	Failures	Logs	Raw logs	🔍
✖	Native Tests - Cache	`Build`	Failures	Logs	Raw logs	🔍
✖	Native Tests - gRPC	`Build`	Failures	Logs	Raw logs	🔍
✖	Native Tests - Misc3	`Build`	Failures	Logs	Raw logs	🔍
✖	Native Tests - Misc4	`Build`	Failures	Logs	Raw logs	🔍

Full information is available in the Build summary check run.
You can consult the Develocity build scans.

Failures

⚙️ JVM Tests - JDK 17 #

- Failing: extensions/vertx-http/deployment integration-tests/funqy-google-cloud-functions integration-tests/google-cloud-functions and 2 more
! Skipped: devtools/cli extensions/agroal/deployment extensions/amazon-lambda-http/deployment and 390 more

📦 extensions/vertx-http/deployment

✖ io.quarkus.vertx.http.testrunner.params.TestParameterizedTestCase.testParameterizedTests line 47 - History

📦 integration-tests/funqy-google-cloud-functions

✖ io.quarkus.funqy.gcp.functions.test.GreetingFunctionsCloudEventTest.test - History

✖ io.quarkus.funqy.gcp.functions.test.GreetingFunctionsPubsubTest.test - History

✖ io.quarkus.funqy.gcp.functions.test.GreetingFunctionsStorageTest.test - History

📦 integration-tests/google-cloud-functions

✖ io.quarkus.gcp.function.test.BackgroundFunctionStorageTestCase.test - History

✖ io.quarkus.gcp.function.test.CloudEventStorageTestCase.test - History

✖ io.quarkus.gcp.function.test.HttpFunctionRandomPortTestCase. - History

✖ io.quarkus.gcp.function.test.HttpFunctionTestCase.test - History

✖ io.quarkus.gcp.function.test.RawBackgroundFunctionPubSubTestCase.test - History

📦 integration-tests/kubernetes-client-devservices

✖ io.quarkus.kubernetes.client.devservices.it.DevServicesKubernetesITest.shouldReturnAllKeys line 24 - History

📦 integration-tests/picocli-native

✖ io.quarkus.it.picocli.BeforeEachLaunchTest.testLaunchAnnotationOnly - History

✖ io.quarkus.it.picocli.BeforeEachLaunchTest.testLaunchResult(LaunchResult) - History

✖ io.quarkus.it.picocli.PicocliTest.testBasicReflection(LaunchResult) - History

✖ io.quarkus.it.picocli.PicocliTest.testCommandWithArgGroup(LaunchResult) - History

✖ io.quarkus.it.picocli.PicocliTest.testCompletionReflection - History

✖ io.quarkus.it.picocli.PicocliTest.testDefaultValueProvider(LaunchResult) - History

✖ io.quarkus.it.picocli.PicocliTest.testDynamicProxy(LaunchResult) - History

✖ io.quarkus.it.picocli.PicocliTest.testDynamicVersionProvider(LaunchResult) - History

✖ io.quarkus.it.picocli.PicocliTest.testExcludeLogCapturing(QuarkusMainLauncher) line 51 - History

✖ io.quarkus.it.picocli.PicocliTest.testI18s(QuarkusMainLauncher) line 115 - History

✖ io.quarkus.it.picocli.PicocliTest.testIncludeLogCommand(QuarkusMainLauncher) line 59 - History

✖ io.quarkus.it.picocli.PicocliTest.testMethodSubCommand(QuarkusMainLauncher) line 40 - History

✖ io.quarkus.it.picocli.PicocliTest.testParentCommand(LaunchResult) - History

✖ io.quarkus.it.picocli.PicocliTest.testUnmatched(LaunchResult) - History

⚙️ JVM Tests - JDK 21 #

- Failing: extensions/vertx-http/deployment integration-tests/funqy-google-cloud-functions integration-tests/google-cloud-functions and 2 more
! Skipped: devtools/cli extensions/agroal/deployment extensions/amazon-lambda-http/deployment and 390 more