Don't retry browser tests if the bot is pretty red. Also, retry properly all

tools/testing/dart/test_runner.dart

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 /**
  * Classes and methods for executing tests.
  *
  * This module includes:
  * - Managing parallel execution of tests, including timeout checks.
  * - Evaluating the output of each test as pass/fail/crash/timeout.
@@ skipping 182 matching lines @@
     // exceptions.
     return (!timedOut && ((exitCode < 0) || (exitCode == 253)));
   }
 
   bool get hasTimedOut() => timedOut;
 
   bool get didFail() {
     if (testCase is !BrowserTestCase) return (exitCode != 0 && !hasCrashed);
 
     // Browser case:
-    // Browser tests fail unless stdout contains
-    // 'Content-Type: text/plain\nPASS'.
-    String previous_line = '';
-    for (String line in stdout) {
-      if (line == 'PASS' && previous_line == 'Content-Type: text/plain') {
-        return (exitCode != 0 && !hasCrashed);

[Emily Fortuna, 2012/02/17 19:34:24]

Sometimes when DRT crashes with a memory dump, and the exitCode == 1 and
!hasCrashed == true, meaning we never scan through the error output below to see
if we should retry the test. Therefore, now we're first testing the stderr to
determine if a retry is necessary.

[Siggi Cherem (dart-lang), 2012/02/17 19:51:38]

I thought in that case we wouldn't have PASS in the output?

[Emily Fortuna, 2012/02/17 21:03:03]

Sometimes it does, and sometimes it doesn't when DRT crashes. It's strange.


On 2012/02/17 19:51:38, sigmund wrote:

-      }
-      previous_line = line;
-    }
-
     // If the browser test failed, it may have been because DumpRenderTree
     // and the virtual framebuffer X server didn't hook up, or DRT crashed with
     // a core dump.
     for (String line in stderr) {
       if (line.contains('Gtk-WARNING **: cannot open display: :99') ||
         line.contains('Failed to run command. return code=1')) {
         // If we get the X server error, or DRT crashes with a core dump, retry
         // the test. 
         requestRetry = true;
         return true;
       }
     }
+
+    // Browser tests fail unless stdout contains
+    // 'Content-Type: text/plain\nPASS'.
+    String previous_line = '';
+    for (String line in stdout) {
+      if (line == 'PASS' && previous_line == 'Content-Type: text/plain') {
+        return (exitCode != 0 && !hasCrashed);
+      }
+      previous_line = line;
+    }
+
     return true;
   }
 
   // Reverse result of a negative test.
   bool get hasFailed() => (testCase.isNegative ? !didFail : didFail);
 }
 
 /**
  * A RunningProcess actually runs a test, getting the command lines from
  * its [TestCase], starting the test process (and first, a compilation
  * process if the TestCase is a [BrowserTestCase]), creating a timeout
  * timer, and recording the results in a new [TestOutput] object, which it
  * attaches to the TestCase.  The lifetime of the RunningProcess is limited
  * to the time it takes to start the process, run the process, and record
  * the result; there are no pointers to it, so it should be available to
  * be garbage collected as soon as it is done.
  */
 class RunningProcess {
   Process process;
   TestCase testCase;
   bool timedOut = false;
   Date startTime;
   Timer timeoutTimer;
   List<String> stdout;
   List<String> stderr;
   List<Function> handlers;
+  bool allowRetries;
 
-  RunningProcess(TestCase this.testCase);
+  RunningProcess(TestCase this.testCase, bool this.allowRetries);

[Siggi Cherem (dart-lang), 2012/02/17 19:51:38]

I think when using this.xxx our style is to omit the type.

[Emily Fortuna, 2012/02/17 21:03:03]

Done.

 
   void exitHandler(int exitCode) {
     new TestOutput(testCase, exitCode, timedOut, stdout,
                    stderr, new Date.now().difference(startTime));
     process.close();
     timeoutTimer.cancel();
     if (testCase.output.unexpectedOutput && testCase.configuration['verbose']) {
       print(testCase.displayName);
       for (var line in testCase.output.stderr) print(line);
       for (var line in testCase.output.stdout) print(line);
     }
-    if (testCase.configuration['component'] == 'webdriver' &&
+    if (allowRetries && testCase.configuration['component'] == 'webdriver' &&
         testCase.output.unexpectedOutput && testCase.numRetries > 0) {
       // Selenium tests can be flaky. Try rerunning.
       testCase.output.requestRetry = true;
     }
     if (testCase.output.requestRetry) {
       testCase.output.requestRetry = false;
       this.timedOut = false;
       testCase.dynamic.numRetries--;
       print("Potential flake. Re-running " + testCase.displayName);
       this.start();
@@ skipping 422 matching lines @@
         _progress.done(test_arg);
         _tryRunTest();
         oldCallback(test_arg);
       };
       test.completedHandler = wrapper;
       if (test.configuration['component'] == 'dartc'  &&
           test.displayName != 'dartc/junit_tests') {
         _ensureDartcBatchRunnersStarted(test.executablePath);
         _getDartcBatchRunnerProcess().startTest(test);
       } else {
-        new RunningProcess(test).start();
+        // If we've failed more than four tests, a lot of tests are likely

[Siggi Cherem (dart-lang), 2012/02/17 19:51:38]

When is numFailedTest incremented?

I thought that if you fail, retry, and pass, then the numFailedTest is not
incremented.

If that's the case, then you simply need to check _progress.numFailedTests != 0
(as soon as one test is marked as failing after all retries, the bot will be
marked red, so we wouldn't need to retry any other test beyond that point).

[Emily Fortuna, 2012/02/17 21:03:03]

The progress counter keeps track of the *actual* failures (ones that failed
after all the retries, or if it wasn't allowed to retry) in the _failedTests
variable in test_progress.dart. Currently the variable is private, so I added an
gettor for it.

Yes, numFailedTests is not incremented. numFailedTests is here for the case
where the bot is super red (for example, when IE was failing 1/2 of its tests),
we don't bother allowing retries when more than 5 tests have *actually* failed.


On 2012/02/17 19:51:38, sigmund wrote:

[Emily Fortuna, 2012/02/17 21:04:45]

*Edit to last paragraph: 
Yes, numFailedTests is not incremented if you fail, retry and then pass.


On 2012/02/17 21:03:03, Emily Fortuna wrote:

+        // broken. Don't continue to retry running tests if our tree is already
+        // red. At this point rerunning to test for flakes takes more time than
+        // it's worth.  
+        new RunningProcess(test, _progress.numFailedTests < 5).start();

[Emily Fortuna, 2012/02/17 19:34:24]

Magic number... no me gusta :-(

[Siggi Cherem (dart-lang), 2012/02/17 19:51:38]

maybe we can do != 0?

       }
       _numProcesses++;
     }
   }
 }