Change Process.start to return a future that completes with a

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.
  */
 #library("test_runner");
 
 #import("dart:io");
 #import("dart:isolate");
 #import("status_file_parser.dart");
 #import("test_progress.dart");
 #import("test_suite.dart");
 
 const int NO_TIMEOUT = 0;
 const int SLOW_TIMEOUT_MULTIPLIER = 4;
 
 typedef void TestCaseEvent(TestCase testCase);
 typedef void ExitCodeEvent(int exitCode);
-typedef bool EnqueueMoreWork(ProcessQueue queue);
+typedef void EnqueueMoreWork(ProcessQueue queue);
 
 /** A command executed as a step in a test case. */
 class Command {
   /** Path to the executable of this command. */
   String executable;
 
   /** Command line arguments to the executable. */
   List<String> arguments;
 
   /** The actual command line that will be executed. */
@@ skipping 566 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 (currentStep == totalSteps) { // done with test command
+    if (timedOut) {
+      // Test timed out before it could complete.
+      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 == ie, 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();
         processQueue._getBatchRunner(testCase).startTest(testCase);
@@ skipping 19 matching lines @@
   void start() {
     Expect.isFalse(testCase.expectedOutcomes.contains(SKIP));
     stdout = new List<String>();
     stderr = new List<String>();
     currentStep = 0;
     startTime = new Date.now();
     runCommand(testCase.commands[currentStep++], stepExitHandler);
   }
 
   void runCommand(Command command, void exitHandler(int exitCode)) {
-    process = Process.start(command.executable, command.arguments);
-    process.onExit = exitHandler;
-    process.onError = (e) {
-      print("Error starting process:");
+    void processErrorHandler(e) {
+      print("Process error:");
       print("  Command: $command");
       print("  Error: $e");
       testComplete(-1, false);
-    };
-    InputStream stdoutStream = process.stdout;
-    InputStream stderrStream = process.stderr;
-    StringInputStream stdoutStringStream = new StringInputStream(stdoutStream);
-    StringInputStream stderrStringStream = new StringInputStream(stderrStream);
-    stdoutStringStream.onLine =
-        makeReadHandler(stdoutStringStream, stdout);
-    stderrStringStream.onLine =
-        makeReadHandler(stderrStringStream, stderr);
-    if (timeoutTimer == null) {
-      // Create one timeout timer when starting test case, remove it at end.
-      timeoutTimer = new Timer(1000 * testCase.timeout, timeoutHandler);
     }
+    Future processFuture = Process.start(command.executable, command.arguments);

[ahe, 2012/10/11 18:39:50]

Consider using cascading here.

[Mads Ager (google), 2012/10/12 08:44:46]

I did and for this code I found it too confusing because the body of the 'then'
part is too big.

+    processFuture.then((p) {
+      process = p;
+      process.onExit = exitHandler;
+      process.onError = processErrorHandler;
+      InputStream stdoutStream = process.stdout;
+      InputStream stderrStream = process.stderr;
+      StringInputStream stdoutStringStream =
+          new StringInputStream(stdoutStream);

[Emily Fortuna, 2012/10/11 18:23:08]

consider:
var stdoutStringStream = new StringInputStream(stdoutStream);

otherwise we're starting to look an awful lot like Java

[Mads Ager (google), 2012/10/12 08:44:46]

Agreed! I kept it as it was but I will be more than happy to go to 'var' here.
:-)

I collapsed this to two lines instead of four. No reason to declare stdoutStream
or stderrStream here either.

+      StringInputStream stderrStringStream =
+          new StringInputStream(stderrStream);
+      stdoutStringStream.onLine =
+          makeReadHandler(stdoutStringStream, stdout);
+      stderrStringStream.onLine =
+          makeReadHandler(stderrStringStream, stderr);
+      if (timeoutTimer == null) {
+        // Create one timeout timer when starting test case, remove it at end.
+        timeoutTimer = new Timer(1000 * testCase.timeout, timeoutHandler);
+      }
+      // If the timeout fired in between two commands, kill the just
+      // started process immediately.
+      if (timedOut) process.kill();

[ricow1, 2012/10/11 17:27:34]

Why not just do p.kill() at the start of this closure and then return?

[Mads Ager (google), 2012/10/12 08:44:46]

Because we need the exit handler set up so that we will get to the part of the
code that marks the test as completed and moves on to the next test.

Since we need that handler set up I thought that it would be cleanest if we
always set up everything and then in the rare timeout case kill the process
afterwards.

+    });
+    processFuture.handleException((e) {

[ahe, 2012/10/11 18:39:50]

Shouldn't this API be the same as process.onError so you don't have to wrap the
exception handler.

[Mads Ager (google), 2012/10/12 08:44:46]

That would be nice, yes. I'll eliminate onError on process instead which will
avoid this issue with double-up error handling as well.

+      processErrorHandler(e);
+      return true;
+    });
   }
 
   void timeoutHandler(Timer unusedTimer) {
     timedOut = true;
-    process.kill();
+    if (process != null) process.kill();
   }
 }
 
 /**
  * This class holds a value, that can be changed.  It is used when
  * closures need a shared value, that they can all change and read.
  */
 class MutableValue<T> {
   MutableValue(T this.value);
   T value;
@@ skipping 200 matching lines @@
           line = _stdoutStream.readLine();
         }
         line = _stderrStream.readLine();
         while (line != null) {
           _testStderr.add(line);
           line = _stderrStream.readLine();
         }
         _stderrDrained = true;
         _stdoutDrained = true;
         _process.close();
         _startProcess(() { _reportResult(); });

[ricow1, 2012/10/11 17:27:34]

why not just pass _reportResult in here instead of making an extra closure?

[Mads Ager (google), 2012/10/12 08:44:46]

Done.

       } else {  // No active test case running.
         _process.close();
         _process = null;
       }
     }
     return handler;
   }
 
   void _timeoutHandler(ignore) {
     _process.onExit = makeExitHandler(">>> TEST TIMEOUT");
     _process.kill();
   }
 
