Wait for @AfterEach method's VertxTestContext in case a @Test method failed #148
Conversation
There was a problem hiding this comment.
Thanks for this PR @alsin
Please see comments inline. When updating the PR, please sign-off your commits (using the email that is registered at the Eclipse Fdn for your contributor agreement).
| final boolean isNotInAfterEachMethod = Optional.ofNullable(invocationContext) | ||
| .map(ReflectiveInvocationContext::getExecutable) | ||
| .map(executable -> executable.getAnnotation(AfterEach.class)) | ||
| .isEmpty(); | ||
|
|
||
| if (isNotInAfterEachMethod) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
This might fix the problem for @AfterEach methods, but not for @AfterAll ones.
The extension creates a new VertxTestContext for each intercepted method. I think the criterion for waiting the completion of the context should be that the execution exception hasn't changed during the invocation
There was a problem hiding this comment.
There's no such problem for @AfterAll - the extension context which is passed into
public void interceptAfterAllMethod(Invocation<Void> invocation, ReflectiveInvocationContext<Method> invocationContext, ExtensionContext extensionContext) throws Throwable {does not contain the exception thrown in the @Test method.
There was a problem hiding this comment.
Makes sense. Can you please rework the test so that it passes?
There was a problem hiding this comment.
I don't think it's possible - the thing is it has to fail the VertxTestContext in the test method so that the bug occurs in the @AfterEach method and we have something to test. Otherwise, if the test context is succeeded, the bug never reveals itself in the @AfterEach. It's kind of a paradox - I don't really know how to test this properly to be honest as the test should fail in order to pass 🤔 What I can do is remove the test completely but then any possible regression could be overlooked in the future. Can you suggest me something here, please?
There was a problem hiding this comment.
I think I might have an idea how to write such a test. Let me try on it, I'll let you know soon if I find a way :)
There was a problem hiding this comment.
Thank you, looking forward to it!
There was a problem hiding this comment.
Ok, I think I've got a working solution. Actually, I didn't notice that there are already some other JUnit tests in the project that are excluded from running by the surefire plugin using the @Tag solution but are run programmatically from another tests without those tags. I decided to go a bit different way using the @Disabled annotation which is then deactivated in another test. Please, check it out! Thank you!
src/test/java/io/vertx/junit5/tests/WaitForContextInAfterEachMethodTest.java
Outdated
Show resolved
Hide resolved
alsin
force-pushed
the
fix-context-ignorance-in-after-each
branch
3 times, most recently
from
5cf6865 to
b41756b
Compare
alsin
force-pushed
the
fix-context-ignorance-in-after-each
branch
from
b41756b to
cdec66d
Compare
2 participants
Motivation:
If a
@Testmethod fails then potentialVertxTestContextinjected into a@AfterEachmethod (if any, of course) is not awaited.For instance, a test might want to perform test data cleanup in a series of asynchronous operations in such a @AfterEach method and
VertxTestContextcould be used to gracefully wait for their end of operation. Unfortunately, as of now, if a test fails it does not wait for the test context injected into an after test method.Conformance:
You should have signed the Eclipse Contributor Agreement as explained in https://github.com/eclipse/vert.x/blob/master/CONTRIBUTING.md
Please also make sure you adhere to the code style guidelines: https://github.com/vert-x3/wiki/wiki/Vert.x-code-style-guidelines