Expose the Observatory URL when debugging.

This also refactors BrowserManager so that it has full control over the
Browser instances it manages.

Closes #295

[email protected]

Review URL: https://codereview.chromium.org//1258363003 .

Author: nex3

lib/src/backend/test_platform.dart

@@ -21,7 +21,7 @@ class TestPlatform {
   /// Dartium content shell.
   static const TestPlatform contentShell = const TestPlatform._(
       "Dartium Content Shell", "content-shell",
-      isBrowser: true, isBlink: true, isDartVM: true);
+      isBrowser: true, isBlink: true, isDartVM: true, isHeadless: true);
 
   /// Google Chrome.
   static const TestPlatform chrome = const TestPlatform._("Chrome", "chrome",
@@ -30,7 +30,7 @@ class TestPlatform {
   /// PhantomJS.
   static const TestPlatform phantomJS = const TestPlatform._(
       "PhantomJS", "phantomjs",
-      isBrowser: true, isJS: true, isBlink: true);
+      isBrowser: true, isJS: true, isBlink: true, isHeadless: true);
 
   /// Mozilla Firefox.
   static const TestPlatform firefox = const TestPlatform._("Firefox", "firefox",
@@ -82,8 +82,12 @@ class TestPlatform {
   /// Whether this platform uses the Blink rendering engine.
   final bool isBlink;
 
+  /// Whether this platform has no visible window.
+  final bool isHeadless;
+
   const TestPlatform._(this.name, this.identifier, {this.isDartVM: false,
-      this.isBrowser: false, this.isJS: false, this.isBlink: false});
+      this.isBrowser: false, this.isJS: false, this.isBlink: false,
+      this.isHeadless: false});
 
   String toString() => name;
 }

lib/src/runner.dart

@@ -27,7 +27,7 @@ import 'utils.dart';
 
 /// The set of platforms for which debug flags are (currently) not supported.
 final _debugUnsupportedPlatforms = new Set.from(
-    [TestPlatform.vm, TestPlatform.phantomJS, TestPlatform.contentShell]);
+    [TestPlatform.vm, TestPlatform.phantomJS]);
 
 /// A class that loads and runs tests based on a [Configuration].
 ///
@@ -231,17 +231,39 @@ class Runner {
       _reporter.pause();
 
       var bold = _configuration.color ? '\u001b[1m' : '';
+      var yellow = _configuration.color ? '\u001b[33m' : '';
       var noColor = _configuration.color ? '\u001b[0m' : '';
       print('');
-      print(wordWrap(
-          "${bold}The test runner is paused.${noColor} Open the dev console in "
-              "${suite.platform} and set breakpoints. Once you're finished, "
-              "return to this terminal and press Enter."));
 
-      await race([
+      if (suite.platform.isDartVM) {
+        var url = suite.environment.observatoryUrl;
+        if (url == null) {
+          print("${yellow}Observatory URL not found. Make sure you're using "
+              "${suite.platform.name} 1.11 or later.$noColor");
+        } else {
+          print("Observatory URL: $bold$url$noColor");
+        }
+      }
+
+      var buffer = new StringBuffer(
+          "${bold}The test runner is paused.${noColor} ");
+      if (!suite.platform.isHeadless) {
+        buffer.write("Open the dev console in ${suite.platform} ");
+        if (suite.platform.isDartVM) buffer.write("or ");
+      } else {
+        buffer.write("Open ");
+      }
+      if (suite.platform.isDartVM) buffer.write("the Observatory ");
+
+      buffer.write("and set breakpoints. Once you're finished, return to this "
+          "terminal and press Enter.");
+
+      print(wordWrap(buffer.toString()));
+
+      await inCompletionOrder([
         suite.environment.displayPause(),
         cancelableNext(stdinLines)
-      ]);
+      ]).first;
     } finally {
       _reporter.resume();
     }

lib/src/runner/browser/browser.dart

