Don't replicate timeout calculation for browser tests. They do their own timing

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 592 matching lines @@
   /**
    * Process exit handler called at the end of every command. It internally
    * treats all but the last command as compilation steps. The last command is
    * the actual test and its output is analyzed in [testComplete].
    */
   void stepExitHandler(int exitCode) {
     process.close();
     process = null;
     int totalSteps = testCase.commands.length;
     String suffix =' (step $currentStep of $totalSteps)';
-    if (timedOut) {
-      // Test timed out before it could complete.
+    if (timedOut &&
+        !(testCase.usesWebDriver && !testCase.configuration['noBatch'])) {

[Jennifer Messerly, 2012/10/23 22:50:19]

might want to add a property for:

    testCase.usesWebDriver && !testCase.configuration['noBatch']

perhaps "testCase.usesBatchRunner"?

[Emily Fortuna, 2012/10/23 23:11:25]

But the dartc tests also "usesBatchRunner"...

[Mads Ager (google), 2012/10/24 05:56:14]

I don't understand why this should change. If we are at the last step which is
running in the browser, the timer should have been cancelled and we should not
get the timedOut case here. If we get the timedOut case here we have not started
the browser part yet and one of the steps before starting the browser timed out
and the test itself times out.

Do the browser test cases have separate test output implementations that
interpret the timedout information and exit code differently? Does it not notice
that the test is timing out if the exit code is 0?

[Mads Ager (google), 2012/10/24 12:03:47]

Something is completely off here. I don't see how usesWebDriver can ever be
true? It is false by default in testCase. BrowserTestCase does not pass in a
usesWebDriver argument, so for all browser test cases it is false too. For other
test cases, we create the test cases passing in a function instead of a
boolean?!?

Emily, could you have a look at this. I think what has been added here makes no
difference at all because the usesWebDriver 'bool' is never true.

+      // Non-webdriver test timed out before it could complete. Webdriver tests
+      // run their own timeouts by timing from the launch of the browser (which
+      // could be delayed).
       testComplete(0, true);
     } else if (currentStep == totalSteps) {
       // Done with all test commands.
       testComplete(exitCode, false);
     } else if (exitCode != 0) {
       // One of the steps failed.
       stderr.add('test.dart: Compilation failed$suffix, exit code $exitCode\n');
       testComplete(exitCode, true);
     } else {
       // One compilation step successfully completed, move on to the
       // next step.
       stderr.add('test.dart: Compilation finished $suffix\n');
       stdout.add('test.dart: Compilation finished $suffix\n');
       if (currentStep == totalSteps - 1 && testCase.usesWebDriver &&
           !testCase.configuration['noBatch']) {
         // Note: processQueue will always be non-null for runtime == ie9, ie10,
         // ff, safari, chrome, opera. (It is only null for runtime == vm)
         // This RunningProcess object is done, and hands over control to
         // BatchRunner.startTest(), which handles reporting, etc.
         timeoutTimer.cancel();

[Mads Ager (google), 2012/10/24 05:56:14]

Here we cancel the timer before starting the browser. So if it fires, it is one
of the steps before the browser that times out.

         processQueue._getBatchRunner(testCase).startTest(testCase);
       } else {
         runCommand(testCase.commands[currentStep++], stepExitHandler);
       }
     }
   }
 
   VoidFunction makeReadHandler(StringInputStream source,
                                List<String> destination) {
     void handler () {
@@ skipping 636 matching lines @@
         // 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.
         bool allowRetry = _MAX_FAILED_NO_RETRY > _progress.numFailedTests;
         new RunningProcess(test, allowRetry, this).start();
       }
       _numProcesses++;
     }
   }
 }