[#771] Fix @RepeatedTest running N*N times in container mode#846
[#771] Fix @RepeatedTest running N*N times in container mode#846rhusar wants to merge 2 commits into
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideEnsures JUnit 5 @RepeatedTest methods in container mode execute Arquillian SPI lifecycles exactly once per repetition instead of N*N, by correcting context storage for templates, skipping duplicate template invocations, wiring run mode in the test harness, and tightening tests around lifecycle invocations rather than JUnit’s test counters. Sequence diagram for intercepting JUnit5 test templates in container modesequenceDiagram
participant JUnitJupiter as JUnitJupiter
participant ArquillianExtension as ArquillianExtension
participant ContextStore as ContextStore
participant Invocation as Invocation
JUnitJupiter->>ArquillianExtension: interceptTestTemplateMethod(invocation, invocationContext, extensionContext)
ArquillianExtension->>ContextStore: isRegisteredTemplate(invocationContext.getExecutable())
ContextStore-->>ArquillianExtension: boolean
alt template not registered
ArquillianExtension->>ArquillianExtension: interceptInvocation(invocation, extensionContext)
else template already registered
ArquillianExtension->>Invocation: skip()
end
ArquillianExtension->>ArquillianExtension: throwError(result)
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
ArquillianExtension.interceptTestTemplateMethod, once you detect a registered template and callinvocation.skip(), consider returning immediately instead of falling through tothrowError(result)so the control flow for the skipped path is clearer and doesn’t depend on the prior value ofresult. - The new
fireCustomLifecyclestub inJUnitJupiterRepeatedTestCasemutates allRunModeEvents torunAsClient=false; consider scoping this behavior more narrowly or documenting why it is safe for all events in this test to avoid surprising interactions if additional event types are introduced in future test setups.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ArquillianExtension.interceptTestTemplateMethod`, once you detect a registered template and call `invocation.skip()`, consider returning immediately instead of falling through to `throwError(result)` so the control flow for the skipped path is clearer and doesn’t depend on the prior value of `result`.
- The new `fireCustomLifecycle` stub in `JUnitJupiterRepeatedTestCase` mutates all `RunModeEvent`s to `runAsClient=false`; consider scoping this behavior more narrowly or documenting why it is safe for all events in this test to avoid surprising interactions if additional event types are introduced in future test setups.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Opening as draft, as I am not fully confident. But in general, before the fix, getTemplateStore() used context.getStore(...) — the per-repetition context. After the fix, context.getParent().orElse(context) goes up to the method context, which is shared by all three repetitions. Repetition 1 registers there, repetitions 2 and 3 see it → adaptor.test() fires once. Concerns that need to be evalauted: getParent() returning empty, @ParameterizedTest, @nested (tests already added and are passing) and invocation.skip() addition — without it, it would throw "Chain of InvocationInterceptors never called invocation" for the skipped repetitions @jamezp @jasondlee @nlisker @mskacelik if you want to review/test/etc |
|
Tested if |
|
@mskacelik You know what that sounds good. You can just open a PR against my branch (rhusar:771-fix-repeated-test-container-mode). |
| verify(adaptor, times(1)).afterClass(any(Class.class), any(LifecycleMethodExecutor.class)); | ||
| verify(adaptor, times(3)).before(any(Object.class), any(Method.class), any(LifecycleMethodExecutor.class)); | ||
| verify(adaptor, times(3)).after(any(Object.class), any(Method.class), any(LifecycleMethodExecutor.class)); | ||
| verify(adaptor, times(1)).test(any(TestMethodExecutor.class)); |
There was a problem hiding this comment.
Why would the test only be executed once? Maybe I don't understand this test framework though.
There was a problem hiding this comment.
@jamezp Good question, my understanding is that in container mode - which was the reason why the test was passing before as it was in the client mode - adaptor.test() sends the test to the container via the ARQ protocol. The container executes the test method remotely - once.
So what is the alternative - the client side would call adaptor.test() for each of the 3 repetitions, and the container would execute just the method body once per call and NOT expand to the @RepeatedTest template. The problem we have now is that both are expanding so we do get N*N.
|
Ok I think I found some issue when I started to work on the Notice that RUN 2,3 are PASSING (maybe because of the skip?), before they were all failing: You can actually reproduce this if you put a failing test in the |
Signed-off-by: Radoslav Husar <radosoft@gmail.com>
ContextStore.getTemplateStore() used the per-invocation ExtensionContext, so each repetition had its own template registry and isRegisteredTemplate() never saw prior registrations. Use the parent context store which is shared across repetitions. Store the TestResult from the first invocation and replay it for subsequent repetitions so that failures are correctly reported for all repetitions. Also skip the JUnit5 invocation for already-registered templates and enable the integration test. Signed-off-by: Radoslav Husar <radosoft@gmail.com>
rhusar
force-pushed
the
771-fix-repeated-test-container-mode
branch
from
fa9b623 to
8056add
Compare
|
@mskacelik Nice catch! The results needs to be propagated in this scenario better, updated the branch with attempt to replay the result |
It's almost there. It seems that when at least one of the repeated tests fails, all attempts are marked as failures. However, that is incorrect. See: @ExtendWith(ArquillianExtension.class)
class SimpleRepeatedArqTest {
static int i = 0;
@Deployment
public static WebArchive createDeployment() {
return ShrinkWrap.create(WebArchive.class, "repeated-test.war");
}
@RepeatedTest(3)
void failingRepeatedTest() {
boolean result = i++ % 2 == 0;
System.out.println(">>> Result: " + result);
Assertions.assertTrue(result);
}
}Test output: Without ARQ (comment out |
|
Created a testing repo with the reproducer, if you would like to test it out: https://github.com/mskacelik/arquillian-jetty-examples/ |
|
The IdentifiedTestException returned from the container (1st |
|
@mskacelik Can you have a look at this comment above? #846 (comment) I believe that's what you are describing here. Basically, to get the behavior you are describing we would need to rework this to expand the template on the client, not the server. |
4 participants
Fixes #771
Summary by Sourcery
Ensure JUnit 5 @RepeatedTest methods execute container lifecycle only once per logical test rather than N×N times in container mode.
Bug Fixes:
Tests: