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 a88ac675d37a6ccae9e8b8723c6c6ece3ddd1a9a..5652ad410e47c5b39a585accfe367e5388f1a675 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'];

[Emily Fortuna, 2012/02/21 18:19:35]

Why use one dash here (-batch) and two dashes below (--batch)?

[Jennifer Messerly, 2012/02/21 18:58:43]

I didn't want to change the DartC test runner. Likewise, I don't think Python's
option parser library will handle a single "-". It's ugly but I'm not sure what
else to do.

[Emily Fortuna, 2012/02/21 19:35:33]

Ah, okay. Carry on!

+  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);
 }
 
 
@@ -217,7 +224,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;
       }
@@ -240,6 +247,7 @@ class TestOutput {
  * be garbage collected as soon as it is done.
  */
 class RunningProcess {
+  ProcessQueue processQueue;
   Process process;
   TestCase testCase;
   bool timedOut = false;
@@ -249,7 +257,7 @@ class RunningProcess {
   List<String> stderr;
   List<Function> handlers;
 
-  RunningProcess(TestCase this.testCase);
+  RunningProcess(TestCase this.testCase, this.processQueue);

[Emily Fortuna, 2012/02/21 18:19:35]

Careful here. The VM uses the RunningProcess class in their own standalone test
suite. Either we need to modify their standalone test framework with the updated
constructor for RunningProcess, or make processQueue optional.

[Jennifer Messerly, 2012/02/21 18:58:43]

Done.

[Emily Fortuna, 2012/02/21 19:35:33]

FYI: I know this because I made the same mistake when I added an argument to
allow retrying of tests. :-) When you gclient sync, we're going to have to
resolve having multiple "optional" arguments.
(http://code.google.com/p/dart/source/diff?spec=svn4404&r=4372&format=side&pat...).

 
   void exitHandler(int exitCode) {
     new TestOutput(testCase, exitCode, timedOut, stdout,
@@ -285,7 +293,11 @@ 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') {
+        processQueue._getBatchRunner(testCase).startTest(testCase);

[Emily Fortuna, 2012/02/21 19:35:33]

probably need to add a check to make sure processQueue isn't null since it's
"optional" now.

+      } else {
+        runCommand(testCase.executablePath, testCase.arguments, exitHandler);
+      }
     }
   }
 
@@ -342,9 +354,9 @@ class RunningProcess {
   }
 }
 
-
-class DartcBatchRunnerProcess {
+class BatchRunnerProcess {
   String _executable;
+  List<String> _batchArguments;
 
   Process _process;
   StringInputStream _stdoutStream;
@@ -356,7 +368,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;
 
@@ -372,6 +390,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(() {
@@ -386,10 +405,20 @@ 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 browser

[Emily Fortuna, 2012/02/21 19:35:33]

I just reread this comment -- I think you left out some words or something got
edited out. Also, :%s/send/Send

[Jennifer Messerly, 2012/02/21 21:29:56]

fixed.

Side note: what's the deal with this capitalization rule? I haven't seen that
enforced in any CL previously. It seems a bit strict--especially since the rules
for capitalization after a colon isn't even agreed upon in English:
http://en.wikipedia.org/wiki/Colon_(punctuation)#Use_of_capitals

[Emily Fortuna, 2012/02/21 21:40:33]

Ah, perhaps, I'm being overzealous in applying capitalization. I was following
this:
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Commen....
In the TODO section, it doesn't explicitly say the first word of the TODO has to
be capitalized, but the examples do so:
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=TODO_C....


ps. Fun sidenote, enjoy the example returns in FetchBigtableRows from the first
link.

On 2012/02/21 21:29:56, John Messerly wrote:

+        // the open browser processes. TODO(jmesserly): send a signal once
+        // that's supported, see dartbug.com/1756.
+        new Timer((e) { if (!closed) _process.kill(); }, 30000);

[Emily Fortuna, 2012/02/21 18:19:35]

Where's 30000 coming from?

[Jennifer Messerly, 2012/02/21 18:58:43]

Needed a timeout :) Unfortunately, we don't have access to a testCase at this
point. And it's not clear if that would be the right timeout to use, since this
is a timeout for how long we think run_selenium will take to shutdown, not how
long an individual test needs.

I thought about storing it in a variable, but I wasn't sure that a level of
indirection adds anything. The hope was that the line was fairly self
explanatory ("in 30s, if the process is not closed, kill it")

[Emily Fortuna, 2012/02/21 19:35:33]

Call me a curmudgeon, but I'd like it if you made it a variable that says
something to that effect (SELENIUM_SHUTDOWN_TIME). Yes, it adds a level of
indirection, but it explains how you (somewhat randomly) picked the number. :-)

[Jennifer Messerly, 2012/02/21 21:29:56]

Added a comment to that effect. Also moved the timer line down, since the
"graceful shutdown" comment applies to the "--terminate".

+        _process.stdin.write('--terminate\n'.charCodes());
+      } else {
+        _process.kill();
+      }
     }
   }
 
@@ -399,8 +428,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) {
@@ -456,20 +486,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>();
@@ -481,7 +509,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
@@ -507,8 +534,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.
@@ -531,7 +559,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 = '';
@@ -574,8 +602,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'],

[Emily Fortuna, 2012/02/21 18:19:35]

(+ firefox-bin) yay.

+        'chrome': ['chromedriver', chromeName]};
     for (String name in processNames[browserUsed]) {
       Process process = null;
       if (new Platform().operatingSystem() == 'windows') {
@@ -620,7 +649,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/') ||
@@ -657,21 +686,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.');
@@ -699,12 +734,11 @@ 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 {
-        new RunningProcess(test).start();
+        new RunningProcess(test, this).start();
       }
       _numProcesses++;
     }