Make Safari tests more robust.

tools/testing/dart/browser_controller.dart

 // Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file

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

This is not rebased to the current version of browser_controller, which has many
changes.

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

Done.

 // 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 'android.dart';
 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();
   final StringBuffer eventLog = new StringBuffer();
 }
 
 /** Class describing the interface for communicating with browsers. */
 abstract class Browser {
   BrowserOutput _allBrowserOutput = new BrowserOutput();
   BrowserOutput _testBrowserOutput = new BrowserOutput();
@@ skipping 63 matching lines @@
     return browser;
   }
 
   static const List<String> SUPPORTED_BROWSERS =
     const ['safari', 'ff', 'firefox', 'chrome', 'ie9', 'ie10',
            'ie11', 'dartium'];
 
   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);
   }
 
   void _logEvent(String event) {
     String toLog = "$this ($id) - $event \n";
     if (debugPrint) print("usageLog: $toLog");
     if (logger != null) logger(toLog);
 
@@ skipping 153 matching lines @@
   Future<bool> start(String url);
 }
 
 class Safari extends Browser {
   /**
    * We get the safari version by parsing a version file
    */
   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;
-    });
-  }
-
-  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;
-            });
-      } else {
-        _logEvent("${paths.current} is not present");
-        return deleteIfExists(paths);
-      }
-    });
+    // 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);
   }
 
   // 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);
+  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 {
+        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);
+    }
+
+    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() {
     /**
      * Example of the file:
      * <?xml version="1.0" encoding="UTF-8"?>
      * <!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
      * <plist version="1.0">
      * <dict>
      *       <key>BuildVersion</key>
@@ skipping 40 matching lines @@
         if (!cleared) {
           _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);
+            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");
           return false;
         });
       });
     });
   }
 
   String toString() => "Safari";
@@ skipping 453 matching lines @@
  * 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);
 
   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)
   Function logger;
   int browserIdCount = 0;
 
   bool underTermination = false;
   int numBrowserGetTestTimeouts = 0;
 
   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>();
   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;
       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) {
@@ skipping 382 matching lines @@
   ///                           and run tests ...
   /// GET /next_test/BROWSER_ID -- returns "WAIT" "TERMINATE" or "url#id"
   /// where url is the test to run, and id is the id of the test.
   /// If there are currently no available tests the waitSignal is send
   /// back. If we are in the process of terminating the terminateSignal
   /// is send back and the browser will stop requesting new tasks.
   /// POST /report/BROWSER_ID?id=NUM -- sends back the dom of the executed
   ///                                   test
 
   final String localIp;
+  final bool useIframe;
+  final bool requiresFocus;
 
   static const String driverPath = "/driver";
   static const String nextTestPath = "/next_test";
   static const String reportPath = "/report";
   static const String statusUpdatePath = "/status_update";
   static const String startedPath = "/started";
   static const String waitSignal = "WAIT";
   static const String terminateSignal = "TERMINATE";
 
   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'];
     return HttpServer.bind(localIp, test_driver_error_port)
       .then(setupErrorServer)
       .then(setupDispatchingServer);
   }
 
   void setupErrorServer(HttpServer server) {
     errorReportingServer = server;
@@ skipping 43 matching lines @@
     });
     server.addHandler(startedPath, (HttpRequest request) {
       noCache(request);
       handleStarted(request, browserId(request, startedPath),
                    testId(request));
     });
 
     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');
       }
       if (prefix == nextTestPath) {
         textResponse = getNextTest(browserId(request, prefix));
         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}");
           print("Textresponse $textResponse");
           throw "Error returning content to browser: $error";
         }
       });
     };
     server.addHandler(driverPath, makeSendPageHandler(driverPath));
     server.addHandler(nextTestPath, makeSendPageHandler(nextTestPath));
@@ skipping 46 matching lines @@
             "produce driver url");
       exit(1);
       // This should never happen - exit immediately;
     }
     var port = configuration['_servers_'].port;
     return "http://$localIp:$port/driver/$browserId";
   }
 
 
   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;
 
     function startTesting() {
       var number_of_tests = 0;
       var current_id;
       var next_id;
 
       // Has the test in the current iframe reported that it is done?
       var test_completed = true;
@@ skipping 109 matching lines @@
         if (use_iframe) {
           if (html_test) {
             window.addEventListener('detect_errors', setChildHandlers, false);
             embedded_iframe.onload = checkChildHandlersInstalled;
           } else {
             embedded_iframe.onload = null;
           }
           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 {
           if (typeof testing_window != 'undefined') {
             testing_window.close();
           }
           testing_window = window.open(url);
         }
         test_started = false;
         test_completed = false;
@@ skipping 151 matching lines @@
 
       if (!html_test) {
         window.addEventListener('message', messageHandler, false);
         waitForDone = false;
       }
       getNextTask();
     }
   </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>
 """;
     return driverContent;
   }
 }