[py] improve how tests are run against the grid server

[titusfortner]

User description

🔗 Related Issues

Splitting this out from #16814 so we can manage it independently

💥 What does this PR do?

• Changes running remote as a specific driver to an arg that works for any browser
• Now have chrome-remote and firefox-remote targets with tags that can be filtered on
• Use bazel java by default (fallback to which java)
• Use bazel built grid by default (fallback to selenium manager download)

🔧 Implementation Notes

• This starts a new grid for each parallel process because I couldn't get it to reuse one without causing more problems. At least in CI we aren't running in parallel, but local execution might suffer.

💡 Additional Considerations

• remote_connection_tests.py are failing now because they were pointing to the wrong server, and I'm not sure they are testing what we think they are testing. Needs to be fixed, but outside the scope of this PR

🔄 Types of changes

• New feature (non-breaking change which adds functionality and tests!)

---

PR Type

Enhancement

---

Description

• Refactored remote testing to use --remote flag instead of driver selection

• Added support for browser-specific remote test targets (chrome-remote, firefox-remote)

• Integrated Bazel-built Java and Grid server with fallback to system/Selenium Manager

• Enhanced Server class with configurable java_path and startup_timeout parameters

• Marked Firefox-specific tests as xfail_remote to skip when running remotely

---

Diagram Walkthrough

flowchart LR
  A["Test Configuration"] -->|"--remote flag"| B["Remote Mode Detection"]
  B -->|"is_remote property"| C["Driver Initialization"]
  C -->|"command_executor"| D["Grid Server"]
  E["Bazel Java Runtime"] -->|"java_path"| D
  F["Bazel Built JAR"] -->|"jar_path"| D
  G["System Java/Selenium Manager"] -->|"fallback"| D
  D -->|"status_url"| H["Remote WebDriver"]

Loading

File Walkthrough

	Relevant files
Enhancement	2 files conftest.py Refactored remote testing to use --remote flag                      +84/-56  server.py Added java_path and startup_timeout configuration                +34/-3
Configuration changes	2 files pytest.bzl Added data attribute to py_test rule                                          +1/-0      BUILD.bazel Created browser-specific remote test targets with Bazel integration +50/-27
Tests	6 files chrome_network_emulation_tests.py Marked test as xfail_remote                                                            +1/-0      ff_installs_addons_tests.py Marked module as xfail_remote for Firefox-specific APIs    +2/-0      ff_takes_full_page_screenshots_tests.py Marked module as xfail_remote for Firefox-specific APIs    +5/-0      remote_connection_tests.py Updated tests to use Grid server and marked as xfail          +13/-14  remote_custom_locator_tests.py Updated fixture to use Grid server command_executor            +5/-2      remote_firefox_profile_tests.py Updated test to use Grid server command_executor                  +3/-2
Additional files	2 files driver_finder_tests.py [link]    selenium_manager_tests.py [link]

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Untrusted executable path Description: The remote Grid server startup may execute an attacker-controlled java binary if SE_BAZEL_JAVA_LOCATION (and its referenced java-location.txt content) can be influenced in the test environment, leading to arbitrary code execution when Server.start() invokes subprocess.Popen. conftest.py [442-465] Referred Code r = Runfiles.Create() java_location_txt = r.Rlocation("_main/" + os.environ.get("SE_BAZEL_JAVA_LOCATION")) try: with open(java_location_txt, encoding="utf-8") as handle: read = handle.read().strip() rel_path = read[len("external/") :] if read.startswith("external/") else read java_path = r.Rlocation(rel_path) except Exception: java_path = None built_jar = "selenium/java/src/org/openqa/selenium/grid/selenium_server_deploy.jar" jar_path = r.Rlocation(built_jar) remote_env = os.environ.copy() if sys.platform == "linux": # There are issues with window size/position when running Firefox # under Wayland, so we use XWayland instead. remote_env["MOZ_ENABLE_WAYLAND"] = "0" server = Server(env=remote_env, startup_timeout=60) ... (clipped 3 lines)
Ticket Compliance
🟡	🎫 #1234
🟡	🎫 #5678
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
🔴	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: None path handling: The remote server fixture may raise a runtime exception because java_path can be None (set in the exception handler) but is still passed to Path(java_path).exists() without validation. Referred Code except Exception: java_path = None built_jar = "selenium/java/src/org/openqa/selenium/grid/selenium_server_deploy.jar" jar_path = r.Rlocation(built_jar) remote_env = os.environ.copy() if sys.platform == "linux": # There are issues with window size/position when running Firefox # under Wayland, so we use XWayland instead. remote_env["MOZ_ENABLE_WAYLAND"] = "0" server = Server(env=remote_env, startup_timeout=60) if Path(java_path).exists(): server.java_path = java_path if Path(jar_path).exists(): server.path = jar_path Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Unstructured stdout logs: The server startup/shutdown flow writes operational messages to stdout via print(...), which may be unsuitable for environments requiring structured logging and centralized log redaction controls. Referred Code try: with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as sock: sock.connect((host, self.port)) raise ConnectionError(f"Selenium server is already running, or something else is using port {self.port}") except ConnectionRefusedError: print("Starting Selenium server...") self.process = subprocess.Popen(command, env=self.env) print(f"Selenium server running as process: {self.process.pid}") if not self._wait_for_server(timeout=self.startup_timeout): raise TimeoutError(f"Timed out waiting for Selenium server at {self.status_url}") print("Selenium server is ready") Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Address performance of parallel test execution The current approach of starting a new Grid server for each parallel test worker degrades performance. Implement a sharing mechanism, like a file-based lock, to ensure only one server instance is created and used by all workers. Examples: py/conftest.py [436-471] def server(request): is_remote = request.config.getoption("remote") if not is_remote: yield None return r = Runfiles.Create() java_location_txt = r.Rlocation("_main/" + os.environ.get("SE_BAZEL_JAVA_LOCATION")) try: ... (clipped 26 lines) Solution Walkthrough: Before: # in conftest.py @pytest.fixture(autouse=True, scope="session") def server(request): is_remote = request.config.getoption("remote") if not is_remote: yield None return # ... setup ... # This block is executed by each parallel worker server = Server(...) server.port = free_port() # Each worker gets a new port server.start() # Each worker starts a new Grid server yield server server.stop() After: # in conftest.py (conceptual, using pytest-xdist) @pytest.fixture(autouse=True, scope="session") def server(request, worker_id): # worker_id is provided by pytest-xdist if not request.config.getoption("remote"): yield None return # Use a file for coordination across processes info_file = Path("server_info.txt") if worker_id == "gw0": # The main worker # This worker starts the server server = Server(...) server.start() info_file.write_text(server.status_url) # Share server URL yield server server.stop() info_file.unlink() else: # Other workers # Wait for the main worker to start the server while not info_file.exists(): time.sleep(0.1) status_url = info_file.read_text() # Yield a representation of the shared server yield SharedServer(status_url) Suggestion importance[1-10]: 9 __ Why: This suggestion correctly identifies a major performance issue for local development, which the PR author also acknowledged, and proposes a valid architectural solution to fix it.	High
General	Use correct Bazel location macro In py/BUILD.bazel, replace $(rootpath :java-location) with $(location :java-location) to correctly pass the filesystem path of java-location.txt to the test environment. py/BUILD.bazel [535-537] env = { - "SE_BAZEL_JAVA_LOCATION": "$(rootpath :java-location)", + "SE_BAZEL_JAVA_LOCATION": "$(location :java-location)", }, Apply / Chat Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies that $(rootpath) is the wrong macro for getting a filesystem path at execution time and proposes using $(location), which is critical for the feature to work correctly.	High
	Improve error handling for Java path discovery Refactor the Java path discovery logic by first checking if the SE_BAZEL_JAVA_LOCATION environment variable is set and replacing the broad except Exception with more specific exception handling. py/conftest.py [442-451] r = Runfiles.Create() -java_location_txt = r.Rlocation("_main/" + os.environ.get("SE_BAZEL_JAVA_LOCATION")) -try: - with open(java_location_txt, encoding="utf-8") as handle: - read = handle.read().strip() - rel_path = read[len("external/") :] if read.startswith("external/") else read - java_path = r.Rlocation(rel_path) -except Exception: - java_path = None +java_location_file = os.environ.get("SE_BAZEL_JAVA_LOCATION") +java_path = None +if java_location_file: + try: + java_location_txt = r.Rlocation("_main/" + java_location_file) + with open(java_location_txt, encoding="utf-8") as handle: + read = handle.read().strip() + if read: + rel_path = read[len("external/") :] if read.startswith("external/") else read + java_path = r.Rlocation(rel_path) + except (IOError, TypeError) as e: + print(f"Could not get java path from runfiles, falling back to system PATH. Error: {e}", file=sys.stderr) + java_path = None Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly points out that a broad except Exception can hide errors and proposes more robust error handling, which improves code quality and debuggability.	Low
Possible issue	Re-initialize driver object for test isolation To ensure test isolation, unconditionally re-initialize the global selenium_driver object in the driver fixture for each test, removing the if selenium_driver is None: check. py/conftest.py [325-333] @pytest.fixture def driver(request, server): global selenium_driver driver_class = getattr(request, "param", "Chrome").lower() - if selenium_driver is None: - selenium_driver = Driver(driver_class, request) + selenium_driver = Driver(driver_class, request) if server: selenium_driver._server = server Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a significant test isolation issue where the global selenium_driver object is not re-initialized, leading to state leakage between tests, and provides a correct fix.	Medium
	Provide command executor to remote WebDriver In test_browser_specific_method, correctly initialize webdriver.Remote by passing the command_executor argument derived from the server fixture. py/test/selenium/webdriver/remote/remote_connection_tests.py [29-40] pytestmark = pytest.mark.xfail(reason="Tests not working as intended") def test_browser_specific_method(firefox_options, webserver, server, request): """Uses Firefox specific method.""" - with webdriver.Remote(options=firefox_options) as driver: - driver.get(f"{webserver}/simpleTest.html") + command_executor = server.status_url[: -len("/status")] + with webdriver.Remote(command_executor=command_executor, options=firefox_options) as driver: + driver.get(f"{webserver.where_is('simpleTest.html')}") screenshot = driver.execute("FULL_PAGE_SCREENSHOT")["value"] result = base64.b64decode(screenshot) kind = filetype.guess(result) assert kind is not None assert kind.mime == "image/png" Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a bug where the webdriver.Remote is missing the command_executor, causing the test to fail, and provides the correct fix using the server fixture.	Medium
Learned best practice	Cleanup process on startup failure If startup times out (or another exception occurs), terminate the spawned process in a try/finally to avoid leaving an orphaned Grid server running. py/selenium/webdriver/remote/server.py [208-213] print("Starting Selenium server...") self.process = subprocess.Popen(command, env=self.env) -print(f"Selenium server running as process: {self.process.pid}") -if not self._wait_for_server(timeout=self.startup_timeout): - raise TimeoutError(f"Timed out waiting for Selenium server at {self.status_url}") -print("Selenium server is ready") +try: + print(f"Selenium server running as process: {self.process.pid}") + if not self._wait_for_server(timeout=self.startup_timeout): + raise TimeoutError(f"Timed out waiting for Selenium server at {self.status_url}") + print("Selenium server is ready") +except Exception: + if self.process and self.process.poll() is None: + self.process.terminate() + self.process.wait() + self.process = None + raise Apply / Chat Suggestion importance[1-10]: 5 __ Why: Relevant best practice - Ensure external resources/processes are cleaned up in a finally block so failures cannot leak resources.	Low
Update

[cgoldberg]

LGTM besides the tiny change I suggested. Thanks.