-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
[py] Feat/16741 improve sm test #17393
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
base: trunk
Are you sure you want to change the base?
Changes from all commits
1f09bc1
8aa3a5a
4df7cbf
5148fea
8a8c3e2
ffd0320
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 | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -812,6 +812,51 @@ BROWSER_TESTS = { | |||||||||||
| ] | ||||||||||||
| ] | ||||||||||||
|
|
||||||||||||
| # Generate test-<browser>-sm targets (Selenium Manager integration tests) | ||||||||||||
| # These only run when SE_FORCE_BROWSER_DOWNLOAD=true is set and use | ||||||||||||
| # --pin_browsers=false to test SM download resolution. | ||||||||||||
|
Comment on lines
+816
to
+817
|
||||||||||||
| # These only run when SE_FORCE_BROWSER_DOWNLOAD=true is set and use | |
| # --pin_browsers=false to test SM download resolution. | |
| # These only run when SE_FORCE_BROWSER_DOWNLOAD=true is set and are | |
| # intended to be run with --pin_browsers=false (for example in CI) to | |
| # test SM download resolution. |
Copilot
AI
Apr 25, 2026
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.
These Selenium Manager integration tests rely on network access and on running with --pin_browsers=false, but the repo’s remote Bazel config forces --//common:pin_browsers and only filters out tests tagged skip-rbe. Without a skip-rbe tag here, running this suite under --config=rbe is likely to fail (and may attempt unwanted downloads). Add skip-rbe to the tags for these test-*-sm targets to prevent accidental RBE execution.
| "requires-network", | |
| "requires-network", | |
| "skip-rbe", |
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.
1. test-*-sm missing skip-rbe tag 📎 Requirement gap ☼ Reliability
The new Selenium Manager Bazel test suites are not tagged with skip-rbe, so they can execute under Bazel RBE even though they require network/browser downloads. This violates the requirement to ensure Selenium Manager-specific tests run on GitHub Actions but are skipped/disabled on RBE.
Agent Prompt
## Issue description
The new Selenium Manager Bazel test suites (`test-*-sm`) are eligible to run under RBE because they are missing the `skip-rbe` tag. These tests require network/browser downloads and should be restricted to GitHub Actions, not RBE.
## Issue Context
RBE runs tests with `--test_tag_filters=-skip-rbe` (so only targets explicitly tagged `skip-rbe` are excluded). The new suites currently have tags like `requires-network` but not `skip-rbe`.
## Fix Focus Areas
- py/BUILD.bazel[829-837]
- .bazelrc.remote[36-36]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -18,12 +18,14 @@ | |||||||||||||||||
| import os | ||||||||||||||||||
| import subprocess | ||||||||||||||||||
| import sys | ||||||||||||||||||
| from pathlib import Path | ||||||||||||||||||
| from unittest.mock import patch | ||||||||||||||||||
|
|
||||||||||||||||||
| import pytest | ||||||||||||||||||
|
|
||||||||||||||||||
| from selenium.common.exceptions import SessionNotCreatedException | ||||||||||||||||||
| from selenium.webdriver.chrome.service import Service | ||||||||||||||||||
| from selenium.webdriver.common.driver_finder import DriverFinder | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| @pytest.mark.no_driver_after_test | ||||||||||||||||||
|
|
@@ -142,6 +144,41 @@ def test_service_allows_reusing_stdout_for_logging(clean_driver, clean_options, | |||||||||||||||||
| browser2.quit() | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| def _is_within_cache(path: Path, cache_dir: Path) -> bool: | ||||||||||||||||||
| """Check if a path is within a given cache directory.""" | ||||||||||||||||||
| try: | ||||||||||||||||||
| path.relative_to(cache_dir) | ||||||||||||||||||
| return True | ||||||||||||||||||
| except ValueError: | ||||||||||||||||||
| return False | ||||||||||||||||||
|
Comment on lines
+149
to
+153
|
||||||||||||||||||
| try: | |
| path.relative_to(cache_dir) | |
| return True | |
| except ValueError: | |
| return False | |
| normalized_path = path.expanduser().resolve() | |
| normalized_cache_dir = cache_dir.expanduser().resolve() | |
| return normalized_path.is_relative_to(normalized_cache_dir) |
Copilot
AI
Apr 25, 2026
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.
SE_FORCE_BROWSER_DOWNLOAD is a boolean config key in Selenium Manager (parsed from env as "true"/"false"). The skip condition currently only checks for the variable being present, so SE_FORCE_BROWSER_DOWNLOAD=false would still run this test even though SM won't force downloads, making the cache assertions unreliable. Update the skipif condition to check the value (e.g., normalize and compare to "true").
| not os.environ.get("SE_FORCE_BROWSER_DOWNLOAD"), | |
| reason="Only runs when SE_FORCE_BROWSER_DOWNLOAD is set", | |
| os.environ.get("SE_FORCE_BROWSER_DOWNLOAD", "").strip().lower() != "true", | |
| reason='Only runs when SE_FORCE_BROWSER_DOWNLOAD is set to "true"', |
Copilot
AI
Apr 25, 2026
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.
The Selenium Manager cache path is configured via SE_CACHE_PATH (and defaults to ~/.cache/selenium). This test reads SE_CACHE, so if CI/users override the cache location via SE_CACHE_PATH, the _is_within_cache assertions can fail even when Selenium Manager behaves correctly. Use SE_CACHE_PATH here for consistency with the other service tests and Manager configuration.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -18,11 +18,13 @@ | |||||||||||||||||
| import os | ||||||||||||||||||
| import subprocess | ||||||||||||||||||
| import sys | ||||||||||||||||||
| from pathlib import Path | ||||||||||||||||||
| from unittest.mock import patch | ||||||||||||||||||
|
|
||||||||||||||||||
| import pytest | ||||||||||||||||||
|
|
||||||||||||||||||
| from selenium.common.exceptions import SessionNotCreatedException | ||||||||||||||||||
| from selenium.webdriver.common.driver_finder import DriverFinder | ||||||||||||||||||
| from selenium.webdriver.edge.service import Service | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
|
|
@@ -142,6 +144,41 @@ def test_service_allows_reusing_stdout_for_logging(clean_driver, clean_options, | |||||||||||||||||
| browser2.quit() | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| def _is_within_cache(path: Path, cache_dir: Path) -> bool: | ||||||||||||||||||
| """Check if a path is within a given cache directory.""" | ||||||||||||||||||
| try: | ||||||||||||||||||
| path.relative_to(cache_dir) | ||||||||||||||||||
| return True | ||||||||||||||||||
| except ValueError: | ||||||||||||||||||
| return False | ||||||||||||||||||
|
Comment on lines
+149
to
+153
|
||||||||||||||||||
| try: | |
| path.relative_to(cache_dir) | |
| return True | |
| except ValueError: | |
| return False | |
| normalized_path = path.expanduser().resolve() | |
| normalized_cache_dir = cache_dir.expanduser().resolve() | |
| return normalized_path.is_relative_to(normalized_cache_dir) |
Copilot
AI
Apr 25, 2026
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.
SE_FORCE_BROWSER_DOWNLOAD is parsed by Selenium Manager as a boolean ("true"/"false"). This skipif only checks whether the env var exists, so SE_FORCE_BROWSER_DOWNLOAD=false would still run the test even though SM won't force downloads. Update the condition to check the value (case-insensitive) instead of mere presence.
| not os.environ.get("SE_FORCE_BROWSER_DOWNLOAD"), | |
| os.environ.get("SE_FORCE_BROWSER_DOWNLOAD", "").lower() != "true", |
Copilot
AI
Apr 25, 2026
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.
On Windows, Selenium Manager installs Microsoft Edge via MSI into the system location rather than the SM cache (see rust/src/lib.rs where Edge-on-Windows is explicitly noted as using system path, not cache). This test unconditionally asserts browser_path is within the cache directory, which will fail on win32 even when SM is working as designed. Adjust the assertion to allow the system install path for Edge on Windows (or skip the browser-in-cache check on that platform) while still asserting the driver is in cache.
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -18,12 +18,14 @@ | |||||||||
| import os | ||||||||||
| import subprocess | ||||||||||
| import sys | ||||||||||
| from pathlib import Path | ||||||||||
| from unittest.mock import patch | ||||||||||
|
|
||||||||||
| import pytest | ||||||||||
|
|
||||||||||
| from selenium.common.exceptions import SessionNotCreatedException | ||||||||||
| from selenium.webdriver import Firefox | ||||||||||
| from selenium.webdriver.common.driver_finder import DriverFinder | ||||||||||
| from selenium.webdriver.firefox.options import Options | ||||||||||
| from selenium.webdriver.firefox.service import Service | ||||||||||
|
|
||||||||||
|
|
@@ -93,6 +95,41 @@ def test_service_allows_reusing_stdout_for_logging(clean_driver, clean_options, | |||||||||
| browser2.quit() | ||||||||||
|
|
||||||||||
|
|
||||||||||
| def _is_within_cache(path: Path, cache_dir: Path) -> bool: | ||||||||||
| """Check if a path is within a given cache directory.""" | ||||||||||
| try: | ||||||||||
| path.relative_to(cache_dir) | ||||||||||
| return True | ||||||||||
| except ValueError: | ||||||||||
| return False | ||||||||||
|
Comment on lines
+98
to
+104
|
||||||||||
|
|
||||||||||
|
|
||||||||||
| @pytest.mark.skipif( | ||||||||||
| not os.environ.get("SE_FORCE_BROWSER_DOWNLOAD"), | ||||||||||
| reason="Only runs when SE_FORCE_BROWSER_DOWNLOAD is set", | ||||||||||
|
Comment on lines
+108
to
+109
|
||||||||||
| not os.environ.get("SE_FORCE_BROWSER_DOWNLOAD"), | |
| reason="Only runs when SE_FORCE_BROWSER_DOWNLOAD is set", | |
| os.environ.get("SE_FORCE_BROWSER_DOWNLOAD", "").lower() != "true", | |
| reason="Only runs when SE_FORCE_BROWSER_DOWNLOAD is set to true", |
Copilot
AI
Apr 25, 2026
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.
The Selenium Manager cache path env var is SE_CACHE_PATH (and defaults to ~/.cache/selenium). This test uses SE_CACHE, which can cause false failures when the cache is overridden via SE_CACHE_PATH. Update this to read SE_CACHE_PATH for consistency with Selenium Manager configuration.
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.
2. Bazel filter skips sm suites
🐞 Bug≡ CorrectnessAgent Prompt
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools