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:
us...@infra.apache.org