@@ -25,6 +25,13 @@ typedef Future<Process> StartBrowserFn();
 abstract class Browser {
   String get name;
 
+  /// The Observatory URL for this browser.
+  ///
+  /// This will throw an [UnsupportedError] for browsers that aren't running the
+  /// Dart VM, and return `null` if the Observatory URL can't be found.
+  Future<Uri> get observatoryUrl =>
+      throw new UnsupportedError("$name doesn't support Observatory.");
+
   /// The underlying process.
   ///
   /// This will fire once the process has started successfully.

lib/src/runner/browser/browser_manager.dart

@@ -7,6 +7,7 @@ library test.runner.browser.browser_manager;
 import 'dart:async';
 import 'dart:convert';
 
+import 'package:async/async.dart';
 import 'package:http_parser/http_parser.dart';
 import 'package:pool/pool.dart';
 
@@ -17,18 +18,32 @@ import '../../util/multi_channel.dart';
 import '../../util/remote_exception.dart';
 import '../../util/stack_trace_mapper.dart';
 import '../../utils.dart';
+import '../application_exception.dart';
 import '../environment.dart';
 import '../load_exception.dart';
 import '../runner_suite.dart';
+import 'browser.dart';
+import 'chrome.dart';
+import 'content_shell.dart';
+import 'dartium.dart';
+import 'firefox.dart';
 import 'iframe_test.dart';
+import 'internet_explorer.dart';
+import 'phantom_js.dart';
+import 'safari.dart';
 
 /// A class that manages the connection to a single running browser.
 ///
 /// This is in charge of telling the browser which test suites to load and
 /// converting its responses into [Suite] objects.
 class BrowserManager {
-  /// The browser that this is managing.
-  final TestPlatform browser;
+  /// The browser instance that this is connected to via [_channel].
+  final Browser _browser;
+
+  // TODO(nweiz): Consider removing the duplication between this and
+  // [_browser.name].
+  /// The [TestPlatform] for [_browser].
+  final TestPlatform _platform;
 
   /// The channel used to communicate with the browser.
   ///
@@ -60,16 +75,78 @@ class BrowserManager {
   CancelableCompleter _pauseCompleter;
 
   /// The environment to attach to each suite.
-  _BrowserEnvironment _environment;
+  Future<_BrowserEnvironment> _environment;
+
+  /// Starts the browser identified by [platform] and has it connect to [url].
+  ///
+  /// [url] should serve a page that establishes a WebSocket connection with
+  /// this process. That connection, once established, should be emitted via
+  /// [future].
+  ///
+  /// Returns the browser manager, or throws an [ApplicationException] if a
+  /// connection fails to be established.
+  static Future<BrowserManager> start(TestPlatform platform, Uri url,
+      Future<CompatibleWebSocket> future) {
+    var browser = _newBrowser(url, platform);
+
+    var completer = new Completer();
+
+    // TODO(nweiz): Gracefully handle the browser being killed before the
+    // tests complete.
+    browser.onExit.then((_) {
+      throw new ApplicationException(
+          "${platform.name} exited before connecting.");
+    }).catchError((error, stackTrace) {
+      if (completer.isCompleted) return;
+      completer.completeError(error, stackTrace);
+    });
+
+    future.then((webSocket) {
+      if (completer.isCompleted) return;
+      completer.complete(new BrowserManager._(browser, platform, webSocket));
+    }).catchError((error, stackTrace) {
+      browser.close();
+      if (completer.isCompleted) return;
+      completer.completeError(error, stackTrace);
+    });
+
+    return completer.future.timeout(new Duration(seconds: 30), onTimeout: () {
+      browser.close();
+      throw new ApplicationException(
+          "Timed out waiting for ${platform.name} to connect.");
+    });
+  }
+
+  /// Starts the browser identified by [browser] and has it load [url].
+  static Browser _newBrowser(Uri url, TestPlatform browser) {
+    switch (browser) {
+      case TestPlatform.dartium: return new Dartium(url);
+      case TestPlatform.contentShell: return new ContentShell(url);
+      case TestPlatform.chrome: return new Chrome(url);
+      case TestPlatform.phantomJS: return new PhantomJS(url);
+      case TestPlatform.firefox: return new Firefox(url);
+      case TestPlatform.safari: return new Safari(url);
+      case TestPlatform.internetExplorer: return new InternetExplorer(url);
+      default:
+        throw new ArgumentError("$browser is not a browser.");
+    }
+  }
 
   /// Creates a new BrowserManager that communicates with [browser] over
   /// [webSocket].
-  BrowserManager(this.browser, CompatibleWebSocket webSocket)
+  BrowserManager._(this._browser, this._platform, CompatibleWebSocket webSocket)
       : _channel = new MultiChannel(
           webSocket.map(JSON.decode),
           mapSink(webSocket, JSON.encode)) {
-    _environment = new _BrowserEnvironment(this);
-    _channel.stream.listen(_onMessage, onDone: _onDone);
+    _environment = _loadBrowserEnvironment();
+    _channel.stream.listen(_onMessage, onDone: close);
+  }
+
+  /// Loads [_BrowserEnvironment].
+  Future<_BrowserEnvironment> _loadBrowserEnvironment() async {
+    var observatoryUrl;
+    if (_platform.isDartVM) observatoryUrl = await _browser.observatoryUrl;
+    return new _BrowserEnvironment(this, observatoryUrl);
   }
 
   /// Tells the browser the load a test suite from the URL [url].
@@ -84,7 +161,7 @@ class BrowserManager {
       {StackTraceMapper mapper}) async {
     url = url.replace(fragment: Uri.encodeFull(JSON.encode({
       "metadata": metadata.serialize(),
-      "browser": browser.identifier
+      "browser": _platform.identifier
     })));
 
     // The stream may close before emitting a value if the browser is killed
@@ -130,13 +207,14 @@ class BrowserManager {
         throw new LoadException(
             path,
             "Timed out waiting for the test suite to connect on "
-                "${browser.name}.");
+                "${_platform.name}.");
       });
     });
 
     if (response == null) {
       closeIframe();
-      return null;
+      throw new LoadException(
+          path, "Connection closed before test suite loaded.");
     }
 
     if (response["type"] == "loadException") {
@@ -152,12 +230,12 @@ class BrowserManager {
           asyncError.stackTrace);
     }
 
