[SUREFIRE-2276] - Fix for retries of JUnit TestTemplate tests #788

hubertgrzeskowiak · 2024-10-03T07:50:54Z

This PR fixes https://issues.apache.org/jira/browse/SUREFIRE-2276

The bug causes tests that use JUnit's @TestTemplate to be classified as flakes rather than failures with Surefire retries enabled when any of the invocations succeeds. Since flakes do not fail the run by default, this would cause false positives.

The test case testJupiterEngineWithTestTemplateNotClassifiedAsFlake shows a discrepancy in test results when reruns are enabled and fails without the fix from latest commit. The other test case testJupiterEngineWithParameterizedTestsNotClassifiedAsFlake was added for posterity - it worked before and continues do so with the fix. We added it since parameterized tests in JUnit use @TestTemplate under the hood.

Happy to update the docs, too, if somebody could point me in the right direction for this. It's a bug specific to surefire.rerunFailingTestsCount, so it may be helpful to mention it on the docs for that prop.

Following this checklist to help us incorporate your
contribution quickly and easily:

Make sure there is a JIRA issue filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
Each commit in the pull request should have a meaningful subject line and body.
Format the pull request title like [SUREFIRE-XXX] - Fixes bug in ApproximateQuantiles,
where you replace SUREFIRE-XXX with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the
commit message.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Run mvn clean install to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
You have run the integration tests successfully (mvn -Prun-its clean install).

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.

Our company, Atlassian, has a corporate CLA signed, and we added our team to it.
Contributors: Alex Courtis, Bing Xu, Dominik Franke and Hubert Grzeskowiak

hubertgrzeskowiak · 2024-10-09T02:37:48Z

Ah, damn. I did not consider that this project needs to be buildable with older Java and Junit... Fixing now

hubertgrzeskowiak · 2024-10-11T04:10:57Z

Sorry for the delay. I had some trouble with the animal-sniffer plugin locally, but it seems to be working now. Would you mind re-running CI?

hubertgrzeskowiak · 2024-10-14T00:44:32Z

This is ready to merge from my side :)

fxbonnet · 2024-10-14T05:45:09Z

...junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/RunListenerAdapter.java

            boolean hasParameterizedParent = collectAllTestIdentifiersInHierarchy(testIdentifier)
                    .filter(identifier -> !identifier.getSource().isPresent())
                    .map(TestIdentifier::getLegacyReportingName)
                    .anyMatch(legacyReportingName -> legacyReportingName.matches("^\\[.+]$"));
+            boolean isTestTemplate = testIdentifier.getLegacyReportingName().matches("^.*\\[\\d+]$");


Does this work only for numeric parameters?

Thanks for the good question!

A test template function gets invoked within a loop (stream to be precise), once for each invocation context. This legacy name contains the index of the invocation and is unrelated to any params the method actually receives, and even unrelated to the display name we set in the invocation context.

I just also double checked by changing the integers used in the test fixtures to strings, and the tests pass just fine.

michael-o · 2024-10-29T09:46:13Z

Does this still hold true after your change: https://maven.apache.org/surefire/maven-surefire-plugin/examples/rerun-failing-tests.html?

michael-o

Please consider my previous comment and rebase. I cannot push to your fork.

hubertgrzeskowiak · 2024-10-30T00:20:55Z

Does this still hold true after your change: https://maven.apache.org/surefire/maven-surefire-plugin/examples/rerun-failing-tests.html?

Here's an abbreviated surefire report XML:

<testsuite ...>
  ...
  <testcase name="testTemplatePartiallyFails()[1]" classname="pkg.FieldSettingTest" time="0.012">
    <failure message="expected: &lt;42&gt; but was: &lt;0&gt;" type="org.opentest4j.AssertionFailedError">
    ...
    </failure>
    <rerunFailure message="expected: &lt;42&gt; but was: &lt;0&gt;" type="org.opentest4j.AssertionFailedError">
    ...
    </rerunFailure>
  </testcase>
  <testcase name="testTemplatePartiallyFails()[2]" classname="pkg.FieldSettingTest" time="0.001"/>
</testsuite>

And here's a snippet from the logs:

[INFO] Results:
[INFO] 
[ERROR] Failures: 
[ERROR] pkg.FieldSettingTest.testTemplatePartiallyFails()[1]
[ERROR]   Run 1: FieldSettingTest.testTemplatePartiallyFails:28 expected: <42> but was: <0>
[ERROR]   Run 2: FieldSettingTest.testTemplatePartiallyFails:28 expected: <42> but was: <0>
[INFO] 
[INFO] 
[ERROR] Tests run: 2, Failures: 1, Errors: 0, Skipped: 0

As intended, there is no mention of flakes.

hubertgrzeskowiak · 2024-10-30T00:21:12Z

Merged upstream master into our branch too.

@michael-o Please have another look

michael-o · 2024-10-30T08:12:41Z

Does this still hold true after your change: https://maven.apache.org/surefire/maven-surefire-plugin/examples/rerun-failing-tests.html?

Here's an abbreviated surefire report XML:

<testsuite ...>
  ...
  <testcase name="testTemplatePartiallyFails()[1]" classname="pkg.FieldSettingTest" time="0.012">
    <failure message="expected: &lt;42&gt; but was: &lt;0&gt;" type="org.opentest4j.AssertionFailedError">
    ...
    </failure>
    <rerunFailure message="expected: &lt;42&gt; but was: &lt;0&gt;" type="org.opentest4j.AssertionFailedError">
    ...
    </rerunFailure>
  </testcase>
  <testcase name="testTemplatePartiallyFails()[2]" classname="pkg.FieldSettingTest" time="0.001"/>
</testsuite>

And here's a snippet from the logs:

[INFO] Results:
[INFO] 
[ERROR] Failures: 
[ERROR] pkg.FieldSettingTest.testTemplatePartiallyFails()[1]
[ERROR]   Run 1: FieldSettingTest.testTemplatePartiallyFails:28 expected: <42> but was: <0>
[ERROR]   Run 2: FieldSettingTest.testTemplatePartiallyFails:28 expected: <42> but was: <0>
[INFO] 
[INFO] 
[ERROR] Tests run: 2, Failures: 1, Errors: 0, Skipped: 0

As intended, there is no mention of flakes.

That's not what I meant. Rephrasing: After the change from your has been applied, is the information on that valid still valid and true or does it require modification? In other words: The page describes the ideal behavior and the code was wrong previously because it did not cover this specific case?

hubertgrzeskowiak · 2024-10-30T08:32:17Z

@michael-o Ah, now I get you. In short: yes, the page is still correct.

The page does not mention the specific case of test templates, but only uses the general term "test". With the change from this PR, we treat each invocation context on a test template method as its own test. This is intentional as the test template (as the name suggests) is only a template for tests and not a test in itself. Retries apply to that invocation and should work correctly with this change applied.

michael-o · 2024-10-30T08:37:54Z

Running tests now...

hubertgrzeskowiak · 2024-10-31T02:27:19Z

@michael-o It looks like the PR was closed without a merge. Was that intentional or a mis-click?

hubertgrzeskowiak · 2024-10-31T02:34:22Z

Oh wait, I can see it on master. Looks like this repo is just using a funny merge strategy..?
Anyways, thanks @michael-o !

michael-o · 2024-10-31T07:00:50Z

Oh wait, I can see it on master. Looks like this repo is just using a funny merge strategy..? Anyways, thanks @michael-o !

We have a clean FF merge policy. No intermediate junk.

hubertgrzeskowiak added 3 commits September 26, 2024 21:27

Sample code for Junit5's TestContext reporting bug

5f3d378

Parametrised pom and failing test case

01faf7f

Fix for test template retries. Also added test for parametrised tests

8aaf0a6

Fix for compilation on Java 8 and older Junit versions

4f52c2d

fxbonnet reviewed Oct 14, 2024

View reviewed changes

michael-o requested changes Oct 29, 2024

View reviewed changes

Merge remote-tracking branch 'github/master'

4cb82fe

hubertgrzeskowiak requested a review from michael-o October 30, 2024 00:24

michael-o approved these changes Oct 30, 2024

View reviewed changes

michael-o closed this in e1f94a0 Oct 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SUREFIRE-2276] - Fix for retries of JUnit TestTemplate tests #788

[SUREFIRE-2276] - Fix for retries of JUnit TestTemplate tests #788

hubertgrzeskowiak commented Oct 3, 2024 •

edited

Loading

hubertgrzeskowiak commented Oct 9, 2024

hubertgrzeskowiak commented Oct 11, 2024

hubertgrzeskowiak commented Oct 14, 2024

fxbonnet Oct 14, 2024

hubertgrzeskowiak Oct 14, 2024

michael-o commented Oct 29, 2024

michael-o left a comment

hubertgrzeskowiak commented Oct 30, 2024 •

edited

Loading

hubertgrzeskowiak commented Oct 30, 2024 •

edited

Loading

michael-o commented Oct 30, 2024

hubertgrzeskowiak commented Oct 30, 2024

michael-o commented Oct 30, 2024

hubertgrzeskowiak commented Oct 31, 2024

hubertgrzeskowiak commented Oct 31, 2024 •

edited

Loading

michael-o commented Oct 31, 2024

[SUREFIRE-2276] - Fix for retries of JUnit TestTemplate tests #788

[SUREFIRE-2276] - Fix for retries of JUnit TestTemplate tests #788

Conversation

hubertgrzeskowiak commented Oct 3, 2024 • edited Loading

hubertgrzeskowiak commented Oct 9, 2024

hubertgrzeskowiak commented Oct 11, 2024

hubertgrzeskowiak commented Oct 14, 2024

fxbonnet Oct 14, 2024

Choose a reason for hiding this comment

hubertgrzeskowiak Oct 14, 2024

Choose a reason for hiding this comment

michael-o commented Oct 29, 2024

michael-o left a comment

Choose a reason for hiding this comment

hubertgrzeskowiak commented Oct 30, 2024 • edited Loading

hubertgrzeskowiak commented Oct 30, 2024 • edited Loading

michael-o commented Oct 30, 2024

hubertgrzeskowiak commented Oct 30, 2024

michael-o commented Oct 30, 2024

hubertgrzeskowiak commented Oct 31, 2024

hubertgrzeskowiak commented Oct 31, 2024 • edited Loading

michael-o commented Oct 31, 2024

hubertgrzeskowiak commented Oct 3, 2024 •

edited

Loading

hubertgrzeskowiak commented Oct 30, 2024 •

edited

Loading

hubertgrzeskowiak commented Oct 30, 2024 •

edited

Loading

hubertgrzeskowiak commented Oct 31, 2024 •

edited

Loading