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

[hubertgrzeskowiak]

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:

• [x] 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.
• [x] Each commit in the pull request should have a meaningful subject line and body.
• [x] 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.
• [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• [x] Run mvn clean install to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
• [x] 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.

• [x] I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004

• [x] 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]

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

[hubertgrzeskowiak]

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]

This is ready to merge from my side :)

[michael-o]

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

[hubertgrzeskowiak]

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]

Merged upstream master into our branch too.

@michael-o Please have another look

[michael-o]

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]

@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]

Running tests now...

[hubertgrzeskowiak]

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

[hubertgrzeskowiak]

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]

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.

[jira-importer]

Resolve #2668