Start browsers sequentially in testing scripts, as needed.

tools/testing/dart/browser_controller.dart

 // Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file
 // 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:async";
 import "dart:convert" show LineSplitter, UTF8, JSON;
 import "dart:core";
 import "dart:io";
+import "dart:math" show max, min;
 
 import 'android.dart';
 import 'http_server.dart';
 import 'path.dart';
 import 'utils.dart';
 
 class BrowserOutput {
   final StringBuffer stdout = new StringBuffer();
   final StringBuffer stderr = new StringBuffer();
   final StringBuffer eventLog = new StringBuffer();
@@ skipping 116 matching lines @@
     } else {
       _logEvent("The process is already dead.");
       return new Future.value(true);
     }
   }
 
   /**
    * Start the browser using the supplied argument.
    * This sets up the error handling and usage logging.
    */
-  Future<bool> startBrowser(String command,
+  Future<bool> startBrowserProcess(String command,
                             List<String> arguments,
                             {Map<String,String> environment}) {
     return Process.start(command, arguments, environment: environment)
         .then((startedProcess) {
       _logEvent("Started browser using $command ${arguments.join(' ')}");
       process = startedProcess;
       // Used to notify when exiting, and as a return value on calls to
       // close().
       var doneCompleter = new Completer();
       done = doneCompleter.future;
@@ skipping 218 matching lines @@
           _logEvent("Could not clear cache");
           return false;
         }
         // Get the version and log that.
         return getVersion().then((version) {
           _logEvent("Got version: $version");
           return Directory.systemTemp.createTemp().then((userDir) {
             _cleanup = () { userDir.deleteSync(recursive: true); };
             _createLaunchHTML(userDir.path, url);
             var args = ["${userDir.path}/launch.html"];
-            return startBrowser(_binary, args);
+            return startBrowserProcess(_binary, args);
           });
         }).catchError((error) {
           _logEvent("Running $_binary --version failed with $error");
           return false;
         });
       });
     });
   }
 
   String toString() => "Safari";
@@ skipping 39 matching lines @@
     // Get the version and log that.
     return _getVersion().then((success) {
       if (!success) return false;
       _logEvent("Got version: $_version");
 
       return Directory.systemTemp.createTemp().then((userDir) {
         _cleanup = () { userDir.deleteSync(recursive: true); };
         var args = ["--user-data-dir=${userDir.path}", url,
                     "--disable-extensions", "--disable-popup-blocking",
                     "--bwsi", "--no-first-run"];
-        return startBrowser(_binary, args, environment: _getEnvironment());
+        return startBrowserProcess(_binary, args, environment: _getEnvironment());
       });
     }).catchError((e) {
       _logEvent("Running $_binary --version failed with $e");
       return false;
     });
   }
 
   String toString() => "Chrome";
 }
 
@@ skipping 21 matching lines @@
       if (!success) {
         _logEvent("Could not clear cache, exiting");
         return false;
       }
       var args = ["-SimulateApplication",
                   "/Applications/Xcode.app/Contents/Developer/Platforms/"
                   "iPhoneSimulator.platform/Developer/SDKs/"
                   "iPhoneSimulator7.1.sdk/Applications/MobileSafari.app/"
                   "MobileSafari",
                   "-u", url];
-      return startBrowser(_binary, args)
+      return startBrowserProcess(_binary, args)
         .catchError((e) {
           _logEvent("Running $_binary --version failed with $e");
           return false;
         });
     });
   }
 
   String toString() => "SafariMobileSimulator";
 }
 
