From 625a2e0cd35092861345e382c06c110cc3d7e732 Mon Sep 17 00:00:00 2001 From: Stephen McGruer Date: Tue, 22 Sep 2020 09:16:38 +0000 Subject: [PATCH] Bug 1665366 [wpt PR 25567] - Reland "[webdriver] Close old windows at the end of each test as well as beginning", a=testonly Automatic update from web-platform-tests Reland "[webdriver] Close old windows at the end of each test as well as beginning" (#25567) This is a reland of #24879 (which was reverted in #25544). It updates the code to only close the windows if the user hasn't asked us to pause after a test has run. From the original PR: ---- Previously, we closed old windows at the start of each test. This was nice in terms of defensiveness (don't assume the last run left the world in a good state), but made it hard to find problematic tests that left dialogs open (since they wouldn't throw until the next test). Instead, this patch does both - close both at the start and end of a test. This should improve the blaming situation, whilst still being defensive. There are potential performance implications to this patch, however test runs are inconclusive. Full runs of Chrome and Safari show +- 2%, which is possibly within margin of error. Running locally, some directories showed a ~2% slowdown, whilst others had little or no difference. -- wpt-commits: ed3c2cd80ae2766813412695306c31716f3c82a7 wpt-pr: 25567 --- .../tools/wptrunner/wptrunner/executors/base.py | 6 ++++++ .../wptrunner/executors/executorwebdriver.py | 15 ++++++++++++++- .../wptrunner/tests/browsers/test_webkitgtk.py | 2 ++ 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/testing/web-platform/tests/tools/wptrunner/wptrunner/executors/base.py b/testing/web-platform/tests/tools/wptrunner/wptrunner/executors/base.py index e8a776b59356..b5820a380856 100644 --- a/testing/web-platform/tests/tools/wptrunner/wptrunner/executors/base.py +++ b/testing/web-platform/tests/tools/wptrunner/wptrunner/executors/base.py @@ -37,6 +37,12 @@ def executor_kwargs(test_type, server_config, cache_manager, run_info_data, executor_kwargs["webdriver_binary"] = kwargs.get("webdriver_binary") executor_kwargs["webdriver_args"] = kwargs.get("webdriver_args") + # By default the executor may try to cleanup windows after a test (to best + # associate any problems with the test causing them). If the user might + # want to view the results, however, the executor has to skip that cleanup. + if kwargs["pause_after_test"] or kwargs["pause_on_unexpected"]: + executor_kwargs["cleanup_after_test"] = False + return executor_kwargs diff --git a/testing/web-platform/tests/tools/wptrunner/wptrunner/executors/executorwebdriver.py b/testing/web-platform/tests/tools/wptrunner/wptrunner/executors/executorwebdriver.py index e0414669372e..ad79600aeb8f 100644 --- a/testing/web-platform/tests/tools/wptrunner/wptrunner/executors/executorwebdriver.py +++ b/testing/web-platform/tests/tools/wptrunner/wptrunner/executors/executorwebdriver.py @@ -369,7 +369,8 @@ class WebDriverTestharnessExecutor(TestharnessExecutor): def __init__(self, logger, browser, server_config, timeout_multiplier=1, close_after_done=True, capabilities=None, debug_info=None, - supports_eager_pageload=True, **kwargs): + supports_eager_pageload=True, cleanup_after_test=True, + **kwargs): """WebDriver-based executor for testharness.js tests""" TestharnessExecutor.__init__(self, logger, browser, server_config, timeout_multiplier=timeout_multiplier, @@ -383,6 +384,7 @@ class WebDriverTestharnessExecutor(TestharnessExecutor): self.close_after_done = close_after_done self.window_id = str(uuid.uuid4()) self.supports_eager_pageload = supports_eager_pageload + self.cleanup_after_test = cleanup_after_test def is_alive(self): return self.protocol.is_alive() @@ -409,7 +411,10 @@ class WebDriverTestharnessExecutor(TestharnessExecutor): def do_testharness(self, protocol, url, timeout): format_map = {"url": strip_server(url)} + # The previous test may not have closed its old windows (if something + # went wrong or if cleanup_after_test was False), so clean up here. parent_window = protocol.testharness.close_old_windows() + # Now start the test harness protocol.base.execute_script("window.open('about:blank', '%s', 'noopener')" % self.window_id) test_window = protocol.testharness.get_test_window(self.window_id, @@ -446,6 +451,14 @@ class WebDriverTestharnessExecutor(TestharnessExecutor): done, rv = handler(result) if done: break + + # Attempt to cleanup any leftover windows, if allowed. This is + # preferable as it will blame the correct test if something goes wrong + # closing windows, but if the user wants to see the test results we + # have to leave the window(s) open. + if self.cleanup_after_test: + protocol.testharness.close_old_windows() + return rv def wait_for_load(self, protocol): diff --git a/testing/web-platform/tests/tools/wptrunner/wptrunner/tests/browsers/test_webkitgtk.py b/testing/web-platform/tests/tools/wptrunner/wptrunner/tests/browsers/test_webkitgtk.py index be00dc4a6b77..6150d50acf93 100644 --- a/testing/web-platform/tests/tools/wptrunner/wptrunner/tests/browsers/test_webkitgtk.py +++ b/testing/web-platform/tests/tools/wptrunner/wptrunner/tests/browsers/test_webkitgtk.py @@ -39,6 +39,8 @@ def test_webkitgtk_certificate_domain_list(product): kwargs["webkit_port"] = "gtk" kwargs["binary"] = None kwargs["webdriver_binary"] = None + kwargs["pause_after_test"] = False + kwargs["pause_on_unexpected"] = False with ConfigBuilder(browser_host="example.net", alternate_hosts={"alt": "example.org"}, subdomains={"a", "b"},