-    return new RunnerSuite(_environment, response["tests"].map((test) {
+    return new RunnerSuite(await _environment, response["tests"].map((test) {
       var testMetadata = new Metadata.deserialize(test['metadata']);
       var testChannel = suiteChannel.virtualChannel(test['channel']);
       return new IframeTest(test['name'], testMetadata, testChannel,
           mapper: mapper);
-    }), platform: browser, metadata: metadata, path: path,
+    }), platform: _platform, metadata: metadata, path: path,
         onClose: () => closeIframe());
   }
 
@@ -183,12 +261,15 @@ class BrowserManager {
     _pauseCompleter.complete();
   }
 
-  /// The callback called when the WebSocket is closed.
-  void _onDone() {
+  /// Closes the manager and releases any resources it owns, including closing
+  /// the browser.
+  Future close() => _closeMemoizer.runOnce(() {
     _closed = true;
     if (_pauseCompleter != null) _pauseCompleter.complete();
     _pauseCompleter = null;
-  }
+    return _browser.close();
+  });
+  final _closeMemoizer = new AsyncMemoizer();
 }
 
 /// An implementation of [Environment] for the browser.
@@ -197,7 +278,9 @@ class BrowserManager {
 class _BrowserEnvironment implements Environment {
   final BrowserManager _manager;
 
-  _BrowserEnvironment(this._manager);
+  final Uri observatoryUrl;
+
+  _BrowserEnvironment(this._manager, this.observatoryUrl);
 
   CancelableFuture displayPause() => _manager._displayPause();
 }

lib/src/runner/browser/content_shell.dart

@@ -11,6 +11,8 @@ import '../../utils.dart';
 import '../application_exception.dart';
 import 'browser.dart';
 
+final _observatoryRegExp = new RegExp(r"^Observatory listening on ([^ ]+)");
+
 /// A class for running an instance of the Dartium content shell.
 ///
 /// Most of the communication with the browser is expected to happen via HTTP,
@@ -21,37 +23,45 @@ import 'browser.dart';
 class ContentShell extends Browser {
   final name = "Content Shell";
 
-  ContentShell(url, {String executable})
-      : super(() => _startBrowser(url, executable));
-
-  /// Starts a new instance of content shell open to the given [url], which may
-  /// be a [Uri] or a [String].
-  ///
-  /// If [executable] is passed, it's used as the content shell executable.
-  /// Otherwise the default executable name for the current OS will be used.
-  static Future<Process> _startBrowser(url, [String executable]) async {
-    if (executable == null) executable = _defaultExecutable();
-
-    var process = await Process.start(
-        executable, ["--dump-render-tree", url.toString()],
-        environment: {"DART_FLAGS": "--checked"});
-
-    lineSplitter.bind(process.stderr).listen((line) {
-      if (line != "[dartToStderr]: Dartium build has expired") return;
-
-      // TODO(nweiz): link to dartlang.org once it has download links for
-      // content shell
-      // (https://github.com/dart-lang/www.dartlang.org/issues/1164).
-      throw new ApplicationException(
-          "You're using an expired content_shell. Upgrade to the latest "
-              "version:\n"
-          "http://gsdview.appspot.com/dart-archive/channels/stable/release/"
-              "latest/dartium/");
-    });
-
-    return process;
+  final Future<Uri> observatoryUrl;
+
+  factory ContentShell(url, {String executable}) {
+    var completer = new Completer.sync();
+    return new ContentShell._(() async {
+      if (executable == null) executable = _defaultExecutable();
+
+      var process = await Process.start(
+          executable, ["--dump-render-tree", url.toString()],
+          environment: {"DART_FLAGS": "--checked"});
+
+      // The first observatory URL emitted is for the empty start page; the
+      // second is actually for the host page.
+      completer.complete(lineSplitter.bind(process.stdout).map((line) {
+        var match = _observatoryRegExp.firstMatch(line);
+        if (match == null) return null;
+        return Uri.parse(match[1]);
+      }).where((uri) => uri != null).first);
+
+      lineSplitter.bind(process.stderr).listen((line) {
+        if (line != "[dartToStderr]: Dartium build has expired") return;
+
+        // TODO(nweiz): link to dartlang.org once it has download links for
+        // content shell
+        // (https://github.com/dart-lang/www.dartlang.org/issues/1164).
+        throw new ApplicationException(
+            "You're using an expired content_shell. Upgrade to the latest "
+                "version:\n"
+            "http://gsdview.appspot.com/dart-archive/channels/stable/release/"
+                "latest/dartium/");
+      });
+
+      return process;
+    }, completer.future);
   }
 
+  ContentShell._(Future<Process> startBrowser(), this.observatoryUrl)
+      : super(startBrowser);
+
   /// Return the default executable for the current operating system.
   static String _defaultExecutable() =>
       Platform.isWindows ? "content_shell.exe" : "content_shell";