@@ skipping 51 matching lines @@
       .catchError((error) {
         _logEvent("Deleting recovery dir failed with $error");
         return false;
       });
   }
 
   Future<bool> start(String url) {
     _logEvent("Starting ie browser on: $url");
     return clearCache().then((_) => getVersion()).then((version) {
       _logEvent("Got version: $version");
-      return startBrowser(_binary, [url]);
+      return startBrowserProcess(_binary, [url]);
     });
   }
   String toString() => "IE";
 
 }
 
 
 class AndroidBrowserConfig {
   final String name;
   final String package;
@@ skipping 69 matching lines @@
 
 
 class AndroidChrome extends Browser {
   static const String viewAction = 'android.intent.action.VIEW';
   static const String mainAction = 'android.intent.action.MAIN';
   static const String chromePackage = 'com.android.chrome';
   static const String browserPackage = 'com.android.browser';
   static const String firefoxPackage = 'org.mozilla.firefox';
   static const String turnScreenOnPackage = 'com.google.dart.turnscreenon';
 
-  AndroidEmulator _emulator;
   AdbDevice _adbDevice;
 
   AndroidChrome(this._adbDevice);
 
   Future<bool> start(String url) {
     var browserIntent = new Intent(
         viewAction, browserPackage, '.BrowserActivity', url);
     var chromeIntent = new Intent(viewAction, chromePackage, '.Main', url);
     var firefoxIntent = new Intent(viewAction, firefoxPackage, '.App', url);
     var turnScreenOnIntent =
@@ skipping 79 matching lines @@
       version = versionResult.stdout;
       _logEvent("Got version: $version");
 
       return Directory.systemTemp.createTemp().then((userDir) {
         _createPreferenceFile(userDir.path);
         _cleanup = () { userDir.deleteSync(recursive: true); };
         var args = ["-profile", "${userDir.path}",
                     "-no-remote", "-new-instance", url];
         var environment = new Map<String,String>.from(Platform.environment);
         environment["MOZ_CRASHREPORTER_DISABLE"] = "1";
-        return startBrowser(_binary, args, environment: environment);
+        return startBrowserProcess(_binary, args, environment: environment);
 
       });
     }).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 {
+class BrowserStatus {
   Browser browser;
   BrowserTest currentTest;
 
   // 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 a really stable system.
   BrowserTest lastTest;
   bool timeout = false;
   Timer nextTestTimeout;
-  Stopwatch timeSinceRestart = new Stopwatch();
+  Stopwatch timeSinceRestart = new Stopwatch()..start();
 
-  BrowserTestingStatus(Browser this.browser);
+  BrowserStatus(Browser this.browser);
 }
 
 
 /**
  * Describes a single test to be run in the browser.
  */
 class BrowserTest {
   // TODO(ricow): Add timeout callback instead of the string passing hack.
   Function doneCallback;
   String url;
@@ skipping 49 matching lines @@
   final BrowserOutput browserOutput;
   final bool didTimeout;
 
   BrowserTestOutput(
       this.delayUntilTestStarted, this.duration, this.lastKnownMessage,
       this.browserOutput, {this.didTimeout: false});
 }
 
 /**
  * Encapsulates all the functionality for running tests in browsers.
  * The interface is rather simple. After starting, the runner tests

[ahe, 2016/04/05 16:14:08]

Remove "The interface is rather simple."

[Bill Hesse, 2016/04/07 15:24:10]

Done.

Added info about starting browsers and starting test server.

  * are simply added to the queue and a the supplied callbacks are called
  * whenever a test completes.
  */
 class BrowserTestRunner {
   static const int MAX_NEXT_TEST_TIMEOUTS = 10;
   static const Duration NEXT_TEST_TIMEOUT = const Duration(seconds: 60);
   static const Duration RESTART_BROWSER_INTERVAL = const Duration(seconds: 60);
 
+  /// If the queue was recently empty, don't start another browser.
+  static const Duration MIN_NONEMPTY_QUEUE_TIME = const Duration(seconds: 1);
+
   final Map configuration;
+  BrowserTestingServer testingServer;

[ahe, 2016/04/05 16:14:07]

What is a testingServer?

[Bill Hesse, 2016/04/07 15:24:10]

Added to class documentation.

 
   final String localIp;
   String browserName;
-  final int maxNumBrowsers;
+  int maxNumBrowsers;
   bool checkedMode;
+  int numBrowsers = 0;
   // Used to send back logs from the browser (start, stop etc)
   Function logger;
-  int browserIdCount = 0;
 
+  int browserIdCounter = 1;
+
+  bool testServerStarted = false;

[ahe, 2016/04/05 16:14:08]

What is a testServer? Is it the same as a testingServer?

[Bill Hesse, 2016/04/07 15:24:10]

Changed to testingServerStarted.

   bool underTermination = false;
   int numBrowserGetTestTimeouts = 0;
-
+  DateTime lastEmptyTestQueueTime = new DateTime.now();
+  String _currentStartingBrowserId;
   List<BrowserTest> testQueue = new List<BrowserTest>();
-  Map<String, BrowserTestingStatus> browserStatus =
-      new Map<String, BrowserTestingStatus>();
+  Map<String, BrowserStatus> browserStatus =
+      new Map<String, BrowserStatus>();
 
   var adbDeviceMapping = new Map<String, AdbDevice>();
+  List<AdbDevice> idleAdbDevices;
+
   // 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>();
+
   Map<int, String> doubleReportingOutputs = new Map<int, String>();
+  List<String> timedOut = [];
 
-  BrowserTestingServer testingServer;
+  // We will start a new browser when the test queue hasn't been empty
+  // recently, we have fewer than maxNumBrowsers browsers, and there is
+  // no other browser instance currently starting up.

[ahe, 2016/04/05 16:14:08]

Let's say that the queue hasn't been empty recently, but you recently started a
browser. Should that also delay starting a new browser?

[Bill Hesse, 2016/04/07 15:24:10]

No, except that a new browser shouldn't start until the previously started one
requests a test (and gets it, so the queue is more likely to be empty if there
are now enough browsers).

+  bool get queueWasEmptyRecently {
+    return testQueue.isEmpty ||
+        new DateTime.now().difference(lastEmptyTestQueueTime) <
+            MIN_NONEMPTY_QUEUE_TIME;
+  }
+
+  // While a browser is starting, but has not requested its first test, its
+  // browserId is stored in _currentStartingBrowserId.
+  // When no browser is currently starting, _currentStartingBrowserId is null.
+  bool get aBrowserIsCurrentlyStarting => _currentStartingBrowserId != null;
+  void markCurrentlyStarting(String id) {
+   _currentStartingBrowserId = id;
+  }
+  void markNotCurrentlyStarting(String id) {
+    if (_currentStartingBrowserId == id) _currentStartingBrowserId = null;
+  }
+
+  // If [browserName] doesn't support opening new windows, we use new iframes
+  // instead.
+  bool get useIframe =>
+     !Browser.BROWSERS_WITH_WINDOW_SUPPORT.contains(browserName);
 
   /**
    * The TestRunner takes the testingServer in as a constructor parameter in
    * case we wish to have a testing server with different behavior (such as the
-   * case for performance testing.
+   * case for performance testing. Note that the testingServer handlers are
+   * overwritten with handlers for this specific configuration, in that case.

[ahe, 2016/04/05 16:14:07]

This comment is confusing and not balanced (missing close parenthesis).

[Bill Hesse, 2016/04/07 15:24:10]

Rewritten

    */
   BrowserTestRunner(this.configuration,
                     this.localIp,
                     this.browserName,
                     this.maxNumBrowsers,
                     {BrowserTestingServer this.testingServer}) {
     checkedMode = configuration['checked'];
+    if (browserName == 'ff') browserName = 'firefox';

[ahe, 2016/04/05 16:14:07]

This looks like a hack.

[Bill Hesse, 2016/04/07 15:24:10]

It is. I recall that there was a failure if we normalized the name at the
beginning of the program, in test_options, which is why this is local.

   }
 
-  Future<bool> start() {
-    // If [browserName] doesn't support opening new windows, we use new iframes
-    // instead.
-    bool useIframe =
-        !Browser.BROWSERS_WITH_WINDOW_SUPPORT.contains(browserName);
+  Future start() async {
     if (testingServer == null) {
       testingServer = new BrowserTestingServer(
           configuration, localIp, useIframe);
     }
-    return testingServer.start().then((_) {
-      testingServer.testDoneCallBack = handleResults;
-      testingServer.testStatusUpdateCallBack = handleStatusUpdate;
-      testingServer.testStartedCallBack = handleStarted;
-      testingServer.nextTestCallBack = getNextTest;
-      return getBrowsers().then((browsers) {
-        var futures = [];
-        for (var browser in browsers) {
-          var url = testingServer.getDriverUrl(browser.id);
-          var future = browser.start(url).then((success) {
-            if (success) {
-              var status = new BrowserTestingStatus(browser);
-              browserStatus[browser.id] = status;
-              status.nextTestTimeout = createNextTestTimer(status);
-              status.timeSinceRestart.start();
-            }
-            return success;
-          });
-          futures.add(future);
-        }
-        return Future.wait(futures).then((values) {
-          return !values.contains(false);
-        });
-      });
-    });
+    await testingServer.start();
+    testingServer
+    ..testDoneCallBack = handleResults
+    ..testStatusUpdateCallBack = handleStatusUpdate
+    ..testStartedCallBack = handleStarted
+    ..nextTestCallBack = getNextTest;

[ahe, 2016/04/05 16:14:07]

Indent all .. lines by four.

[Bill Hesse, 2016/04/07 15:24:10]

Done.  Will be reformatted by the follow-up autoformatting CL.

+    if (browserName == 'chromeOnAndroid') {
+      var idbNames = await AdbHelper.listDevices();
+      idleAdbDevices = new List.from(idbNames.map((id) => new AdbDevice(id)));
+      maxNumBrowsers = min(maxNumBrowsers, idleAdbDevices.length);
+    }
+    testServerStarted = true;
+    requestBrowser();
   }
 
-  Future<List<Browser>> getBrowsers() {
-    // TODO(kustermann): This is a hackisch way to accomplish it and should
-    // be encapsulated
-    var browsersCompleter = new Completer();
-    var androidBrowserCreationMapping = {
-      'chromeOnAndroid' : (AdbDevice device) => new AndroidChrome(device),
-      'ContentShellOnAndroid' : (AdbDevice device) => new AndroidBrowser(
-          device,
-          contentShellOnAndroidConfig,
-          checkedMode,
-          configuration['drt']),
-      'DartiumOnAndroid' : (AdbDevice device) => new AndroidBrowser(
-          device,
-          dartiumOnAndroidConfig,
-          checkedMode,
-          configuration['dartium']),
-    };
-    if (androidBrowserCreationMapping.containsKey(browserName)) {
-      AdbHelper.listDevices().then((deviceIds) {
-        if (deviceIds.length > 0) {
-          var browsers = [];
-          for (int i = 0; i < deviceIds.length; i++) {
-            var id = "BROWSER$i";
-            var device = new AdbDevice(deviceIds[i]);
-            adbDeviceMapping[id] = device;
-            var browser = androidBrowserCreationMapping[browserName](device);
-            browsers.add(browser);
-            // We store this in case we need to kill the browser.
-            browser.id = id;
-          }
-          browsersCompleter.complete(browsers);
-        } else {
-          throw new StateError("No android devices found.");
-        }
-      });
-    } else {
-      var browsers = [];
-      for (int i = 0; i < maxNumBrowsers; i++) {
-        var id = "BROWSER$browserIdCount";
-        browserIdCount++;
-        var browser = getInstance();
-        browsers.add(browser);
-        // We store this in case we need to kill the browser.
-        browser.id = id;
-      }
-      browsersCompleter.complete(browsers);
-    }
-    return browsersCompleter.future;
+  /// requestBrowser() is called whenever we might want to start an additional
+  /// browser instance.
+  /// It is called when starting the BrowserTestRunner, and whenever a browser
+  /// is killed, whenever a new test is enqueued, or whenever a browser
+  /// finishes a test.
+  /// So we are guaranteed that this will always eventually be called, as long
+  /// as the test queue isn't empty.
+  void requestBrowser() {
+    if (!testServerStarted) return;
+    if (underTermination) return;
+    if (numBrowsers == maxNumBrowsers) return;
+    if (aBrowserIsCurrentlyStarting) return;
+    if (numBrowsers > 0 && queueWasEmptyRecently) return;
+    createBrowser();
   }
 
-  var timedOut = [];
+  void createBrowser() {
+    final id = "BROWSER$browserIdCounter";
+    ++browserIdCounter;

[ahe, 2016/04/05 16:14:08]

Convert to method:

String getNextBrowserId() => "BROWSER${browserIdCounter++}";

+    final url = testingServer.getDriverUrl(id);

[ahe, 2016/04/05 16:14:08]

Is this a String or a Uri? Add type to make it clear?

[Bill Hesse, 2016/04/07 15:24:10]

Done.

+    Browser browser;
+    if (browserName == 'chromeOnAndroid') {
+      AdbDevice device = idleAdbDevices.removeLast(); 
+      adbDeviceMapping[id] = device;
+      browser = new AndroidChrome(device);
+    } else {
+      var path = Locations.getBrowserLocation(browserName, configuration);
+      browser = new Browser.byName(browserName, path, checkedMode);
+      browser.logger = logger;
+    }
+    browser.id = id;
+    markCurrentlyStarting(id);
+    final status = new BrowserStatus(browser);
+    browserStatus[id] = status;
+    numBrowsers++;
+    status.nextTestTimeout = createNextTestTimer(status);
+    browser.start(url);
+  }
 
   void handleResults(String browserId, String output, int testId) {
     var status = browserStatus[browserId];
     if (testCache.containsKey(testId)) {
       doubleReportingOutputs[testId] = output;
       return;
     }
 
     if (status == null || status.timeout) {
       // We don't do anything, this browser is currently being killed and
@@ skipping 51 matching lines @@
 
     if (status != null && !status.timeout && status.currentTest != null) {
       status.currentTest.timeoutTimer.cancel();
       status.currentTest.timeoutTimer =
           createTimeoutTimer(status.currentTest, status);
       status.currentTest.delayUntilTestStarted =
           status.currentTest.stopwatch.elapsed;
     }
   }
 
-  void handleTimeout(BrowserTestingStatus status) {
+  void handleTimeout(BrowserStatus 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.
     if (status.timeout) {
       DebugLogger.error(
           "Got test timeout for an already restarting browser");
       return;
     }
     status.timeout = true;
     timedOut.add(status.currentTest.url);
     var id = status.browser.id;
@@ skipping 13 matching lines @@
           status.currentTest.stopwatch.elapsed,
           lastKnownMessage,
           status.browser.testBrowserOutput,
           didTimeout: true);
       status.currentTest.doneCallback(browserTestOutput);
       status.lastTest = status.currentTest;
       status.currentTest = null;
 
       // We don't want to start a new browser if we are terminating.
       if (underTermination) return;
-      restartBrowser(id);
+      removeBrowser(id);
+      requestBrowser();
     });
   }
 
-  void restartBrowser(String id) {
-    if (browserName.contains('OnAndroid')) {
-      DebugLogger.info("Restarting browser $id");
+  /// Remove a browser that has closed from our data structures that track
+  /// open browsers. Check if we want to replace it with a new browser.
+  void removeBrowser(String id) {
+    if (browserName == 'chromeOnAndroid') {
+      idleAdbDevices.add(adbDeviceMapping.remove(id));
     }
-    var browser;
-    var new_id = id;
-    if (browserName == 'chromeOnAndroid') {
-      browser = new AndroidChrome(adbDeviceMapping[id]);
-    } else if (browserName == 'ContentShellOnAndroid') {
-      browser = new AndroidBrowser(adbDeviceMapping[id],
-                                   contentShellOnAndroidConfig,
-                                   checkedMode,
-                                   configuration['drt']);
-    } else if (browserName == 'DartiumOnAndroid') {
-      browser = new AndroidBrowser(adbDeviceMapping[id],
-                                   dartiumOnAndroidConfig,
-                                   checkedMode,
-                                   configuration['dartium']);
-    } else {
-      browserStatus.remove(id);
-      browser = getInstance();
-      new_id = "BROWSER$browserIdCount";
-      browserIdCount++;
-    }
-    browser.id = new_id;
-    var status = new BrowserTestingStatus(browser);
-    browserStatus[new_id] = status;
-    status.nextTestTimeout = createNextTestTimer(status);
-    status.timeSinceRestart.start();
-    browser.start(testingServer.getDriverUrl(new_id)).then((success) {
-      // We may have started terminating in the mean time.
-      if (underTermination) {
-        if (status.nextTestTimeout != null) {
-          status.nextTestTimeout.cancel();
-          status.nextTestTimeout = null;
-        }
-        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) {
-        // TODO(ricow): Handle this better.
-        print("This is bad, should never happen, could not start browser");
-        exit(1);
-      }
-    });
+    markNotCurrentlyStarting(id);
+    browserStatus.remove(id);
+    --numBrowsers;
   }
 
   BrowserTest getNextTest(String browserId) {
+    markNotCurrentlyStarting(browserId);
     var status = browserStatus[browserId];
     if (status == null) return null;
     if (status.nextTestTimeout != null) {
       status.nextTestTimeout.cancel();
       status.nextTestTimeout = null;
     }
     if (testQueue.isEmpty) return null;
 
     // We are currently terminating this browser, don't start a new test.
     if (status.timeout) return null;
 
-    // Restart content_shell and dartium on Android if they have been
+    // Restart Internet Explorer if it has been
     // running for longer than RESTART_BROWSER_INTERVAL. The tests have
     // had flaky timeouts, and this may help.
-    if ((browserName == 'ContentShellOnAndroid' ||
-         browserName == 'DartiumOnAndroid' ||
-         browserName == 'ie10' ||
+    if ((browserName == 'ie10' ||
          browserName == 'ie11') &&
         status.timeSinceRestart.elapsed > RESTART_BROWSER_INTERVAL) {
       var id = status.browser.id;
       // Reset stopwatch so we don't trigger again before restarting.
       status.timeout = true;
       status.browser.close().then((_) {
         // We don't want to start a new browser if we are terminating.
         if (underTermination) return;
-        restartBrowser(id);
+        removeBrowser(id);
+        requestBrowser();
       });
       // Don't send a test to the browser we are restarting.
       return null;
     }
 
     BrowserTest test = testQueue.removeLast();

[ahe, 2016/04/05 16:14:08]

There's an opportunity to create a class that represents a test queue. It would
then keep track of when it was last empty.

abstract class TestQueue {
  final List<BrowserTest> tests;

  bool get hasBeenEmptyRecently;

  BrowserTest removeLast();

  void addTest(BrowserTest test);
}

+    // If our queue isn't empty, try starting more browsers
+    if (testQueue.isEmpty) {
+      lastEmptyTestQueueTime = new DateTime.now();
+    } else {
+      requestBrowser();
+    }
     if (status.currentTest == null) {
       status.currentTest = test;
       status.currentTest.lastKnownMessage = '';
     } else {
       // TODO(ricow): Handle this better.
       print("Browser requested next test before reporting previous result");
       print("This happened for browser $browserId");
       print("Old test was: ${status.currentTest.url}");
       print("The test before that was: ${status.lastTest.url}");
       print("Timed out tests:");
       for (var v in timedOut) {
         print("  $v");
       }
       exit(1);
     }
 
     status.currentTest.timeoutTimer = createTimeoutTimer(test, status);
     status.currentTest.stopwatch = new Stopwatch()..start();
 
     // Reset the test specific output information (stdout, stderr) on the
     // browser, since a new test is being started.
     status.browser.resetTestBrowserOutput();
     status.browser.logBrowserInfoToTestBrowserOutput();
-    if (browserName.contains('OnAndroid')) {
-      DebugLogger.info("Browser $browserId getting test ${test.url}");
-    }
-
     return test;
   }
 
-  Timer createTimeoutTimer(BrowserTest test, BrowserTestingStatus status) {
+  /// This creates a timer that is active while a test is running on a browser.

[ahe, 2016/04/05 16:14:07]

Remove "this" which is implied.

[Bill Hesse, 2016/04/07 15:24:10]

Done.

+  Timer createTimeoutTimer(BrowserTest test, BrowserStatus status) {
     return new Timer(new Duration(seconds: test.timeout),
                      () { handleTimeout(status); });
   }
 
-  Timer createNextTestTimer(BrowserTestingStatus status) {
+  /// This creates a timer that is active while no test is running on the

[ahe, 2016/04/05 16:14:08]

Ditto.

[Bill Hesse, 2016/04/07 15:24:10]

Done.

+  /// browser. It has finished one test, and it has not requested a new test.
+  Timer createNextTestTimer(BrowserStatus status) {
     return new Timer(BrowserTestRunner.NEXT_TEST_TIMEOUT,
                      () { handleNextTestTimeout(status); });
   }
 
   void handleNextTestTimeout(status) {
     DebugLogger.warning(
         "Browser timed out before getting next test. Restarting");
     if (status.timeout) return;
     numBrowserGetTestTimeouts++;
     if (numBrowserGetTestTimeouts >= MAX_NEXT_TEST_TIMEOUTS) {
       DebugLogger.error(
           "Too many browser timeouts before getting next test. Terminating");
       terminate().then((_) => exit(1));
     } else {
       status.timeout = true;
-      status.browser.close().then((_) => restartBrowser(status.browser.id));
+      status.browser.close().then((_) {
+        removeBrowser(status.browser.id);
+        requestBrowser();
+      });
     }
   }
 
   void queueTest(BrowserTest test) {

[ahe, 2016/04/05 16:14:08]

enqueueTest?

[Bill Hesse, 2016/04/07 15:24:10]

Done.

     testQueue.add(test);
+    requestBrowser();
   }
 
   void printDoubleReportingTests() {
     if (doubleReportingOutputs.length == 0) return;
     // TODO(ricow): die on double reporting.
     // Currently we just report this here, we could have a callback to the
     // encapsulating environment.
     print("");
     print("Double reporting tests");
     for (var id in doubleReportingOutputs.keys) {
       print("  ${testCache[id]}");
     }
 
     DebugLogger.warning("Double reporting tests:");
     for (var id in doubleReportingOutputs.keys) {
       DebugLogger.warning("${testCache[id]}, output: ");
       DebugLogger.warning("${doubleReportingOutputs[id]}");
       DebugLogger.warning("");
       DebugLogger.warning("");
     }
   }
 
-  Future<bool> terminate() {
+  // TODO(26191): Call a unified fatalError(), that shuts down all subprocesses.
+  // This just kills the browsers in this BrowserTestRunner instance.
+  Future terminate() async {
     var browsers = [];
     underTermination = true;
     testingServer.underTermination = true;
-    for (BrowserTestingStatus status in browserStatus.values) {
+    for (BrowserStatus status in browserStatus.values) {
       browsers.add(status.browser);
       if (status.nextTestTimeout != null) {
         status.nextTestTimeout.cancel();
         status.nextTestTimeout = null;
       }
     }
-    // Success if all the browsers closed successfully.
-    bool success = true;
-    Future closeBrowser(Browser b) {
-      return b.close().then((bool closeSucceeded) {
-        if (!closeSucceeded) {
-          success = false;
-        }
-      });
+    for (Browser b in browsers) {
+      await b.close();
     }
-    return Future.forEach(browsers, closeBrowser).then((_) {
-      testingServer.errorReportingServer.close();
-      printDoubleReportingTests();
-      return success;
-    });
-  }
-
-  Browser getInstance() {
-    if (browserName == 'ff') browserName = 'firefox';
-    var path = Locations.getBrowserLocation(browserName, configuration);
-    var browser = new Browser.byName(browserName, path, checkedMode);
-    browser.logger = logger;
-    return browser;
+    testingServer.errorReportingServer.close();
+    printDoubleReportingTests();
   }
 }
 
 class BrowserTestingServer {
   final Map configuration;
   /// Interface of the testing server:
   ///
   /// GET /driver/BROWSER_ID -- This will get the driver page to fetch
   ///                           and run tests ...
   /// GET /next_test/BROWSER_ID -- returns "WAIT" "TERMINATE" or "url#id"
@@ skipping 498 matching lines @@
     </div>
     <div id="embedded_iframe_div" class="test box">
       <iframe style="width:100%;height:100%;" id="embedded_iframe"></iframe>
     </div>
   </body>
 </html>
 """;
     return driverContent;
   }
 }