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

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 4d8917b3dfa2e99ea71a636271e7b55686b6f71e..567ba0d4b56231f37c85c089c5a7439fec7d5f6d 100644
--- a/tools/testing/dart/test_runner.dart
+++ b/tools/testing/dart/test_runner.dart
@@ -22,7 +22,7 @@ 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 {
@@ -609,14 +609,22 @@ class RunningProcess {
    */
   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 &&
@@ -656,31 +664,44 @@ class RunningProcess {
   }
 
   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();
   }
 }
 
@@ -915,21 +936,30 @@ class BatchRunnerProcess {
     _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;
+    });
   }
 }
 
@@ -1078,29 +1108,39 @@ class ProcessQueue {
         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;
+      });
     }
   }
 
@@ -1149,26 +1189,34 @@ class ProcessQueue {
     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;
+        });
       }
     };
   }