Implement testing support for "--compiler=none --runtime=dartium"

tools/testing/dart/browser_controller.dart

Index: tools/testing/dart/browser_controller.dart
diff --git a/tools/testing/dart/browser_controller.dart b/tools/testing/dart/browser_controller.dart
index f37086348f27e1f5ae50a1b2da88c722835a3357..9fb739c940522ff78711551154dd2bb487eefde2 100644
--- a/tools/testing/dart/browser_controller.dart
+++ b/tools/testing/dart/browser_controller.dart
@@ -53,6 +53,8 @@ abstract class Browser {
       return new Firefox();
     } else if (name == 'chrome') {
       return new Chrome();
+    } else if (name == 'dartium') {
+      return new Dartium();
     } else if (name == 'safari') {
       return new Safari();
     } else if (name.startsWith('ie')) {
@@ -63,7 +65,7 @@ abstract class Browser {
   }
 
   static const List<String> SUPPORTED_BROWSERS =
-    const ['safari', 'ff', 'firefox', 'chrome', 'ie9', 'ie10'];
+    const ['safari', 'ff', 'firefox', 'chrome', 'ie9', 'ie10', 'dartium'];
 
   static const List<String> BROWSERS_WITH_WINDOW_SUPPORT =
       const ['safari', 'ff', 'firefox', 'chrome'];
@@ -109,8 +111,11 @@ abstract class Browser {
    * 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((startedProcess) {
+  Future<bool> startBrowser(String command,
+                            List<String> arguments,
+                            {Map<String,String> environment}) {
+    return Process.start(command, arguments, environment: environment)
+        .then((startedProcess) {
       process = startedProcess;
       // Used to notify when exiting, and as a return value on calls to
       // close().
@@ -316,17 +321,20 @@ class Safari extends Browser {
 
 
 class Chrome extends Browser {
-  static String _binary = _getBinary();
-
+  String _binary;

[kasperl, 2013/10/01 13:15:55]

final? You'll need to initialize it in the initializer list.

[kustermann, 2013/10/01 15:41:59]

I've tried:

  final String _binary;
  ...
  Chrome() : _binary = _getBinary();
  String _getBinary() { ... }

Results in

'file:///...../dart/tools/testing/dart/browser_controller.dart': Error: line 327
pos 24: illegal implicit access to receiver 'this'
  Chrome() : _binary = _getBinary();

I guess this only works if the method is static.

   String _version = "Version not found yet";
 
-  // This is extracted to a function since we may need to support several
-  // locations.
-  static String _getWindowsBinary() {
-    return "C:\\Program Files (x86)\\Google\\Chrome\\Application\\chrome.exe";
+  Chrome() {
+    _binary = _getBinary();
   }
 
-  static String _getBinary() {
+  String _getBinary() {
+    // This is extracted to a function since we may need to support several

[kasperl, 2013/10/01 13:15:55]

So this is just planning for future changes?

[kustermann, 2013/10/01 15:41:59]

Yeah, rico put this in.

It's not really necessary at the moment. I'll remove the function.

+    // locations.
+    String _getWindowsBinary() {
+      return "C:\\Program Files (x86)\\Google\\Chrome\\Application\\chrome.exe";
+    }
+
     if (Platform.isWindows) return _getWindowsBinary();
     if (Platform.isMacOS) {
       return "/Applications/Google Chrome.app/Contents/MacOS/Google Chrome";
@@ -334,6 +342,8 @@ class Chrome extends Browser {
     if (Platform.isLinux) return 'google-chrome';
   }
 
+  Map<String, String> _getEnvironment() => null;
+
   Future<bool> _getVersion() {
     if (Platform.isWindows) {
       // The version flag does not work on windows.
@@ -375,7 +385,7 @@ class Chrome extends Browser {
         var args = ["--user-data-dir=${userDir.path}", url,
                     "--disable-extensions", "--disable-popup-blocking",
                     "--bwsi", "--no-first-run"];
-        return startBrowser(_binary, args);
+        return startBrowser(_binary, args, environment: _getEnvironment());
       });
     }).catchError((e) {
       _logEvent("Running $_binary --version failed with $e");
@@ -386,6 +396,27 @@ class Chrome extends Browser {
   String toString() => "Chrome";
 }
 
+class Dartium extends Chrome {
+  String _getBinary() {
+    if (Platform.operatingSystem == 'macos') {
+      return new Path('client/tests/dartium/Chromium.app/Contents/'
+                      'MacOS/Chromium').toNativePath();
+    }
+    return new Path('client/tests/dartium/chrome').toNativePath();
+  }
+
+  Map<String, String> _getEnvironment() {
+    var environment = new Map<String,String>.from(Platform.environment);
+    // By setting this environment variable, dartium will forward "print()"
+    // calls in dart to the top-level javascript function "dartPrint()" if
+    // available.
+    environment['DART_FORWARDING_PRINT'] = '1';
+    return environment;
+  }
+
+  String toString() => "Dartium";
+}
+
 class IE extends Browser {
 
   static const String binary =
@@ -620,6 +651,7 @@ class BrowserTestRunner {
   // 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>();
+  Map<int, String> doubleReportingOutputs = new Map<int, String>();
 
   BrowserTestingServer testingServer;
 
@@ -697,6 +729,7 @@ class BrowserTestRunner {
     var status = browserStatus[browserId];
     if (testCache.containsKey(testId)) {
       doubleReportingTests.add(testId);

[kasperl, 2013/10/01 13:15:55]

Isn't the (linked) key set of the doubleReportingOutputs enough to keep track of
the doubleReportingTests? Why have two data structures for this?

[kustermann, 2013/10/01 15:41:59]

The keys in doubleReportingOutputs are enough, that's correct.

+      doubleReportingOutputs[testId] = output;
       return;
     }
 
@@ -840,6 +873,14 @@ class BrowserTestRunner {
     for (var id in doubleReportingTests) {
       print("  ${testCache[id]}");
     }
+
+    DebugLogger.warning("Double reporting tests:");
+    for (var id in doubleReportingTests) {
+      DebugLogger.warning("  ${testCache[id]}, output: ");
+      DebugLogger.warning("${doubleReportingOutputs[id]}");
+      DebugLogger.warning("");
+      DebugLogger.warning("");
+    }

[kustermann, 2013/10/01 12:55:06]

I don't know if I should submit this, but we'd get valuable information on the
buildbot, why double reporting tests are broken.

[kasperl, 2013/10/01 13:15:55]

I'd submit it.

   }
 
   Future<bool> terminate() {