From 2cfab04acdb2709d2b788c814213dbc6a4d79b38 Mon Sep 17 00:00:00 2001 From: Rob Figueiredo Date: Wed, 18 Mar 2020 21:57:32 -0400 Subject: [PATCH] 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 --- closure/testing/closure_js_test.bzl | 2 ++ closure/testing/externs/phantom.js | 32 +++++++++++++++++++++++++++- closure/testing/phantomjs_harness.js | 11 +++++++++- closure/testing/phantomjs_test.bzl | 13 ++++++++--- 4 files changed, 53 insertions(+), 5 deletions(-) diff --git a/closure/testing/closure_js_test.bzl b/closure/testing/closure_js_test.bzl index 6ea7e201ed..9bdd13d207 100644 --- a/closure/testing/closure_js_test.bzl +++ b/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, diff --git a/closure/testing/externs/phantom.js b/closure/testing/externs/phantom.js index cb4ffc02ab..1959ef9449 100644 --- a/closure/testing/externs/phantom.js +++ b/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/ diff --git a/closure/testing/phantomjs_harness.js b/closure/testing/phantomjs_harness.js index 9c83170850..058d4ca23d 100644 --- a/closure/testing/phantomjs_harness.js +++ b/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; diff --git a/closure/testing/phantomjs_test.bzl b/closure/testing/phantomjs_test.bzl index da37701447..5f4e735838 100644 --- a/closure/testing/phantomjs_test.bzl +++ b/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,