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

tools/testing/dart/test_runner.dart

Index: tools/testing/dart/test_runner.dart
===================================================================
--- tools/testing/dart/test_runner.dart	(revision 4362)
+++ tools/testing/dart/test_runner.dart	(working copy)
@@ -200,16 +200,6 @@
     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.
@@ -222,6 +212,17 @@
         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;
   }
 
@@ -248,8 +249,9 @@
   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,
@@ -261,7 +263,7 @@
       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;
@@ -704,7 +706,11 @@
         _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++;
     }