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

tools/testing/dart/browser_controler.dart

Index: tools/testing/dart/browser_controler.dart
===================================================================
--- tools/testing/dart/browser_controler.dart	(revision 0)
+++ tools/testing/dart/browser_controler.dart	(revision 0)
@@ -0,0 +1,673 @@
+// Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file

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

2013

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

Done.

+// 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.
+library browser;
+
+import "dart:io";
+import "dart:async";
+import "dart:core";

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

You could sort them.

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

Done.

+
+
+/** Class describing the interface for communicating with browsers. */
+class Browser {
+  // Browsers actually takes a while to cleanup after itself when closing
+  // Give it sufficient time to do that.
+  static final Duration killRepeatInternal = const Duration(seconds: 10);
+  static final int killRetries = 5;
+  StringBuffer _stdout = new StringBuffer();
+  StringBuffer _stderr = new StringBuffer();
+  StringBuffer _usageLog = new StringBuffer();
+  // This function is called when the process is closed.
+  // This is extracted to an external function so that we can do additional
+  // functionality when the process closes (cleanup and call onExit)
+  Function _processClosed;
+  // This is called after the process is closed, after _processClosed has
+  // been called, but before onExit. Subclasses can use this to cleanup
+  // any browser specific resources (temp directories, profiles, etc)
+  Function _cleanup;
+
+  /** The version of the browser - normally set when starting a browser */
+  String version = "";
+  /**
+   * The underlying process - don't mess directly with this if you don't
+   * know what you are doing (this is an interactive process that needs
+   * special threatment to not leak).
+   */
+  Process process;
+
+  /**
+   * Id of the browser
+   */
+  String id;
+
+  /** Callback that will be executed when the browser has closed */
+  Function onClose;
+
+  /** Print everything (stdout, stderr, usageLog) whenever we add to it */
+  bool debugPrint = false;
+
+  void _logEvent(String event) {
+    String toLog = "$this ($id) - ${new DateTime.now()}: $event \n";
+    if (debugPrint) print("usageLog: $toLog");
+    _usageLog.write(toLog);
+  }
+
+  void _addStdout(String output) {
+    if (debugPrint) print("stdout: $output");
+    _stdout.write(output);
+  }
+
+  void _addStderr(String output) {
+    if (debugPrint) print("stderr: $output");
+    _stderr.write(output);
+  }
+
+  // Kill the underlying process using the supplied kill function
+  // If there is a alternativeKillFunction we will use that after trying
+  // the default killFunction.
+  Future _killIt(killFunction, retries, [alternativeKillFunction = null]) {
+    Completer<bool> completer = new Completer<bool>();
+
+    // To capture non successfull attempts we set up a timer that will
+    // trigger a retry (using the alternativeKillFunction if supplied).
+    Timer timer =  new Timer(killRepeatInternal, () {
+      // Remove the handler, we will set this again in the call to killIt
+      // below
+      if (retries <= 0) {
+        _logEvent("Could not kill the process, not trying anymore");
+        // TODO(ricow): Should we crash the test script here and
+        // write out all our log. This is basically not a situation
+        // that we want to ignore. We could potentially have a handler we
+        // can call if this happens, which will shutdown the main process
+        // with info that people should contact [ricow,kustermann,?]
+        completer.complete(false);
+      }
+      _logEvent("Could not kill the process, retrying");
+      var nextKillFunction = killFunction;
+      if (alternativeKillFunction != null) {
+        nextKillFunction = alternativeKillFunction;
+      }
+      _killIt(nextKillFunction, --retries).then((success) {

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

Why not 'retries -1'?

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

Done.

+        completer.complete(success);
+      });
+    });
+
+    // Make sure we intercept onExit calls and eliminate the timer.
+    _processClosed = () {
+      timer.cancel();
+      _logEvent("Proccess exited, cancel timer in kill loop");
+      _processClosed = null;
+      completer.complete(true);
+    };
+
+
+    _logEvent("calling kill function");
+    if (killFunction()) {
+      // We successfully sent the signal.
+      _logEvent("killing signal sent");
+    } else {
+      _logEvent("The process is already dead, kill signal could not be send");
+      completer.complete(true);
+    }
+    return completer.future;
+  }
+
+
+  /** Close the browser */
+  Future<bool> close() {
+    _logEvent("Close called on browser");
+    if (process == null) {
+      _logEvent("No process open, nothing to kill.");
+      return new Future.immediate(true);
+    }
+    var killFunction = process.kill;
+    // We use a SIGKILL signal if we don't kill the process in the first go.
+    var alternativeKillFunction =
+        () { return process.kill(ProcessSignal.SIGKILL);};
+    return _killIt(killFunction, killRetries, alternativeKillFunction);
+  }
+
+  /**
+   * Start the browser using the supplied argument.
+   * This sets up the error handling and usage logging.
+   */
+  Future<bool> startBrowser(String command, List<String> arguments) {
+    return Process.start(command, arguments).then((p) {
+      process = p;
+      p.stdout.transform(new StringDecoder()).listen((data) {
+        _addStdout(data);
+      }, onError: (e) {
+        // This should _never_ happen, but we really want this in the log
+        // if it actually does due to dart:io or vm bug.
+        _usageLog.add("An error occured in the process stdout handling: $e");
+      });
+
+      p.stderr.transform(new StringDecoder()).listen((data) {
+        _addStderr(data);
+      }, onError: (e) {
+        // This should _never_ happen, but we really want this in the log
+        // if it actually does due to dart:io or vm bug.
+        _usageLog.add("An error occured in the process stderr handling: $e");
+        });

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

Indentation.

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

Done.

+
+      process.exitCode.then((exitCode) {
+        _logEvent("Browser closed with exitcode $exitCode");
+        if (_processClosed != null) _processClosed();
+        if (_cleanup != null) _cleanup();
+        if (onClose != null) onClose(exitCode);
+        });
+      return true;
+    }).catchError((e) {
+      _logEvent("Running $binary $arguments failed with $e");
+      return false;
+    });
+  }
+
+  /**
+   * Get any stdout that the browser wrote during execution.
+   */
+  String get stdout => _stdout.toString();
+  String get stderr => _stderr.toString();
+  String get usageLog => _usageLog.toString();
+
+  String toString();
+  /** Starts the browser loading the given url */
+  Future<bool> start(String url);
+}
+
+
+class Chrome extends Browser {
+  /**
+   * The binary used to run chrome - changing this can be nececcary for
+   * testing or using non standard chrome installation.
+   */
+  const String binary = "google-chrome";

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

We probably need to adjust the path to the binary on bots (in particular for
windows).

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

Yes, we may

+
+  Future<bool> start(String url) {
+    _logEvent("Starting chrome browser on: $url");
+    // Get the version and log that.
+    return Process.run(binary, ["--version"]).then((var versionResult) {
+      if (versionResult.exitCode != 0) {
+        _logEvent("Failed to chrome get version");
+        _logEvent("Make sure $binary is a valid program for running chrome");
+        return new Future.immediate(false);
+      }
+      version = versionResult.stdout;
+      _logEvent("Got version: $version");
+
+      return new Directory('').createTemp().then((userDir) {
+        _cleanup = () { userDir.delete(recursive: true); };
+        var args = ["--user-data-dir=${userDir.path}", url,
+                    "--disable-extensions", "--disable-popup-blocking",
+                    "--bwsi"];
+        return startBrowser(binary, args);
+
+      });
+    }).catchError((e) {
+      _logEvent("Running $binary --version failed with $e");
+      return false;
+    });
+  }
+
+  String toString() => "Chrome";
+}
+
+class Firefox extends Browser {
+  /**
+   * The binary used to run firefox - changing this can be nececcary for
+   * testing or using non standard firefox installation.
+   */
+  const String binary = "firefox";
+
+  const String enablePopUp =
+      "user_pref(\"dom.disable_open_during_load\", false);";
+
+  Future _createPreferenceFile(var path) {
+    var file = new File("${path.toString()}/user.js");
+    var randomFile = file.openSync(FileMode.WRITE);
+    randomFile.writeStringSync(enablePopUp);
+    randomFile.close();
+  }
+
+
+  Future<bool> start(String url) {
+    _logEvent("Starting firefox browser on: $url");
+    // Get the version and log that.
+    return Process.run(binary, ["--version"]).then((var versionResult) {
+      if (versionResult.exitCode != 0) {
+        _logEvent("Failed to firefox get version");
+        _logEvent("Make sure $binary is a valid program for running firefox");
+        return new Future.immediate(false);
+      }
+      version = versionResult.stdout;
+      _logEvent("Got version: $version");
+
+      return new Directory('').createTemp().then((userDir) {
+        _createPreferenceFile(userDir.path);
+        _cleanup = () { userDir.delete(recursive: true); };
+        var args = ["-profile", "${userDir.path}",
+                    "-no-remote", "-new-instance", url];
+        return startBrowser(binary, args);
+
+      });
+    }).catchError((e) {
+      _logEvent("Running $binary --version failed with $e");
+      return false;
+    });
+  }
+
+  String toString() => "Firefox";
+}
+
+
+/**
+ * Describes the current state of a browser used for testing.
+ */
+class BrowserTestingStatus {
+// TODO(ricow): Add prefetching to the browsers. We spend a lot of time waiting
+// for the next test. Handling timeouts is the hard part of this!
+
+  
+  Browser browser;
+  BrowserTest currentlyRunning;
+  // This is currently not used for anything except for error reporting.
+  // Given the usefulness of this in debugging issues this should not be
+  // removed even when we have really stable system.
+  BrowserTest last;
+  // 
+  bool timeout = false;
+  BrowserTestingStatus(Browser this.browser);
+}
+
+
+/**
+ * Describes a single test to be run int the browser.
+ */
+class BrowserTest {
+  // TODO(ricow): Add timeout callback instead of the string passing hack.
+  Function doneCallback;
+  String url;
+  int timeout;
+  // We store this here for easy access when tests time out (instead of
+  // capturing this in a closure)
+  Timer timeoutTimer;
+
+  // Used for debugging, this is simply a unique identifier assigned to each
+  // test.
+  int id;
+  static int _idCounter = 0;
+
+  BrowserTest(this.url, this.doneCallback, this.timeout) {
+    id = _idCounter++;
+  }
+}
+
+
+/**
+ * Encapsulates all the functionality for running tests in browsers.
+ * The interface is rather simple. After starting the runner tests
+ * are simply added to the queue and a the supplied callbacks are called
+ * whenever a test completes.
+ */
+class BrowserTestRunner {
+  int maxNumBrowsers;

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

I think we should always use '_' for private members. It makes the code easier
to read IMHO.

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

That depends on how easy you want testing to be

+  String browserName;
+
+  bool underTermination = false;
+
+  List<BrowserTest> testQueue = new List<BrowserTest>();
+  Map<String, BrowserTestingStatus> browserStatus =
+      new Map<String, BrowserTestingStatus>();
+  // This cache is used to guarantee that we never see double reporting.
+  // If we do we need to provide developers with this information.
+  // We don't add urls to the cache until we have run it.
+  Map<int, String> testCache = new Map<int, String>();
+  List<int> doubleReportingTests = new List<int>();
+
+  BrowserTestingServer testingServer;
+
+  BrowserTestRunner(String this.browserName, int this.maxNumBrowsers);
+
+  Future<bool> start() {
+    testingServer = new BrowserTestingServer();
+    return testingServer.start().then((_) {
+      testingServer.testDoneCallBack = handleResults;
+      testingServer.nextTestCallBack = getNextTest;
+      var futures = [];
+      for (int i = 0; i < maxNumBrowsers; i++) {
+        var browser = getInstance();
+        var id = "BROWSER$i";
+        // We store this in case we need to kill the browser.
+        browser.id = id;
+        var future =
+            browser.start(testingServer.getDriverUrl(id)).then((success) {
+              if (success) {
+                browserStatus[id] = new BrowserTestingStatus(browser);
+              }
+              return success;
+            });
+        futures.add(future);
+      }
+      return Future.wait(futures).then((values) {
+        return !values.contains(false);
+      });
+    });
+  }
+
+  var timedOut = [];
+
+  void handleResults(String browserId, String output, int testId) {
+    var status = browserStatus[browserId];
+    if (testCache.containsKey(testId)) {
+      doubleReportingTests.add(testId);

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

Maybe we should make the buildbot red in case of "double reports"?

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

I think that would make sense yes, but until the unit testing framework has been
fixed I don't want to do this. Additionally, I think we should do it in the
termination step, that way we can actually report all the double reporting tests
in one go, and then exit(1) (or return false from the future)

+      return; 
+    }
+
+    if (status.timeout) {
+      // We don't do anything, this browser is currently being killed and
+      // replaced.
+    } else if (status.currentlyRunning != null) {
+      status.currentlyRunning.timeoutTimer.cancel();
+      if (status.currentlyRunning.id != testId) {
+        print("Expected test id ${status.currentlyRunning.id} for"
+              "${status.currentlyRunning.url}");
+        print("Got test id ${testId}");
+        print("Last test id was ${status.last.id} for "
+              "${status.currentlyRunning.url}");
+        throw("This should never happen, wrong test id");
+      }
+      testCache[testId] = status.currentlyRunning.url;
+      status.currentlyRunning.doneCallback(output);
+      status.last = status.currentlyRunning;
+      status.currentlyRunning = null;
+    } else {
+      print("\nThis is bad, should never happen, handleResult no test");
+      print("URL: ${status.last.url}");
+      print(output);
+      terminate().then((_) {
+        exit(1);
+      });
+    }
+  }
+
+  void handleTimeout(BrowserTestingStatus status) {
+    // We simply kill the browser and starts up a new one!
+    // We could be smarter here, but it does not seems like it is worth it.
+    status.timeout = true;
+    timedOut.add(status.currentlyRunning.url);
+    // Start the new browser first
+    var browser = getInstance();
+    var id = status.browser.id;
+    browser.start(testingServer.getDriverUrl(id)).then((success) {

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

IMHO this is not the way we should do it. We should first kill the old browser,
wait until it's gone and then kick off a new browser.

In the mobile case it just doesn't work [since we can't have multiple browsers
open] and it's also not a clean way to do it this way. What if the old browser
and the new browser request new tasks at the same time (with the same
browserId)?

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

I did not really think about the mobile case here, more on getting the next
browser up and running as fast as possible so that we can continue testing.
Given the issue on mobile and the relatively small gain from this I will change
the order

+      // We may have started terminating in the mean time.
+      if (underTermination) {
+        browser.close().then((success) {
+            // We should never hit this, print it out.
+            if (!success) {
+              print("Could not kill browser ($id) started due to timeout");
+            }
+          });
+        return;
+      }
+      if (success) {
+        status.browser.close().then((closed) {
+          if (!closed) {
+            // BAD, we could not kill the browser.
+            print("could not kill browser $id");
+          }
+        });
+        browser.id = id;
+        status.browser = browser;
+        status.timeout = false;
+      } else {
+        // TODO(ricow): Handle this better.
+        print("This is bad, should never happen, could not start browser");
+        exit(1);
+      }
+    });
+
+    status.currentlyRunning.doneCallback("TIMEOUT");
+    status.currentlyRunning = null;
+  }
+
+  BrowserTest getNextTest(String browserId) {
+    if (testQueue.isEmpty) return null;
+    var status = browserStatus[browserId];
+    if (status == null) return null;
+    // We are currently terminating this browser, don't start a new test.
+    if (status.timeout) return null;
+    BrowserTest test = testQueue.removeLast();
+    if (status.currentlyRunning == null) {
+      status.currentlyRunning = test;
+    } else {
+      // TODO(ricow): Handle this better.
+      print("This is bad, should never happen, getNextTest all full");
+      print("Old test was: ${status.currentlyRunning.url}");
+      print("Timed out tests:");
+      for (var v in timedOut) {
+        print("  $v");
+      }
+      exit(1);
+    }
+    Timer timer = new Timer(new Duration(seconds: test.timeout),
+                            () { handleTimeout(status); });
+    status.currentlyRunning.timeoutTimer = timer;
+    return test;
+  }
+
+  void queueTest(BrowserTest test) {
+    testQueue.add(test);
+  }
+
+  void printDoubleReportingTests() {
+    if (doubleReportingTests.length == 0) return;
+    // Currently we just report this here, we could have a callback to the
+    // encapsulating environment.
+    print("");
+    print("Double reporting tests");
+    for (var id in doubleReportingTests) {
+      print("  ${testCache[id]}");
+    }
+  }
+
+  Future<bool> terminate() {
+    var futures = [];
+    underTermination = true;
+    testingServer.underTermination = true;
+    for (BrowserTestingStatus status in browserStatus.values) {
+      futures.add(status.browser.close());
+    }
+    return Future.wait(futures).then((values) {
+      testingServer.httpServer.close();
+      printDoubleReportingTests();
+      return !values.contains(false);
+    });
+  }
+
+  Browser getInstance() {
+    if (browserName == "chrome") {
+      return new Chrome();
+    } else if (browserName == "firefox") {
+      return new Firefox();
+    }
+    throw "Non supported browser for browser controller";
+  }
+}
+
+class BrowserTestingServer {
+  const String server = "127.0.0.1";

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

Please make a comment descripting the API (see for example http_server.dart)

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

Done.

+  const String driverPath = "/driver";
+  const String nextTestPath = "/next_test";
+  const String reportPath = "/report";
+  const String waitSignal = "WAIT";
+  const String terminateSignal = "TERMINATE";
+
+  var testCount = 0;
+  var httpServer;
+  bool underTermination = false;
+
+  Function testDoneCallBack;
+  Function nextTestCallBack;
+
+  Future start() {
+    return HttpServer.bind(server, 0).then((createdServer) {
+      httpServer = createdServer;
+      void handler(HttpRequest request) {
+        if (request.uri.path.startsWith(reportPath)) {
+          var browserId = request.uri.path.substring(reportPath.length + 1);
+          var testId = int.parse(request.queryParameters["id"].split("=")[1]);
+          
+          handleReport(request, browserId, testId);
+          // handleReport will asynchroniously fetch the data and will handle
+          // the closing of the streams.
+          return;
+        }
+        var textResponse = "";
+        if (request.uri.path.startsWith(driverPath)) {
+          var browserId = request.uri.path.substring(driverPath.length + 1);
+          textResponse = getDriverPage(browserId);
+        } else if (request.uri.path.startsWith(nextTestPath)) {
+          var browserId = request.uri.path.substring(nextTestPath.length + 1);
+          textResponse = getNextTest(browserId);
+        } else {
+          // We silently ignore other requests.
+        }
+      request.response.write(textResponse);

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

It is important to drain the request stream (even if it is just a GET request)
in order for 'dart:io' to close the socket.

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

Done.

+      request.response.close();

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

I'm not 100% sure if we need to catch the future here as well?

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

The checked in binary does not return a future on close (afaik), that is why it
is done below on the done future. That future is the same as returned by close()

+      request.response.done.catchError((error) {

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

Indentation

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

Done.

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

Done.

+        if (!underTermination) {
+          print("URI ${request.uri}");
+          print("Textresponse $textResponse");
+          throw("Error returning content to browser: $error");
+        }
+      });
+      }
+      void errorHandler(e) {
+        if (!underTermination) print("Error occured in httpserver: $e");
+      };
+      httpServer.listen(handler, onError: errorHandler);
+      return true;
+    });
+  }
+
+  void handleReport(HttpRequest request, String browserId, var testId) {
+    StringBuffer buffer = new StringBuffer();
+    request.transform(new StringDecoder()).listen((data) {

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

You could use 'fold()'.

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

No can do. Checked in bin: r20101, fold introduced: r21258

+      buffer.write(data);
+      }, onDone: () {
+        String back = buffer.toString();
+        request.response.close();
+        testDoneCallBack(browserId, back, testId);
+      }, onError: (error) {print(error);});
+  }
+
+  String getNextTest(String browserId) {
+    var nextTest = nextTestCallBack(browserId);
+    if (underTermination) {
+      // Browsers will be killed shortly, send them a terminate signal so
+      // that they stop pulling.
+      return terminateSignal;
+    } else if (nextTest == null) {
+      // We don't currently have any tests ready for consumption, wait.
+      return waitSignal;
+    } else {
+      return "${nextTest.url}#id=${nextTest.id}";
+    }
+  }
+
+  String getDriverUrl(String browserId) {
+    if (httpServer == null) {
+      print("Bad browser testing server, you are not started yet. Can't "
+            "produce driver url");
+      exit(1);
+      // This should never happen - exit immediately;
+    }
+    return "http://$server:${httpServer.port}/driver/$browserId";
+  }
+
+
+  String getDriverPage(String browserId) {
+    String driverContent = """
+<!DOCTYPE html><html>
+<head>
+  <title>Driving page</title>
+  <script type='text/javascript'>
+    var numberOfTests = 0;
+    var currentId;
+    var testing_window;
+
+    function newTaskHandler() {
+      if(this.readyState == this.DONE) {
+        if(this.status == 200) {

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

spaces after 'if'

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

Done, plus above (no stealing my signature comment :-))

+          if (this.responseText == '$waitSignal') {
+            setTimeout(getNextTask, 500);
+          } else if (this.responseText == 'TERMINATE') {
+            // Don't do anything, we will be killed shortly.
+          } else {
+            // TODO(ricow): Do something more clever here.
+            if (nextTask != undefined) alert('This is really bad');
+            var split = this.responseText.split('#');

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

Please document how the response text looks like.

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

Done.

+            var nextTask = split[0];
+            id = split[1];
+            run(nextTask);
+          }
+        } else {
+          // We are basically fucked - do something clever.
+        }
+      }
+    }
+
+    function getNextTask() {
+      var client = new XMLHttpRequest();
+      client.onreadystatechange = newTaskHandler;
+      client.open('GET', '$nextTestPath/$browserId');
+      client.send();
+    }
+
+    function run(url) {
+      numberOfTests++;
+      document.getElementById('number').innerHTML = numberOfTests;
+      if (testing_window == undefined) {
+        testing_window = window.open(url);
+      } else {
+        testing_window.location = url;
+      }
+    }
+
+    function reportMessage(msg) {
+      var client = new XMLHttpRequest();
+      function handleReady() {
+        if (this.readyState == this.DONE) {
+          getNextTask();
+        }
+      }
+      client.onreadystatechange = handleReady;
+      client.open('POST', '$reportPath/${browserId}?id=' + id);
+      client.setRequestHeader('Content-type',
+                              'application/x-www-form-urlencoded');
+      client.send(msg);
+      // TODO(ricow) add error handling to somehow report the fact that 
+      // we could not send back a result.
+    }
+
+    function messageHandler(e) {
+      var msg = e.data;
+      if (typeof msg != 'string') return;
+      reportMessage(msg);
+    }
+
+    window.addEventListener('message', messageHandler, false);
+    waitForDone = false;
+
+    getNextTask();
+
+  </script>
+</head>
+  <body>
+    Dart test driver, number of tests: <div id="number"></div>
+  </body>
+</html>
+""";
+    return driverContent;
+  }
+}