reuse the same browser when running webdriver tests

tools/testing/dart/test_runner.dart

Index: tools/testing/dart/test_runner.dart
diff --git a/tools/testing/dart/test_runner.dart b/tools/testing/dart/test_runner.dart
index f9b07c647457c3f7f9a0b223ff136fc747dd09bb..7eddd3f73339cc679b4a7c3b8698506c21829d74 100644
--- a/tools/testing/dart/test_runner.dart
+++ b/tools/testing/dart/test_runner.dart
@@ -99,6 +99,9 @@ class TestCase {
     return "$component ${mode}_$arch";
   }
 
+  List<String> get batchRunnerArguments() => ['-batch'];
+  List<String> get batchTestArguments() => arguments;
+
   void completed() { completedHandler(this); }
 }
 
@@ -146,6 +149,10 @@ class BrowserTestCase extends TestCase {
     }
     numRetries = 2; // Allow two retries to compensate for flaky browser tests.
   }
+
+  List<String> get batchRunnerArguments() => [arguments[0], '--batch'];
+  List<String> get batchTestArguments() =>
+      arguments.getRange(1, arguments.length - 1);
 }
 
 
@@ -208,7 +215,7 @@ class TestOutput {
       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. 
+        // the test.
         requestRetry = true;
         return true;
       }
