phantomjs_test: resilience improvements (#453)

phantomjs tests fail spuriously on our CI infrastructure with two different
errors:

1. G_testRunner is not defined.

   I believe this is due to the harness setTimeout of 200ms firing before the js
   binary under test finished loading.

2. The test completes successfully, but Bazel does not get the message and
   eventually reports it as TIMEOUT.

   I believe this is due to the test runner resource fetch timing out.

These errors are addressed as follows:

1. Add an attribute to enable phantomjs debug logging.

2. Load the test runner as the last script instead of the first. That eliminates
   the race to solve issue 1.

3. Detect and log resource timeouts and retry the test. Assuming the resource
   timeouts happen with a low enough frequency, this should get tests that
   suffer one resource timeout to succeed on retry. Additionally, the logging
   will make the situation clear -- previously nothing was logged.

   If resource timeouts continue to be an issue, the next step would be to
   increase the 2s hardcoded timeout, or make it configurable.

Co-authored-by: Goktug Gokdogan <[email protected]>

closure/testing/closure_js_test.bzl

@@ -33,6 +33,7 @@ def closure_js_test(
         suppress = None,
         visibility = None,
         tags = [],
+        debug = False,
         **kwargs):
     if not srcs:
         fail("closure_js_test rules can not have an empty 'srcs' list")
@@ -81,6 +82,7 @@ def closure_js_test(
             name = shard,
             runner = str(Label("//closure/testing:phantomjs_jsunit_runner")),
             deps = [":%s_bin" % shard],
+            debug = debug,
             html = html,
             visibility = visibility,
             tags = tags,

closure/testing/externs/phantom.js

@@ -230,6 +230,12 @@ phantomjs.Page = class {
    * @return {void}
    */
   onLoadStarted() {}
+
+
+  /**
+   * @param {!phantomjs.Server.ResourceError} resource
+   */
+  onResourceTimeout(resource) {}
 };
 
 /**
@@ -278,7 +284,6 @@ phantomjs.Server.Request = class {};
  */
 phantomjs.Server.Request.prototype.url;
 
-
 /**
  * @record
  * @see http://phantomjs.org/api/webserver/method/listen.html
@@ -316,6 +321,31 @@ phantomjs.Server.Response = class {
 };
 
 
+/**
+ * @record
+ * @see https://phantomjs.org/api/webpage/handler/on-resource-error.html
+ */
+phantomjs.Server.ResourceError = class {};
+
+/**
+ * @type {string}
+ * @const
+ */
+phantomjs.Server.ResourceError.prototype.url;
+
+/**
+ * @type {number}
+ * @const
+ */
+phantomjs.Server.ResourceError.prototype.errorCode;
+
+/**
+ * @type {string}
+ * @const
+ */
+phantomjs.Server.ResourceError.prototype.errorString;
+
+
 /**
  * @record
  * @see http://phantomjs.org/api/webserver/

closure/testing/phantomjs_harness.js

@@ -189,6 +189,14 @@ function onLoadFinished(status) {
   }
 }
 
+/**
+ * Callback when a script fails to load in time.
+ * @param {!phantomjs.Server.ResourceError} resource
+ */
+function onResourceTimeout(resource) {
+    system.stderr.writeLine('Resource timed out: ' + resource.url);
+    retry();
+}
 
 /**
  * Callback when webpage shows an alert dialog.
@@ -246,7 +254,6 @@ function retry() {
   main();
 }
 
-
 /**
  * Attempts to run the test.
  */
@@ -288,6 +295,8 @@ function main() {
   page.onConsoleMessage = onConsoleMessage;
   page.onError = onError;
   page.onLoadFinished = onLoadFinished;
+  page.onResourceTimeout = onResourceTimeout;
+
   // XXX: If PhantomJS croaks, fail sooner rather than later.
   //      https://github.com/ariya/phantomjs/issues/10652
   page.settings.resourceTimeout = 2000;

closure/testing/phantomjs_test.bzl

@@ -32,20 +32,23 @@ def _impl(ctx):
     srcs = []
     direct_srcs = []
     deps = unfurl(ctx.attr.deps, provider = "closure_js_library")
-    deps.append(ctx.attr.runner)
     for dep in deps:
         if hasattr(dep, "closure_js_binary"):
             direct_srcs += [dep.closure_js_binary.bin]
         else:
             srcs += [dep.closure_js_library.srcs]
     srcs = depset(direct_srcs, transitive = srcs)
+    deps.append(ctx.attr.runner)
 
     args = [
         "#!/bin/sh\nexec " + ctx.executable._phantomjs.short_path,
-        ctx.attr.harness.closure_js_binary.bin.short_path,
-        ctx.file.html.short_path,
     ]
+    if ctx.attr.debug:
+        args += ["--debug=true"]
+    args += [ctx.attr.harness.closure_js_binary.bin.short_path]
+    args += [ctx.file.html.short_path]
     args += [long_path(ctx, src) for src in srcs.to_list()]
+    args += [long_path(ctx, src) for src in ctx.attr.runner.closure_js_library.srcs.to_list()]
     ctx.actions.write(
         is_executable = True,
         output = ctx.outputs.executable,
@@ -82,6 +85,10 @@ phantomjs_test = rule(
             default = Label("//closure/testing:empty.html"),
         ),
         "data": attr.label_list(allow_files = True),
+        "debug": attr.bool(
+            doc = "Activate phantomjs debug logging",
+            default = False,
+        ),
         "_phantomjs": attr.label(
             default = Label("//third_party/phantomjs"),
             executable = True,