Make Safari tests more robust.

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..b023bb0e90ac83f49a969b1c1c957722ccd9388c 100644
--- a/tools/testing/dart/browser_controller.dart
+++ b/tools/testing/dart/browser_controller.dart
@@ -13,6 +13,9 @@ import 'http_server.dart';
 import 'path.dart';
 import 'utils.dart';
 
+import 'reset_safari.dart' show
+    killAndResetSafari;
+
 class BrowserOutput {
   final StringBuffer stdout = new StringBuffer();
   final StringBuffer stderr = new StringBuffer();
@@ -96,6 +99,15 @@ abstract class Browser {
   static const List<String> BROWSERS_WITH_WINDOW_SUPPORT =
       const ['ie11', 'ie10'];
 
+  /// If [browserName] doesn't support Window.open, we use iframes instead.
+  static bool requiresIframe(String browserName) {
+    return !BROWSERS_WITH_WINDOW_SUPPORT.contains(browserName);
+  }
+
+  static bool requiresFocus(String browserName) {
+    return browserName == "safari";
+  }
+
   // TODO(kustermann): add standard support for chrome on android
   static bool supportedBrowser(String name) {
     return SUPPORTED_BROWSERS.contains(name);
@@ -269,58 +281,50 @@ class Safari extends Browser {
   static const String versionFile =
       "/Applications/Safari.app/Contents/version.plist";
 
-  /**
-   * Directories where safari stores state. We delete these if the deleteCache
-   * is set
-   */
-  static const List<String> CACHE_DIRECTORIES =
-      const ["Library/Caches/com.apple.Safari",
-             "Library/Safari",
-             "Library/Saved Application State/com.apple.Safari.savedState",
-             "Library/Caches/Metadata/Safari"];
-
+  static const String safariBundleLocation = "/Applications/Safari.app/";
 
   Future<bool> allowPopUps() {
-    var command = "defaults";
-    var args = ["write", "com.apple.safari",
-                "com.apple.Safari.ContentPageGroupIdentifier."
-                "WebKit2JavaScriptCanOpenWindowsAutomatically",
-                "1"];
-    return Process.run(command, args).then((result) {
-      if (result.exitCode != 0) {
-        _logEvent("Could not disable pop-up blocking for safari");
-        return false;
-      }
-      return true;
-    });
+    // DO NOT SUBMIT.

[Bill Hesse, 2016/04/25 08:20:18]

This comment says "do not submit".  Should we remove this function, and calls to
it, completely?

[ahe, 2016/05/24 08:44:42]

Done.

+    return new Future.value(true);
   }
 
-  Future<bool> deleteIfExists(Iterator<String> paths) {
-    if (!paths.moveNext()) return new Future.value(true);
-    Directory directory = new Directory(paths.current);
-    return directory.exists().then((exists) {
-      if (exists) {
-        _logEvent("Deleting ${paths.current}");
-        return directory.delete(recursive: true)
-	    .then((_) => deleteIfExists(paths))
-	    .catchError((error) {
-	      _logEvent("Failure trying to delete ${paths.current}: $error");
-	      return false;
-	    });
+  // Clears the cache if the static deleteCache flag is set.
+  // Returns false if the command to actually clear the cache did not complete.
+  Future<bool> clearCache() async {
+    if (!Browser.deleteCache) return true;
+
+    Completer completer = new Completer();
+    handleUncaughtError(error, StackTrace stackTrace) {
+      if (!completer.isCompleted) {
+        completer.completeError(error, stackTrace);
       } else {
-        _logEvent("${paths.current} is not present");
-        return deleteIfExists(paths);
+        throw new AsyncError(error, stackTrace);
       }
-    });
-  }
+    }
+    Zone parent = Zone.current;

[Bill Hesse, 2016/04/25 08:20:18]

Add a comment for why we are using the zone - is it only to change printing to
logging, or is it also giving error handling that we couldn't get another way.

[ahe, 2016/05/24 08:44:42]

Done.

+    ZoneSpecification specification = new ZoneSpecification(
+        print: (Zone self, ZoneDelegate delegate, Zone zone, String line) {
+          delegate.run(parent, () {
+            _logEvent(line);
+          });
+        });
+    Future zoneWrapper() {
+      Uri safariUri = Uri.base.resolve(safariBundleLocation);
+      return new Future(() => killAndResetSafari(bundle: safariUri))
+          .then(completer.complete);
+    }
 
-  // Clears the cache if the static deleteCache flag is set.
-  // Returns false if the command to actually clear the cache did not complete.
-  Future<bool> clearCache() {
-    if (!Browser.deleteCache) return new Future.value(true);
-    var home = Platform.environment['HOME'];
-    Iterator iterator = CACHE_DIRECTORIES.map((s) => "$home/$s").iterator;
-    return deleteIfExists(iterator);
+    runZoned(
+        zoneWrapper, zoneSpecification: specification,
+        onError: handleUncaughtError);
+
+    try {
+      await completer.future;
+      return true;
+    } catch (error, st) {
+      _logEvent("Unable to reset Safari: $error$st");
+      return false;
+    }
   }
 
   Future<String> getVersion() {
@@ -381,8 +385,10 @@ class Safari extends Browser {
           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);
+            var args = [
+                "-d", "-i", "-m", "-s", "-u", _binary,
+                "${userDir.path}/launch.html"];
+            return startBrowser("/usr/bin/caffeinate", args);
           });
         }).catchError((error) {
           _logEvent("Running $_binary --version failed with $error");
@@ -856,7 +862,7 @@ class BrowserTestRunner {
   final Map configuration;
 
   final String localIp;
-  String browserName;
+  final String browserName;
   final int maxNumBrowsers;
   bool checkedMode;
   // Used to send back logs from the browser (start, stop etc)
@@ -877,30 +883,28 @@ class BrowserTestRunner {
   Map<int, String> testCache = new Map<int, String>();
   Map<int, String> doubleReportingOutputs = new Map<int, String>();
 
-  BrowserTestingServer testingServer;
+  final BrowserTestingServer testingServer;
 
   /**
    * 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

[Bill Hesse, 2016/04/25 08:20:18]

If we are removing the testingServer argument, then remove this comment too.

[ahe, 2016/05/24 08:44:42]

Done.

    * case for performance testing.
    */
-  BrowserTestRunner(this.configuration,
-                    this.localIp,
-                    this.browserName,
-                    this.maxNumBrowsers,
-                    {BrowserTestingServer this.testingServer}) {
-    checkedMode = configuration['checked'];
-  }
+  BrowserTestRunner(
+      Map configuration,
+      String localIp,
+      String browserName,
+      this.maxNumBrowsers)
+      : configuration = configuration,
+        localIp = localIp,
+        browserName = browserName,
+        checkedMode = configuration['checked'],
+        testingServer = new BrowserTestingServer(
+            configuration, localIp,
+            Browser.requiresIframe(browserName),
+            Browser.requiresFocus(browserName));
 
   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);
-    if (testingServer == null) {
-      testingServer = new BrowserTestingServer(
-          configuration, localIp, useIframe);
-    }
     return testingServer.start().then((_) {
       testingServer.testDoneCallBack = handleResults;
       testingServer.testStatusUpdateCallBack = handleStatusUpdate;
@@ -1303,6 +1307,8 @@ class BrowserTestingServer {
   ///                                   test
 
   final String localIp;
+  final bool useIframe;
+  final bool requiresFocus;
 
   static const String driverPath = "/driver";
   static const String nextTestPath = "/next_test";
@@ -1315,14 +1321,14 @@ class BrowserTestingServer {
   var testCount = 0;
   var errorReportingServer;
   bool underTermination = false;
-  bool useIframe = false;
 
   Function testDoneCallBack;
   Function testStatusUpdateCallBack;
   Function testStartedCallBack;
   Function nextTestCallBack;
 
-  BrowserTestingServer(this.configuration, this.localIp, this.useIframe);
+  BrowserTestingServer(
+      this.configuration, this.localIp, this.useIframe, this.requiresFocus);
 
   Future start() {
     var test_driver_error_port = configuration['test_driver_error_port'];
@@ -1386,7 +1392,9 @@ class BrowserTestingServer {
     makeSendPageHandler(String prefix) => (HttpRequest request) {
       noCache(request);
       var textResponse = "";
+      bool isStartup = false;
       if (prefix == driverPath) {
+        isStartup = true;
         textResponse = getDriverPage(browserId(request, prefix));
         request.response.headers.set('Content-Type', 'text/html');
       }
@@ -1395,7 +1403,21 @@ class BrowserTestingServer {
         request.response.headers.set('Content-Type', 'text/plain');
       }
       request.response.write(textResponse);
-      request.listen((_) {}, onDone: request.response.close);
+      closeResponse(_) {
+        // Ignoring the returned closure as it returns the 'done' future which
+        // alread has catchError installed (see below).
+        request.response.close();
+      }
+      request.listen((_) {}, onDone: () {
+        if (isStartup && requiresFocus) {

[Bill Hesse, 2016/04/25 08:20:19]

Could we put this into the BrowserController object instead, and have it called
by getDriverPage?  That would mean the subprocess is created, and waited on,
before the page is written to the client, instead of after the page is written
but before the response is closed, but that should not matter.

I like having the platform-specific code in the BrowserController, and keeping
the testing server very generic.

If this is moved, then maybe we have a Browser object when handling it, so the
"requiresFocus" could become a getter instead of static, so the Safari class
could just override it with true.  That would also be a plus. The code creating
the subprocess could even move to the Safari class, which would be much much
better.

[ahe, 2016/05/24 08:44:42]

Done.

+          Process.run(
+              "/usr/bin/osascript",
+              ['-e', 'tell application "Safari" to activate'])
+          .then(closeResponse);
+        } else {
+          closeResponse(null);
+        }
+      });
       request.response.done.catchError((error) {
         if (!underTermination) {
           print("URI ${request.uri}");
@@ -1462,32 +1484,33 @@ class BrowserTestingServer {
 
 
   String getDriverPage(String browserId) {
+

[Bill Hesse, 2016/04/25 08:20:19]

I think this blank line should go.  Do we run this file through the formatter?

[ahe, 2016/05/24 08:44:42]

Done.

     var errorReportingUrl =
         "http://$localIp:${errorReportingServer.port}/$browserId";
     String driverContent = """
 <!DOCTYPE html><html>
 <head>
+  <title>Driving page</title>
   <style>
-    body {
-      margin: 0;
-    }
-    .box {
-      overflow: hidden;
-      overflow-y: auto;
-      position: absolute;
-      left: 0;
-      right: 0;
-    }
-    .controller.box {
-      height: 75px;
-      top: 0;
-    }
-    .test.box {
-      top: 75px;
-      bottom: 0;
-    }
+.big-notice {
+  background-color: red;
+  color: white;
+  font-weight: bold;
+  font-size: xx-large;
+  text-align: center;
+}
+.controller.box {
+  white-space: nowrap;
+  overflow: scroll;
+  height: 6em;
+}
+body {
+  font-family: sans-serif;
+}
+body div {
+  padding-top: 10px;
+}
   </style>
-  <title>Driving page</title>
   <script type='text/javascript'>
     var STATUS_UPDATE_INTERVAL = 10000;
 
@@ -1617,7 +1640,8 @@ class BrowserTestingServer {
           embedded_iframe_div.removeChild(embedded_iframe);
           embedded_iframe = document.createElement('iframe');
           embedded_iframe.id = "embedded_iframe";
-          embedded_iframe.style="width:100%;height:100%";
+          embedded_iframe.width='800px';
+          embedded_iframe.height='600px';
           embedded_iframe_div.appendChild(embedded_iframe);
           embedded_iframe.src = url;
         } else {
@@ -1789,13 +1813,26 @@ class BrowserTestingServer {
   </script>
 </head>
   <body onload="startTesting()">
+
+    <div class='big-notice'>
+      Please keep this window in focus at all times.
+    </div>
+
+    <div>
+      Some browsers, Safari, in particular, may pause JavaScript when not
+      visible to conserve power consumption and CPU resources. In addition,
+      some tests of focus events will not work correctly if this window doesn't
+      have focus. It's also advisable to close any other programs that may open
+      modal dialogs, for example, Chrome with Calendar open.
+    </div>
+
     <div class="controller box">
     Dart test driver, number of tests: <span id="number"></span><br>
     Currently executing: <span id="currently_executing"></span><br>
     Unhandled error: <span id="unhandled_error"></span>
     </div>
     <div id="embedded_iframe_div" class="test box">
-      <iframe style="width:100%;height:100%;" id="embedded_iframe"></iframe>
+      <iframe id="embedded_iframe"></iframe>
     </div>
   </body>
 </html>