-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Fix parallel browser test harness to also work with Firefox on Windows. #25275
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
62c4e03
90b6d7e
cac56ff
54d34f6
d571e9b
16819f6
f4c1bb2
cee618b
f4abd11
a12d00d
8c7b3d3
982174f
df587ca
46cd393
5cd1b57
82bb79b
2210afa
b4f114c
aabbba6
4846cec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ | |
import json | ||
import logging | ||
import os | ||
import psutil | ||
import re | ||
import shlex | ||
import shutil | ||
|
@@ -36,7 +37,7 @@ | |
import clang_native | ||
import jsrun | ||
import line_endings | ||
from tools.shared import EMCC, EMXX, DEBUG | ||
from tools.shared import EMCC, EMXX, DEBUG, exe_suffix | ||
from tools.shared import get_canonical_temp_dir, path_from_root | ||
from tools.utils import MACOS, WINDOWS, read_file, read_binary, write_binary, exit_with_error | ||
from tools.settings import COMPILE_TIME_SETTINGS | ||
|
@@ -87,6 +88,7 @@ | |
# file to track which tests were flaky so they can be graphed in orange color to | ||
# visually stand out. | ||
flaky_tests_log_filename = os.path.join(path_from_root('out/flaky_tests.txt')) | ||
browser_spawn_lock_filename = os.path.join(path_from_root('out/browser_spawn_lock')) | ||
|
||
|
||
# Default flags used to run browsers in CI testing: | ||
|
@@ -116,6 +118,7 @@ class FirefoxConfig: | |
data_dir_flag = '-profile ' | ||
default_flags = () | ||
headless_flags = '-headless' | ||
executable_name = exe_suffix('firefox') | ||
|
||
@staticmethod | ||
def configure(data_dir): | ||
|
@@ -938,8 +941,25 @@ def make_dir_writeable(dirname): | |
|
||
|
||
def force_delete_dir(dirname): | ||
make_dir_writeable(dirname) | ||
utils.delete_dir(dirname) | ||
"""Deletes a directory. Returns whether deletion succeeded.""" | ||
if not os.path.exists(dirname): | ||
return True | ||
assert not os.path.isfile(dirname) | ||
|
||
try: | ||
make_dir_writeable(dirname) | ||
utils.delete_dir(dirname) | ||
except PermissionError as e: | ||
# This issue currently occurs on Windows when running browser tests e.g. | ||
# on Firefox browser. Killing Firefox browser is not 100% watertight, and | ||
# occassionally a Firefox browser process can be left behind, holding on | ||
# to a file handle, preventing the deletion from succeeding. | ||
# We expect this issue to only occur on Windows. | ||
if not WINDOWS: | ||
raise e | ||
print(f'Warning: Failed to delete directory "{dirname}"\n{e}') | ||
return False | ||
return True | ||
|
||
|
||
def force_delete_contents(dirname): | ||
|
@@ -2501,6 +2521,81 @@ def configure_test_browser(): | |
EMTEST_BROWSER += f" {config.headless_flags}" | ||
|
||
|
||
def list_processes_by_name(exe_name): | ||
pids = [] | ||
if exe_name: | ||
for proc in psutil.process_iter(): | ||
try: | ||
pinfo = proc.as_dict(attrs=['pid', 'name', 'exe']) | ||
if pinfo['exe'] and exe_name in pinfo['exe'].replace('\\', '/').split('/'): | ||
pids.append(psutil.Process(pinfo['pid'])) | ||
except psutil.NoSuchProcess: # E.g. "process no longer exists (pid=13132)" (code raced to acquire the iterator and process it) | ||
pass | ||
|
||
return pids | ||
|
||
|
||
class FileLock: | ||
"""Implements a filesystem-based mutex, with an additional feature that it | ||
returns an integer counter denoting how many times the lock has been locked | ||
before (during the current python test run instance)""" | ||
def __init__(self, path): | ||
self.path = path | ||
self.counter = 0 | ||
|
||
def __enter__(self): | ||
# Acquire the lock | ||
while True: | ||
try: | ||
self.fd = os.open(self.path, os.O_CREAT | os.O_EXCL | os.O_WRONLY) | ||
break | ||
except FileExistsError: | ||
time.sleep(0.1) | ||
# Return the locking count number | ||
try: | ||
self.counter = int(open(f'{self.path}_counter').read()) | ||
except Exception: | ||
pass | ||
return self.counter | ||
|
||
def __exit__(self, *a): | ||
# Increment locking count number before releasing the lock | ||
with open(f'{self.path}_counter', 'w') as f: | ||
f.write(str(self.counter + 1)) | ||
# And release the lock | ||
os.close(self.fd) | ||
try: | ||
os.remove(self.path) | ||
except Exception: | ||
pass # Another process has raced to acquire the lock, and will delete it. | ||
|
||
|
||
def move_browser_window(pid, x, y): | ||
"""Utility function to move the top-level window owned by given process to | ||
(x,y) coordinate. Used to ensure each browser window has some visible area.""" | ||
import win32gui | ||
import win32process | ||
|
||
def enum_windows_callback(hwnd, _unused): | ||
_, win_pid = win32process.GetWindowThreadProcessId(hwnd) | ||
if win_pid == pid and win32gui.IsWindowVisible(hwnd): | ||
rect = win32gui.GetWindowRect(hwnd) | ||
win32gui.MoveWindow(hwnd, x, y, rect[2] - rect[0], rect[3] - rect[1], True) | ||
return True | ||
|
||
win32gui.EnumWindows(enum_windows_callback, None) | ||
|
||
|
||
def increment_suffix_number(str_with_maybe_suffix): | ||
match = re.match(r"^(.*?)(?:_(\d+))?$", str_with_maybe_suffix) | ||
if match: | ||
base, number = match.groups() | ||
if number: | ||
return f'{base}_{int(number) + 1}' | ||
|
||
return f'{str_with_maybe_suffix}_1' | ||
|
||
|
||
class BrowserCore(RunnerCore): | ||
# note how many tests hang / do not send an output. if many of these | ||
# happen, likely something is broken and it is best to abort the test | ||
|
@@ -2517,15 +2612,19 @@ def __init__(self, *args, **kwargs): | |
|
||
@classmethod | ||
def browser_terminate(cls): | ||
cls.browser_proc.terminate() | ||
# If the browser doesn't shut down gracefully (in response to SIGTERM) | ||
# after 2 seconds kill it with force (SIGKILL). | ||
try: | ||
cls.browser_proc.wait(2) | ||
except subprocess.TimeoutExpired: | ||
logger.info('Browser did not respond to `terminate`. Using `kill`') | ||
cls.browser_proc.kill() | ||
cls.browser_proc.wait() | ||
for proc in cls.browser_procs: | ||
try: | ||
proc.terminate() | ||
# If the browser doesn't shut down gracefully (in response to SIGTERM) | ||
# after 2 seconds kill it with force (SIGKILL). | ||
try: | ||
proc.wait(2) | ||
except (subprocess.TimeoutExpired, psutil.TimeoutExpired): | ||
logger.info('Browser did not respond to `terminate`. Using `kill`') | ||
proc.kill() | ||
proc.wait() | ||
except (psutil.NoSuchProcess, ProcessLookupError): | ||
pass | ||
|
||
@classmethod | ||
def browser_restart(cls): | ||
|
@@ -2544,9 +2643,18 @@ def browser_open(cls, url): | |
if worker_id is not None: | ||
# Running in parallel mode, give each browser its own profile dir. | ||
browser_data_dir += '-' + str(worker_id) | ||
if os.path.exists(browser_data_dir): | ||
utils.delete_dir(browser_data_dir) | ||
|
||
# Delete old browser data directory. | ||
if WINDOWS: | ||
# If we cannot (the data dir is in use on Windows), switch to another dir. | ||
while not force_delete_dir(browser_data_dir): | ||
browser_data_dir = increment_suffix_number(browser_data_dir) | ||
else: | ||
force_delete_dir(browser_data_dir) | ||
|
||
# Recreate the new data directory. | ||
os.mkdir(browser_data_dir) | ||
|
||
if is_chrome(): | ||
config = ChromeConfig() | ||
elif is_firefox(): | ||
|
@@ -2561,7 +2669,41 @@ def browser_open(cls, url): | |
|
||
browser_args = shlex.split(browser_args) | ||
logger.info('Launching browser: %s', str(browser_args)) | ||
cls.browser_proc = subprocess.Popen(browser_args + [url]) | ||
|
||
if WINDOWS and is_firefox(): | ||
cls.launch_browser_harness_windows_firefox(worker_id, config, browser_args, url) | ||
else: | ||
cls.browser_procs = [subprocess.Popen(browser_args + [url])] | ||
|
||
@classmethod | ||
def launch_browser_harness_windows_firefox(cls, worker_id, config, browser_args, url): | ||
''' Dedicated function for launching browser harness on Firefox on Windows, | ||
which requires extra care for window positioning and process tracking.''' | ||
|
||
with FileLock(browser_spawn_lock_filename) as count: | ||
# Firefox is a multiprocess browser. On Windows, killing the spawned | ||
# process will not bring down the whole browser, but only one browser tab. | ||
# So take a delta snapshot before->after spawning the browser to find | ||
# which subprocesses we launched. | ||
if worker_id is not None: | ||
procs_before = list_processes_by_name(config.executable_name) | ||
cls.browser_procs = [subprocess.Popen(browser_args + [url])] | ||
# Give Firefox time to spawn its subprocesses. Use an increasing timeout | ||
# as a crude way to account for system load. | ||
if worker_id is not None: | ||
time.sleep(2 + count * 0.3) | ||
procs_after = list_processes_by_name(config.executable_name) | ||
# Make sure that each browser window is visible on the desktop. Otherwise | ||
# browser might decide that the tab is backgrounded, and not load a test, | ||
# or it might not tick rAF()s forward, causing tests to hang. | ||
if worker_id is not None and not EMTEST_HEADLESS: | ||
# On Firefox on Windows we needs to track subprocesses that got created | ||
# by Firefox. Other setups can use 'browser_proc' directly to terminate | ||
# the browser. | ||
cls.browser_procs = list(set(procs_after).difference(set(procs_before))) | ||
# Wrap window positions on a Full HD desktop area modulo primes. | ||
for proc in cls.browser_procs: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This shouldn't be needed in headless mode. Headless should bypass the window active tracking stuff. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, updated. |
||
move_browser_window(proc.pid, (300 + count * 47) % 1901, (10 + count * 37) % 997) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of putting
Perhaps append There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok I moved to a separate function. |
||
|
||
@classmethod | ||
def setUpClass(cls): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -564,6 +564,8 @@ def set_env(name, option_value): | |
|
||
# Remove any old test files before starting the run | ||
utils.delete_file(common.flaky_tests_log_filename) | ||
utils.delete_file(common.browser_spawn_lock_filename) | ||
utils.delete_file(f'{common.browser_spawn_lock_filename}_counter') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it not possible to store the count in the lock file itself? (i.e. can we avoid separate counter file?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did that first, but got odd behavior with O_EXCL when O_CREAT was not always unconditionally passed. So reverted to this simpler two-file form |
||
|
||
def prepend_default(arg): | ||
if arg.startswith('test_'): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I lean towards beta, so we're not living so near the bleeding edge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pre-existing. We already use firefox-nightly elsewhere in this file. We can consider changing separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linux Firefox runs also download Nightly, so I did not want to diverge there. Maybe if we want to test beta, we'd switch both Linux and Windows Firefoxes at the same time, as a separate PR?