reuse the same browser when running webdriver tests

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 81 matching lines @@
 
   int get timeout() => configuration['timeout'];
 
   String get configurationString() {
     final component = configuration['component'];
     final mode = configuration['mode'];
     final arch = configuration['arch'];
     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); }
 }
 
 
 /**
  * BrowserTestCase has an extra compilation command that is run in a separate
  * process, before the regular test is run as in the base class [TestCase].
  * If the compilation command fails, then the rest of the test is not run.
  */
 class BrowserTestCase extends TestCase {
@@ skipping 27 matching lines @@
                                                   expectedOutcomes,
                                                   isNegative) {
     if (compilerPath != null) {
       commandLine = 'execution command: $commandLine';
       String compilationCommand =
           '$compilerPath ${Strings.join(compilerArguments, " ")}';
       commandLine = 'compilation command: $compilationCommand\n$commandLine';
     }
     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);
 }
 
 
 /**
  * TestOutput records the output of a completed test: the process's exit code,
  * the standard output and standard error, whether the process timed out, and
  * the time the process took to run.  It also contains a pointer to the
  * [TestCase] this is the output of.
  */
 class TestOutput {
@@ skipping 51 matching lines @@
       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.
     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. 
+        // the test.
         requestRetry = true;
         return true;
       }
     }
     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 {
+  ProcessQueue processQueue;
   Process process;
   TestCase testCase;
   bool timedOut = false;
   Date startTime;
   Timer timeoutTimer;
   List<String> stdout;
   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,
                    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);
@@ skipping 15 matching lines @@
   }
 
   void compilerExitHandler(int exitCode) {
     if (exitCode != 0) {
       stderr.add('test.dart: Compilation step failed (exit code $exitCode)\n');
       exitHandler(exitCode);
     } else {
       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);
+      }
     }
   }
 
   Function makeReadHandler(StringInputStream source, List<String> destination) {
     return () {
       if (source.closed) return;  // TODO(whesse): Remove when bug is fixed.
       var line = source.readLine();
       while (null != line) {
         destination.add(line);
         line = source.readLine();
@@ skipping 36 matching lines @@
         makeReadHandler(stderrStringStream, stderr);
     timeoutTimer = new Timer(timeoutHandler, 1000 * testCase.timeout);
   }
 
   void timeoutHandler(Timer unusedTimer) {
     timedOut = true;
     process.kill();
   }
 }
 
-
-class DartcBatchRunnerProcess {
+class BatchRunnerProcess {
   String _executable;
+  List<String> _batchArguments;
 
   Process _process;
   StringInputStream _stdoutStream;
   StringInputStream _stderrStream;
 
   TestCase _currentTest;
   List<String> _testStdout;
   List<String> _testStderr;
   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;
 
   void startTest(TestCase testCase) {
     _currentTest = testCase;
     if (_process === null) {
       // Start process if not yet started.
       _executable = testCase.executablePath;
       _startProcess(() {
         doStartTest(testCase);
       });
     } else if (testCase.executablePath != _executable) {
       // Restart this runner with the right executable for this test
       // if needed.
       _executable = testCase.executablePath;
+      _batchArguments = testCase.batchRunnerArguments;
       _process.exitHandler = (exitCode) {
         _process.close();
         _startProcess(() {
           doStartTest(testCase);
         });
       };
       _process.kill();
     } else {
       doStartTest(testCase);
     }
   }
 
   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();
+      }
     }
   }
 
   void doStartTest(TestCase testCase) {
     _startTime = new Date.now();
     _testStdout = new List<String>();
     _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) {
     return Strings.join(arguments, ' ') + '\n';
   }
 
   int _reportResult(String output) {
     var test = _currentTest;
     _currentTest = null;
 
@@ skipping 35 matching lines @@
   }
 
   void _exitHandler(exitCode) {
     if (_timer != null) _timer.cancel();
     _process.close();
     _startProcess(() {
       _reportResult(">>> TEST CRASH");
     });
   }
 
-  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>();
     _testStderr = new List<String>();
     _stdoutStream.lineHandler = _readOutput(_stdoutStream, _testStdout);
     _stderrStream.lineHandler = _readOutput(_stderrStream, _testStderr);
     _process.exitHandler = _exitHandler;
     _process.startHandler = then;
   }
 }
 
