Repository navigation
Await dynamic tests when a node's execution fails - #6142
mpkorstanje merged 8 commits into
Conversation
When a node's execute() method threw an exception after submitting dynamic tests to its DynamicTestExecutor, NodeTestTask skipped awaitFinished() and executed the node's after behavior and reported it as finished while the dynamic tests were still pending. With parallel execution, these dynamic tests then finished after their parent, or even after the end of the test plan. In Jupiter, this happened when the stream of invocations of a test template or class template (e.g. the arguments source of a @ParameterizedTest or @ParameterizedClass) or the stream of children of a DynamicContainer threw an exception. NodeTestTask now awaits the submitted dynamic tests regardless of whether execute() failed. The original exception remains the primary failure. Issue: junit-team#6141 Signed-off-by: IWillBurn <eugene.boyarnikov@gmail.com>
When the stream returned by a @testfactory method threw an exception after providing some dynamic tests, TestFactoryTestDescriptor skipped awaitFinished() and executed the after-each callbacks and methods of the factory method while these dynamic tests were still executing in parallel. TestFactoryTestDescriptor now awaits the provided dynamic tests regardless of whether the stream threw an exception, and Node#execute documents that submitted dynamic tests are finished before the node's after behavior, even if execute() throws. Issue: junit-team#6141 Signed-off-by: IWillBurn <eugene.boyarnikov@gmail.com>
The dynamic tests now wait until the node's execute() method or the stream has thrown, which establishes the order deterministically. Issue: junit-team#6141 Signed-off-by: IWillBurn <eugene.boyarnikov@gmail.com>
The end-to-end tests could not reliably fail without the fix: a dynamic test can only be released when the source fails, after which it races with its parent. The unit tests release the dynamic test only once its node either awaits it or calls after(), and verify that a test factory awaits its dynamic tests when its stream fails. Issue: junit-team#6141 Signed-off-by: IWillBurn <eugene.boyarnikov@gmail.com>
|
Follow-up to the latch review comment: The latch-based tests passed CI, but replacing the sleeps with latches showed that the end-to-end tests can't reliably reproduce the bug, so I removed them. To show it, invocation Instead, there are two deterministic tests:
I can add an end-to-end test back if you'd like, but as far as I understand, in Jupiter it can only be one of two: latch-based, which reliably passes with the fix but doesn't reliably fail without it, or time-based (sleep or timeout), which catches the bug but depends on timing. |
I don't think that is necessary. It is unfortunate that we lose the illustrative examples but I don't see any seams we can use to delay the execution of the dynamic tests. |
Issue: junit-team#6141 Signed-off-by: IWillBurn <eugene.boyarnikov@gmail.com>
mpkorstanje
left a comment
There was a problem hiding this comment.
LGTM. Cheers, excellent pull request.
This comment has been minimized.
This comment has been minimized.
We plan to ship 6.2.0-M1 soon. Since this bug has been present for a long time I'd say we don't need to backport it at this time. |
When
Node.execute()throws after it has already submitted dynamic tests,NodeTestTaskskipsDynamicTestExecutor.awaitFinished(). With parallel execution, thenode's
after()behavior runs and the node is reported as finished while its dynamictests are still pending, so they finish after their parent or even after the test plan
(see #6141 for a reproducer). In Jupiter, this affects a
@ParameterizedTest, a@ParameterizedClass, or aDynamicContainerwhose stream throws. Similarly, the@AfterEachmethods of a@TestFactorywhose stream throws run while its dynamic testsare still executing.
NodeTestTasknow awaits the submitted dynamic tests regardless of whetherexecute()threw. The original exception remains the primary failure.TestFactoryTestDescriptorawaits the provided dynamic tests before the after-eachcallbacks and methods, even if the stream threw. The
Node#executeJavadoc nowdocuments this guarantee.
The new tests in
HierarchicalTestExecutorTestsandParallelExecutionIntegrationTestsfail on
mainwith both executor service implementations. The release notes entriesare in 6.2.0-M1; I'm happy to move them to a 6.1.x patch release if you'd like to
backport the fix.
Closes #6141
I hereby agree to the terms of the JUnit Contributor License Agreement.
Definition of Done
@APIannotations (no API changes;Node#executeJavadoc updated)