Invoke lifecycle hooks around each dynamic test #735
Conversation
sbrannen
changed the title
| } | ||
|
|
||
| protected void invokeBetweenBeforeAndAfter(JupiterEngineExecutionContext context, | ||
| Consumer<JupiterEngineExecutionContext> action) { | ||
| ThrowableCollector throwableCollector = context.getThrowableCollector(); | ||
|
|
||
| // @formatter:off | ||
| invokeBeforeEachCallbacks(context); |
There was a problem hiding this comment.
Please don't mess with the indentation art.
In other words, please retain the original indentation so that diffs are easier to follow.
| return context; | ||
| } | ||
|
|
||
| protected void invokeBetweenBeforeAndAfter(JupiterEngineExecutionContext context, |
There was a problem hiding this comment.
I don't like this method name: it does not align nicely with the existing methods in that class.
Something like invokeTest() or executeTest() would make more sense here.
| } | ||
|
|
||
| protected void invokeBetweenBeforeAndAfter(JupiterEngineExecutionContext context, | ||
| Consumer<JupiterEngineExecutionContext> action) { |
There was a problem hiding this comment.
The term action here is insufficient to convey its purpose.
Something like testInvoker or testExecutor would be more suitable.
| @@ -69,6 +70,18 @@ public boolean isLeaf() { | |||
| } | |||
|
|
|||
| @Override | |||
| public JupiterEngineExecutionContext execute(JupiterEngineExecutionContext context, | |||
There was a problem hiding this comment.
Please explain in great detail the need to override the execute() method, not only here but also in the commit message.
There was a problem hiding this comment.
I choose to override the execute() method here to prevent calls to before and after hooks around a container execution.
This is because a hook implementation can not know on which (of a container or a test) it is executed.
I assumed that @BeforeEach meant before each test, in this particular case, unlike what's said in the annotations javadoc.
A cleaner way can be to declare a new annotation @BeforeEachTest and let callback implementations know about the type of test (container or not).
If this is something you prefer, I can give it a try.
There was a problem hiding this comment.
Unless I'm mistaken, lifecycle callbacks currently apply to @TestFactory methods.
So it appears that you have removed this functionality, which some people may depend on.
There was a problem hiding this comment.
At the moment, no, I don't think we want to introduce any additional lifecycle annotations.
There was a problem hiding this comment.
What do you think about invoking lifecycle callbacks around @TestFactory methods and around each dynamic tests and making implementations aware of the type (container or test) they are invoked on ?
This could be done by adding an isContainer:boolean method to the TestExtensionContext like on the TestDescriptor interface.
I tried to improve the MockitoExtension by adding a reset of mocked field / parameters in Store before each test.
This is why I removed callback invocation around @TestFactory methods in favor of dynamic tests in the first place.
|
Thanks for the PR! 👍 I only had a few minutes to briefly review it within the browser. So if my review comments are vague, please ask for clarification. |
|
I have some serious concerns about management of the Specifically: What happens if a dynamic test results in a failure (i.e., exception thrown)? Does the What happens if an extension stores state in the In general, in JUnit Jupiter it is required that each execution of a node in the test tree gets a new execution context. So without explicit tests in place that confirm proper behavior for the above requirement, we will not be able to merge this PR. |
|
I understand. To answer to these questions and provide a solution, can you tell be me if dynamic tests are supposed to be executable in parallel (as the executor is stream-based). I was not able to find in the user guide or in the javadoc, if there is any concerns about parallelism and how this should be handled in |
At the moment, no. There is currently zero support for parallel test execution with JUnit Jupiter. See #60. So, please do not add any support related to parallel test execution in conjunction with this PR. |
|
It's been a while since I have time to spend on this. After considerations, it may not be such a good idea to try micro-management around I suggest that we close this PR if that is ok with you. |
|
Sure, thanks for getting back to us! |
Overview
@BeforeEach,@AfterEachand associated callback extensions run once around@TestFactorymethods.The proposition here is to run these hooks around each dynamic tests created by the factory method.
Hooks are not invoked around the factory method, because it is a container and type (container or test) cannot be determined from a hook implementation.
I hereby agree to the terms of the JUnit Contributor License Agreement.