Add failOnFlakeCount option #333

oehme · 2021-01-28T11:55:26Z

To clarify https://issues.apache.org/jira/browse/SUREFIRE-1878 for further discussion.

maven-failsafe-plugin/src/main/java/org/apache/maven/plugin/failsafe/VerifyMojo.java

maven-surefire-plugin/src/main/java/org/apache/maven/plugin/surefire/SurefirePlugin.java

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

maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireHelper.java

Tibor17 · 2021-01-28T13:10:37Z

@oehme
Thx, that's nice but I have a technical problem I described to your colleague in the Slack. The problem is the ID of the runs. It's still the same and Surefire cannot modify it. That's the reason why the re-run works with same ID of test run. Once you set failOnFlakeCount we would have to identify these test runs as different ones. I want to say that these would become ordinal tests in the XML report.
The next issue with understanding failOnFlakeCount is the count as integer. I would understand to have boolean but the integer is strange because nobody has explained how to handle this number and how to mark these tests.

Tibor17 · 2021-01-28T13:17:26Z

@oehme
Do you want to have test set resolution for entire suite?
result.getFlakes() < reportParameters.getFailOnFlakeCount()
We understood the Jira issue that you want to have boolean related to individual Test and not the test set.
Pls clarify and change the description in Jira ticket as necessary.

oehme · 2021-01-28T13:22:00Z

It's about the whole test set. I've added a clarification to the issue description.

I don't understand the point about IDs. I'm not changing anything about test IDs, I'm just adding validation at the end of the execution, similar to failOnError.

Tibor17 · 2021-01-28T13:31:09Z

@oehme
The IDs because it was related to what I have said that our understanding of rerun was at the level of individual test. If we want to have only a cumulative counter then it is different story of course.

oehme · 2021-01-28T13:33:42Z

Got it, thanks!

So is this option something you'd consider adding? In that case I'd flesh out the PR some more.

maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireHelper.java

Tibor17 · 2021-01-28T14:05:13Z

@oehme
I think the class SurefireHelper.java has unit tests. Try to cover every change you made. Thx

oehme · 2021-01-29T12:18:57Z

I've clarified the documentation and added unit tests. I've removed the integration test, since this feature is not junit-platform specific, but fully contained in SurefireHelper.

Are there any other things I should add or modify?

l0rinc · 2021-01-29T12:23:08Z

maven-failsafe-plugin/src/main/java/org/apache/maven/plugin/failsafe/VerifyMojo.java

@@ -139,6 +139,13 @@
    @Parameter( property = "failIfNoTests" )
    private Boolean failIfNoTests;

+    /**
+     * Set this to a value greater than 0 to fail the whole test set if the cumulative number of flakes reaches
+     * this threshold. Set to 0 to allow an unlimited number of flakes.


based on the code a negative number would do the same, right?
Should we maybe rather fail in that case?

@oehme
Actually yes, we should and there is something like "verifyParameters" method.
Have you used the flag to serialize the content in the method getConfigChecksum?

l0rinc · 2021-01-29T12:24:23Z

maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireHelper.java

-        if ( firstForkException == null && !result.isTimeout() && result.isErrorFree() )
+        boolean isError = firstForkException != null || result.isTimeout() || !result.isErrorFree();
+        boolean isFlaky = reportParameters.getFailOnFlakeCount() > 0
+            && result.getFlakes() >= reportParameters.getFailOnFlakeCount();


minor: could we have getFailOnFlakeCount on the same side on both cases to simplify comparison?

@paplorinc
What you mean?

Just an observation that X > 0 && X < Y may be easier to read than X > 0 && Y >= X.
Don't have strong feelings about it, just a note

@paplorinc
I think result.getFlakes() >= reportParameters.getFailOnFlakeCount() is okay because it is a condition saying variable >= constant and we are "watching" variable and not the constant. In maths is the same as you say but it is another wording in the sentence.

l0rinc · 2021-01-29T12:25:00Z

maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireHelper.java

@@ -142,7 +142,10 @@ public static void reportExecution( SurefireReportParameters reportParameters, R
                                        PluginConsoleLogger log, Exception firstForkException )
        throws MojoFailureException, MojoExecutionException
    {
-        if ( firstForkException == null && !result.isTimeout() && result.isErrorFree() )
+        boolean isError = firstForkException != null || result.isTimeout() || !result.isErrorFree();
+        boolean isFlaky = reportParameters.getFailOnFlakeCount() > 0


this stores more than whether we're flaky, rather something like: isFlakyError, right?

minor i would say

l0rinc · 2021-01-29T12:27:52Z

maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/SurefireHelperTest.java

+                    + "[date].dump, [date]-jvmRun[N].dump and [date].dumpstream." );
+            return;
+        }
+        fail( "Expected MojoFailureException with message "


minor: you could add this inside the try in which case we wouldn't need a return in the catch

Tibor17 · 2021-01-29T13:05:09Z

maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireHelper.java

@@ -286,7 +289,15 @@ private static String createErrorMessage( SurefireReportParameters reportParamet
        }
        else
        {
-            msg.append( "There are test failures.\n\nPlease refer to " )
+            if ( result.getFailures() > 0 )


@oehme
Pls see the class IntegrationTestPlugin. It reports this text other way. The same messages should be there as well but the solution is different from this one.

The IntegrationTestMojo does not report this text. The VerifyMojo does and uses the same logic as the SurefirePlugin

@oehme
ok just fine!

Tibor17 · 2021-01-29T20:36:04Z

maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/SurefireHelperTest.java

+                    + "[date].dump, [date]-jvmRun[N].dump and [date].dumpstream." );
+            return;
+        }
+        fail( "Expected MojoFailureException with message "


@oehme
Pls move this line right after the line 168, and remove the return. It would behave the same, We normally do it this way.

Tibor17 · 2021-01-31T12:07:28Z

I would have used that normally, but all the other methods in this test didn't use it either, but instead used the pattern shown here. I generally always go with the pattern that some codebase already uses.

Happy to use ExpectedException. Should I also adjust the other existing methods in this test?

Pls first use ExpectedException for your two methods and then squash your commits into one with a name [SUREFIRE-1878] Add failOnFlakeCount option.
Then concentrate on the ExpectedException for the other methods and make extra commit.
+1 LGTM

oehme · 2021-02-01T13:39:26Z

All fixed up

Tibor17 · 2021-02-01T13:59:21Z

All fixed up

Thx. Waiting for the CI.

Tibor17 · 2021-02-01T19:15:32Z

maven-failsafe-plugin/src/main/java/org/apache/maven/plugin/failsafe/VerifyMojo.java

+     * Set this to a value greater than 0 to fail the whole test set if the cumulative number of flakes reaches
+     * this threshold. Set to 0 to allow an unlimited number of flakes.
+     */
+    @Parameter( property = "surefire.failOnFlakeCount", defaultValue = "0" )


Here the property surefire.failOnFlakeCount must become failsafe.failOnFlakeCount. See the other properties in this class.

Tibor17 · 2021-02-01T21:43:48Z

maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireHelper.java

+            }
+            else
+            {
+                msg.append( "There are too many flakes." );


@oehme
I think this would not be printeted very often even if it could because there is if-else.
We do not have IT test but both messages could be printed along. Why the message There are too many flakes. does not have the concrete numbers. We have both numbers available and they are the cause why the message is going to be printed. I only want to give the user all reason why the plugin crashed.

Done, both messages are reported now if both conditions are true. Also, the number of flakes and failOnFlakeCount are reported now.

@oehme
Thx, a good job!

Tibor17 · 2021-02-01T22:13:31Z

maven-surefire-plugin/src/main/java/org/apache/maven/plugin/surefire/SurefirePlugin.java

@@ -880,4 +900,20 @@ protected final ForkNodeFactory getForkNode()
    {
        return forkNode;
    }
+
+    @Override
+    boolean verifyParameters() throws MojoFailureException, MojoExecutionException


This must not exist in both Plugin classes. See the super method in AbstractSurefireMojo and add a new warn* method and a call which finally calls the abstract method getFailOnFlakeCount.

SurefirePlugin and VerifyMojo do not have a common base class.

I am not talking about VerifyMojo, nothing but the SurefirePlugin class.

But why would the super class contain logic that only applies to a single subclass?

Nevermind, I see the if-else in that method now. Moved call up to the base class.

But why would the super class contain logic that only applies to a single subclass?

Both plugins, Surefire and Failsafe, have 90% the same parameters. So they always contained the method verifyParameters() in the super class. The VerifyMojo is an exception and it has never had so many config params before.

Tibor17 · 2021-02-01T22:19:16Z

maven-surefire-plugin/src/main/java/org/apache/maven/plugin/surefire/SurefirePlugin.java

+    @Override
+    boolean verifyParameters() throws MojoFailureException, MojoExecutionException
+    {
+        if ( failOnFlakeCount < 0 )


As in my previous comment, this must be in the abstract class where are all the warning check method calls made in the section:

else { ensureEnableProcessChecker(); convertDeprecatedForkMode(); ensureWorkingDirectoryExists(); ensureParallelRunningCompatibility(); ensureThreadCountWithPerThread(); warnIfUselessUseSystemClassLoaderParameter(); warnIfDefunctGroupsCombinations(); warnIfRerunClashes(); warnIfWrongShutdownValue(); warnIfNotApplicableSkipAfterFailureCount(); warnIfIllegalTempDir(); warnIfForkCountIsZero(); printDefaultSeedIfNecessary(); }

See above, this wouldn't make sense because IntegrationTestMojo does not have failOnFlakeCount. That's in the VerifyMojo, which doesn't have the same base class.

@oehme
I am not talking about VerifyMojo, nothing but the SurefirePlugin class.

Tibor17 · 2021-02-03T02:09:25Z

@oehme
Do you have somewhere a backup of the initial IT test?

@eolivelli
I am glad of this code. WDYT, can we push it to master?

eolivelli

LGTM

sure, I like this feature

thank you @oehme

@Tibor17 can out please push to an ASF repo branch in order to see CI validating the patch ?

oehme · 2021-02-03T08:43:53Z

@Tibor17 I don't, but I can easily add one back if you want. I removed it because this feature isn't specific to a particular provider and all the logic could be unit tested, so I didn't feel like an extra integ test was necessary anymore.

eolivelli · 2021-02-03T08:59:29Z

@oehme I think there is not need to add the IT

@Tibor17 ?

Tibor17 · 2021-02-03T09:18:06Z

I also believe that it would be fine without IT but we should cover every config param by IT. It would be the highest guarantee that the client would not fire a bug against us that a fresh feature is dead ;-)

oehme · 2021-02-03T13:04:35Z

Added the IT back

Tibor17 · 2021-02-04T21:59:00Z

@oehme
@eolivelli
Thx for contributing!

Tibor17 · 2021-02-04T22:05:58Z

@oehme
@eolivelli
One more thing I have noticed which could be made better is to improve the condition in method warnIfIllegalFailOnFlakeCount() because this should fire an exception in a condition with setting rerunFailingTestsCount. The documentation of rerun should mention a new config param. This was also forgotten.

oehme · 2021-02-05T15:19:55Z

If you want I can send another PR, otherwise I'll let you take care of it.

Tibor17 · 2021-02-06T10:33:35Z

@oehme
Hi, that would be great, thank you.

oehme · 2021-02-08T09:40:00Z

I've looked at this and I think it would be inconsistent: Only the SurefirePlugin has both failOnFlakeCount and rerunFailingTestCount. For integration tests, the two options are split between IntegrationTestMojo and VerifyMojo. So my suggestion would be to leave the documentation/validation as-is. Unless I'm missing an option for cross-mojo validation/documentation.

Tibor17 · 2021-02-09T15:56:04Z

We have two plugins: surefire and failsafe.
The one you mentioned VerifyMojo is still the same failsafe plugin but a separate goal.
In normal practice to add one sentecnce to the existing documentation. Last time we added skipAfterFailureCount to the re-run. Here we should add a next chapter Re-run and fail the build upon flaky test count. That's all.

oehme · 2021-02-09T16:48:23Z

I was referring to the additional validation you wanted me to add - That's possible for surefire (both properties in the same Mojo), but as far as I can see not for failsafe (properties spread over two Mojos)

Tibor17 · 2021-02-09T17:29:46Z

@oehme
We have some old JIRA issue about improving the validations reported by @krosenvold . This can be reused and fixed separately, aside of this issue.

oehme marked this pull request as draft January 28, 2021 11:55

l0rinc approved these changes Jan 28, 2021

View reviewed changes

Tibor17 reviewed Jan 28, 2021

View reviewed changes

maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireHelper.java Outdated Show resolved Hide resolved

oehme force-pushed the fail-on-flake-count branch from 1ab3178 to d2fc069 Compare January 29, 2021 12:17

oehme marked this pull request as ready for review January 29, 2021 12:17

oehme changed the title ~~Prototype failOnFlakeCount option~~ Add failOnFlakeCount option Jan 29, 2021

l0rinc approved these changes Jan 29, 2021

View reviewed changes

Tibor17 reviewed Jan 29, 2021

View reviewed changes

oehme force-pushed the fail-on-flake-count branch from 24cfe0d to d60a67b Compare February 1, 2021 13:38

Tibor17 reviewed Feb 1, 2021

View reviewed changes

oehme force-pushed the fail-on-flake-count branch 2 times, most recently from e066d51 to 27a8f9e Compare February 2, 2021 10:58

eolivelli approved these changes Feb 3, 2021

View reviewed changes

oehme added 2 commits February 3, 2021 14:04

[SUREFIRE-1878] Add failOnFlakeCount option

81b11dc

Simplify SurefireHelperTest

9d1c640

oehme force-pushed the fail-on-flake-count branch from 27a8f9e to 9d1c640 Compare February 3, 2021 13:04

Tibor17 merged commit 1c2e739 into apache:master Feb 4, 2021

Add failOnFlakeCount option #333

Add failOnFlakeCount option #333

Conversation

oehme commented Jan 28, 2021

Tibor17 commented Jan 28, 2021

Tibor17 commented Jan 28, 2021 • edited Loading

oehme commented Jan 28, 2021 • edited Loading

Tibor17 commented Jan 28, 2021 • edited Loading

oehme commented Jan 28, 2021

Tibor17 commented Jan 28, 2021

oehme commented Jan 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l0rinc Jan 30, 2021 • edited Loading

Choose a reason for hiding this comment

Tibor17 Jan 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tibor17 commented Jan 31, 2021 • edited Loading

oehme commented Feb 1, 2021

Tibor17 commented Feb 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oehme Feb 2, 2021 • edited Loading

Choose a reason for hiding this comment

Tibor17 Feb 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tibor17 commented Feb 3, 2021

eolivelli left a comment

Choose a reason for hiding this comment

oehme commented Feb 3, 2021

eolivelli commented Feb 3, 2021

Tibor17 commented Feb 3, 2021

oehme commented Feb 3, 2021

Tibor17 commented Feb 4, 2021

Tibor17 commented Feb 4, 2021

oehme commented Feb 5, 2021

Tibor17 commented Feb 6, 2021

oehme commented Feb 8, 2021

Tibor17 commented Feb 9, 2021 • edited Loading

oehme commented Feb 9, 2021

Tibor17 commented Feb 9, 2021

Tibor17 commented Jan 28, 2021 •

edited

Loading

oehme commented Jan 28, 2021 •

edited

Loading

Tibor17 commented Jan 28, 2021 •

edited

Loading

l0rinc Jan 30, 2021 •

edited

Loading

Tibor17 Jan 31, 2021 •

edited

Loading

Tibor17 commented Jan 31, 2021 •

edited

Loading

oehme Feb 2, 2021 •

edited

Loading

Tibor17 Feb 3, 2021 •

edited

Loading

Tibor17 commented Feb 9, 2021 •

edited

Loading