"Thread per test class" via MultiEnvTestEngine
#9453
Conversation
...g/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvTestEngine.java
Show resolved
Hide resolved
| import org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutorService; | ||
| import org.junit.platform.engine.support.hierarchical.ThrowableCollector; | ||
|
|
||
| public class ThreadPerTestClassExecutionTestEngine |
There was a problem hiding this comment.
One minor drawback here is that this new engine is only used as a delegate to MultiEnvTestEngine, never as a "top-level" engine. IOW, only modules that include :nessie-multi-env-test-engine will benefit from the improvement.
Granted, most "problematic" modules have a dependency on the multi-env engine anyways. But i was thinking of the kubernetes operator PR: this module does not have multi-env tests, so it wouldn't benefit from these improvements as it is today. But maybe it's just a matter of adding a dependency to :nessie-multi-env-test-engine there.
There was a problem hiding this comment.
Or use JUnit, if junit-team/junit5#3939 gets merged and released.
| /** Returns {@code junit-jupiter-engine} as the artifact ID. */ | ||
| @Override | ||
| public Optional<String> getArtifactId() { | ||
| return Optional.of("junit-jupiter-engine"); |
There was a problem hiding this comment.
I'm not sure this is correct. Do we have to use this ID to make stuff work?
There was a problem hiding this comment.
I mean: would a Nessie artifact ID work here?
There was a problem hiding this comment.
The ID doesn't really matter. And with the Junit-PR this would go away entirely.
...ngine/src/test/java/org/projectnessie/tools/compatibility/engine/TestMultiEnvTestEngine.java
Show resolved
Hide resolved
...src/main/java/org/projectnessie/junit/engine/ThreadPerTestClassExecutionExecutorService.java
Show resolved
Hide resolved
...g/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvTestFilter.java
Show resolved
Hide resolved
...g/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvTestEngine.java
Show resolved
Hide resolved
See projectnessie#9441 for a complete description of the underlying problem. TL;DR is: `ThreadLocal`s from various 3rd party libraries leak into the single `Test worker` thread that runs all the tests, resulting in TL objects/suppliers from the various Quarkus test class loaders, eventually leading to nasty OOMs. This change updates the `MultiEnvTestEngine` by using the new `ThreadPerTestClassExecutionExecutorService` and also "assimilate" really all tests, even the non-multi-env tests, so that those also run on a thread per test-class. The logic to distinguish multi-env from non-multi-env tests via `MultiEnvExtensionRegistry.registerExtension()` via test discovery is not perfect (but good enough), it can add multi-env tests to the non-multi-env tests, so an additional check is needed there. Since each test class runs on "its own thread", the `ThreadLocal`s are registered on that thread. Once the test class finishes, the thread is disposed and its thread locals become eligible for garbage collection, which is what is needed. The bump of the max-heap size for test workers is also reduced back to 2g (was changed in projectnessie#9433). Fixes projectnessie#9441
snazy
force-pushed
the
multi-env-thread-per-test-class
branch
from
79eb782
to
7789b10
Compare
| // Also execute all other tests via the MultiEnv test engine to get the "thread per | ||
| // test-class" behavior. | ||
| registry() | ||
| .probablyNotMultiEnv() |
There was a problem hiding this comment.
Why collect test descriptors in an "extension" registry? Why not reuse preliminaryResult here and filter out all multi-env tests?
We already rediscover tests for each env. ID, so we're probably not saving much time by collecting test descriptors during the extension registration process... On the other hand using a discovery result directly would be much more clear... WDYT?
There was a problem hiding this comment.
The result is sadly not correct that way. We can make it "perfect" in a follow-up.
See #9441 for a complete description of the underlying problem. TL;DR is:
ThreadLocals from various 3rd party libraries leak into the singleTest workerthread that runs all the tests, resulting in TL objects/suppliers from the various Quarkus test class loaders, eventually leading to nasty OOMs.This change updates the
MultiEnvTestEngineby using the newThreadPerTestClassExecutionExecutorServiceand also "assimilate" really all tests, even the non-multi-env tests, so that those also run on a thread per test-class. The logic to distinguish multi-env from non-multi-env tests viaMultiEnvExtensionRegistry.registerExtension()via test discovery is not perfect (but good enough), it can add multi-env tests to the non-multi-env tests, so an additional check is needed there.Since each test class runs on "its own thread", the
ThreadLocals are registered on that thread. Once the test class finishes, the thread is disposed and its thread locals become eligible for garbage collection, which is what is needed.The bump of the max-heap size for test workers is also reduced back to 2g (was changed in #9433).
Fixes #9441