-
 /**
  * ProcessQueue is the master control class, responsible for running all
  * the tests in all the TestSuites that have been registered.  It includes
  * a rate-limited queue to run a limited number of tests in parallel,
  * a ProgressIndicator which prints output when tests are started and
  * and completed, and a summary report when all tests are completed,
  * and counters to determine when all of the tests in all of the test suites
  * have completed.
  *
  * 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;
   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;
+  // 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.
   Map<String, List<TestInformation>> _testCache;
   /**
    * String indicating the browser used to run the tests. Empty if no browser
    * used.
    */
   String browserUsed;
 
   ProcessQueue(int this._maxProcesses,
                String progress,
                Date startTime,
                bool printTiming,
                Function this._enqueueMoreWork,
                [bool this._verbose = false,
                 bool this._listTests = false,
                 bool this._keepGeneratedTests = false])
       : _tests = new Queue<TestCase>(),
         _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 = '';
   }
 
   /**
    * Registers a TestSuite so that all of its tests will be run.
    */
   void addTestSuite(TestSuite testSuite) {
     _activeTestListers++;
@@ skipping 22 matching lines @@
   /**
    * Sometimes Webdriver doesn't close every browser window when it's done
    * with a test. At the end of all tests we clear out any neglected processes
    * that are still running.
    */
   void killZombieBrowsers() {
     String chromeName = 'chrome';
     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') {
         process = new Process.start(
             'C:\\Windows\\System32\\taskkill.exe', ['/F', '/IM', name + '.exe',
             '/T']);
       } else {
         process = new Process.start('killall', ['-9', name]);
       }
 
@@ skipping 24 matching lines @@
       _progress.allDone();
     }
   }
 
   void _checkDone() {
     // When there are no more active test listers ask for more work
     // from process queue users.
     if (_activeTestListers == 0 && !_enqueueMoreWork(this)) {
       _progress.allTestsKnown();
       if (_tests.isEmpty() && _numProcesses == 0) {
-        _terminateDartcBatchRunners();
+        _terminateBatchRunners();
         if (_keepGeneratedTests || _temporaryDirectory == null) {
           _cleanupAndMarkDone();
         } else if (!_temporaryDirectory.startsWith('/tmp/') ||
                    _temporaryDirectory.contains('/../')) {
           // Let's be extra careful, since rm -rf is so dangerous.
           print('Temporary directory $_temporaryDirectory unsafe to delete!');
           _cleanupAndMarkDone();
         } else {
           // TODO(dart:1211): Use delete(recursive=true) in Dart when it is
           // implemented, and add Windows support.
@@ skipping 16 matching lines @@
 
   void _runTest(TestCase test) {
     if (test.configuration['component'] == 'webdriver') {
       browserUsed = test.configuration['browser'];
     }
     _progress.testAdded();
     _tests.add(test);
     _tryRunTest();
   }
 
-  void _terminateDartcBatchRunners() {
-    _batchProcesses.forEach((runner) => runner.terminate());
-  }
-
-  void _ensureDartcBatchRunnersStarted(String executable) {
-    if (_batchProcesses.length == 0) {
-      for (int i = 0; i < _maxProcesses; i++) {
-        _batchProcesses.add(new DartcBatchRunnerProcess(executable));
+  void _terminateBatchRunners() {
+    for (var runners in _batchProcesses.getValues()) {
+      for (var runner in runners) {
+        runner.terminate();
       }
     }
   }
 
-  DartcBatchRunnerProcess _getDartcBatchRunnerProcess() {
-    for (int i = 0; i < _batchProcesses.length; i++) {
-      var runner = _batchProcesses[i];
+  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++) {
+        runners[i] = new BatchRunnerProcess(test);
+      }
+      _batchProcesses[component] = runners;
+    }
+
+    for (var runner in runners) {
       if (!runner.active) return runner;
     }
     throw new Exception('Unable to find inactive batch runner.');
   }
 
   void _tryRunTest() {
     _checkDone();
     if (_numProcesses < _maxProcesses && !_tests.isEmpty()) {
       TestCase test = _tests.removeFirst();
       if (_verbose) print(test.commandLine);
       if (_listTests) {
         final String tab = '\t';
         String outcomes =
             Strings.join(new List.from(test.expectedOutcomes), ',');
         print(test.displayName + tab + outcomes + tab + test.isNegative +
               tab + Strings.join(test.arguments, tab));
         return;
       }
       _progress.start(test);
       Function oldCallback = test.completedHandler;
       Function wrapper = (TestCase test_arg) {
         _numProcesses--;
         _progress.done(test_arg);
         _tryRunTest();
         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++;
     }
   }
 }