-
Notifications
You must be signed in to change notification settings - Fork 36
Wait for @AfterEach method's VertxTestContext in case a @Test method failed #148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
alsin
wants to merge
6
commits into
eclipse-vertx:master
Choose a base branch
from
alsin:fix-context-ignorance-in-after-each
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
cdec66d
Wait for @AfterEach method's VertxTestContext in case a @Test method …
alsin cd99564
Rework test to be run programmatically from another test
alsin 85a4010
Rework test to be run programmatically from another test
alsin 685fe9d
Some reformatting, imports expansions
alsin 77c4f8d
Fix typo in disabled annotation comment
alsin 650bf72
Clean up comments
alsin File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
77 changes: 77 additions & 0 deletions
77
src/test/java/io/vertx/junit5/tests/RunAfterEachContextCheckTest.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| package io.vertx.junit5.tests; | ||
|
|
||
| import org.junit.jupiter.api.Test; | ||
| import org.junit.platform.engine.DiscoverySelector; | ||
| import org.junit.platform.engine.TestExecutionResult; | ||
| import org.junit.platform.engine.discovery.DiscoverySelectors; | ||
| import org.junit.platform.engine.support.descriptor.ClassSource; | ||
| import org.junit.platform.launcher.EngineFilter; | ||
| import org.junit.platform.launcher.Launcher; | ||
| import org.junit.platform.launcher.LauncherDiscoveryRequest; | ||
| import org.junit.platform.launcher.TestExecutionListener; | ||
| import org.junit.platform.launcher.TestIdentifier; | ||
| import org.junit.platform.launcher.core.LauncherDiscoveryRequestBuilder; | ||
| import org.junit.platform.launcher.core.LauncherFactory; | ||
| import org.junit.platform.launcher.listeners.SummaryGeneratingListener; | ||
| import org.junit.platform.launcher.listeners.TestExecutionSummary; | ||
|
|
||
| import java.util.concurrent.atomic.AtomicReference; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
|
|
||
| class RunAfterEachContextCheckTest { | ||
|
|
||
| @Test | ||
| void runsWaitForContextInAfterEachMethodTestAndChecksAfterAllSucceeded() { | ||
| // Select only the target class | ||
| final DiscoverySelector selector = DiscoverySelectors.selectClass(WaitForContextInAfterEachMethodTest.class); | ||
|
|
||
| // Collect a summary for assertions | ||
| final SummaryGeneratingListener summaryListener = new SummaryGeneratingListener(); | ||
|
|
||
| // Capture the class container result specifically (to assert @AfterAll behavior) | ||
| final AtomicReference<TestExecutionResult.Status> classStatus = new AtomicReference<>(); | ||
|
|
||
| final TestExecutionListener captureClassStatus = new TestExecutionListener() { | ||
| @Override | ||
| public void executionFinished(final TestIdentifier id, final TestExecutionResult result) { | ||
| id.getSource() | ||
| .ifPresent(source -> { | ||
| if (id.isContainer() && source instanceof ClassSource) { | ||
| final ClassSource cs = (ClassSource) source; | ||
| if (cs.getClassName().equals(WaitForContextInAfterEachMethodTest.class.getName())) { | ||
| classStatus.set(result.getStatus()); | ||
| } | ||
| } | ||
| }); | ||
| } | ||
| }; | ||
|
|
||
| final LauncherDiscoveryRequest request = LauncherDiscoveryRequestBuilder.request() | ||
| .selectors(selector) | ||
| .filters(EngineFilter.includeEngines("junit-jupiter")) | ||
| // Make @Disabled inert for this run | ||
| .configurationParameter( | ||
| "junit.jupiter.conditions.deactivate", "org.junit.*DisabledCondition") | ||
| // Make execution deterministic | ||
| .configurationParameter("junit.jupiter.execution.parallel.enabled", "false") | ||
| .build(); | ||
|
|
||
| final Launcher launcher = LauncherFactory.create(); | ||
| launcher.registerTestExecutionListeners(summaryListener, captureClassStatus); | ||
|
|
||
| launcher.execute(request); | ||
|
|
||
| final TestExecutionSummary summary = summaryListener.getSummary(); | ||
|
|
||
| // The single test method intentionally fails | ||
| assertEquals(1, summary.getTestsFoundCount(), "Expect exactly one test discovered"); | ||
| assertEquals(1, summary.getTestsFailedCount(), "The test should fail via context.failNow()"); | ||
| assertEquals(0, summary.getContainersFailedCount(), "The test class container should not fail"); | ||
|
|
||
| // Critically: the class container (where @AfterAll runs) must be SUCCESSFUL | ||
| assertEquals(TestExecutionResult.Status.SUCCESSFUL, | ||
| classStatus.get(), | ||
| "@AfterAll must complete its VertxTestContext without failure"); | ||
| } | ||
| } |
41 changes: 41 additions & 0 deletions
41
src/test/java/io/vertx/junit5/tests/WaitForContextInAfterEachMethodTest.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| package io.vertx.junit5.tests; | ||
|
|
||
| import io.vertx.core.Vertx; | ||
| import io.vertx.junit5.VertxExtension; | ||
| import io.vertx.junit5.VertxTestContext; | ||
| import org.junit.jupiter.api.AfterAll; | ||
| import org.junit.jupiter.api.AfterEach; | ||
| import org.junit.jupiter.api.Disabled; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.junit.jupiter.api.extension.ExtendWith; | ||
|
|
||
| import java.util.concurrent.atomic.AtomicBoolean; | ||
|
|
||
| @Disabled("Executed only via programmatic launcher from RunAfterEachContextCheckTest") | ||
| @ExtendWith({VertxExtension.class}) | ||
| public class WaitForContextInAfterEachMethodTest { | ||
| static final AtomicBoolean afterTestContextAwaited = new AtomicBoolean(false); | ||
|
|
||
| @Test | ||
| void test(final VertxTestContext context) { | ||
| context.failNow(new RuntimeException("Failing test through context")); | ||
| } | ||
|
|
||
| @AfterAll | ||
| static void afterAll(final VertxTestContext context) { | ||
| if (afterTestContextAwaited.get()) { | ||
| context.completeNow(); | ||
| } else { | ||
| context.failNow(new RuntimeException("afterTest context was not awaited")); | ||
| } | ||
| } | ||
|
|
||
| @AfterEach | ||
| void afterTest(final Vertx vertx, final VertxTestContext context) { | ||
| vertx.setTimer(100, id -> { | ||
| afterTestContextAwaited.set(true); | ||
| context.completeNow(); | ||
| }); | ||
| } | ||
|
|
||
| } |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might fix the problem for
@AfterEachmethods, but not for@AfterAllones.The extension creates a new
VertxTestContextfor 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 invocationThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no such problem for
@AfterAll- the extension context which is passed intodoes not contain the exception thrown in the
@Testmethod.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Can you please rework the test so that it passes?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's possible - the thing is it has to fail the
VertxTestContextin 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, looking forward to it!
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@Tagsolution but are run programmatically from another tests without those tags. I decided to go a bit different way using the@Disabledannotation which is then deactivated in another test. Please, check it out! Thank you!