@@ -242,6 +249,7 @@ class TestOutput {
  * be garbage collected as soon as it is done.
  */
 class RunningProcess {
+  ProcessQueue processQueue;
   Process process;
   TestCase testCase;
   bool timedOut = false;
@@ -252,7 +260,8 @@ class RunningProcess {
   List<Function> handlers;
   bool allowRetries = false;
 
-  RunningProcess(TestCase this.testCase, [this.allowRetries]);
+  RunningProcess(TestCase this.testCase,
+      [this.allowRetries, this.processQueue]);
 
   void exitHandler(int exitCode) {
     new TestOutput(testCase, exitCode, timedOut, stdout,
@@ -264,7 +273,7 @@ class RunningProcess {
       for (var line in testCase.output.stderr) print(line);
       for (var line in testCase.output.stdout) print(line);
     }
-    if (allowRetries != null && allowRetries 
+    if (allowRetries != null && allowRetries
         && testCase.configuration['component'] == 'webdriver' &&
         testCase.output.unexpectedOutput && testCase.numRetries > 0) {
       // Selenium tests can be flaky. Try rerunning.
@@ -289,7 +298,13 @@ class RunningProcess {
       process.close();
       stderr.add('test.dart: Compilation finished, starting execution\n');
       stdout.add('test.dart: Compilation finished, starting execution\n');
-      runCommand(testCase.executablePath, testCase.arguments, exitHandler);
+      if (testCase.configuration['component'] == 'webdriver') {
+        // Note: processQueue will always be non-null for component == webdriver
+        // (It is only null for component == vm)
+        processQueue._getBatchRunner(testCase).startTest(testCase);
+      } else {
+        runCommand(testCase.executablePath, testCase.arguments, exitHandler);
+      }
     }
   }
 
@@ -346,9 +361,9 @@ class RunningProcess {
   }
 }
 
-
-class DartcBatchRunnerProcess {
+class BatchRunnerProcess {
   String _executable;
+  List<String> _batchArguments;
 
   Process _process;
   StringInputStream _stdoutStream;
@@ -360,7 +375,13 @@ class DartcBatchRunnerProcess {
   Date _startTime;
   Timer _timer;
 
-  DartcBatchRunnerProcess(String this._executable);
+  bool _isWebDriver;
+
+  BatchRunnerProcess(TestCase testCase) {
+    _executable = testCase.executablePath;
+    _batchArguments = testCase.batchRunnerArguments;
+    _isWebDriver = testCase.configuration['component'] == 'webdriver';
+  }
 
   bool get active() => _currentTest != null;
 
@@ -376,6 +397,7 @@ class DartcBatchRunnerProcess {
       // Restart this runner with the right executable for this test
       // if needed.
       _executable = testCase.executablePath;
+      _batchArguments = testCase.batchRunnerArguments;
       _process.exitHandler = (exitCode) {
         _process.close();
         _startProcess(() {
@@ -390,10 +412,23 @@ class DartcBatchRunnerProcess {
 
   void terminate() {
     if (_process !== null) {
+      bool closed = false;
       _process.exitHandler = (exitCode) {
+        closed = true;
         _process.close();
       };
-      _process.kill();
+      if (_isWebDriver) {
+        // Use a graceful shutdown so our Selenium script can close
+        // the open browser processes. TODO(jmesserly): Send a signal once
+        // that's supported, see dartbug.com/1756.
+        _process.stdin.write('--terminate\n'.charCodes());
+
+        // In case the run_selenium process didn't close, kill it after 30s
+        bool shutdownMillisecs = 30000;
+        new Timer((e) { if (!closed) _process.kill(); }, shutdownMillisecs);
+      } else {
+        _process.kill();
+      }
     }
   }
 
@@ -403,8 +438,9 @@ class DartcBatchRunnerProcess {
     _testStderr = new List<String>();
     _stdoutStream.lineHandler = _readOutput(_stdoutStream, _testStdout);
     _stderrStream.lineHandler = _readOutput(_stderrStream, _testStderr);
-    _timer = new Timer(_timeoutHandler(testCase), testCase.timeout * 1000);
-    _process.stdin.write(_createArgumentsLine(testCase.arguments).charCodes());
+    _timer = new Timer(_timeoutHandler, testCase.timeout * 1000);
+    var line = _createArgumentsLine(testCase.batchTestArguments);
+    _process.stdin.write(line.charCodes());
   }
 
   String _createArgumentsLine(List<String> arguments) {
@@ -460,20 +496,18 @@ class DartcBatchRunnerProcess {
     });
   }
 
-  Function _timeoutHandler(TestCase test) {
-    return (ignore) {
-      _process.exitHandler = (exitCode) {
-        _process.close();
-        _startProcess(() {
-          _reportResult(">>> TEST TIMEOUT");
-        });
-      };
-      _process.kill();
+  void _timeoutHandler(ignore) {
+    _process.exitHandler = (exitCode) {
+      _process.close();
+      _startProcess(() {
+        _reportResult(">>> TEST TIMEOUT");
+      });
     };
+    _process.kill();
   }
 
   void _startProcess(then) {
-    _process = new Process.start(_executable, ['-batch']);
+    _process = new Process.start(_executable, _batchArguments);
     _stdoutStream = new StringInputStream(_process.stdout);
     _stderrStream = new StringInputStream(_process.stderr);
     _testStdout = new List<String>();
@@ -485,7 +519,6 @@ class DartcBatchRunnerProcess {
   }
 }
 
-
 /**
  * ProcessQueue is the master control class, responsible for running all
  * the tests in all the TestSuites that have been registered.  It includes
@@ -513,8 +546,9 @@ class ProcessQueue {
   Queue<TestCase> _tests;
   ProgressIndicator _progress;
   String _temporaryDirectory;
-  // For dartc batch processing we keep a list of batch processes.
-  List<DartcBatchRunnerProcess> _batchProcesses;
+  // For dartc/selenium batch processing we keep a list of batch processes.
+  Map<String, List<BatchRunnerProcess>> _batchProcesses;
+
   // Cache information about test cases per test suite. For multiple
   // configurations there is no need to repeatedly search the file
   // system, generate tests, and search test files for options.
@@ -537,7 +571,7 @@ class ProcessQueue {
         _progress = new ProgressIndicator.fromName(progress,
                                                    startTime,
                                                    printTiming),
-        _batchProcesses = new List<DartcBatchRunnerProcess>(),
+        _batchProcesses = new Map<String, List<BatchRunnerProcess>>(),
         _testCache = new Map<String, List<TestInformation>>() {
     if (!_enqueueMoreWork(this)) _progress.allDone();
     browserUsed = '';
@@ -580,8 +614,9 @@ class ProcessQueue {
     if (new Platform().operatingSystem() == 'macos') {
       chromeName = 'Google\ Chrome';
     }
-    Map<String, List<String>> processNames = {'ie': ['iexplore'], 'safari':
-        ['Safari'], 'ff': ['firefox'], 'chrome': ['chromedriver', chromeName]};
+    Map<String, List<String>> processNames = {'ie': ['iexplore'],
+        'safari': ['Safari'], 'ff': ['firefox', 'firefox-bin'],
+        'chrome': ['chromedriver', chromeName]};
     for (String name in processNames[browserUsed]) {
       Process process = null;
       if (new Platform().operatingSystem() == 'windows') {
@@ -626,7 +661,7 @@ class ProcessQueue {
     if (_activeTestListers == 0 && !_enqueueMoreWork(this)) {
       _progress.allTestsKnown();
       if (_tests.isEmpty() && _numProcesses == 0) {
-        _terminateDartcBatchRunners();
+        _terminateBatchRunners();
         if (_keepGeneratedTests || _temporaryDirectory == null) {
           _cleanupAndMarkDone();
         } else if (!_temporaryDirectory.startsWith('/tmp/') ||
@@ -663,21 +698,27 @@ class ProcessQueue {
     _tryRunTest();
   }
 
-  void _terminateDartcBatchRunners() {
-    _batchProcesses.forEach((runner) => runner.terminate());
+  void _terminateBatchRunners() {
+    for (var runners in _batchProcesses.getValues()) {
+      for (var runner in runners) {
+        runner.terminate();
+      }
+    }
   }
 
-  void _ensureDartcBatchRunnersStarted(String executable) {
-    if (_batchProcesses.length == 0) {
+  BatchRunnerProcess _getBatchRunner(TestCase test) {
+    // Start batch processes if needed
+    var component = test.configuration['component'];
+    var runners = _batchProcesses[component];
+    if (runners == null) {
+      runners = new List<BatchRunnerProcess>(_maxProcesses);
       for (int i = 0; i < _maxProcesses; i++) {
-        _batchProcesses.add(new DartcBatchRunnerProcess(executable));
+        runners[i] = new BatchRunnerProcess(test);
       }
+      _batchProcesses[component] = runners;
     }
-  }
 
-  DartcBatchRunnerProcess _getDartcBatchRunnerProcess() {
-    for (int i = 0; i < _batchProcesses.length; i++) {
-      var runner = _batchProcesses[i];
+    for (var runner in runners) {
       if (!runner.active) return runner;
     }
     throw new Exception('Unable to find inactive batch runner.');
@@ -705,20 +746,19 @@ class ProcessQueue {
         oldCallback(test_arg);
       };
       test.completedHandler = wrapper;
-      if (test.configuration['component'] == 'dartc'  &&
+      if (test.configuration['component'] == 'dartc' &&
           test.displayName != 'dartc/junit_tests') {
-        _ensureDartcBatchRunnersStarted(test.executablePath);
-        _getDartcBatchRunnerProcess().startTest(test);
+        _getBatchRunner(test).startTest(test);
       } else {
         // 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 
+        // 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).start();
+        bool allowRetry = _MAX_FAILED_NO_RETRY > _progress.numFailedTests;
+        new RunningProcess(test, allowRetry, this).start();
       }
       _numProcesses++;
     }