Skip to content

Add ITs for AfterAll/AfterClass teardown failure#3338

Open
XN137 wants to merge 2 commits intoapache:masterfrom
XN137:afterall-afterclass-failures
Open

Add ITs for AfterAll/AfterClass teardown failure#3338
XN137 wants to merge 2 commits intoapache:masterfrom
XN137:afterall-afterclass-failures

Conversation

@XN137
Copy link
Copy Markdown
Contributor

@XN137 XN137 commented Mar 29, 2026

Add integration tests exposing broken surefire behavior when JUnit 4
@AfterClass or JUnit 5 @AfterAll methods always throw. The tests
document the following bugs on master:

  • XML report has errors="0" despite an <error> element in a testcase
  • The teardown error is misclassified as a flake (flakes="1")
  • With rerunFailingTestsCount, the build passes green because the
    deterministic teardown failure is swallowed as a flake
  • Passing test methods are needlessly re-executed (rerunCount + 1 times)

@XN137
Copy link
Copy Markdown
Contributor Author

XN137 commented Mar 29, 2026

@olamy as requested in #3329 (comment) i am adding ITs to document the current (faulty?) behavior

@XN137 XN137 force-pushed the afterall-afterclass-failures branch from bf63960 to 76fd2bb Compare March 30, 2026 06:33
@XN137 XN137 marked this pull request as ready for review March 30, 2026 07:36
Add integration tests exposing broken surefire behavior when JUnit 4
`@AfterClass` or JUnit 5 `@AfterAll` methods always throw. The tests
document the following bugs on master:

- XML report has `errors="0"` despite an `<error>` element in a testcase
- The teardown error is misclassified as a flake (`flakes="1"`)
- With `rerunFailingTestsCount`, the build passes green because the
  deterministic teardown failure is swallowed as a flake
- Passing test methods are needlessly re-executed (`rerunCount + 1` times)
@XN137 XN137 force-pushed the afterall-afterclass-failures branch from 76fd2bb to fdc1b42 Compare March 31, 2026 10:05
@elharo
Copy link
Copy Markdown
Contributor

elharo commented Apr 2, 2026

This is arguable. If the test method passes, I'm not sure a tearDown failure should be marked as failed. The test ran. The test passed. And then something else happened.

@XN137
Copy link
Copy Markdown
Contributor Author

XN137 commented Apr 3, 2026

I agree that it's arguable what the exact behavior should be but assuming we take your viewpoint, then in the case with rerunFailingTestsCount>0 it doesnt make sense to run the passing test methods repeatedly just because the (invisible) teardown was failing.

another viewpoint could be:
if i have a silly bug in my teardown logic i want to hear about, as it might mean we fail to clean up resources properly and cause issues with other tests later.

Copy link
Copy Markdown
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said, it's arguable. There are reasonable cases to be made on both sides here. Continuing to think about this, I think errors=0 is true and should not be changed. The test passed. That much is correct.

The test is not flaky (unless the tearDown failure is flaky) so counting this as a flake and rerunning the test is a bug.

And probably there should be a separate way to indicate to the user that tearDown failed (tearDownErrors = 1 or something along those lines) but if we mix this up with test failures then the dev could be looking in the wrong place to debug the failure

@XN137
Copy link
Copy Markdown
Contributor Author

XN137 commented Apr 6, 2026

i have now removed all code comments stating what is and isnt a bug, so the tests now only encode the current behavior on the master branch.

@elharo were you looking for any other kind of changes?

@XN137
Copy link
Copy Markdown
Contributor Author

XN137 commented Apr 6, 2026

also i ran the ITs against older releases of surefire to compare current behavior to previous versions.

(note that i had to delete the .assertContainsText("name=\"initializationError\"") as it only exists on master)

when going to 3.5.5 for junit4:

[INFO] Running org.apache.maven.surefire.its.JUnit4FailingAfterClassIT
[INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.402 s -- in org.apache.maven.surefire.its.JUnit4FailingAfterClassIT
[INFO] 
[INFO] Results:
[INFO] 
[INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0

but we have to change junit4 assertion to:

outputValidator.assertThatLogLine(containsString("testOne passed"), is(1));

when going to 3.5.5 for junit5:

[INFO] Running org.apache.maven.surefire.its.JUnit5FailingAfterAllIT
[INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.617 s -- in org.apache.maven.surefire.its.JUnit5FailingAfterAllIT
[INFO] 
[INFO] Results:
[INFO] 
[INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0

when going to 3.5.4 for junit4:

[INFO] Running junit4.AlwaysFailingAfterClassTest
testOne passed
testTwo passed
[ERROR] Tests run: 3, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.001 s <<< FAILURE! -- in junit4.AlwaysFailingAfterClassTest
[ERROR] junit4.AlwaysFailingAfterClassTest -- Time elapsed: 0.001 s <<< ERROR!
java.lang.IllegalStateException: AfterClass always fails
	at junit4.AlwaysFailingAfterClassTest.tearDown(AlwaysFailingAfterClassTest.java:15)

when going to 3.5.4 for junit5:

[INFO] Running junit5.AlwaysFailingAfterAllTest
testOne passed
testTwo passed
[ERROR] Tests run: 3, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.003 s <<< FAILURE! -- in junit5.AlwaysFailingAfterAllTest
[ERROR] junit5.AlwaysFailingAfterAllTest -- Time elapsed: 0.003 s <<< ERROR!
java.lang.IllegalStateException: AfterAll always fails
	at junit5.AlwaysFailingAfterAllTest.tearDown(AlwaysFailingAfterAllTest.java:15)

so as far as i can tell 3.5.4 reported class teardown exceptions as errors and 3.5.5 stopped doing that.
this behavior change isnt really mentioned in the release notes:
https://github.com/apache/maven-surefire/releases/tag/surefire-3.5.5

but b4a53de caught my eye and i confirmed locally that if i check out 3.5.5 and revert that change, the reporting of teardown issues goes back to "normal" (i.e. 3.5.4 and before).

also only junit5 used to rerun test methods for rerunFailingTestsCount when class teardown fails - junit4 did not do that but does so now on master (i guess this is expected since master uses the common platform for execution?).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants