Address review

Author: juj

test/common.py

@@ -139,6 +139,12 @@ def configure(data_dir):
 class SafariConfig:
   default_flags = ('', )
   executable_name = 'Safari'
+  # For the macOS 'open' command, pass
+  #   --new: to make a new Safari app be launched, rather than add a tab to an existing Safari process/window
+  #   --fresh: do not restore old tabs (e.g. if user had old navigated windows open)
+  #   --background: Open the new Safari window behind the current Terminal window, to make following the test run more pleasing (this is for convenience only)
+  #   -a <exe_name>: The path to the executable to open, in this case Safari
+  launch_prefix = ('open', '--new', '--fresh', '--background', '-a')
 
   @staticmethod
   def configure(data_dir):
@@ -2589,6 +2595,8 @@ def browser_open(cls, url):
       config = FirefoxConfig()
     elif is_safari():
       config = SafariConfig()
+    elif EMTEST_BROWSER_AUTO_CONFIG:
+      exit_with_error(f'EMTEST_BROWSER_AUTO_CONFIG only currently works with firefox, chrome and safari. EMTEST_BROWSER was "{EMTEST_BROWSER}"')
 
     # Prepare the browser data directory, if it uses one.
     if EMTEST_BROWSER_AUTO_CONFIG and config and hasattr(config, 'data_dir_flag'):
@@ -2609,22 +2617,15 @@ def browser_open(cls, url):
       # Recreate the new data directory.
       os.mkdir(browser_data_dir)
 
-      if not config:
-        exit_with_error(f'EMTEST_BROWSER_AUTO_CONFIG only currently works with firefox, chrome and safari. EMTEST_BROWSER was "{EMTEST_BROWSER}"')
       if WINDOWS:
         # Escape directory delimiter backslashes for shlex.split.
         browser_data_dir = browser_data_dir.replace('\\', '\\\\')
       config.configure(browser_data_dir)
       browser_args += f' {config.data_dir_flag}"{browser_data_dir}"'
 
     browser_args = shlex.split(browser_args)
-    if is_safari():
-      # For the macOS 'open' command, pass
-      #   --new: to make a new Safari app be launched, rather than add a tab to an existing Safari process/window
-      #   --fresh: do not restore old tabs (e.g. if user had old navigated windows open)
-      #   --background: Open the new Safari window behind the current Terminal window, to make following the test run more pleasing (this is for convenience only)
-      #   -a <exe_name>: The path to the executable to open, in this case Safari
-      browser_args = ['open', '--new', '--fresh', '--background', '-a'] + browser_args
+    if hasattr(config, 'launch_prefix'):
+      browser_args = list(config.launch_prefix) + browser_args
 
     logger.info('Launching browser: %s', str(browser_args))
 
@@ -2661,7 +2662,7 @@ def launch_browser_harness_with_proc_snapshot_workaround(cls, parallel_harness,
         # by the delta before->after.
         cls.browser_procs = list(set(procs_after).difference(set(procs_before)))
         if len(cls.browser_procs) == 0:
-          logger.warning('Could not detect the launched browser subprocesses. The test harness may not be able to close browser windows if a test hangs, and at harness exit.')
+          exit_with_error('Could not detect the launched browser subprocesses. The test harness will not be able to close the browser after testing is done, so aborting the test run here.')
 
       # Firefox on Windows quirk:
       # Make sure that each browser window is visible on the desktop. Otherwise

test/test_browser.py

@@ -18,7 +18,7 @@
 from urllib.request import urlopen
 
 import common
-from common import BrowserCore, RunnerCore, path_from_root, has_browser, Reporting, is_chrome, is_firefox, CHROMIUM_BASED_BROWSERS
+from common import BrowserCore, RunnerCore, path_from_root, has_browser, Reporting, is_chrome, is_firefox, is_safari, CHROMIUM_BASED_BROWSERS
 from common import create_file, parameterized, ensure_dir, disabled, flaky, test_file, WEBIDL_BINDER
 from common import read_file, EMRUN, no_wasm64, no_2gb, no_4gb, copytree, skip_if, skip_if_simple
 from common import requires_wasm2js, parameterize, find_browser_test_file, with_all_sjlj