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);
-      }
-      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.
+    // a core dump. Sometimes DRT crashes after it has set the stdout to PASS,
+    // so we have to do this check first.
     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, this.allowRetries);
 
   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 218 matching lines @@
  *
  * Because multiple configurations may be run on each test suite, the
  * ProcessQueue contains a cache in which a test suite may record information
  * about its list of tests, and may retrieve that information when it is called
  * upon to enqueue its tests again.
  */
 class ProcessQueue {
   int _numProcesses = 0;
   int _activeTestListers = 0;
   int _maxProcesses;
+  /** The number of tests we allow to actually fail before we stop retrying. */
+  int MAX_FAILED_NO_RETRY = 4;

[Siggi Cherem (dart-lang), 2012/02/17 21:34:17]

make it private (_MAX...)

   bool _verbose;
   bool _listTests;
   bool _keepGeneratedTests;
   Function _enqueueMoreWork;
   Queue<TestCase> _tests;
   ProgressIndicator _progress;
   String _temporaryDirectory;
   // For dartc batch processing we keep a list of batch processes.
   List<DartcBatchRunnerProcess> _batchProcesses;
   // Cache information about test cases per test suite. For multiple
@@ skipping 184 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();
+        // Once we've actually failed a test, technically, we wouldn't need to
+        // bother retrying any subsequent tests since the bot is already red. 
+        // However, we continue to retry tests until we have actually failed 
+        // four tests (arbitrarily chosen) for more debugable output, so that 
+        // the developer doesn't waste his or her time trying to fix a bunch of 
+        // tests that appear to be broken but were actually just flakes that 
+        // didn't get retried because there had already been one failure.
+        new RunningProcess(test, 
+            (MAX_FAILED_NO_RETRY - _progress.numFailedTests) > 0).start();

[Siggi Cherem (dart-lang), 2012/02/17 21:34:17]

nit =)

_progress.numFailedTests < _MAX_FAILED_NO_RETRY

       }
       _numProcesses++;
     }
   }
 }