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

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;
 import "dart:core";
 import "dart:io";
 
@@ skipping 613 matching lines @@
 
 
 class Firefox extends Browser {
   static const String enablePopUp =
       'user_pref("dom.disable_open_during_load", false);';
   static const String disableDefaultCheck =
       'user_pref("browser.shell.checkDefaultBrowser", false);';
   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);
     randomFile.writeStringSync(disableDefaultCheck);
     randomFile.writeStringSync(disableScriptTimeLimit);
     randomFile.close();
   }
 
   Future<bool> start(String url) {
     _logEvent("Starting firefox browser on: $url");
@@ skipping 30 matching lines @@
  */
 class BrowserTestingStatus {
   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 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.

 }
 
 
 /**
  * Describes a single test to be run int the browser.
  */
 class BrowserTest {
   // TODO(ricow): Add timeout callback instead of the string passing hack.
   Function doneCallback;
   String url;
@@ skipping 44 matching lines @@
   final bool checkedMode; // needed for dartium
 
   String localIp;
   String browserName;
   int maxNumBrowsers;
   // Used to send back logs from the browser (start, stop etc)
   Function logger;
   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 =
       new Map<String, BrowserTestingStatus>();
 
   var adbDeviceMapping = new Map<String, AdbDevice>();
   // 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>();
@@ skipping 26 matching lines @@
       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) {
-              browserStatus[browser.id] = new BrowserTestingStatus(browser);
+              browserStatus[browser.id] =
+                  new BrowserTestingStatus(browser, this);
             }
             return success;
           });
           futures.add(future);
         }
         return Future.wait(futures).then((values) {
           return !values.contains(false);
         });
       });
     });
@@ skipping 62 matching lines @@
 
       if (status.currentTest.id != testId) {
         print("Expected test id ${status.currentTest.id} for"
               "${status.currentTest.url}");
         print("Got test id ${testId}");
         print("Last test id was ${status.lastTest.id} for "
               "${status.currentTest.url}");
         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(
           status.currentTest.delayUntilTestStarted,
           status.currentTest.stopwatch.elapsed,
           output,
           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}");
       print(output);
       terminate().then((_) {
         exit(1);
       });
     }
   }
 
@@ skipping 42 matching lines @@
           status.currentTest.delayUntilTestStarted,
           status.currentTest.stopwatch.elapsed,
           lastKnownMessage,
           status.browser.testBrowserOutput,
           didTimeout: true);
       status.currentTest.doneCallback(browserTestOutput);
       status.currentTest = null;
 
       // 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);
-      } else {
-        browserStatus.remove(id);
-        browser = getInstance();
-        new_id = "BROWSER$browserIdCount";
-        browserIdCount++;
-        browserStatus[new_id] = new BrowserTestingStatus(browser);
-      }
-      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");
-          exit(1);
-        }
-      });
+      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 {
+        // TODO(ricow): Handle this better.
+        print("This is bad, should never happen, could not start browser");
+        exit(1);
+      }
+    });
+  }
+
+  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;
 
     BrowserTest test = testQueue.removeLast();
     if (status.currentTest == null) {
       status.currentTest = test;
       status.currentTest.lastKnownMessage = '';
     } else {
       // TODO(ricow): Handle this better.
       print("This is bad, should never happen, getNextTest all full");
@@ skipping 44 matching lines @@
       DebugLogger.warning("");
     }
   }
 
   Future<bool> terminate() {
     var futures = [];
     underTermination = true;
     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();
       testingServer.errorReportingServer.close();
       printDoubleReportingTests();
       return !values.contains(false);
     });
   }
 
   Browser getInstance() {
@@ skipping 378 matching lines @@
     Dart test driver, number of tests: <div id="number"></div><br>
     Currently executing: <div id="currently_executing"></div><br>
     Unhandled error: <div id="unhandled_error"></div>
     <iframe id="embedded_iframe"></iframe>
   </body>
 </html>
 """;
     return driverContent;
   }
 }