Do not restart browser when there is test type switch (#40556)

* Do not restart browser when test type changes

At least for Chrome, parameters used to start Chrome won't change when
test type changes (except for wdspec tests).

This change tries to switch to a different executor where allowed, and
reuse the webdriver session from the previous executor.

A browser side flag is used to control whether this behavior should be
enabled or not. It is up to each browser vendor to decide whether it
should be enabled or not.

Bug: 341581718

* add type annotation

* fix unit tests

* update per review comments

* Apply suggestions from code review

Co-authored-by: Jonathan Lee <[email protected]>

* check protocol class when switch executor

* assert protocol class matches in setup

---------

Co-authored-by: Jonathan Lee <[email protected]>

Author: WeizhongX

tools/wptrunner/wptrunner/browsers/base.py

@@ -110,6 +110,10 @@ def setup(self) -> None:
         """Used for browser-specific setup that happens at the start of a test run"""
         pass
 
+    def restart_on_test_type_change(self, new_test_type: str, old_test_type: str) -> bool:
+        """Determines if a restart is needed when there is a test type switch."""
+        return True
+
     def settings(self, test: Test) -> BrowserSettings:
         """Dictionary of metadata that is constant for a specific launch of a browser.
 

tools/wptrunner/wptrunner/browsers/chrome.py

@@ -6,12 +6,12 @@
 from .base import get_timeout_multiplier   # noqa: F401
 from .base import cmd_arg
 from ..executors import executor_kwargs as base_executor_kwargs
-from ..executors.executorwebdriver import WebDriverCrashtestExecutor  # noqa: F401
 from ..executors.base import WdspecExecutor  # noqa: F401
 from ..executors.executorchrome import (  # noqa: F401
     ChromeDriverPrintRefTestExecutor,
     ChromeDriverRefTestExecutor,
     ChromeDriverTestharnessExecutor,
+    ChromeDriverCrashTestExecutor
 )
 
 
@@ -22,7 +22,7 @@
                               "reftest": "ChromeDriverRefTestExecutor",
                               "print-reftest": "ChromeDriverPrintRefTestExecutor",
                               "wdspec": "WdspecExecutor",
-                              "crashtest": "WebDriverCrashtestExecutor"},
+                              "crashtest": "ChromeDriverCrashTestExecutor"},
                  "browser_kwargs": "browser_kwargs",
                  "executor_kwargs": "executor_kwargs",
                  "env_extras": "env_extras",
@@ -194,6 +194,13 @@ def update_properties():
 
 
 class ChromeBrowser(WebDriverBrowser):
+    def restart_on_test_type_change(self, new_test_type: str, old_test_type: str) -> bool:
+        # Restart the test runner when switch from/to wdspec tests. Wdspec test
+        # is using a different protocol class so a restart is always needed.
+        if "wdspec" in [old_test_type, new_test_type]:
+            return True
+        return False
+
     def make_command(self):
         return [self.webdriver_binary,
                 cmd_arg("port", str(self.port)),

tools/wptrunner/wptrunner/executors/base.py

@@ -286,13 +286,17 @@ def __init__(self, logger, browser, server_config, timeout_multiplier=1,
                                  "prefs": {}}
         self.protocol = None  # This must be set in subclasses
 
-    def setup(self, runner):
+    def setup(self, runner, protocol=None):
         """Run steps needed before tests can be started e.g. connecting to
         browser instance
 
-        :param runner: TestRunner instance that is going to run the tests"""
+        :param runner: TestRunner instance that is going to run the tests.
+        :param protocol: protocol connection to reuse if not None"""
         self.runner = runner
-        if self.protocol is not None:
+        if protocol is not None:
+            assert isinstance(protocol, self.protocol_cls)
+            self.protocol = protocol
+        elif self.protocol is not None:
             self.protocol.setup(runner)
 
     def teardown(self):
@@ -630,7 +634,8 @@ def __init__(self, logger, browser, server_config, webdriver_binary,
         self.binary = binary
         self.binary_args = binary_args
 
-    def setup(self, runner):
+    def setup(self, runner, protocol=None):
+        assert protocol is None, "Switch executor not allowed for wdspec tests."
         self.protocol = self.protocol_cls(self, self.browser)
         super().setup(runner)
 

tools/wptrunner/wptrunner/executors/executorchrome.py

@@ -215,6 +215,10 @@ class ChromeDriverProtocol(WebDriverProtocol):
     vendor_prefix = "goog"
 
 
+class ChromeDriverCrashTestExecutor(WebDriverCrashtestExecutor):
+    protocol_cls = ChromeDriverProtocol
+
+
 class ChromeDriverRefTestExecutor(WebDriverRefTestExecutor, _SanitizerMixin):  # type: ignore
     protocol_cls = ChromeDriverProtocol
 
@@ -226,8 +230,8 @@ def __init__(self, *args, reuse_window=False, **kwargs):
         super().__init__(*args, **kwargs)
         self.protocol.reuse_window = reuse_window
 
-    def setup(self, runner):
-        super().setup(runner)
+    def setup(self, runner, protocol=None):
+        super().setup(runner, protocol)
         # Chromium requires the `background-sync` permission for reporting APIs
         # to work. Not all embedders (notably, `chrome --headless=old`) grant
         # `background-sync` by default, so this CDP call ensures the permission
@@ -248,8 +252,8 @@ def setup(self, runner):
 class ChromeDriverPrintRefTestExecutor(ChromeDriverRefTestExecutor):
     protocol_cls = ChromeDriverProtocol
 
-    def setup(self, runner):
-        super().setup(runner)
+    def setup(self, runner, protocol=None):
+        super().setup(runner, protocol)
         self.protocol.pdf_print.load_runner()
         self.has_window = False
         with open(os.path.join(here, "reftest.js")) as f:

tools/wptrunner/wptrunner/executors/executoredge.py

@@ -38,8 +38,8 @@ def __init__(self, *args, reuse_window=False, **kwargs):
 class EdgeDriverPrintRefTestExecutor(EdgeDriverRefTestExecutor):
     protocol_cls = EdgeDriverProtocol
 
-    def setup(self, runner):
-        super().setup(runner)
+    def setup(self, runner, protocol=None):
+        super().setup(runner, protocol)
         self.protocol.pdf_print.load_runner()
         self.has_window = False
         with open(os.path.join(here, "reftest.js")) as f:

tools/wptrunner/wptrunner/executors/executormarionette.py

@@ -952,8 +952,8 @@ def __init__(self, logger, browser, server_config, timeout_multiplier=1,
         if marionette is None:
             do_delayed_imports()
 
-    def setup(self, runner):
-        super().setup(runner)
+    def setup(self, runner, protocol=None):
+        super().setup(runner, protocol)
         for extension_path in self.install_extensions:
             self.logger.info("Installing extension from %s" % extension_path)
             addons = Addons(self.protocol.marionette)
@@ -1081,8 +1081,8 @@ def get_implementation(self, reftest_internal):
         return (InternalRefTestImplementation if reftest_internal
                 else RefTestImplementation)(self)
 
-    def setup(self, runner):
-        super().setup(runner)
+    def setup(self, runner, protocol=None):
+        super().setup(runner, protocol)
         for extension_path in self.install_extensions:
             self.logger.info("Installing extension from %s" % extension_path)
             addons = Addons(self.protocol.marionette)
@@ -1335,8 +1335,8 @@ def __init__(self, logger, browser, server_config, timeout_multiplier=1,
                                            debug=debug,
                                            **kwargs)
 
-    def setup(self, runner):
-        super().setup(runner)
+    def setup(self, runner, protocol=None):
+        super().setup(runner, protocol)
         if not isinstance(self.implementation, InternalRefTestImplementation):
             self.protocol.pdf_print.load_runner()
 

tools/wptrunner/wptrunner/executors/process.py

@@ -10,7 +10,7 @@ def __init__(self, *args, **kwargs):
         self.interactive = (False if self.debug_info is None
                             else self.debug_info.interactive)
 
-    def setup(self, runner):
+    def setup(self, runner, protocol=None):
         self.runner = runner
         self.runner.send_message("init_succeeded")
         return True

tools/wptrunner/wptrunner/testrunner.py

@@ -30,6 +30,11 @@ def release_mozlog_lock():
                                  'browser_cls', 'browser_kwargs'])
 
 
+ExecutorImplementation = namedtuple('ExecutorImplementation',
+                                ['executor_cls', 'executor_kwargs',
+                                 'executor_browser_cls', 'executor_browser_kwargs'])
+
+
 class StopFlag:
     """Synchronization for coordinating a graceful exit."""
 
@@ -75,11 +80,13 @@ class TestRunner:
                          parent TestRunnerManager process
     :param executor: TestExecutor object that will actually run a test.
     """
-    def __init__(self, logger, command_queue, result_queue, executor, recording):
+    def __init__(self, logger, command_queue, result_queue, executor_implementation, recording):
         self.command_queue = command_queue
         self.result_queue = result_queue
-
-        self.executor = executor
+        browser = executor_implementation.executor_browser_cls(
+            **executor_implementation.executor_browser_kwargs)
+        self.executor = executor_implementation.executor_cls(
+            logger, browser, **executor_implementation.executor_kwargs)
         self.name = mpcontext.get_context().current_process().name
         self.logger = logger
         self.recording = recording
@@ -108,6 +115,28 @@ def teardown(self):
         self.command_queue = None
         self.browser = None
 
+    def switch_executor(self, executor_implementation):
+        assert self.executor is not None
+        # reuse the current protocol connection
+        protocol = self.executor.protocol
+        self.executor.protocol = None
+        self.executor.teardown()
+        browser = executor_implementation.executor_browser_cls(
+            **executor_implementation.executor_browser_kwargs)
+        self.executor = executor_implementation.executor_cls(
+            self.logger, browser, **executor_implementation.executor_kwargs)
+        if type(self.executor.protocol) is not type(protocol):
+            self.send_message("switch_executor_failed")
+            self.logger.error("Protocol type does not match, switch executor failed.")
+            return
+        try:
+            self.executor.setup(self, protocol)
+        except Exception:
+            self.send_message("switch_executor_failed")
+        else:
+            self.send_message("switch_executor_succeeded")
+        self.logger.debug("Switch Executor done")
+
     def run(self):
         """Main loop accepting commands over the pipe and triggering
         the associated methods"""
@@ -118,6 +147,7 @@ def run(self):
                                 traceback.format_exc())
             raise
         commands = {"run_test": self.run_test,
+                    "switch_executor": self.switch_executor,
                     "reset": self.reset,
                     "stop": self.stop,
                     "wait": self.wait}
@@ -157,9 +187,8 @@ def send_message(self, command, *args):
 
 
 def start_runner(runner_command_queue, runner_result_queue,
-                 executor_cls, executor_kwargs,
-                 executor_browser_cls, executor_browser_kwargs,
-                 capture_stdio, stop_flag, recording):
+                 executor_implementation, capture_stdio,
+                 stop_flag, recording):
     """Launch a TestRunner in a new process"""
 
     def send_message(command, *args):
@@ -179,9 +208,11 @@ def handle_error(e):
 
     with capture.CaptureIO(logger, capture_stdio):
         try:
-            browser = executor_browser_cls(**executor_browser_kwargs)
-            executor = executor_cls(logger, browser, **executor_kwargs)
-            with TestRunner(logger, runner_command_queue, runner_result_queue, executor, recording) as runner:
+            with TestRunner(logger,
+                            runner_command_queue,
+                            runner_result_queue,
+                            executor_implementation,
+                            recording) as runner:
                 try:
                     runner.run()
                 except KeyboardInterrupt:
@@ -301,6 +332,8 @@ class _RunnerManagerState:
     running = namedtuple("running", ["subsuite", "test_type", "test", "test_group", "group_metadata"])
     restarting = namedtuple("restarting", ["subsuite", "test_type", "test", "test_group",
                                            "group_metadata", "force_stop"])
+    switching_executor = namedtuple("switching_executor",
+                                    ["subsuite", "test_type", "test", "test_group", "group_metadata"])
     error = namedtuple("error", [])
     stop = namedtuple("stop", ["force_stop"])
 
@@ -482,6 +515,11 @@ def wait_event(self):
                 "test_ended": self.test_ended,
                 "wait_finished": self.wait_finished,
             },
+            RunnerManagerState.switching_executor:
+            {
+                "switch_executor_succeeded": self.switch_executor_succeeded,
+                "switch_executor_failed": self.switch_executor_failed,
+            },
             RunnerManagerState.restarting: {},
             RunnerManagerState.error: {},
             RunnerManagerState.stop: {},
@@ -589,18 +627,11 @@ def start_test_runner(self):
         assert self.remote_queue is not None
         self.logger.info("Starting runner")
         impl = self.test_implementations[(self.state.subsuite, self.state.test_type)]
-        self.executor_cls = impl.executor_cls
-        self.executor_kwargs = impl.executor_kwargs
-        self.executor_kwargs["group_metadata"] = self.state.group_metadata
-        self.executor_kwargs["browser_settings"] = self.browser.browser_settings
-        executor_browser_cls, executor_browser_kwargs = self.browser.browser.executor_browser()
+        self.executor_implementation = self.get_executor_implementation(impl)
 
         args = (self.remote_queue,
                 self.command_queue,
-                self.executor_cls,
-                self.executor_kwargs,
-                executor_browser_cls,
-                executor_browser_kwargs,
+                self.executor_implementation,
                 self.capture_stdio,
                 self.child_stop_flag,
                 self.recording)
@@ -613,6 +644,16 @@ def start_test_runner(self):
         self.logger.debug("Test runner started")
         # Now we wait for either an init_succeeded event or an init_failed event
 
+    def get_executor_implementation(self, impl):
+        executor_kwargs = impl.executor_kwargs
+        executor_kwargs["group_metadata"] = self.state.group_metadata
+        executor_kwargs["browser_settings"] = self.browser.browser_settings
+        executor_browser_cls, executor_browser_kwargs = self.browser.browser.executor_browser()
+        return ExecutorImplementation(impl.executor_cls,
+                                      executor_kwargs,
+                                      executor_browser_cls,
+                                      executor_browser_kwargs)
+
     def init_succeeded(self):
         assert isinstance(self.state, RunnerManagerState.initializing)
         self.browser.after_init()
@@ -671,8 +712,9 @@ def run_test(self):
             # Factor of 3 on the extra timeout here is based on allowing the executor
             # at least test.timeout + 2 * extra_timeout to complete,
             # which in turn is based on having several layers of timeout inside the executor
-            wait_timeout = (self.state.test.timeout * self.executor_kwargs['timeout_multiplier'] +
-                            3 * self.executor_cls.extra_timeout)
+            timeout_multiplier = self.executor_implementation.executor_kwargs['timeout_multiplier']
+            wait_timeout = (self.state.test.timeout * timeout_multiplier +
+                            3 * self.executor_implementation.executor_cls.extra_timeout)
             self.timer = threading.Timer(wait_timeout, self._timeout)
             self.timer.name = f"{self.name}-timeout"
 
@@ -796,7 +838,8 @@ def test_ended(self, test, results):
                                             test.min_assertion_count,
                                             test.max_assertion_count)
 
-        file_result.extra["test_timeout"] = test.timeout * self.executor_kwargs['timeout_multiplier']
+        timeout_multiplier = self.executor_implementation.executor_kwargs['timeout_multiplier']
+        file_result.extra["test_timeout"] = test.timeout * timeout_multiplier
         if self.browser.browser_pid:
             file_result.extra["browser_pid"] = self.browser.browser_pid
 
@@ -832,6 +875,23 @@ def wait_finished(self, rerun=False):
         # post-stop processing
         return self.after_test_end(self.state.test, not rerun, force_rerun=rerun)
 
+    def switch_executor_succeeded(self):
+        assert isinstance(self.state, RunnerManagerState.switching_executor)
+        return RunnerManagerState.running(self.state.subsuite,
+                                          self.state.test_type,
+                                          self.state.test,
+                                          self.state.test_group,
+                                          self.state.group_metadata)
+
+    def switch_executor_failed(self):
+        assert isinstance(self.state, RunnerManagerState.switching_executor)
+        return RunnerManagerState.restarting(self.state.subsuite,
+                                             self.state.test_type,
+                                             self.state.test,
+                                             self.state.test_group,
+                                             self.state.group_metadata,
+                                             False)
+
     def after_test_end(self, test, restart, force_rerun=False, force_stop=False):
         assert isinstance(self.state, RunnerManagerState.running)
         # Mixing manual reruns and automatic reruns is confusing; we currently assume
@@ -844,12 +904,20 @@ def after_test_end(self, test, restart, force_rerun=False, force_stop=False):
             if subsuite != self.state.subsuite:
                 self.logger.info(f"Restarting browser for new subsuite:{subsuite}")
                 restart = True
-            elif test_type != self.state.test_type:
-                self.logger.info(f"Restarting browser for new test type:{test_type}")
-                restart = True
             elif self.restart_on_new_group and test_group is not self.state.test_group:
                 self.logger.info("Restarting browser for new test group")
                 restart = True
+            elif test_type != self.state.test_type:
+                if self.browser.browser.restart_on_test_type_change(test_type, self.state.test_type):
+                    self.logger.info(f"Restarting browser for new test type:{test_type}")
+                    restart = True
+                else:
+                    self.logger.info(f"Switching executor for new test type: {self.state.test_type} => {test_type}")
+                    impl = self.test_implementations[subsuite, test_type]
+                    self.executor_implementation = self.get_executor_implementation(impl)
+                    self.send_message("switch_executor", self.executor_implementation)
+                    return RunnerManagerState.switching_executor(
+                        subsuite, test_type, test, test_group, group_metadata)
         else:
             subsuite = self.state.subsuite
             test_type = self.state.test_type