-  void _startProcess(then) {
-    _process = Process.start(_executable, _batchArguments);
-    _stdoutStream = new StringInputStream(_process.stdout);
-    _stderrStream = new StringInputStream(_process.stderr);
-    _process.onExit = makeExitHandler(">>> TEST CRASH");
-    _process.onError = (e) {
-      print("Error starting process:");
-      print("  Command: $_executable ${Strings.join(_batchArguments, ' ')}");
-      print("  Error: $e");
-      // If there is an error starting a batch process, chances are that
-      // it will always fail. So rather than re-trying a 1000+ times, we
-      // exit.
-      exit(1);
-    };
-    _process.onStart = then;
+  void _processErrorHandler(e) {
+    print("Process error:");
+    print("  Command: $_executable ${Strings.join(_batchArguments, ' ')}");
+    print("  Error: $e");
+    // If there is an error starting a batch process, chances are that
+    // it will always fail. So rather than re-trying a 1000+ times, we
+    // exit.

[ahe, 2012/10/11 18:39:50]

+1000!

+    exit(1);
+  }
+
+  _startProcess(then) {

[ricow1, 2012/10/11 17:27:34]

should we change "then" to another name here to not confuse this with the Future
then (e.g., setupDone or simply callback

[Emily Fortuna, 2012/10/11 18:23:08]

+1

[Mads Ager (google), 2012/10/12 08:44:46]

Done.

+    Future processFuture = Process.start(_executable, _batchArguments);
+    processFuture.then((p) {
+      _process = p;
+      _stdoutStream = new StringInputStream(_process.stdout);
+      _stderrStream = new StringInputStream(_process.stderr);
+      _process.onExit = makeExitHandler(">>> TEST CRASH");
+      _process.onError = _processErrorHandler;
+      then();
+    });
+    processFuture.handleException((e) {
+      _processErrorHandler(e);

[ahe, 2012/10/11 18:39:50]

Aren't you getting tired of wrapping this? ;-)

+      return true;
+    });
   }
 }
 
 /**
  * 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
@@ skipping 128 matching lines @@
     if (!_isSeleniumAvailable && !_startingServer) {
       _startingServer = true;
 
       // Check to see if the jar was already running before the program started.
       String cmd = 'ps';
       var arg = ['aux'];
       if (Platform.operatingSystem == 'windows') {
         cmd = 'tasklist';
         arg.add('/v');
       }
-      Process p = Process.start(cmd, arg);
-      final StringInputStream stdoutStringStream =
-          new StringInputStream(p.stdout);
-      p.onError = (e) {
+
+      processErrorHandler(e) {
         print("Error starting process:");
         print("  Command: $cmd ${Strings.join(arg, ' ')}");
         print("  Error: $e");
         // TODO(ahe): How to report this as a test failure?
         exit(1);
-      };
-      stdoutStringStream.onLine = () {
-        var line = stdoutStringStream.readLine();
-        while (null != line) {
-          if (const RegExp(r".*selenium-server-standalone.*").hasMatch(line)) {
-            _seleniumAlreadyRunning = true;
-            resumeTesting();
+      }
+
+      Future processFuture = Process.start(cmd, arg);
+      processFuture.then((p) {

[ahe, 2012/10/11 18:39:50]

A type for p would be nice.

[Mads Ager (google), 2012/10/12 08:44:46]

Done.

+        final StringInputStream stdoutStringStream =
+            new StringInputStream(p.stdout);
+        p.onError = processErrorHandler;
+        stdoutStringStream.onLine = () {
+          var line = stdoutStringStream.readLine();
+          while (null != line) {
+            var regexp = const RegExp(r".*selenium-server-standalone.*");
+            if (regexp.hasMatch(line)) {
+              _seleniumAlreadyRunning = true;
+              resumeTesting();
+            }
+            line = stdoutStringStream.readLine();
           }
-          line = stdoutStringStream.readLine();
-        }
-        if (!_isSeleniumAvailable) {
-          _startSeleniumServer();
-        }
-      };
+          if (!_isSeleniumAvailable) {
+            _startSeleniumServer();
+          }
+        };
+      });
+      processFuture.handleException((e) {

[ahe, 2012/10/11 18:39:50]

Boring! ;-)

[Mads Ager (google), 2012/10/12 08:44:46]

Yawn! ;-)

+        processErrorHandler(e);
+        return true;
+      });
     }
   }
 
   void _runTest(TestCase test) {
     if (test.usesWebDriver) {
       browserUsed = test.configuration['browser'];

[Mads Ager (google), 2012/10/11 16:00:54]

This is not right. browserUsed is supposed to be a String but
configuration['browser'] is a bool. We probably need to use
configuration['runtime'] here, but I don't want to make that change now. There
are enough moving pieces in this change already. Emily, since browserUsed
becomes a boolean here which is not equal to any string, can we just get rid of
this and the special cases we have based on browserUsed?

[Emily Fortuna, 2012/10/11 18:23:08]

Right. I've put in this CL: https://codereview.chromium.org/11098075/ to fix
this.

[Mads Ager (google), 2012/10/12 08:44:46]

Thank you!

       if (_needsSelenium) _ensureSeleniumServerRunning();
     }
     _progress.testAdded();
     _tests.add(test);
     _tryRunTest();
   }
 
   /**
    * Monitor the output of the Selenium server, to know when we are ready to
    * begin running tests.
@@ skipping 22 matching lines @@
   void _startSeleniumServer() {
     // Get the absolute path to the Selenium jar.
     String filePath = TestUtils.testScriptPath;
     String pathSep = Platform.pathSeparator;
     int index = filePath.lastIndexOf(pathSep);
     filePath = '${filePath.substring(0, index)}${pathSep}testing${pathSep}';
     var lister = new Directory(filePath).list();
     lister.onFile = (String file) {
       if (const RegExp(r"selenium-server-standalone-.*\.jar").hasMatch(file)
           && _seleniumServer == null) {
-        _seleniumServer = Process.start('java', ['-jar', file]);
-        _seleniumServer.onError = (e) {
-          print("Error starting process:");
+        void processErrorHandler(e) {
+          print("Process error:");
           print("  Command: java -jar $file");
           print("  Error: $e");
           // TODO(ahe): How to report this as a test failure?
           exit(1);
-        };
-        // Heads up: there seems to an obscure data race of some form in
-        // the VM between launching the server process and launching the test
-        // tasks that disappears when you read IO (which is convenient, since
-        // that is our condition for knowing that the server is ready).
-        StringInputStream stdoutStringStream =
-            new StringInputStream(_seleniumServer.stdout);
-        StringInputStream stderrStringStream =
-            new StringInputStream(_seleniumServer.stderr);
-        stdoutStringStream.onLine =
-            makeSeleniumServerHandler(stdoutStringStream);
-        stderrStringStream.onLine =
-            makeSeleniumServerHandler(stderrStringStream);
+        }
+        Future processFuture = Process.start('java', ['-jar', file]);
+        processFuture.then((server) {
+          _seleniumServer = server;
+          _seleniumServer.onError = processErrorHandler;
+          // Heads up: there seems to an obscure data race of some form in
+          // the VM between launching the server process and launching the test
+          // tasks that disappears when you read IO (which is convenient, since
+          // that is our condition for knowing that the server is ready).
+          StringInputStream stdoutStringStream =
+              new StringInputStream(_seleniumServer.stdout);
+          StringInputStream stderrStringStream =
+              new StringInputStream(_seleniumServer.stderr);
+          stdoutStringStream.onLine =
+              makeSeleniumServerHandler(stdoutStringStream);
+          stderrStringStream.onLine =
+              makeSeleniumServerHandler(stderrStringStream);
+        });
+        processFuture.handleException((e) {
+          processErrorHandler(e);
+          return true;
+        });
       }
     };
   }
 
   Future _terminateBatchRunners() {
     var futures = new List();
     for (var runners in _batchProcesses.getValues()) {
       for (var runner in runners) {
         futures.add(runner.terminate());
       }
@@ skipping 65 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++;
     }
   }
 }