[py] Feat/16741 improve sm test#17393
Conversation
selenium-ci
added
C-py
iampopovich
changed the title
There was a problem hiding this comment.
Pull request overview
Adds Python integration coverage for Selenium Manager’s browser/driver resolution by introducing dedicated Bazel test suites (Chrome/Edge/Firefox) and a new CI job to run them across Ubuntu/Windows/macOS.
Changes:
- Added new
test-<browser>-smBazel test suites (plus aggregatedtest-sm) that run targeted SM resolution checks. - Added new service-level tests for Chrome/Edge/Firefox that use
DriverFinderto assert resolved browser/driver paths are executable and located as expected. - Added a new
selenium-manager-testsGitHub Actions job in the Python CI workflow to run these suites cross-platform with--pin_browsers=false.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
py/test/selenium/webdriver/chrome/chrome_service_tests.py |
Adds an SM-focused resolution test using DriverFinder and cache-path assertions. |
py/test/selenium/webdriver/edge/edge_service_tests.py |
Adds an SM-focused resolution test using DriverFinder and cache-path assertions. |
py/test/selenium/webdriver/firefox/firefox_service_tests.py |
Adds an SM-focused resolution test using DriverFinder and cache-path assertions. |
py/BUILD.bazel |
Adds new Bazel test-*-sm targets and an aggregated test-sm suite. |
.github/workflows/ci-python.yml |
Adds a new CI job to run the new SM suites on ubuntu/windows/macos with --pin_browsers=false. |
| assert _is_within_cache(driver_path, cache_dir), f"Driver path outside cache: {driver_path}" | ||
| assert _is_within_cache(browser_path, cache_dir), f"Browser path outside cache: {browser_path}" |
There was a problem hiding this comment.
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.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| cache_dir = Path(os.environ.get("SE_CACHE_PATH", Path.home() / ".cache" / "selenium")) | ||
| service = Service() |
There was a problem hiding this comment.
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.
| cache_dir = Path(os.environ.get("SE_CACHE_PATH", Path.home() / ".cache" / "selenium")) | ||
| service = Service() |
There was a problem hiding this comment.
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.
| # These only run when SE_FORCE_BROWSER_DOWNLOAD=true is set and use | ||
| # --pin_browsers=false to test SM download resolution. |
There was a problem hiding this comment.
The comment says these suites "use --pin_browsers=false", but that flag is only applied in the GitHub Actions job and isn’t enforced by the Bazel targets themselves. Consider rewording to indicate the suites are intended to be run with --pin_browsers=false (e.g., in CI) to avoid confusing local runs.
| # 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. |
Review Summary by QodoAdd Selenium Manager integration tests for Python bindings
WalkthroughsDescription• Add Selenium Manager integration tests for Chrome, Edge, Firefox • Verify driver and browser resolution via DriverFinder into SM cache • Implement conditional tests using SE_FORCE_BROWSER_DOWNLOAD environment variable • Add CI workflow job for cross-platform SM test execution File Changes1. py/test/selenium/webdriver/chrome/chrome_service_tests.py
|
Code Review by Qodo
1. test-*-sm missing skip-rbe tag
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| env = { | ||
| "SE_FORCE_BROWSER_DOWNLOAD": "true", | ||
| "SE_SKIP_DRIVER_IN_PATH": "true", | ||
| }, | ||
| tags = [ | ||
| "manager", | ||
| "no-sandbox", | ||
| "requires-network", | ||
| ], |
There was a problem hiding this comment.
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
| bazel test \ | ||
| --keep_going \ | ||
| --build_tests_only \ | ||
| --flaky_test_attempts 3 \ | ||
| --local_test_jobs 1 \ | ||
| --pin_browsers=false \ | ||
| --test_size_filters medium \ | ||
| --test_tag_filters="manager,-ie,-safari" \ | ||
| //py:test-sm |
There was a problem hiding this comment.
2. Bazel filter skips sm suites 🐞 Bug ≡ Correctness
The new CI job filters by --test_tag_filters="manager,...", but py_test_suite generates native.test_suite(name="test-*-sm") targets without propagating tags, so these suite targets don’t have the manager tag and can be filtered out (preventing the underlying SM pytest targets from running). This can make the new CI job pass while executing none of the intended Selenium Manager tests.
Agent Prompt
### Issue description
The CI job runs `bazel test` with `--test_tag_filters=manager`, but the intermediate `test-%s-sm` targets are `native.test_suite` rules created by `py_test_suite` without inheriting tags. This can cause Bazel’s tag filtering to drop those suite targets, so the job may run none of the intended Selenium Manager tests.
### Issue Context
- The job already targets `//py:test-sm`, so the positive tag filter is redundant.
- `py_test_suite` forwards `tags` to the underlying `pytest_test(...)` rules, but not to the `native.test_suite(...)` wrapper.
### Fix Focus Areas
Choose one:
- Simplest: remove the positive tag/size filters in the CI job and rely on explicit target selection.
- More robust: update `py/private/suite.bzl` so `native.test_suite(...)` receives `tags` (and any other relevant attrs) when `py_test_suite` is called.
- .github/workflows/ci-python.yml[64-72]
- py/private/suite.bzl[18-50]
- py/BUILD.bazel[815-858]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| not os.environ.get("SE_FORCE_BROWSER_DOWNLOAD"), | ||
| reason="Only runs when SE_FORCE_BROWSER_DOWNLOAD is set", |
There was a problem hiding this comment.
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"', |
| try: | ||
| path.relative_to(cache_dir) | ||
| return True | ||
| except ValueError: | ||
| return False |
There was a problem hiding this comment.
Cache path checks will be more robust if you normalize paths first (e.g., handle SE_CACHE_PATH values containing ~ and avoid ../symlink issues). Consider expanduser()/resolve() before doing the “within cache” assertion. Also, since Python >=3.10 is required, Path.is_relative_to() can replace this custom _is_within_cache helper and avoid the try/except.
| 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) |
|
|
||
|
|
||
| @pytest.mark.skipif( | ||
| not os.environ.get("SE_FORCE_BROWSER_DOWNLOAD"), |
There was a problem hiding this comment.
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", |
| try: | ||
| path.relative_to(cache_dir) | ||
| return True | ||
| except ValueError: | ||
| return False |
There was a problem hiding this comment.
Consider normalizing paths (expanduser()/resolve()) before checking they’re under the cache directory; this avoids failures when SE_CACHE_PATH uses ~ or when SM returns paths containing ../symlinks. Since Python >=3.10 is required, Path.is_relative_to() can replace _is_within_cache and remove the try/except.
| 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) |
| not os.environ.get("SE_FORCE_BROWSER_DOWNLOAD"), | ||
| reason="Only runs when SE_FORCE_BROWSER_DOWNLOAD is set", |
There was a problem hiding this comment.
SE_FORCE_BROWSER_DOWNLOAD is a boolean Selenium Manager config env var (parsed as "true"/"false"). This skipif only checks for presence, so the test would still run with SE_FORCE_BROWSER_DOWNLOAD=false even though SM won’t force browser downloads, making the cache assertions unreliable. Update the condition to validate the value instead of just existence.
| 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", |
| 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 |
There was a problem hiding this comment.
To make the cache containment assertion more robust, normalize paths first (e.g., expanduser()/resolve()), especially if SE_CACHE_PATH contains ~ or SM returns non-normalized paths. Since Python >=3.10 is required, Path.is_relative_to() can replace _is_within_cache and avoid the try/except.
| tags = [ | ||
| "manager", | ||
| "no-sandbox", | ||
| "requires-network", |
There was a problem hiding this comment.
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", |
🔗 Related Issues
python version of #16741
💥 What does this PR do?
This pull request introduces Selenium Manager integration tests for Chrome, Edge, and Firefox, ensuring that both the browser and driver are resolved and downloaded into the Selenium Manager cache as expected. The changes add new test suites and corresponding workflow jobs to run these tests across multiple operating systems. Additionally, new tests are implemented to verify the correct behavior of Selenium Manager's driver and browser resolution logic.
Selenium Manager Integration Testing:
test-<browser>-sm) for Chrome, Edge, and Firefox inpy/BUILD.bazel. These tests run only whenSE_FORCE_BROWSER_DOWNLOADis set, and they verify browser and driver resolution via Selenium Manager. A combinedtest-smsuite aggregates these tests.chrome_service_tests.py,edge_service_tests.py, andfirefox_service_tests.pyto verify that both the driver and browser are resolved to executable files within the Selenium Manager cache when usingDriverFinder. These tests are conditionally run based on theSE_FORCE_BROWSER_DOWNLOADenvironment variable. [1] [2] [3]Continuous Integration Workflow Updates:
selenium-manager-testsjob to.github/workflows/ci-python.ymlto run the Selenium Manager integration tests on Ubuntu, Windows, and macOS, leveraging the new test suites and ensuring cross-platform validation.Test File Imports and Setup:
PathandDriverFinder, supporting the new integration tests. [1] [2] [3]🔧 Implementation Notes
🤖 AI assistance
💡 Additional Considerations
🔄 Types of changes