Added additional Maven profile for skipping known flaky tests #498

Merged
merged 2 commits into from Aug 20, 2013

3 participants

@dotta
Eclipse Scala IDE member

Use the Maven profile skipFlakyTests for not executing tests
that are known to be flaky. Inside Eclipse, you can achieve the
same by passing the system property -DskipFlakyTests=true.

This was done to preventing the Scala/IDE integration script to
fail because of flakiness in our regression suite.

@dotta dotta referenced this pull request in scala/jenkins-scripts Aug 14, 2013
Merged

Pass additional profile to IDE build to ignore known flaky tests #9

@dotta
Eclipse Scala IDE member

And also issued the PR for updating the IDE/Scala script scala/jenkins-scripts#9

@dragos dragos and 1 other commented on an outdated diff Aug 15, 2013
....tests/src/scala/tools/eclipse/maySkipFlakyTest.scala
@@ -0,0 +1,18 @@
+package scala.tools.eclipse
+
+import scala.tools.eclipse.logging.HasLogger
+
+object maySkipFlakyTest extends HasLogger {
+ private final val SkipFlakyTests = "skipFlakyTests"
+ private def skipFlakyTests = "true".equalsIgnoreCase(System.getProperty(SkipFlakyTests))
+
+ /** Doesn't execute the passed `test` if the system property `skipFlakyTests` is set to `true`.
+ *
+ * @param msg The error message returned when the test fails (purpose of this parameter is purely informative).
+ * @parma test The actual test code to be executed.
+ */
+ def apply(msg: String = "")(test: => Unit): Unit = {
+ if(skipFlakyTests) logger.debug("Skipping test known to be flacky.")
@dragos
Eclipse Scala IDE member
dragos added a note Aug 15, 2013

It might be useful to write msg in the debug log (it seems to be unused otherwise).

@dotta
Eclipse Scala IDE member
dotta added a note Aug 15, 2013

msg at the moment is "useful" for documentation, i.e., when you read the test it describes how the test can fail. I didn't think it was useful for the actual execution, which is why it's not used. But I'm of course open to suggestions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dotta
Eclipse Scala IDE member

I've removed the Maven profile, but I'm not convinced this is a good idea. My rational is that we need to feel the pain of spurious test failures to be willing to invest time on fixing them. So, my suggestion would be to re-introduce the maven profile and only retry executing known flacky tests when the profile is enabled (for instance, the Scala/IDE integration script would always run with the profile enable). Anyone against?

@dotta dotta commented on an outdated diff Aug 16, 2013
...dt.core.tests/src/scala/tools/eclipse/FlakyTest.scala
+ * @param methodName The test's method name.
+ * @param errorMsg The error message returned when the test fails.
+ * @param times The total number of attempts that will be performed before bailing out.
+ * @param test The actual test code to execute.
+ */
+ def retry(methodName: String, errorMsg: String = "", times: Int = 5)(test: => Unit): Unit = {
+ @annotation.tailrec
+ def loop(attempt: Int): Unit = {
+ try {
+ logger.debug(s"Test run number: ${attempt})")
+ test
+ logger.debug(s"Test `%{testName}` was successful!")
+ }
+ catch {
+ case _: AssertionError if attempt < times => loop(attempt + 1)
+ case _: AssertionError => logger.debug(s"Bailing out after ${attempt} attempts. The test is failing consistenly, this may actually be a real regression!?")
@dotta
Eclipse Scala IDE member
dotta added a note Aug 16, 2013

Actually, here I need to re-throw the exception, or the test won't fail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dotta dotta Added a ``FlakyTest.retry`` combinator to be re-run tests known to be…
… flacky

In our test suite we have a number of tests that are known to be flacky. The
``FlakyTest.retry`` combinator re-executes the same time up to N times (where
N is defaulted to 5 at the moment).

While we should of course strive to fix all sources of flackiness in tests, this
is a pragmatic solution to prevent spurious build failures.
cfc7ee7
@dragos dragos commented on an outdated diff Aug 17, 2013
...dt.core.tests/src/scala/tools/eclipse/FlakyTest.scala
+ /** Retry to execute the passed `test` up to N `times`.
+ *
+ * @param methodName The test's method name.
+ * @param errorMsg The error message returned when the test fails.
+ * @param times The total number of attempts that will be performed before bailing out.
+ * @param test The actual test code to execute.
+ */
+ def retry(methodName: String, errorMsg: String = "", times: Int = 5)(test: => Unit): Unit = {
+ @annotation.tailrec
+ def loop(attempt: Int): Unit = {
+ try {
+ logger.debug(s"Test run number: ${attempt})")
+ test
+ logger.debug(s"Test `%{testName}` was successful!")
+ }
+ catch {
@dragos
Eclipse Scala IDE member
dragos added a note Aug 17, 2013

extra new-line? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dragos
Eclipse Scala IDE member

I'm ok with having a profile. I'm more worried about making flaky tests "acceptable". By that I mean that a test shouldn't be put behind "retry" before an honest effort was put into finding and fixing the root cause of flakiness. That will be up to us as a team to hold to.

@dragos
Eclipse Scala IDE member

LGTM. I'd like to merge this and add the profile in a separate pull request. It's important to get back PR validation ASAP.

@dotta
Eclipse Scala IDE member

@dragos I just pushed an additional commit to allow execution of the retry combinator only if the retryFlakyTests system property is enabled. One word of advice, I didn't smoke test the last commit, so would be wise to do so before merging (and, please, feel free to update the functionality if you hit any issue). If it's all good and you decide to merge it, could you also please issue a PR on the scala/jenkins-script repo to update the passed profile here (it should be -PretryFlakyTests instead of -PskipFlakyTests).

@dragos
Eclipse Scala IDE member

@dotta It's too late to do it now, I'll come back to it tomorrow. Unfortunately, the PR validation job might still exhibit the usual spurious failures until then.

@dotta dotta Added Maven profile ``retryFlakyTests`` to reduce spurious test failures
Use the Maven profile ``retryFlakyTests`` for being robust against tests that
are known to be flaky. Inside Eclipse, you can achieve the same by passing the
system property ``-DretryFlakyTests=true``.

This profile should only be used by the Scala/IDE integration script to avoid
failing because of flakiness in our regression suite.
c17e58a
@dotta
Eclipse Scala IDE member

@dragos Indeed, the property has to be manually passed to downstream processes. I believe this is needed because we use Tycho (as far as I remember, it works our of the obx for "standard" Maven builds). Anyhow, great catch, I've force-pushed the last commit and added the flag to the JVM args we pass to downstream processes. Check this line.

@dragos
Eclipse Scala IDE member

The failure is spurious.

@dragos dragos merged commit 6e91991 into scala-ide:master Aug 20, 2013

1 check failed

Details default Merged build finished.
@dragos
Eclipse Scala IDE member

Here's the corresponding PR: scala/jenkins-scripts#19

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment