Skip to content

[py] add Selenium Manager integration tests#17645

Merged
titusfortner merged 2 commits into
trunkfrom
sm_test_py
Jun 6, 2026
Merged

[py] add Selenium Manager integration tests#17645
titusfortner merged 2 commits into
trunkfrom
sm_test_py

Conversation

@titusfortner
Copy link
Copy Markdown
Member

🔗 Related Issues

Implements #16741 for Python
Part of #17539

💥 What does this PR do?

  • Adds integration test coverage of Selenium Manager via Driver Finder class

🔧 Implementation Notes

  • Because it goes through Driver Finder, the tests pass with and without pin_browsers being set by taking different paths (or skipping the cache location validation)
  • Selected by tag (--test_tag_filters=se-manager) rather than per-browser target names, so all applicable browsers run in a single invocation per OS (3 jobs, not browser×OS).

🤖 AI assistance

  • No substantial AI assistance used
  • AI assisted (complete below)
    • Tool(s): Claude Code
    • What was generated: the test, BUILD targets, and CI job
    • I reviewed all AI output and can explain the change

💡 Additional Considerations

🔄 Types of changes

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

@qodo-code-review
Copy link
Copy Markdown
Contributor

Review Summary by Qodo

Add Selenium Manager integration tests for Python

🧪 Tests ✨ Enhancement

Grey Divider

Walkthroughs

Description
• Adds comprehensive Selenium Manager integration tests via DriverFinder
• Tests cover driver and browser path resolution with caching
• Supports both pinned and unpinned browser configurations
• Integrates tests into CI pipeline with OS-specific matrix jobs
Diagram
flowchart LR
  A["New Test File"] -->|"Tests DriverFinder"| B["Driver Path Resolution"]
  A -->|"Tests DriverFinder"| C["Browser Path Resolution"]
  B -->|"Validates"| D["Executable Paths"]
  C -->|"Validates"| D
  B -->|"Tests Caching"| E["SE_CACHE_PATH"]
  C -->|"Tests Caching"| E
  F["CI Configuration"] -->|"Runs Tests"| G["Multi-OS Jobs"]
  G -->|"Uses Tag Filter"| H["se-manager"]

Loading

Grey Divider

File Changes

1. py/test/selenium/webdriver/common/driver_finder_tests.py 🧪 Tests +103/-0

Selenium Manager integration tests via DriverFinder

• New test file with 5 integration tests for Selenium Manager via DriverFinder
• Tests driver path resolution, browser path resolution, and cache behavior
• Includes conditional skips for pinned runs, Safari, and platform-specific cases
• Uses pytest fixtures for browser_name, pinned status, and driver_finder setup

py/test/selenium/webdriver/common/driver_finder_tests.py


2. .github/workflows/ci-python.yml ⚙️ Configuration changes +16/-0

Add Selenium Manager CI job matrix

• Adds new selenium-manager-tests job to CI pipeline
• Runs tests across three OS platforms (ubuntu, windows, macos)
• Uses --test_tag_filters=se-manager to select Selenium Manager tests
• Configured with --pin_browsers=false to test Selenium Manager functionality

.github/workflows/ci-python.yml


3. py/BUILD.bazel ⚙️ Configuration changes +29/-5

Configure Bazel targets for driver finder tests

• Defines DRIVER_FINDER_TESTS constant containing the new test file
• Excludes driver_finder tests from common test suites to prevent duplication
• Creates new test--driver-finder targets for each browser
• Tags new targets with se-manager for selective test execution

py/BUILD.bazel


Grey Divider

Qodo Logo

@selenium-ci selenium-ci added C-py Python Bindings B-build Includes scripting, bazel and CI integrations labels Jun 5, 2026
@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented Jun 5, 2026

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0)

Grey Divider


Remediation recommended

