Start browsers sequentially in testing scripts, as needed.

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 a6d061cf9bf248ff2b0506c5877135ec14badd37..c00a7eae0ab9989f1696fb66b650240060761563 100644
--- a/tools/testing/dart/browser_controller.dart
+++ b/tools/testing/dart/browser_controller.dart
@@ -7,6 +7,7 @@ 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';
@@ -772,7 +773,7 @@ class BrowserTestingStatus {
   BrowserTest lastTest;
   bool timeout = false;
   Timer nextTestTimeout;
-  Stopwatch timeSinceRestart = new Stopwatch();
+  Stopwatch timeSinceRestart = new Stopwatch()..start();
 
   BrowserTestingStatus(Browser this.browser);
 }
@@ -852,25 +853,36 @@ 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 test has been empty within this period, don't start another browser.

[ahe, 2016/04/05 13:19:41]

Add newline before comment to make it obvious where the comment applies. Also,
shouldn't it be a documentation comment (///)?

[Bill Hesse, 2016/04/05 14:51:22]

Done.

+  static const Duration MIN_NONEMPTY_QUEUE_TIME = const Duration(seconds: 1);
 
   final Map configuration;
 
   final String localIp;
   String browserName;
-  final int maxNumBrowsers;
+  int maxNumBrowsers;
   bool checkedMode;
   // Used to send back logs from the browser (start, stop etc)
   Function logger;
-  int browserIdCount = 0;
+  int browserIdCounter = 0;

[ahe, 2016/04/05 13:19:42]

Isn't this a count, not a counter. A counter is something that ticks up itself,
a count is a number (an integer).

[Bill Hesse, 2016/04/05 14:51:22]

This is a counter, that ticks up, just to give a non-repeating id string.  It
does not count anything, and does not represent anything as an integer.  It
could start at any number, and maybe should start at 1.

 
+  bool testServerStarted = false;
   bool underTermination = false;
   int numBrowserGetTestTimeouts = 0;
 
+  // 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 browser currently starting up.
+  DateTime lastEmptyTestQueueTime = new DateTime.now();
+  int numBrowsers = 0;
+  String startingBrowserId;
+
   List<BrowserTest> testQueue = new List<BrowserTest>();
   Map<String, BrowserTestingStatus> browserStatus =
       new Map<String, BrowserTestingStatus>();
 
   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.
@@ -878,11 +890,16 @@ class BrowserTestRunner {
   Map<int, String> doubleReportingOutputs = new Map<int, String>();
 
   BrowserTestingServer testingServer;
+  // 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.
    */
   BrowserTestRunner(this.configuration,
                     this.localIp,
@@ -890,92 +907,64 @@ class BrowserTestRunner {
                     this.maxNumBrowsers,
                     {BrowserTestingServer this.testingServer}) {
     checkedMode = configuration['checked'];
+    if (browserName == 'ff') browserName = 'firefox';
   }
 
-  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;
+    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.");
-        }
-      });
+  void requestBrowser() {

[ahe, 2016/04/05 13:19:41]

Who calls this method? Why and when?

[Bill Hesse, 2016/04/05 14:51:22]

Added doc comment.

+    if (!testServerStarted) return;

[ahe, 2016/04/05 13:19:41]

Why is it OK to not start a browser if the test server isn't started?

[Bill Hesse, 2016/04/05 14:51:22]

Because 4 lines above, when the test server is finished starting, we request a
browser then.

+    if (underTermination) return;

[ahe, 2016/04/05 13:19:41]

Who would be requesting a browser during termination?

[Bill Hesse, 2016/04/05 14:51:22]

Another browser that is requesting a new test.  If the queue is nonempty, it
will try starting another browser then.

+    if (numBrowsers == maxNumBrowsers) return;
+    if (startingBrowserId != null) return;

[ahe, 2016/04/05 13:19:42]

What does it mean if startingBrowserId is null?

[Bill Hesse, 2016/04/05 14:51:22]

It means no browser is currently starting.  I considered having a separate
boolean for this, but it is exactly "there is no browser starting".

Added comment at the declaration.

+    if (numBrowsers > 0 &&
+         (testQueue.isEmpty ||
+          lastEmptyTestQueueTime.add(MIN_NONEMPTY_QUEUE_TIME)
+            .isAfter(new DateTime.now()))) return;
+    startBrowser();
+  }
+
+  void startBrowser() {

[ahe, 2016/04/05 13:19:42]

How is this different from Browser.startBrowser?
Can this be reflected in a different name? Using the same method names is
confusing.

[Bill Hesse, 2016/04/05 14:51:22]

Browser.startBrowser is a bad name for that function.  It could be
startBrowserProcess.

This functions creates a browser, saves its info, and starts it on the driver
page.  It could be called createBrowser instead.  Just changing this name, not
both.

+    final id = "BROWSER$browserIdCounter";
+    print("startBrowser called: $id");

[ahe, 2016/04/05 13:19:42]

Debug message?

[Bill Hesse, 2016/04/05 14:51:22]

Done.

+    ++browserIdCounter;
+    final url = testingServer.getDriverUrl(id);
+    Browser browser;
+    if (browserName == 'chromeOnAndroid') {
+      AdbDevice device = idleAdbDevices.removeLast(); 
+      adbDeviceMapping[id] = device;
+      browser = new AndroidChrome(device);
     } 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);
