Surefire-1584: Add option to rerun failing tests for JUnit5 #245

jon-bell · 2019-08-19T15:14:21Z

Hi,
@Col-E and I have put together a patch (with test) to support rerunFailingTestCount for JunitPlatformProvider. Please let us know if you have any questions or need any edits.

eolivelli

@sormuras this is your realm :)

Tibor17 · 2019-08-19T16:43:09Z

@jon-bell
I don't like this huge number of commits and merge commits.
Can you make only one commit on the top of master HEAD?
There are two authors. Did you pull some commits also from other pull requests?
Important: pls run the build mvn install -P run-its and setup your IDEA or Eclipse to the code style (there are config files for Ides) https://maven.apache.org/developers/conventions/code.html
Do not change white spaces and code style against origin/master. Only add/remove/modify code.

jon-bell · 2019-08-20T01:25:52Z

@Tibor17 - This was prepared by my student and I - that's why you see both of our names.

As you requested, I have rebased all of the changes into a single commit. I confirm that mvn install -P run-its passes.

All of the changed/added files are formatted as-per the Maven/IntelliJ code formatter.

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

Tibor17 · 2019-08-20T09:48:14Z

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

@@ -259,4 +261,50 @@ else if ( testSource.filter( ClassSource.class::isInstance ).isPresent() )
            return new String[] { source, source, display, display };
        }
    }
+
+    public String[] toClassMethodNameWithoutPlan( TestIdentifier testIdentifier )


I am not totally convinced about this method because there is already one which returns String[4]. I would vote for a generic approach avoiding code duplication in single class.

Are you referring to toClassMethodName? That couldn't be used because at the time of requesting class names for reruns the plan is set to null. Perhaps adding a null check in the original method that provides the same logic as what's implemented in this extra method would suffice? Or were you thinking of a different approach?

@Tibor17 - We've been trying hard to come up with a work around so that the existing toClassMethodName can be used for both cases, but are stumped with an approach that would be more generic. Do you have any suggestions?

...it-platform/src/main/java/org/apache/maven/surefire/junitplatform/JUnitPlatformProvider.java

surefire-its/src/test/java/org/apache/maven/surefire/its/JUnitPlatformRerunFailingTestsIT.java

Tibor17 · 2019-08-20T10:00:44Z

@jon-bell
Generally, it looks good. I have left only few comments.
Pls open the MOJO classes. You will see rerunFailingTestsCount. Extend the JavaDoc and add a new support for JUnit5 with all known limitations if any exist.

surefire-its/src/test/java/org/apache/maven/surefire/its/JUnitPlatformRerunFailingTestsIT.java

Col-E · 2019-08-20T18:01:32Z

Pls open the MOJO classes. You will see rerunFailingTestsCount. Extend the JavaDoc and add a new support for JUnit5 with all known limitations if any exist.

I found docs for rerunFailingTestsCount in SurefirePlugin.java and IntegrationTestMojo.java where it states: (JUnit 4+ providers)
Would changing this to (JUnit 4+ providers & JUnit 5+ providers) be acceptable?

Tibor17 · 2019-08-20T18:22:15Z

Pls open the MOJO classes. You will see rerunFailingTestsCount. Extend the JavaDoc and add a new support for JUnit5 with all known limitations if any exist.

I found docs for rerunFailingTestsCount in SurefirePlugin.java and IntegrationTestMojo.java where it states: (JUnit 4+ providers)
Would changing this to (JUnit 4+ providers & JUnit 5+ providers) be acceptable?

We of course have to mention JUnit 5 in JavaDoc. You can see the practice in another parameters that we say Since version 3.0.0-M4 ....

Tibor17 · 2019-08-20T18:26:38Z

@Col-E Additionally, we have to update the chart maven-surefire-plugin/src/site/apt/examples/featurematrix.apt.vm and update the line re-run count and type Y with a new remark *3.

Col-E · 2019-08-20T18:36:39Z

We of course have to mention JUnit 5 in JavaDoc. You can see the practice in another parameters that we say Since version 3.0.0-M4 ....

So (JUnit 4+ providers and JUnit 5+ providers since 3.0.0-M4) would be more appropriate then?

Additionally, we have to update the chart maven-surefire-plugin/src/site/apt/examples/featurematrix.apt.vm and update the line re-run count and type Y with a new remark *3.

Is the remark for clarifying support as of 3.0.0-M4?

Tibor17 · 2019-08-20T18:40:15Z

(JUnit 4+ providers and JUnit 5+ providers since 3.0.0-M4)
Is the remark for clarifying support as of 3.0.0-M4?

yes, for both, that's ok and enough as you have mentioned.
The reason is that the users sometimes are not aware that it is not the version they use in the company but the latest with a praticular feature.

Col-E · 2019-08-20T18:42:34Z

Ok, all suggestions have been addressed except for the usage of toClassMethodNameWithoutPlan, which I responded to above. How would you like this to be addressed?

Tibor17 · 2019-08-20T18:44:38Z

The diff in Github is not enough for me, so I have to see the entire class for toClassMethodNameWithoutPlan and think about it.

maven-surefire-plugin/src/site/apt/featurematrix.apt.vm

surefire-its/src/test/java/org/apache/maven/surefire/its/JUnitPlatformRerunFailingTestsIT.java

jon-bell · 2019-09-10T17:43:00Z

@Tibor17 do you have any thoughts about refactoring toClassMethdoNameWithoutPlan?

Tibor17 · 2019-09-10T18:07:08Z

@jon-bell
@Col-E
Thx for reply. I am busy this week and my colleagues have been busy too.
I will try to have a look on the method perhaps today late evening.
This issue is assigned to the release. Don't worry we will not loose it.

Micky002 · 2019-10-01T10:39:47Z

Are there any plans when this will be merged an available?

Tibor17 · 2019-10-01T11:16:10Z

@jon-bell
@Col-E
Sorry for the delay. Let's continue, guys!

So I have compared these two methods, namely:
private String[] toClassMethodName( TestIdentifier testIdentifier )
public String[] toClassMethodNameWithoutPlan( TestIdentifier testIdentifier )

The solution is very simple: add a new boolean parameter in the old method "toClassMethodName".
The above methods are cca 90% similar and that breaks the reusability.
Our problem is only this diff:

(old toClassMethodName)
String[] source = testPlan.getParent( testIdentifier )
     .map( this::toClassMethodName )
     .map( s -> new String[] { s[0], s[1] } )
     .orElse( new String[] { realClassName, realClassName } );

with

(new method toClassMethodNameWithoutPlan)
String[] source = new String[] {realClassName, realClassName};

Tibor17 · 2019-10-01T11:31:36Z

One can see that the method must alter the behavior . In practice we do it easily the way that we have algorithm in one method toClassMethodName(TestIdentifier, boolean) because of 90% reusability called in the accessor methods:

private toClassMethodName(TestIdentifier )
{
    return toClassMethodName( testIdentifier, false );
}

and another public method:

toClassMethodNameWithoutPlan( testIdentifier ) 
{
    return toClassMethodName( testIdentifier, true );
}

In our case toClassMethodName(TestIdentifier, boolean) is the old method with new parameter - boolean. Pls check if the difference between those two methods is what I used in the previous comment because I was too fast to compare in detail. Thx

true: real class/method names without plan
false: with display names if possible

Tibor17 · 2019-10-01T12:00:41Z

@Col-E
thx, the build is running https://builds.apache.org/job/maven-box/job/maven-surefire/job/SUREFIRE-1584/

Col-E · 2019-10-01T12:11:52Z

Pls check if the difference between those two methods is what I used in the previous comment because I was too fast to compare in detail. Thx

I noticed that:

 source = testPlan.getParent( testIdentifier )
          .map( this::toClassMethodName )

should probably be:

 source = testPlan.getParent( testIdentifier )
          .map( i -> toClassMethodName( i, usePlan ) )

I think that should be it for any possible regressions. Fixed in 9886eb5.

Tibor17 · 2019-10-01T13:42:03Z

@Col-E
These are our new test? https://builds.apache.org/job/maven-box/job/maven-surefire/job/SUREFIRE-1584/1/testReport/
There are 4 failed tests.

Col-E · 2019-10-01T15:44:54Z

See: this discussion for the new commit's reasoning. It resolves the ITS failures.

Tibor17 · 2019-10-18T19:28:02Z

@quiram
If I understood the bug right in our code, it should be possible to reproduce it even without Spring Boot. You are trying to use the original commercial project and then you are reducing it somehow?
Take your time, no worries, I will wait because this issue is important for reliability of this feature.

quiram · 2019-10-21T12:05:46Z

I have managed to create a sample project to reproduce this, it's as simple as using @DisplayName, see here. The sample project uses surefire to demonstrate the issue because it requieres less set-up, but the same issue occurs when I try with failsafe.

Since @Col-E will need a new PR, shall I create a new issue to handle this?

Tibor17 · 2019-10-21T12:51:17Z

@quiram
thx a lot.
pls create a new issue on Jira.

Tibor17 · 2019-10-21T12:55:24Z

@quiram
[this line means that every test should fail|https://github.com/quiram/surefire-retry-failed-tests-issue/blob/master/src/test/java/com/github/quiram/SurefireIssueNoRetryTest.java#L13]

quiram · 2019-10-21T14:13:27Z

I've created https://issues.apache.org/jira/browse/SUREFIRE-1701

Also, I fixed the code so the tests will pass as soon as the functionality is fixed (the previous version simply showed the difference in retries as part of the output, but all tests failed nonetheless).

Col-E · 2019-10-21T18:19:04Z

This is actually a one character fix 🤦‍♂️

JUnitPlatformProvider - Line 195

When writing buildLauncherDiscoveryRequestForRerunFailures I called String[] toClassMethodNameWithoutPlan() to fetch the method name source[3], but should be using source[2] instead.

source[3] is the resolved method name (Which uses @DisplayName's value)
source[2] is the actual method name.

Changing 3 to 2 is all that is necessary.

Tibor17 · 2019-10-21T19:33:15Z

@Col-E
How you see my suspicion written in #245 (comment)

Tibor17 · 2019-10-21T19:40:22Z

@Col-E
you want to change 3 to 2?
Should not it be either:
[0, 2] or
[1, 3]
?

Col-E · 2019-10-21T20:07:10Z

I've seen your post and it didn't change behavior in this case, but I think it would be wise to implement just to be safe.

And yes, [0, 2] can also work. When adding @DisplayName to a class using 0 or 1 both seemed to have the same behavior.

quiram · 2019-10-22T10:27:02Z

This is actually a one character fix 🤦‍♂

JUnitPlatformProvider - Line 195

When writing buildLauncherDiscoveryRequestForRerunFailures I called String[] toClassMethodNameWithoutPlan() to fetch the method name source[3], but should be using source[2] instead.

source[3] is the resolved method name (Which uses @DisplayName's value)

source[2] is the actual method name.

Changing 3 to 2 is all that is necessary.

I wish all fixes were as easy as this 😄. I'm glad we got to the bottom of it.

Seijan · 2019-10-30T12:14:55Z

Did you check how this feature works with parameterized tests from JUnit5? I know it's an experimental feature, but we use it quite a lot in our environment. As off now I tried it myself with the latest snapshot version and with a few simple tests, with the results that parameterized tests aren't rerun when they fail, and if one of the tests does succeed the whole test case is marked as a success. This does defy the purpose of the parameterized tests, as they test different behaviours in our case.

Tibor17 · 2019-10-30T15:24:26Z

@Seijan
I agree with you that parameterized tests are very important.
Would you write an integration test in our project and show us how this feature works together with the parameterized tests? If we see the test result, we can better understand the problem and make a fix. Maybe worth to start with parameterized test first without this feature and then continue with second IT having this feature. And we can see the difference.

Tibor17 · 2019-10-30T15:30:24Z

@Col-E @Seijan
IMO the problem would be in the filter. Maybe the name of test method is myTest[0] instead of myTest. My guess. Having the IT would show us the root cause in debugger.

Tibor17 · 2019-10-31T20:21:44Z

@Seijan
@marcphilipp

I have a trivial test and I have several observations with ParameterizedTest.

@ParameterizedTest
@ValueSource(strings = { "Hello" })
void test(String world) {
    if (i++ == 0) {
        throw new RuntimeException();
    }
}

This discoveryRequest does not find our test to run which is this line in our code:
launcher.execute( discoveryRequest, adapter )
It looks like this in debugger. Does it have good values?

The ID of the method run is this:
[engine:junit-jupiter]/[class:pkg.ParamTest]/[test-template:test(java.lang.String)]/[test-template-invocation:#1]

Should we filter the method in the request like we do it in JUnit4 and use test[1] instead of test?

Tibor17 · 2019-10-31T20:32:42Z

@Seijan
If I use two parameters, I always have the same description on the method. So we cannot recognize which run it is and the logic guessed that it was already the rerun. That's the problem.
Instead using [engine:junit-jupiter]/[class:pkg.ParamTest]/[test-template:test(java.lang.String)]/[test-template-invocation:#1] for the method name solves the unique identity of the runs but it does not look so well in the report.
And the next issue I found with @ParameterizedTest is this the method getDisplayName() which returns Hello [1] but it is the method parameter and no display name. I do not know why it does not return null if I have no annotation @DisplayName.

marcphilipp · 2019-11-01T12:53:28Z

@Tibor17 Your MethodSelector is missing the method's parameter types, i.e. there's no test() method, only a test(java.lang.String) one.

For this use case, I'd recommend using a UniqueIdSelector, though. To rerun only the first invocation of the method that would be DiscoverySelectors.selectUniqueId("[engine:junit-jupiter]/[class:pkg.ParamTest]/[test-template:test(java.lang.String)]/[test-template-invocation:#1]"). To rerun all invocations, it would be DiscoverySelectors.selectUniqueId("[engine:junit-jupiter]/[class:pkg.ParamTest]/[test-template:test(java.lang.String)]") which is equivalent to DiscoverySelectors.selectMethod("pkg.ParamTest.test(java.lang.String)").

Tibor17 · 2019-11-01T13:12:42Z

@Col-E
@Seijan
@marcphilipp
What would be the best text representation of each run of param.test?
Although the code has only one Java method, we should represent each combination of parameters as if it was a separate method.
So we know that the class name is pkg.MyTest, method name is called test and the parameter is Hello [1]. And my question is what is the best description of the method in the report.
What a string.

Tibor17 · 2019-11-01T13:18:54Z

@Col-E
Can you pls rework 4f16fcd#diff-b3a2904e92be87f11f2622fbfd122e31R198 and use UniqueId as @marcphilipp described in #245 (comment) ? It will be generic fix for all types of tests including the re-run.

marcphilipp · 2019-11-01T14:56:30Z

What would be the best text representation of each run of param.test?
Although the code has only one Java method, we should represent each combination of parameters as if it was a separate method.
So we know that the class name is pkg.MyTest, method name is called test and the parameter is Hello [1]. And my question is what is the best description of the method in the report.
What a string.

Just to be sure, we're on the same page, the test tree looks like this:

JUnit Jupiter
- pkg.MyTest
  - test(java.lang.String)
    - [1] Hello
    - ...

If you're looking for a technical name for [1] Hello you can call getLegacyReportingName() on the TestIdentifier of the invocation (leaf in the above tree). It will return test(java.lang.String)[1].

Tibor17 · 2019-11-01T15:03:33Z

@Col-E
From @marcphilipp answer #245 (comment) it means that getLegacyReportingName() should be a real fix for the rerun on @ParameterizedTest. Maybe we should open a new ticket and @Seijan can provide us with the ralistic integration test.

Seijan · 2019-11-01T18:27:09Z

@Tibor17
I created the gist just now, maybe it helps even further to understand the problem. https://gist.github.com/Seijan/a0c1900e2ba6dcc6642ea81f1ac6f065
Regarding the best representation, I don't have a solution for it.

Col-E · 2019-11-01T20:22:34Z

@Tibor17 Applied the changes using UniqueId and verified all tests in JUnitPlatformProviderTest pass including the new one created by @Seijan

What title do you want for the PR?

Tibor17 · 2019-11-01T20:39:48Z

@Col-E
It is fix for this PR. It is new code. It was not before, right?
So we have to report issue and make new PR the same way as we did it before.

And then the next issue would go with ParmeterizedTest only and not rerun saying that the Parameterize test should have different name for every combination of parameters. If we use getLegacyReportingName() instead of pure method name in the String[2] then integration test will pass. I mean this test from @Seijan was not IT. It was unit test but we need to have real IT and we have to check the XML report as well as the number of calls of the same method and we do not have to employ the rerun because it was fixed previously by you.

Col-E · 2019-11-01T21:02:59Z

It is fix for this PR. It is new code. It was not before, right?

Yes it is a fix for this PR. I would not call this "new code" though if we are talking in terms of code coverage.

So we have to report issue and make new PR the same way as we did it before.

I agree.

If we use getLegacyReportingName() instead of pure method name in the String[2] then integration test will pass

I do not understand why the legacy name should be used over UniqueId as suggested by #245 (comment)

We can even roll back the changes to RunListenerAdapter

Tibor17 · 2019-11-01T22:15:17Z

@Col-E
I think they call it "legacy" in junit5 becuse it's not related to legacy version of JUnit5 but the description of methods might be similar to the JUnit4. See the JavaDoc. But here I also did not see the returned value. It would be first interesting to debug it, put the breakpoint and see the string. Feel free to show the string how it looks like before we make a real change.

Tibor17 · 2019-11-01T22:20:48Z

@Col-E
Regarding the change in String[2] we talked about before, we have to be careful and run the integration tests for JUnit5. This means *Platform*IT and see if we broke something.

Col-E · 2019-11-03T22:47:16Z

@Tibor17 @Seijan

I'd like to take discussion over @ParameterizedTest over to a new PR that specifically deals with the problem: #252

eolivelli reviewed Aug 19, 2019

View reviewed changes

jon-bell force-pushed the SUREFIRE-1584 branch 2 times, most recently from 7406a95 to 2558e28 Compare August 20, 2019 01:25

Tibor17 reviewed Aug 20, 2019

View reviewed changes

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

Tibor17 reviewed Aug 20, 2019

View reviewed changes

...it-platform/src/main/java/org/apache/maven/surefire/junitplatform/JUnitPlatformProvider.java Outdated Show resolved Hide resolved

Tibor17 reviewed Aug 20, 2019

View reviewed changes

surefire-its/src/test/java/org/apache/maven/surefire/its/JUnitPlatformRerunFailingTestsIT.java Outdated Show resolved Hide resolved

Tibor17 reviewed Aug 20, 2019

View reviewed changes

surefire-its/src/test/java/org/apache/maven/surefire/its/JUnitPlatformRerunFailingTestsIT.java Outdated Show resolved Hide resolved

Tibor17 reviewed Aug 21, 2019

View reviewed changes

maven-surefire-plugin/src/site/apt/featurematrix.apt.vm Outdated Show resolved Hide resolved

Tibor17 reviewed Aug 21, 2019

View reviewed changes

surefire-its/src/test/java/org/apache/maven/surefire/its/JUnitPlatformRerunFailingTestsIT.java Show resolved Hide resolved

Tibor17 reviewed Aug 21, 2019

View reviewed changes

surefire-its/src/test/java/org/apache/maven/surefire/its/JUnitPlatformRerunFailingTestsIT.java Outdated Show resolved Hide resolved

Col-E mentioned this pull request Oct 22, 2019

Surefire-1701: Fix @DisplayName breaking reruns #249

Merged

Seijan mentioned this pull request Oct 30, 2019

Introduce support for retrying failed flaky tests junit-team/junit5#1558

Open

Col-E mentioned this pull request Nov 3, 2019

[SUREFIRE-1711] Support @ParameterizedTest for JUnit 5 test reruns #252

Merged

Surefire-1584: Add option to rerun failing tests for JUnit5 #245

Surefire-1584: Add option to rerun failing tests for JUnit5 #245

Conversation

jon-bell commented Aug 19, 2019

eolivelli left a comment • edited Loading

Choose a reason for hiding this comment

Tibor17 commented Aug 19, 2019 • edited Loading

jon-bell commented Aug 20, 2019

Tibor17 Aug 20, 2019

Choose a reason for hiding this comment

Col-E Aug 20, 2019 • edited Loading

Choose a reason for hiding this comment

jon-bell Aug 27, 2019

Choose a reason for hiding this comment

Tibor17 commented Aug 20, 2019 • edited Loading

Col-E commented Aug 20, 2019

Tibor17 commented Aug 20, 2019

Tibor17 commented Aug 20, 2019 • edited Loading

Col-E commented Aug 20, 2019

Tibor17 commented Aug 20, 2019 • edited Loading

Col-E commented Aug 20, 2019

Tibor17 commented Aug 20, 2019

jon-bell commented Sep 10, 2019

Tibor17 commented Sep 10, 2019

Micky002 commented Oct 1, 2019

Tibor17 commented Oct 1, 2019 • edited Loading

Tibor17 commented Oct 1, 2019 • edited Loading

Tibor17 commented Oct 1, 2019

Col-E commented Oct 1, 2019 • edited Loading

Tibor17 commented Oct 1, 2019

Col-E commented Oct 1, 2019

Tibor17 commented Oct 18, 2019

quiram commented Oct 21, 2019 • edited Loading

Tibor17 commented Oct 21, 2019

Tibor17 commented Oct 21, 2019

quiram commented Oct 21, 2019

Col-E commented Oct 21, 2019

Tibor17 commented Oct 21, 2019

Tibor17 commented Oct 21, 2019

Col-E commented Oct 21, 2019

quiram commented Oct 22, 2019

Seijan commented Oct 30, 2019

Tibor17 commented Oct 30, 2019

Tibor17 commented Oct 30, 2019

Tibor17 commented Oct 31, 2019 • edited Loading

Tibor17 commented Oct 31, 2019

marcphilipp commented Nov 1, 2019

Tibor17 commented Nov 1, 2019 • edited Loading

Tibor17 commented Nov 1, 2019

marcphilipp commented Nov 1, 2019

Tibor17 commented Nov 1, 2019

Seijan commented Nov 1, 2019

Col-E commented Nov 1, 2019

Tibor17 commented Nov 1, 2019 • edited Loading

Col-E commented Nov 1, 2019 • edited Loading

Tibor17 commented Nov 1, 2019 • edited Loading

Tibor17 commented Nov 1, 2019 • edited Loading

Col-E commented Nov 3, 2019

eolivelli left a comment •

edited

Loading

Tibor17 commented Aug 19, 2019 •

edited

Loading

Col-E Aug 20, 2019 •

edited

Loading

Tibor17 commented Aug 20, 2019 •

edited

Loading

Tibor17 commented Aug 20, 2019 •

edited

Loading

Tibor17 commented Aug 20, 2019 •

edited

Loading

Tibor17 commented Oct 1, 2019 •

edited

Loading

Tibor17 commented Oct 1, 2019 •

edited

Loading

Col-E commented Oct 1, 2019 •

edited

Loading

quiram commented Oct 21, 2019 •

edited

Loading

Tibor17 commented Oct 31, 2019 •

edited

Loading

Tibor17 commented Nov 1, 2019 •

edited

Loading

Tibor17 commented Nov 1, 2019 •

edited

Loading

Col-E commented Nov 1, 2019 •

edited

Loading

Tibor17 commented Nov 1, 2019 •

edited

Loading

Tibor17 commented Nov 1, 2019 •

edited

Loading