Introduce SUITE, CLASS scope modes for @Testcontainer annotation#127
Introduce SUITE, CLASS scope modes for @Testcontainer annotation#127rhusar wants to merge 2 commits into
Conversation
|
This is a feature we would need in WildFly clustering test suite when running a group of tests against an Infinispan server and there are also Elytron TS use cases. Opening as draft before I head over for PTO later today so I don't forget all about it while I am off. Nevertheless, seems fully functional but haven't tested it outside of the integration testsuite done here. Note the complications of the TestcontainerRegistryView class, which is working around the injection of the same class but in different scoped; all explained in the Javadoc. This will need adaptation as the #123 and #124 get merged. Have a look @jamezp @jasondlee and might be of interest to @haster too |
35d0681 to
d34a1d1
Compare
|
This is gonna be really useful to us. We re-use our containers between different testclasses, mostly for performance reasons. This PR is shaping up to be exactly what we planned to send in. Claude is also very happy:
|
…annotation, replacing the previous boolean value. SUITE-scoped containers are started once and shared across test classes, CLASS-scoped containers follow the existing per-class behavior, MANUAL-scoped containers are injected but never started/stopped by the framework. Add TestcontainerLifecycle enum for container lifecycle scoping Suite-scoped containers use @SuiteScoped registry created in BeforeSuite, while class-scoped containers use a fresh @ClassScoped registry per test class. TestcontainerRegistryView routes lookups to the correct registry based on lifecycle value. Resolves: arquillian#126 Signed-off-by: Radoslav Husar <rhusar@redhat.com> Signed-off-by: Radoslav Husar <radosoft@gmail.com>
Replace the single TestcontainerLifecycle enum (SUITE, CLASS, MANUAL) with two independent @TestContainer attributes: boolean value() for managed/manual control (matching upstream/main) and TestcontainerScope scope() for CLASS/SUITE scoping. This allows combinations like a manually managed suite-scoped container. Signed-off-by: Radoslav Husar <radosoft@gmail.com>
d34a1d1 to
111aeec
Compare
|
Discussed with @jamezp yesterday and we realized that these concepts are rather orthogonal, so rebased and refactored to actually handle scope and manual mode as 2 separate concepts (i.e. you can have both class and suite scopes manual). Ready for review now. |
|
Also a note on the TestcontainerRegistryView rather than a single collection/registry - this keeps the door open for potential future concurrent test execution where class scoped registries are not shared while the suite one is. |
|
Back from holiday today. Any chance anyone had any more thoughts on this? @jamezp @haster @jasondlee let me know. |
haster
left a comment
There was a problem hiding this comment.
This looks good and useful to me. I've added some comments, and Claude added some more, but nothing major.
|
|
||
| @Test | ||
| public void containersAreDifferentInstances() { | ||
| Assertions.assertNotSame(suiteContainer, classContainer, |
There was a problem hiding this comment.
The injected containers are of different types, so wouldn't this always be true, regardless of scope?
| @ExtendWith(ArquillianExtension.class) | ||
| @RunAsClient | ||
| @TestcontainersRequired(TestAbortedException.class) | ||
| public class MixedLifecycleTest { |
There was a problem hiding this comment.
I think it would be beneficial to have an actual suite of two test classes working together, injecting the same containers, one class and one suitescoped in each. You could then verify that the suitecontainer is the same in both instances but the class containers are different. Not sure how easy that is to actually do, though. 😅
There was a problem hiding this comment.
Perhaps that is more a feature of the other test, focusing only on the suite scope injection. 🤔
One possible solution would be to have two test classes in a suite, both extending the same base class, which holds a static field. First testclass records the injected suitescoped container in that field, second class check it against their own injected container.
| final TestcontainerRegistry instances = new TestcontainerRegistry(); | ||
| containerRegistry.set(instances); | ||
|
|
||
| // Read suite registry before setting class registry — both InstanceProducers share the same generic type |
There was a problem hiding this comment.
(generated by Claude)
The ordering comment documents a real footgun. The same SPI limitation that justified TestcontainerRegistryView (distinct types so Instance<T> can disambiguate) applies here too — a future reader who reorders these two lines (or splits them across methods) silently breaks suite resolution. Consider giving the suite registry its own marker subtype (SuiteTestcontainerRegistry extends TestcontainerRegistry) so the producers are distinguishable by type and the ordering becomes irrelevant.
| description.instance.start(); | ||
| afterTestcontainerStart.fire(new AfterTestcontainerStart(context)); | ||
| } | ||
| } |
There was a problem hiding this comment.
(generated by Claude)
Your earlier comment about keeping TestcontainerRegistryView open for "future concurrent test execution where class scoped registries are not shared while the suite one is" suggests parallel class execution is on the roadmap. In that world, two classes both hitting this !isRunning() check concurrently can both observe it as false and both call start(). Today it's just a TODO, but worth either a guarding comment ("not safe for parallel class execution") or — if parallel is near-term — a synchronized first-start latch per description.
|
I've added some review comments. In general it looks good to me. I haven't had a lot of time on our side to work on this, preparing for a release took precedence, but I've been putting in some work. I think I'll open a small PR in a few minutes. After today, I will be on holiday for a long weekend so likely not to respond until later next week. |
|
Hm, should we have a Scope.METHOD that the testcontainer span is only for individual test method? Just analogous to class/suite. |
|
The injected container would still exist class-wide, correct? It would just be restarted/recreated for every method? Do I understand that correctly? We don't have a specific usecase for this as of now, but I see the symmetry you're referring to. I'd say, if it's easy to implement, sure, if it's too complex then maybe leave it until people ask for it. |
Yes, exactly, that would be it. So at the beginning of every method invocation you get a new container instance so that any state in the container is reset between individual tests.
I dont think we do either, but it's an obvious symmetry. Anyway, lets see what @jamezp thinks when he comes back. I would create a new issue fro that anyway, so this can move already. |
Add TestcontainerLifecycle enum for container lifecycle scoping
Suite-scoped containers use @SuiteScoped registry created in BeforeSuite, while class-scoped containers use a fresh @ClassScoped registry per test class. TestcontainerRegistryView routes lookups to the correct registry based on lifecycle value.
Resolves: #126