Add browser controller and allow it to be used under a flag.

tools/testing/dart/test_runner.dart

Index: tools/testing/dart/test_runner.dart
===================================================================
--- tools/testing/dart/test_runner.dart	(revision 22503)
+++ tools/testing/dart/test_runner.dart	(working copy)
@@ -18,6 +18,7 @@
 import "dart:io" as io;
 import "dart:isolate";
 import "dart:uri";
+import "browser_controler.dart";

[kustermann, 2013/05/13 16:00:00]

browser_controler -> browser_controller + file rename.

[ricow1, 2013/05/14 07:20:58]

Done.

 import "http_server.dart" as http_server;
 import "status_file_parser.dart";
 import "test_progress.dart";
@@ -348,6 +349,8 @@
 
   bool get usesWebDriver => TestUtils.usesWebDriver(configuration['runtime']);
 
+  bool get usesBrowserControler => configuration['use_browser_controller'];

[kustermann, 2013/05/13 16:00:00]

Controler -> Controller

[ricow1, 2013/05/14 07:20:58]

Done.

+
   void completed() { completedHandler(this); }
 
   bool get isFlaky {
@@ -526,7 +529,17 @@
                                      List<int> stderr,
                                      Duration time,
                                      bool compilationSkipped) {
-    if (testCase is BrowserTestCase) {
+    if (testCase.usesBrowserControler) {
+      return new HTMLBrowserCommandOutputImpl(testCase,
+                                              command,
+                                              exitCode,
+                                              incomplete,
+                                              timedOut,
+                                              stdout,
+                                              stderr,
+                                              time,
+                                              compilationSkipped);
+    } else if (testCase is BrowserTestCase) {
       return new BrowserCommandOutputImpl(testCase,
                                           command,
                                           exitCode,
@@ -746,6 +759,36 @@
   }
 }
 
+class HTMLBrowserCommandOutputImpl extends BrowserCommandOutputImpl {
+ HTMLBrowserCommandOutputImpl(
+      testCase,
+      command,
+      exitCode,
+      incomplete,
+      timedOut,
+      stdout,
+      stderr,
+      time,
+      compilationSkipped) :
+    super(testCase,
+          command,
+          exitCode,
+          incomplete,
+          timedOut,
+          stdout,
+          stderr,
+          time,
+          compilationSkipped);
+
+  bool get _browserTestFailure {
+    // We should not need to convert back and forward.
+    var output = decodeUtf8(super.stdout);
+    if (output.contains("FAIL")) return true;
+    return !output.contains("PASS");
+  }
+}
+
+
 // The static analyzer does not actually execute code, so
 // the criteria for success now depend on the text sent
 // to stderr.
@@ -1295,6 +1338,8 @@
   // system, generate tests, and search test files for options.
   Map<String, List<TestInformation>> _testCache;
 
+  Map<String, BrowserTestRunner> _browserTestRunners;
+
   /**
    * String indicating the browser used to run the tests. Empty if no browser
    * used.
@@ -1325,7 +1370,8 @@
         _listTests = listTests,
         _tests = new Queue<TestCase>(),
         _batchProcesses = new Map<String, List<BatchRunnerProcess>>(),
-        _testCache = new Map<String, List<TestInformation>>() {
+        _testCache = new Map<String, List<TestInformation>>(),
+        _browserTestRunners = new Map<String, BrowserTestRunner>(){

[kustermann, 2013/05/13 16:00:00]

space before {

[ricow1, 2013/05/14 07:20:58]

Done.

     _runTests(testSuites);
   }
 
@@ -1343,7 +1389,9 @@
 
   void _checkDone() {
     if (_allTestsWereEnqueued && _tests.isEmpty && _numProcesses == 0) {
-      _terminateBatchRunners().then((_) => _cleanupAndMarkDone());
+      _terminateBatchRunners().then((_) {
+        _terminateBrowserRunners().then((_) => _cleanupAndMarkDone());
+      });
     }
   }
 
@@ -1506,6 +1554,14 @@
     return Future.wait(futures);
   }
 
+  Future _terminateBrowserRunners() {
+    var futures = [];
+    for (BrowserTestRunner runner in _browserTestRunners.values) {
+      futures.add(runner.terminate());
+    }
+    return Future.wait(futures);
+  }
+
   BatchRunnerProcess _getBatchRunner(TestCase test) {
     // Start batch processes if needed
     var compiler = test.configuration['compiler'];
@@ -1524,8 +1580,55 @@
     throw new Exception('Unable to find inactive batch runner.');
   }
 
+  Future<BrowserTestRunner> getBrowserTestRunner(TestCase test) {

[kustermann, 2013/05/13 16:00:00]

You could make it private as well.

[ricow1, 2013/05/14 07:20:58]

Done.

+    var runtime = test.configuration['runtime'];
+    if (_browserTestRunners[runtime] == null) {
+      var testRunner = new BrowserTestRunner(runtime, 4);

[kustermann, 2013/05/13 16:00:00]

Make a constant.

[ricow1, 2013/05/14 07:20:58]

Done.

+      _browserTestRunners[runtime] = testRunner;
+      return testRunner.start().then((started) {
+        if (started) {
+          return testRunner;
+        }
+        return null;

[kustermann, 2013/05/13 16:00:00]

If we're unable to start the browser then we'll get a null pointer exception
further down. Maybe we should just exit() and print a appropriate error message.

[ricow1, 2013/05/14 07:20:58]

Done.

[ricow1, 2013/05/14 07:20:58]

Done 

+      });
+    }
+    return new Future.immediate(_browserTestRunners[runtime]);
+  }
+
+  void startBrowserControllerTest(var test) {

[kustermann, 2013/05/13 16:00:00]

private?

[ricow1, 2013/05/14 07:20:58]

Done.

+    // Get the url.
+    // TODO(ricow): This is not needed when we have eliminated selenium.
+    var nextCommandIndex = test.commandOutputs.keys.length;
+    var url = test.commands[nextCommandIndex].toString().split("--out=")[1];
+    // Remove trailing "
+    url = url.split('"')[0];
+    var callback = (var output) {
+      new CommandOutput.fromCase(test,
+                                 test.commands[nextCommandIndex],
+                                 0,
+                                 false,
+                                 output == "TIMEOUT",
+                                 output.codeUnits,

[kustermann, 2013/05/13 16:00:00]

Please use encodeUtf8String or so. This is supposed to be a list of bytes not a
list of unicode points.

[ricow1, 2013/05/14 07:20:58]

Done.

+                                 [],
+                                 const Duration(seconds: 1),
+                                 false);
+      test.completedHandler(test);
+    };
+    BrowserTest browserTest = new BrowserTest(url,
+                                              callback,
+                                              test.timeout);

[kustermann, 2013/05/13 16:00:00]

Does this fit on one line?

[ricow1, 2013/05/14 07:20:58]

Yes

+    getBrowserTestRunner(test).then((testRunner) {
+      testRunner.queueTest(browserTest);
+    });
+  }
+
   void _tryRunTest() {
     _checkDone();
+    // We oversubscribe the cpus with the started browsers when we use our

[kustermann, 2013/05/13 16:00:00]

This comment seems to be wrong.

+    // internal browser driver. We use a simpler queue based setup.
+    // TODO(ricow): remove most of the hacked selenium code below when
+    // we have eliminated the need.
+
     if (_numProcesses < _maxProcesses && !_tests.isEmpty) {
       TestCase test = _tests.removeFirst();
       if (_listTests) {
@@ -1536,8 +1639,8 @@
         print(fields.join('\t'));
         return;
       }
-      if (test.usesWebDriver && _needsSelenium && !_isSeleniumAvailable || (test
-          is BrowserTestCase && test.waitingForOtherTest)) {
+      if (test.usesWebDriver && _needsSelenium && !_isSeleniumAvailable || 
+          (test is BrowserTestCase && test.waitingForOtherTest)) {
         // The test is not yet ready to run. Put the test back in
         // the queue.  Avoid spin-polling by using a timeout.
         _tests.add(test);
@@ -1598,7 +1701,12 @@
           _tryRunTest();
         };
         test.completedHandler = testCompleted;
-        _getBatchRunner(test).startTest(test);
+
+        if (test.usesBrowserControler) {

[kustermann, 2013/05/13 16:00:00]

I'm not sure if we should make this an attribute of TestCase. It seems to me
that 'usesBrowserControler' is a configuration wide setting. Maybe we should
move this attribute to the configuration?

[ricow1, 2013/05/14 07:20:58]

It is part of the configuration, see how we return it from the test case. I do,
however, think that this provides the best flexibility, we could change the way
we do this right?
Additionally, this is consistent with how we do it for other stuff
(usesWebDriver etc)

+          startBrowserControllerTest(test);
+        } else {
+          _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.