test.dart: Add timeout for browsers that are started but do not fetch tests.

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 6bdc876c416fcae518f7b030f969e4766ef13076..7ea6c64fd3eda3cd310c72f3f4210e290b7c2eb8 100644
--- a/tools/testing/dart/browser_controller.dart
+++ b/tools/testing/dart/browser_controller.dart
@@ -631,7 +631,7 @@ class Firefox extends Browser {
   static const String disableScriptTimeLimit =
       'user_pref("dom.max_script_run_time", 0);';
 
-  Future _createPreferenceFile(var path) {
+  void _createPreferenceFile(var path) {
     var file = new File("${path.toString()}/user.js");
     var randomFile = file.openSync(mode: FileMode.WRITE);
     randomFile.writeStringSync(enablePopUp);
@@ -682,7 +682,15 @@ class BrowserTestingStatus {
   // removed even when we have really stable system.
   BrowserTest lastTest;
   bool timeout = false;
-  BrowserTestingStatus(Browser this.browser);
+  Timer getNextTestTimeout;
+
+  BrowserTestingStatus(Browser this.browser,
+                       BrowserTestRunner runner,
+                       {BrowserTest last: null}) {
+    lastTest = last;
+    getNextTestTimeout = new Timer(BrowserTestRunner.BROWSER_GET_TEST_TIMEOUT,
+        runner.getTimeoutBeforeGetNextTestHandler(this));
+  }

[kustermann, 2014/04/01 20:59:49]

I don't like this, it makes circular dependencies (BrowserTestRunner creates a
BrowserTestingStatus, BrowserTesting status will then ask BrowserTestRunner to
make a timeout handler ...).

Furthermore we should have only one of these. We start a browser, and record
it's current state in this object as long as the browser is alive.

[Bill Hesse, 2014/04/02 17:01:45]

Done.

 }
 
 
@@ -747,6 +755,10 @@ class BrowserTestRunner {
   int browserIdCount = 0;
 
   bool underTermination = false;
+  int numBrowserGetTestTimeouts = 0;
+  static const int MAX_BROWSER_GET_TEST_TIMEOUTS = 10;
+  static const Duration BROWSER_GET_TEST_TIMEOUT = const Duration(seconds: 60);

[kustermann, 2014/04/01 20:59:49]

Move the static variables to the top, before the instance variables.

+
 
   List<BrowserTest> testQueue = new List<BrowserTest>();
   Map<String, BrowserTestingStatus> browserStatus =
@@ -793,7 +805,8 @@ class BrowserTestRunner {
           var url = testingServer.getDriverUrl(browser.id);
           var future = browser.start(url).then((success) {
             if (success) {
-              browserStatus[browser.id] = new BrowserTestingStatus(browser);
+              browserStatus[browser.id] =
+                  new BrowserTestingStatus(browser, this);
             }
             return success;
           });
@@ -876,7 +889,6 @@ class BrowserTestRunner {
         throw("This should never happen, wrong test id");
       }
       testCache[testId] = status.currentTest.url;
-      Stopwatch watch = new Stopwatch()..start();
 
       // Report that the test is finished now
       var browserTestOutput = new BrowserTestOutput(
@@ -886,9 +898,10 @@ class BrowserTestRunner {
           status.browser.testBrowserOutput);
       status.currentTest.doneCallback(browserTestOutput);
 
-      watch.stop();
-      status.lastTest = status.currentTest;
-      status.currentTest = null;
+      browserStatus[browserId] =
+          new BrowserTestingStatus(status.browser,
+                                   this,
+                                   last: status.currentTest);

[kustermann, 2014/04/01 20:59:49]

The intent (I think) was to have *one* BrowserTestingStatus for *every* launched
browser, not for every single test.

[Bill Hesse, 2014/04/02 17:01:45]

Done.  But I think it was better this way.

This one place where a new status wasn't created was still resetting it to be
almost the same as a new object.  And the timer is set both here and all the
places a new one is made.


And all the other ways to write this are uglier: var status = new ; map[id] =
status; status.x = y(status);

But I've changed it as you suggest.

[kustermann, 2014/04/03 12:18:56]

I'm not saying that you can't have methods on the BrowserTestingStatus.
Encapsulation would be very nice, in fact.

I'm just saying that you should have the same BrowserTestingStatus instance for
the same BrowserInstance!

You can also create "new Timer()"s in here (in a reset() method, or whatever),
but pass in a closure, instead of it calling back to BrowserController (which
makes a circle).

     } else {
       print("\nThis is bad, should never happen, handleResult no test");
       print("URL: ${status.lastTest.url}");
@@ -951,53 +964,77 @@ class BrowserTestRunner {
 
       // We don't want to start a new browser if we are terminating.
       if (underTermination) return;
-      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);
-      } else if (browserName == 'DartiumOnAndroid') {
-        browser = new AndroidBrowser(adbDeviceMapping[id],
-                                     dartiumOnAndroidConfig,
-                                     checkedMode);
+      restartBrowser(id);
+    });
+  }
+
+  void restartBrowser(String 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);
+    } else if (browserName == 'DartiumOnAndroid') {
+      browser = new AndroidBrowser(adbDeviceMapping[id],
+                                   dartiumOnAndroidConfig,
+                                   checkedMode);
+    } else {
+      browserStatus.remove(id);
+      browser = getInstance();
+      new_id = "BROWSER$browserIdCount";
+      browserIdCount++;
+      //      browserStatus[new_id] = new BrowserTestingStatus(browser);

[kustermann, 2014/04/01 20:59:49]

Commented code.

[Bill Hesse, 2014/04/02 17:01:45]

moved outside if statement.

+    }
+    browser.id = new_id;
+    browser.start(testingServer.getDriverUrl(new_id)).then((success) {
+      // 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) {
+        browserStatus[browser.id] = new BrowserTestingStatus(browser, this);

[kustermann, 2014/04/01 20:59:49]

You need to set this before starting the browser, otherwise we can end up in a
situation where the browser started and requested the first test, but test.dart
doesn't have it in [browserStatus] yet.

[Bill Hesse, 2014/04/02 17:01:45]

Yes.  This means that the cases that didn't set it until here had a potential
bug.  Moved the earlier one outside the if, so all cases get a status assigned. 
Dropped this assignment.

       } else {
-        browserStatus.remove(id);
-        browser = getInstance();
-        new_id = "BROWSER$browserIdCount";
-        browserIdCount++;
-        browserStatus[new_id] = new BrowserTestingStatus(browser);
+        // TODO(ricow): Handle this better.
+        print("This is bad, should never happen, could not start browser");
+        exit(1);
       }
-      browser.id = new_id;
-      browser.start(testingServer.getDriverUrl(new_id)).then((success) {
-        // 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) {
-          browserStatus[browser.id] = new BrowserTestingStatus(browser);
-        } else {
-          // TODO(ricow): Handle this better.
-          print("This is bad, should never happen, could not start browser");
+    });
+  }
+
+  Function getTimeoutBeforeGetNextTestHandler(BrowserTestingStatus status) {
+    return () {
+      DebugLogger.warning(
+          "Browser timed out before getting next test. Restarting");
+      numBrowserGetTestTimeouts++;
+      if (numBrowserGetTestTimeouts >= MAX_BROWSER_GET_TEST_TIMEOUTS) {
+        DebugLogger.error(
+            "Too many browser timeouts before getting next test. Terminating");
+        terminate().then((_) {
           exit(1);
-        }
+        });
+      }
+      status.browser.close().then((_) {
+        restartBrowser(status.browser.id);
       });

[kustermann, 2014/04/01 20:59:49]

The terminate() above includes the browse.close() calls, and we don't want to
restart them if we hit the limit.

You should add a return statement after calling terminate()...; !

[Bill Hesse, 2014/04/02 17:01:45]

Placed fall-through code in an else { }.

-    });
+    };
   }
 
   BrowserTest getNextTest(String browserId) {
-    if (testQueue.isEmpty) return null;
     var status = browserStatus[browserId];
     if (status == null) return null;
 
+    status.getNextTestTimeout.cancel();

[kustermann, 2014/04/01 20:59:49]

Since we cannot reuse the timer, we might as well set it to null after we've
cancelled it.

[Bill Hesse, 2014/04/02 17:01:45]

Done.

+    if (testQueue.isEmpty) return null;
+
     // We are currently terminating this browser, don't start a new test.
     if (status.timeout) return null;
 
@@ -1062,6 +1099,9 @@ class BrowserTestRunner {
     testingServer.underTermination = true;
     for (BrowserTestingStatus status in browserStatus.values) {
       futures.add(status.browser.close());
+      if (status.getNextTestTimeout != null) {
+        status.getNextTestTimeout.cancel();

[kustermann, 2014/04/01 20:59:49]

See above.

[Bill Hesse, 2014/04/02 17:01:45]

Done.

+      }
     }
     return Future.wait(futures).then((values) {
       testingServer.httpServer.close();