+      var path = Locations.getBrowserLocation(browserName, configuration);
+      browser = new Browser.byName(browserName, path, checkedMode);
+      browser.logger = logger;
     }
-    return browsersCompleter.future;
+    browser.id = id;
+    startingBrowserId = id;
+    final status = new BrowserTestingStatus(browser);
+    browserStatus[id] = status;
+    numBrowsers++;
+    print("Started $id $numBrowsers");

[ahe, 2016/04/05 13:19:42]

Debug?

[Bill Hesse, 2016/04/05 14:51:22]

Done.

+    status.nextTestTimeout = createNextTestTimer(status);

[ahe, 2016/04/05 13:19:42]

What does this do?

[Bill Hesse, 2016/04/05 14:51:22]

Added a doc comment to createNextTestTimer and createTimeoutTimer

There are cases where a test, after reporting success, keeps running code that
somehow destroys the driver page and keeps it from fetching a next test.  This
detects this rare case.  We had a problem with this in the past.  Now there is
always a timeout on a browser, whether it is running a test, or whether it has
reported a test finished but not started the new one.  There is a gap - if it
gets a "WAIT" message back, but then it has its own .5 second timer.

+    browser.start(url);
   }
 
   var timedOut = [];
@@ -1084,63 +1073,22 @@ class BrowserTestRunner {
 
       // We don't want to start a new browser if we are terminating.
       if (underTermination) return;
-      restartBrowser(id);
+      removeBrowser(id);
     });
   }
 
-  void restartBrowser(String id) {
-    if (browserName.contains('OnAndroid')) {
-      DebugLogger.info("Restarting browser $id");
-    }
-    var browser;
-    var new_id = id;
+  void removeBrowser(String id) {

[ahe, 2016/04/05 13:19:42]

This is confusing: remove browser seems to add a browser. What does "remove"
mean?

[Bill Hesse, 2016/04/05 14:51:22]

Remove the entries for that browserID from our data structures, like
browserStatus and numBrowsers.  Only startBrowser (now renamed createBrowser)
adds a browser to our data structures, and only removeBrowser removes it.

Creating a replacement for the removed browser, if appropriate, is the job of
requestBrowser().

     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++;
+      idleAdbDevices.add(adbDeviceMapping.remove(id));
     }
-    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);
-      }
-    });
+    if (startingBrowserId == id) startingBrowserId = null;

[ahe, 2016/04/05 13:19:41]

What does this mean?

[Bill Hesse, 2016/04/05 14:51:22]

startingBrowserId == null when there is no browser currently starting up, but
not yet ready.

+    browserStatus.remove(id);
+    --numBrowsers;
+    requestBrowser();
   }
 
   BrowserTest getNextTest(String browserId) {
+    if (startingBrowserId == browserId) startingBrowserId = null;
     var status = browserStatus[browserId];
     if (status == null) return null;
     if (status.nextTestTimeout != null) {
@@ -1152,12 +1100,10 @@ class BrowserTestRunner {
     // 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;
@@ -1166,13 +1112,19 @@ class BrowserTestRunner {
       status.browser.close().then((_) {
         // We don't want to start a new browser if we are terminating.
         if (underTermination) return;
-        restartBrowser(id);
+        removeBrowser(id);
       });
       // Don't send a test to the browser we are restarting.
       return null;
     }
 
     BrowserTest test = testQueue.removeLast();
+    // 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 = '';
@@ -1196,10 +1148,6 @@ class BrowserTestRunner {
     // 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;
   }
 
@@ -1216,6 +1164,7 @@ class BrowserTestRunner {
   void handleNextTestTimeout(status) {
     DebugLogger.warning(
         "Browser timed out before getting next test. Restarting");
+    if (startingBrowserId == status.browser.id) startingBrowserId = null;

[ahe, 2016/04/05 13:19:41]

?

[Bill Hesse, 2016/04/05 14:51:22]

This is covered by the call to removeBrowser below.  Removed.

     if (status.timeout) return;
     numBrowserGetTestTimeouts++;
     if (numBrowserGetTestTimeouts >= MAX_NEXT_TEST_TIMEOUTS) {
@@ -1224,12 +1173,13 @@ class BrowserTestRunner {
       terminate().then((_) => exit(1));
     } else {
       status.timeout = true;
-      status.browser.close().then((_) => restartBrowser(status.browser.id));
+      status.browser.close().then((_) => removeBrowser(status.browser.id));
     }
   }
 
   void queueTest(BrowserTest test) {
     testQueue.add(test);
+    requestBrowser();
   }
 
   void printDoubleReportingTests() {
@@ -1252,7 +1202,9 @@ class BrowserTestRunner {
     }
   }
 
-  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;
@@ -1263,28 +1215,11 @@ class BrowserTestRunner {
         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();
   }
 }