1. Driver count not validated 🐞 Bug ≡ Correctness
Description
The browser_name fixture claims it requires a single --driver/--browser but only checks for presence
and then silently uses the first entry. Because the CLI option supports multiple values, running
with more than one driver will execute the suite against an arbitrary first browser and ignore the
rest.
Code

py/test/selenium/webdriver/common/driver_finder_tests.py[R30-34]

+def browser_name(request):
+    drivers = request.config.option.drivers
+    if not drivers:
+        pytest.skip("Selenium Manager tests require a single --driver/--browser")
+    return drivers[0].lower()
Evidence
The test expects exactly one driver, but the global pytest options explicitly allow multiple via
action=append, so the current fixture can silently pick the wrong browser.

py/test/selenium/webdriver/common/driver_finder_tests.py[29-35]
py/conftest.py[118-134]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`browser_name` skips only when no `--driver/--browser` is provided, but the option supports multiple values. The fixture then uses `drivers[0]`, which can silently run the suite against the wrong browser.

### Issue Context
In `py/conftest.py`, `--driver`/`--browser` uses `action="append"`, so users/CI can pass multiple drivers.

### Fix Focus Areas
- py/test/selenium/webdriver/common/driver_finder_tests.py[29-35]

### Suggested change
Update the fixture to require exactly one driver:
- `if not drivers or len(drivers) != 1: pytest.skip(...)` (or `pytest.fail(...)` if you want misconfiguration to hard-fail)
- Include the received value(s) in the message for easier debugging.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Quoted driver path breaks ✓ Resolved 🐞 Bug ☼ Reliability
Description
The driver_finder fixture strips quotes from --browser-binary but does not strip quotes from
--driver-binary before passing it into the Service. If the driver path is single-quoted (a pattern
already handled elsewhere in this repo), DriverFinder will treat it as a literal path including
quotes and fail file validation.
Code

py/test/selenium/webdriver/common/driver_finder_tests.py[R47-52]

+    if request.config.option.binary:
+        options.binary_location = _resolve_bazel_path(request.config.option.binary).strip("'")
+
+    executable = request.config.option.executable
+    service = module.service.Service(executable_path=_resolve_bazel_path(executable) if executable else None)
+
Evidence
DriverFinder explicitly treats any non-empty Service.path as an override and requires it to be a
real file; leaving quotes on the path will cause Path(...).is_file() to fail. The repo already
strips quotes from executable paths elsewhere, indicating quoted paths are expected in some
configurations.

py/test/selenium/webdriver/common/driver_finder_tests.py[46-53]
py/conftest.py[225-240]
py/selenium/webdriver/common/service.py[73-94]
py/selenium/webdriver/common/driver_finder.py[56-75]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`driver_finder` normalizes `--browser-binary` by stripping surrounding single quotes, but it passes `--driver-binary` through without stripping. When `Service.path` contains quotes, `DriverFinder` validates it with `Path(path).is_file()` and will fail.

### Issue Context
The existing test harness already anticipates quoted executable paths and strips them in other code paths.

### Fix Focus Areas
- py/test/selenium/webdriver/common/driver_finder_tests.py[46-53]

### Suggested change
Normalize the executable path the same way as the browser binary:
```py
executable = request.config.option.executable
exe_path = _resolve_bazel_path(executable).strip("'") if executable else None
service = module.service.Service(executable_path=exe_path)
```
(Optionally, consider a small helper to avoid duplicating quote-stripping logic.)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Copy link
Copy Markdown
Member

@AutomatedTester AutomatedTester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the one comment which shouldn't block, just want consistency.

Comment thread py/test/selenium/webdriver/common/driver_finder_tests.py
@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented Jun 6, 2026

Code review by qodo was updated up to the latest commit b28e3cf

@titusfortner titusfortner merged commit eb0fc64 into trunk Jun 6, 2026
44 checks passed
@titusfortner titusfortner deleted the sm_test_py branch June 6, 2026 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-build Includes scripting, bazel and CI integrations C-py Python Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants