You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2021/01/28 11:55:39 UTC
[GitHub] [maven-surefire] oehme opened a new pull request #333: Prototype failOnFlakeCount option
oehme opened a new pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333
To clarify https://issues.apache.org/jira/browse/SUREFIRE-1878 for further discussion.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] paplorinc commented on a change in pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
paplorinc commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r567250891
##########
File path: 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
+ && result.getFlakes() >= reportParameters.getFailOnFlakeCount();
Review comment:
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 nte
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r567075080
##########
File path: 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
+ && result.getFlakes() >= reportParameters.getFailOnFlakeCount();
Review comment:
@paplorinc
What you mean?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#issuecomment-773630782
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r568487323
##########
File path: 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
Review comment:
I am not talking about VerifyMojo, nothing but the SurefirePlugin class.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#issuecomment-772358615
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 ;-)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r567414679
##########
File path: 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
+ && result.getFlakes() >= reportParameters.getFailOnFlakeCount();
Review comment:
@paplorinc
I think `result.getFlakes() >= reportParameters.getFailOnFlakeCount()` is okay because it is a condition saying `variable < constant`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r566806966
##########
File path: 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 )
Review comment:
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #333: Prototype failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r566089270
##########
File path: maven-failsafe-plugin/src/main/java/org/apache/maven/plugin/failsafe/VerifyMojo.java
##########
@@ -139,6 +139,12 @@
@Parameter( property = "failIfNoTests" )
private Boolean failIfNoTests;
+ /**
+ * The number of flakes after which the overall test suite will be considered a failure.
+ */
+ @Parameter( property = "surefire.failOnFlakeCount", defaultValue = "0" )
+ private int failOnFlakeCount;
Review comment:
Exactly this was not clear to me when we spoke with the JUnit5 colleagues several days ago.
The logic of `re-run` feature operates at the level of individual Test.
In this PR I guess we are talking about the level of Test Set (cumulative number of flakes) which is different view from ours. But's that's ok if it would be properly clarified.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 edited a comment on pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 edited a comment on pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#issuecomment-770372094
>
>
> 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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] oehme commented on pull request #333: Prototype failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
oehme commented on pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#issuecomment-769048635
It's about the whole test set. 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] paplorinc commented on a change in pull request #333: Prototype failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
paplorinc commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r566056297
##########
File path: maven-failsafe-plugin/src/main/java/org/apache/maven/plugin/failsafe/VerifyMojo.java
##########
@@ -139,6 +139,12 @@
@Parameter( property = "failIfNoTests" )
private Boolean failIfNoTests;
+ /**
+ * The number of flakes after which the overall test suite will be considered a failure.
+ */
+ @Parameter( property = "surefire.failOnFlakeCount", defaultValue = "0" )
+ private int failOnFlakeCount;
Review comment:
Could we clarify if this is total number of flakes per build (is that what "overall" refers to?) or per test (i.e. two test methods are both retried 3 times, does the build fail for `failOnFlakeCount = 3` or `failOnFlakeCount = 6`)?
##########
File path: maven-surefire-plugin/src/main/java/org/apache/maven/plugin/surefire/SurefirePlugin.java
##########
@@ -253,6 +253,12 @@
@Parameter( property = "surefire.rerunFailingTestsCount", defaultValue = "0" )
private int rerunFailingTestsCount;
+ /**
+ * The number of flakes after which the overall test suite will be considered a failure.
+ */
+ @Parameter( property = "surefire.failOnFlakeCount", defaultValue = "0" )
Review comment:
Could we specify here whether `0` means a single retry will fail the build (i.e. similar to disabling retries completely, I guess), or whether this disables the feature (which seems to be the case based on https://github.com/apache/maven-surefire/pull/333/files#diff-bf7f174589af8f08638da17d6aefbe897133b7b77ff363a1e12439c37ea8683eR146, which seems weird to me, maybe we'd have to come up with a better name)
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireHelper.java
##########
@@ -286,7 +288,8 @@ private static String createErrorMessage( SurefireReportParameters reportParamet
}
else
{
- msg.append( "There are test failures.\n\nPlease refer to " )
+ msg.append( result.getFailures() > 0 ? "There are test failures." : "There are too many flakes." );
+ msg.append( "\n\nPlease refer to " )
Review comment:
minor: maybe we could merge these appends, as done in the next few lines
##########
File path: surefire-its/src/test/java/org/apache/maven/surefire/its/JUnitPlatformRerunFailingTestsIT.java
##########
@@ -203,6 +203,19 @@ public void testRerunOneTestMethod()
verifyFailuresOneRetryOneMethod( outputValidator );
}
+ @Test
+ public void testFailOnTooManyFlakes()
+ {
+ OutputValidator outputValidator = unpack().setJUnitVersion( VERSION ).maven().debugLogging()
+ .addGoal( "-Dsurefire.rerunFailingTestsCount=2" )
+ .addGoal( "-Dsurefire.failOnFlakeCount=1" )
Review comment:
could we add a test for `rerunFailingTestsCount=0` also - or is that tested implicitly, being the default?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] paplorinc commented on a change in pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
paplorinc commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r566784637
##########
File path: maven-failsafe-plugin/src/main/java/org/apache/maven/plugin/failsafe/VerifyMojo.java
##########
@@ -139,6 +139,12 @@
@Parameter( property = "failIfNoTests" )
private Boolean failIfNoTests;
+ /**
+ * The number of flakes after which the overall test suite will be considered a failure.
+ */
+ @Parameter( property = "surefire.failOnFlakeCount", defaultValue = "0" )
+ private int failOnFlakeCount;
Review comment:
It's very clear now, thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] oehme commented on a change in pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
oehme commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r568468631
##########
File path: 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
Review comment:
SurefirePlugin and VerifyMojo do not have a common base class.
##########
File path: 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
+ {
+ if ( failOnFlakeCount < 0 )
Review comment:
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.
##########
File path: 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 )
+ {
+ msg.append( "There are test failures." );
+ }
+ else
+ {
+ msg.append( "There are too many flakes." );
Review comment:
Done, both messages are reported now if both conditions are true. Also, the number of flakes and failOnFlakeCount are reported now.
##########
File path: 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
Review comment:
But why would the super class contain logic that only applies to a single subclass?
##########
File path: 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
Review comment:
Nevermind, I see the if-else in that method now.
##########
File path: 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
Review comment:
Nevermind, I see the if-else in that method now. Moved call up to the base class.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r568487013
##########
File path: 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
+ {
+ if ( failOnFlakeCount < 0 )
Review comment:
@oehme
I am not talking about VerifyMojo, nothing but the SurefirePlugin class.
##########
File path: 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
Review comment:
I am not talking about VerifyMojo, nothing but the SurefirePlugin class.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#issuecomment-773634176
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#issuecomment-772155837
@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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #333: Prototype failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#issuecomment-769042152
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#issuecomment-776107657
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 merged pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 merged pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#issuecomment-773630782
@oehme
@eolivelli
Thx for contributing!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 edited a comment on pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 edited a comment on pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#issuecomment-776044868
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](https://github.com/apache/maven-surefire/blob/master/maven-surefire-plugin/src/site/apt/examples/rerun-failing-tests.apt.vm#L150). 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #333: Prototype failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r566089270
##########
File path: maven-failsafe-plugin/src/main/java/org/apache/maven/plugin/failsafe/VerifyMojo.java
##########
@@ -139,6 +139,12 @@
@Parameter( property = "failIfNoTests" )
private Boolean failIfNoTests;
+ /**
+ * The number of flakes after which the overall test suite will be considered a failure.
+ */
+ @Parameter( property = "surefire.failOnFlakeCount", defaultValue = "0" )
+ private int failOnFlakeCount;
Review comment:
Exactly this was not clear to me either when we spoke with the JUnit5 colleagues several days ago.
The logic of `re-run` feature operates at the level of individual Test.
In this PR I guess we are talking about the level of Test Set (cumulative number of flakes) which is different view from ours. But's that's ok if it would be properly clarified.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#issuecomment-776044868
We have two plugins: surefire and failsafe.
The one you mentioned `VerifyMojo` is still the same failsafe plugin but a separate goal.
It normal practice to add one sentecnce to the existing [documentation](https://github.com/apache/maven-surefire/blob/master/maven-surefire-plugin/src/site/apt/examples/rerun-failing-tests.apt.vm#L150). 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] oehme commented on pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
oehme commented on pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#issuecomment-770863637
All fixed up
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] eolivelli commented on pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#issuecomment-772346866
@oehme I think there is not need to add the IT
@Tibor17 ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] oehme commented on pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
oehme commented on pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#issuecomment-776078625
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)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #333: Prototype failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r566113145
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireHelper.java
##########
@@ -142,7 +142,9 @@ public static void reportExecution( SurefireReportParameters reportParameters, R
PluginConsoleLogger log, Exception firstForkException )
throws MojoFailureException, MojoExecutionException
{
- if ( firstForkException == null && !result.isTimeout() && result.isErrorFree() )
+ if ( firstForkException == null && !result.isTimeout() && result.isErrorFree()
Review comment:
@oehme
My proposal:
```
boolean isError = firstForkException != null || result.isTimeout() || !result.isErrorFree();
boolean isFlaky = reportParameters.getFailOnFlakeCount() > 0
&& result.getFlakes() >= reportParameters.getFailOnFlakeCount();
if ( !isError && !isFlaky )
{
...
}
else
{
if ( isError )
{
msg.append( "There are test failures." );
}
if ( isFlaky )
{
msg.append( "There are too many flakes." );
}
msg.append( "\n\nPlease refer to " );
...
...
...
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 merged pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 merged pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] oehme commented on pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
oehme commented on pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#issuecomment-774097056
If you want I can send another PR, otherwise I'll let you take care of it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] oehme commented on a change in pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
oehme commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r568507616
##########
File path: 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
Review comment:
Nevermind, I see the if-else in that method now. Moved call up to the base class.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] oehme commented on a change in pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
oehme commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r568486429
##########
File path: 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 )
+ {
+ msg.append( "There are test failures." );
+ }
+ else
+ {
+ msg.append( "There are too many flakes." );
Review comment:
Done, both messages are reported now if both conditions are true. Also, the number of flakes and failOnFlakeCount are reported now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] oehme commented on pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
oehme commented on pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#issuecomment-775011433
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r567414679
##########
File path: 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
+ && result.getFlakes() >= reportParameters.getFailOnFlakeCount();
Review comment:
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] oehme commented on pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
oehme commented on pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#issuecomment-769772297
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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r567073880
##########
File path: 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.
Review comment:
@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`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 edited a comment on pull request #333: Prototype failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 edited a comment on pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#issuecomment-769053812
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] paplorinc commented on a change in pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
paplorinc commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r566785137
##########
File path: 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.
Review comment:
based on the code a negative number would do the same, right?
Should we maybe rather fail in that case?
##########
File path: 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
+ && result.getFlakes() >= reportParameters.getFailOnFlakeCount();
Review comment:
minor: could we have `getFailOnFlakeCount` on the same side on both cases to simplify comparison?
##########
File path: maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/SurefireHelperTest.java
##########
@@ -156,4 +156,59 @@ public void shouldHandleTestFailure() throws Exception
+ "for the individual test results.\nPlease refer to dump files (if any exist) "
+ "[date].dump, [date]-jvmRun[N].dump and [date].dumpstream.'" );
}
+
+ @Test
+ public void failsIfThereAreTooManyFlakes() throws Exception
+ {
+ RunResult summary = new RunResult( 1, 0, 0, 0, 1 );
+ Mojo reportParameters = new Mojo();
+ reportParameters.setFailOnFlakeCount( 1 );
+ try
+ {
+ reportExecution( reportParameters, summary, null, null );
+ }
+ catch ( MojoFailureException e )
+ {
+ assertThat( e.getLocalizedMessage() )
+ .isEqualTo( "There are too many flakes.\n\nPlease refer to null "
+ + "for the individual test results.\nPlease refer to dump files (if any exist) "
+ + "[date].dump, [date]-jvmRun[N].dump and [date].dumpstream." );
+ return;
+ }
+ fail( "Expected MojoFailureException with message "
Review comment:
minor: you could add this inside the try in which case we wouldn't need a return in the catch
##########
File path: 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
Review comment:
this stores more than whether we're flaky, rather something like: `isFlakyError`, right?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r568075387
##########
File path: 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.
+ */
+ @Parameter( property = "surefire.failOnFlakeCount", defaultValue = "0" )
Review comment:
Here the property `surefire.failOnFlakeCount` must become `failsafe.failOnFlakeCount`. See the other properties in this class.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#issuecomment-770876575
>
>
> All fixed up
Thx. Waiting for the CI.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r567414679
##########
File path: 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
+ && result.getFlakes() >= reportParameters.getFailOnFlakeCount();
Review comment:
@paplorinc
I think `result.getFlakes() >= reportParameters.getFailOnFlakeCount()` is okay because it is a condition saying `variable >= constant`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] oehme commented on a change in pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
oehme commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r568469344
##########
File path: 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
+ {
+ if ( failOnFlakeCount < 0 )
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] oehme commented on a change in pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
oehme commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r568507616
##########
File path: 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
Review comment:
Nevermind, I see the if-else in that method now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] paplorinc commented on a change in pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
paplorinc commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r567250891
##########
File path: 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
+ && result.getFlakes() >= reportParameters.getFailOnFlakeCount();
Review comment:
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 an observation
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] oehme commented on a change in pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
oehme commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r566875005
##########
File path: 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 )
Review comment:
The IntegrationTestMojo does not report this text. The VerifyMojo does and uses the same logic as the SurefirePlugin
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r569070727
##########
File path: 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
Review comment:
>
>
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #333: Prototype failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r566116951
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireHelper.java
##########
@@ -142,7 +142,9 @@ public static void reportExecution( SurefireReportParameters reportParameters, R
PluginConsoleLogger log, Exception firstForkException )
throws MojoFailureException, MojoExecutionException
{
- if ( firstForkException == null && !result.isTimeout() && result.isErrorFree() )
+ if ( firstForkException == null && !result.isTimeout() && result.isErrorFree()
Review comment:
>
>
> Got it, thanks!
>
> So is this option something you'd consider adding? In that case I'd flesh out the PR some more.
Yes I would consider this PR adding to Surefire.
Scary of the future PRs a bit ;-)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r568178204
##########
File path: maven-failsafe-plugin/src/main/java/org/apache/maven/plugin/failsafe/VerifyMojo.java
##########
@@ -236,6 +243,11 @@ protected boolean verifyParameters()
return false;
}
+ if ( failOnFlakeCount < 0 )
Review 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();
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #333: Prototype failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r566092236
##########
File path: surefire-its/src/test/java/org/apache/maven/surefire/its/JUnitPlatformRerunFailingTestsIT.java
##########
@@ -203,6 +203,19 @@ public void testRerunOneTestMethod()
verifyFailuresOneRetryOneMethod( outputValidator );
}
+ @Test
+ public void testFailOnTooManyFlakes()
+ {
+ OutputValidator outputValidator = unpack().setJUnitVersion( VERSION ).maven().debugLogging()
+ .addGoal( "-Dsurefire.rerunFailingTestsCount=2" )
+ .addGoal( "-Dsurefire.failOnFlakeCount=1" )
Review comment:
Then it would become a simple test and it should perform as if `failOnFlakeCount` was disabled. Am I right?
So the `failOnFlakeCount` works only with specified `rerunFailingTestsCount > 0`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r567069271
##########
File path: 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 )
Review comment:
@oehme
ok just fine!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#issuecomment-774443779
@oehme
Hi, that would be great, thank you.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #333: Prototype failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#issuecomment-769075495
@oehme
I think the class `SurefireHelper.java` has unit tests. Try to cover every change you made. Thx
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r569070727
##########
File path: 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
Review comment:
>
>
> But why would the super class contain logic that only applies to a single subclass?
Both plugins, Surefire and Failsafe, have 90% 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] oehme commented on pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
oehme commented on pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#issuecomment-772492203
Added the IT back
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #333: Prototype failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#issuecomment-769053812
@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 than it is different story of course.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] paplorinc commented on a change in pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
paplorinc commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r567250891
##########
File path: 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
+ && result.getFlakes() >= reportParameters.getFailOnFlakeCount();
Review comment:
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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 edited a comment on pull request #333: Prototype failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 edited a comment on pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#issuecomment-769046012
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r568176510
##########
File path: 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
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #333: Prototype failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#issuecomment-769046012
@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 test set.
Pls clarify and change the description in Jira ticket as necessary.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r568179480
##########
File path: 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
+ {
+ if ( failOnFlakeCount < 0 )
Review comment:
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();
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#issuecomment-770372094
>
>
> 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.
Then concentrate on the `ExpectedException` for the other methods and make extra commit.
+1 LGTM
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] oehme commented on a change in pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
oehme commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r568501708
##########
File path: 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
Review comment:
But why would the super class contain logic that only applies to a single subclass?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] oehme edited a comment on pull request #333: Prototype failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
oehme edited a comment on pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#issuecomment-769048635
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r569071541
##########
File path: 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 )
+ {
+ msg.append( "There are test failures." );
+ }
+ else
+ {
+ msg.append( "There are too many flakes." );
Review comment:
@oehme
Thx, a good job!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r568487013
##########
File path: 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
+ {
+ if ( failOnFlakeCount < 0 )
Review comment:
@oehme
I am not talking about VerifyMojo, nothing but the SurefirePlugin class.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] oehme commented on a change in pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
oehme commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r568468631
##########
File path: 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
Review comment:
SurefirePlugin and VerifyMojo do not have a common base class.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] oehme commented on pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
oehme commented on pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#issuecomment-772337965
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] oehme commented on pull request #333: Prototype failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
oehme commented on pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#issuecomment-769055279
Got it, thanks!
So is this option something you'd consider adding? In that case I'd flesh out the PR some more.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #333: Prototype failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r566113145
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireHelper.java
##########
@@ -142,7 +142,9 @@ public static void reportExecution( SurefireReportParameters reportParameters, R
PluginConsoleLogger log, Exception firstForkException )
throws MojoFailureException, MojoExecutionException
{
- if ( firstForkException == null && !result.isTimeout() && result.isErrorFree() )
+ if ( firstForkException == null && !result.isTimeout() && result.isErrorFree()
Review comment:
@oehme
My proposal:
```
boolean isError = firstForkException != null || result.isTimeout() || !result.isErrorFree();
boolean isFlaky = reportParameters.getFailOnFlakeCount() > 0
&& result.getFlakes() >= reportParameters.getFailOnFlakeCount();
if ( isError || isFlaky )
{
...
}
else
{
if ( isError )
{
msg.append( "There are test failures." );
}
if ( isFlaky )
{
msg.append( "There are too many flakes." );
}
msg.append( "\n\nPlease refer to " );
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r568160462
##########
File path: 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 )
+ {
+ msg.append( "There are test failures." );
+ }
+ else
+ {
+ msg.append( "There are too many flakes." );
Review comment:
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r567079918
##########
File path: maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/SurefireHelperTest.java
##########
@@ -156,4 +156,59 @@ public void shouldHandleTestFailure() throws Exception
+ "for the individual test results.\nPlease refer to dump files (if any exist) "
+ "[date].dump, [date]-jvmRun[N].dump and [date].dumpstream.'" );
}
+
+ @Test
+ public void failsIfThereAreTooManyFlakes() throws Exception
+ {
+ RunResult summary = new RunResult( 1, 0, 0, 0, 1 );
+ Mojo reportParameters = new Mojo();
+ reportParameters.setFailOnFlakeCount( 1 );
+ try
+ {
+ reportExecution( reportParameters, summary, null, null );
+ }
+ catch ( MojoFailureException e )
+ {
+ assertThat( e.getLocalizedMessage() )
+ .isEqualTo( "There are too many flakes.\n\nPlease refer to null "
+ + "for the individual test results.\nPlease refer to dump files (if any exist) "
+ + "[date].dump, [date]-jvmRun[N].dump and [date].dumpstream." );
+ return;
+ }
+ fail( "Expected MojoFailureException with message "
Review comment:
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #333: Add failOnFlakeCount option
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #333:
URL: https://github.com/apache/maven-surefire/pull/333#discussion_r567415122
##########
File path: 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
Review comment:
minor i would say
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org