Skip to content

[rust][selenium-manager]: do not ignore browser path when version is specified#17659

Open
Delta456 wants to merge 9 commits into
SeleniumHQ:trunkfrom
Delta456:selenium_manager
Open

[rust][selenium-manager]: do not ignore browser path when version is specified#17659
Delta456 wants to merge 9 commits into
SeleniumHQ:trunkfrom
Delta456:selenium_manager

Conversation

@Delta456

@Delta456 Delta456 commented Jun 9, 2026

Copy link
Copy Markdown
Member

PR Summary by Qodo

Rust Selenium Manager: error on browser-path/version mismatch with ignore escape hatch
🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

Grey Divider

Walkthroughs

User Description

🔗 Related Issues

Fixes #17540

💥 What does this PR do?

SM silently discarded --browser-path and downloaded the requested version when versions didn't match. No error, no warning.

This PR fixes that.

🔧 Implementation Notes

🤖 AI assistance

  • No substantial AI assistance used
  • AI assisted (complete below)
    • Tool(s): Claude Code
    • What was generated:
    • 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!)
AI Description
• Fail fast when --browser-path version mismatches a specific --browser-version.
• Add --browser-version ignore to bypass checks and use the detected version.
• Add Unix-only integration tests for mismatch error and ignore-bypass warning.
Diagram
graph TD
  A["selenium-manager CLI"] --> B["Parse args"] --> C["Discover browser version"] --> D{"browser-version == ignore?"}
  D -->|yes| E["Warn if browser-path set"] --> F["Use detected version"]
  D -->|no| G{"Specific version requested\nAND differs?"}
  G -->|yes, browser-path set| H["Error: mismatch + guidance"]
  G -->|yes, no browser-path| I["Download requested browser"]
  G -->|no| J["Proceed with detected version"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Add explicit flag (e.g., --ignore-browser-version-check)
  • ➕ Avoids overloading --browser-version with a sentinel string
  • ➕ Clearer UX and easier validation/auto-completion
  • ➖ Adds a new public CLI flag to maintain and document
  • ➖ Requires additional parsing and compatibility considerations
2. Keep current behavior but emit warning and continue downloading
  • ➕ Maximizes convenience for users who just want the requested version
  • ➕ Avoids new failure mode in existing automation
  • ➖ Still surprising: silently disregards an explicit --browser-path intent
  • ➖ Harder to debug; may run a different browser than expected

Recommendation: The PR’s approach is the safer default: treat --browser-path as authoritative and error on mismatches to prevent accidental browser swaps. If UX concerns arise, consider a dedicated --ignore-browser-version-check flag later; for now, the sentinel value --browser-version ignore provides an explicit opt-out without expanding the flag surface.

Grey Divider

File Changes

Bug fix (1)
lib.rs Error on browser-path/version mismatch; add ignore bypass +30/-1

Error on browser-path/version mismatch; add ignore bypass

• Captures the original browser path and changes version reconciliation so a version mismatch errors when --browser-path is provided (instead of silently downloading). Introduces a new 'ignore' sentinel for --browser-version to bypass the check, emit a warning, and proceed using the detected version.

rust/src/lib.rs


Tests (1)
browser_tests.rs Add Unix-only tests for mismatch error and ignore behavior +78/-0

Add Unix-only tests for mismatch error and ignore behavior

• Adds a helper that creates an executable fake Chrome script returning a controllable version string on Unix. Introduces tests asserting (1) mismatch with --browser-path returns DATAERR and mentions detected/requested versions and 'ignore', and (2) '--browser-version ignore' logs a bypass warning and uses the detected version.

rust/tests/browser_tests.rs


Grey Divider

Qodo Logo

@qodo-code-review

qodo-code-review Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (3) 📎 Requirement gaps (2) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 14 rules

Grey Divider


Action required

1. --browser-path major mismatch allowed 📎 Requirement gap ≡ Correctness
Description
The new mismatch error path in discover_local_browser() only triggers when
is_browser_version_specific() is true (i.e., the requested --browser-version contains a .), so
a major-only version like 114/105 can still mismatch the detected version at an explicit
--browser-path without failing fast. In that case, later major-version handling can set
download_browser=true and effectively ignore the user-provided path, violating the requirement to
clearly fail when --browser-path and --browser-version conflict.
Code

rust/src/lib.rs[R538-540]

+                    } else if self.is_browser_version_specific()
                        && !self.get_browser_version().eq(&discovered_version)
                    {
Evidence
Compliance ID 1 requires Selenium Manager to fail clearly when --browser-path and
--browser-version conflict, but the PR’s new mismatch error path is gated by
is_browser_version_specific(), which is defined as version.contains("."), so major-only version
requests never enter the fail-fast mismatch logic even when --browser-path is set. The subsequent
major-version reconciliation/mismatch branch can still set download_browser = true and does not
consider original_browser_path, enabling the tool to silently disregard an explicitly provided
browser binary and proceed with download logic.

Selenium Manager must not silently ignore --browser-path when --browser-version mismatches
rust/src/lib.rs[528-553]
rust/src/lib.rs[855-867]
rust/src/lib.rs[508-610]
rust/src/lib.rs[855-861]

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

## Issue description
When `--browser-path` is provided together with a major-only `--browser-version` (e.g., `114`/`105`), a mismatch between the detected browser version at that path and the requested major version does not trigger the new fail-fast mismatch error because it is gated by `is_browser_version_specific()` (versions containing `.`). This can allow later major-version mismatch handling to set `download_browser = true` and effectively ignore the explicit browser path, contrary to the requirement to clearly fail on `--browser-path`/`--browser-version` conflicts.

## Issue Context
Compliance ID 1 requires a clear failure when `--browser-path` and `--browser-version` conflict.
The current mismatch error path only runs when `is_browser_version_specific()` is true (implemented as `version.contains(".")`), so major-only version requests bypass it.
Major-version mismatch handling happens later during major comparisons/reconciliation and can still set `download_browser = true` without checking whether `original_browser_path` / an explicit `--browser-path` was provided.

## Fix Focus Areas
- rust/src/lib.rs[508-610]
- rust/src/lib.rs[855-867]

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


2. browser_version_ignore_no_detectable_browser_test not executed 📘 Rule violation ☼ Reliability
Description
The newly added browser_version_ignore_no_detectable_browser_test() is missing a #[test]
attribute, so Rust’s test harness will not discover or execute it and it won’t protect the new
--browser-version ignore behavior (including the undetectable browser-path error path) from
regressions. This violates the requirement to add/update tests for behavior-changing fixes and
leaves the behavior uncovered by CI.
Code

rust/tests/browser_tests.rs[R186-215]

+#[cfg(unix)]
+fn browser_version_ignore_no_detectable_browser_test() {
+    let silent_browser = create_silent_browser();
+    let mut cmd = get_selenium_manager();
+    let stdout = cmd
+        .args([
+            "--browser",
+            "chrome",
+            "--browser-path",
+            silent_browser.to_str().unwrap(),
+            "--browser-version",
+            "ignore",
+            "--debug",
+        ])
+        .assert()
+        .get_output()
+        .stdout
+        .clone();
+    let stdout_str = std::str::from_utf8(&stdout).unwrap();
+    assert!(
+        !stdout_str.contains("not available for download"),
+        "Should not show misleading download-version error; got: {}",
+        stdout_str
+    );
+    assert!(
+        stdout_str.contains("Could not detect") || stdout_str.contains("No local"),
+        "Should explain that no detectable local browser was found; got: {}",
+        stdout_str
+    );
+}
Evidence
PR Compliance ID 7 requires relevant tests for implemented behavior changes, but the newly added
browser_version_ignore_no_detectable_browser_test() is defined without a #[test] annotation (it
is only gated by #[cfg(unix)]), unlike surrounding tests in the same file which include #[test].
Because Rust’s test harness only discovers functions marked with #[test], this function will not
run, so it currently provides no automated coverage of the new --browser-version ignore behavior
when no detectable local browser version is found.

AGENTS.md: Add or Update Tests for Implemented Fixes/Features (Prefer Small Unit Tests; Avoid Mocks)
rust/tests/browser_tests.rs[186-215]
rust/tests/browser_tests.rs[176-216]

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_version_ignore_no_detectable_browser_test()` is intended as a regression test for the new `--browser-version ignore` behavior when the browser version cannot be detected, but it is missing the `#[test]` attribute so it never executes under Rust’s test harness and provides no CI coverage. As written, it also does not assert success/failure via `.success()` / `.failure()` / `.code(...)`, so even if annotated later it may not deterministically validate behavior.

## Issue Context
This PR introduces a new escape-hatch behavior (`--browser-version ignore`) and an explicit error path when no detectable local browser version is found. Other tests in the same file are properly annotated with `#[test]`, but this one is only gated by `#[cfg(unix)]`, so the intended coverage is currently not actually exercised.

## Fix Focus Areas
- rust/tests/browser_tests.rs[186-216]

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


3. Ignore breaks browser download ✓ Resolved 🐞 Bug ≡ Correctness
Description
If --browser-version ignore is used and no local browser is discovered, setup() proceeds to
download_browser() with original_browser_version="ignore", where it is treated as a non-empty
non-stable version and its “major” parses to 0, triggering the minimum-version download guard and
failing with a misleading error. This makes the newly introduced ignore escape hatch unreliable in
environments where local discovery fails or no browser is installed.
Code

rust/src/lib.rs[R528-538]

+                    if self.is_browser_version_ignore() {
+                        if !original_browser_path.is_empty() {
+                            self.get_logger().warn(format!(
+                                "Browser version check bypassed for {} at {}; using detected version {}",
+                                self.get_browser_name(),
+                                original_browser_path,
+                                discovered_version
+                            ));
+                        }
+                        self.set_browser_version(discovered_version);
+                    } else if self.is_browser_version_specific()
Evidence
The PR adds an ignore special-case only in discover_local_browser(); when discovery fails, the
unchanged download path still consumes the original browser_version string, and
download_browser() enforces a numeric minimum using an integer parse of the major version, which
will be 0 for non-numeric values like ignore.

rust/src/lib.rs[508-624]
rust/src/lib.rs[238-266]
rust/src/lib.rs[1400-1409]
rust/src/lib.rs[1971-1976]

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-version ignore` is introduced as a special value, but it is only handled in `discover_local_browser()` when a local browser version is successfully detected. If local discovery returns `None`, the code can still attempt a browser download with `original_browser_version == "ignore"`, which then trips version parsing/min-version download logic and fails with an incorrect/misleading error.

## Issue Context
- `setup()` passes `original_browser_version` (from config) into `download_browser_if_necessary()`.
- `download_browser()` uses `get_major_browser_version()` and parses it as an integer to enforce a minimum browser version for downloads; for `"ignore"` this effectively becomes `0`.

## Fix Focus Areas
- rust/src/lib.rs[508-624]
- rust/src/lib.rs[238-266]
- rust/src/lib.rs[1400-1409]

### Suggested implementation direction
Choose one (but ensure behavior is explicit and user-friendly):
1. **Treat `ignore` as “unset” for download purposes**: when `is_browser_version_ignore()` is true, behave like `browser_version` is empty/stable in download selection (e.g., request latest) OR clear `original_browser_version` before calling `download_browser_if_necessary()`.
2. **Validate and error early**: if `browser-version==ignore` and there is no explicit browser path (and/or no local browser can be discovered), return a clear error explaining that `ignore` is only meaningful when a local browser is available (especially when paired with `--browser-path`).

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



Remediation recommended

4. Mismatch error lacks override hint 📎 Requirement gap ≡ Correctness ⭐ New
Description
The new mismatch error for --browser-path + --browser-version does not mention an explicit
escape hatch (e.g., --browser-version ignore) to proceed when the mismatch is intentional. This
can block users without clear guidance and does not fully meet the requirement to provide an
override mechanism with clear messaging.
Code

rust/src/lib.rs[R530-538]

+                        if !original_browser_path.is_empty() {
+                            return Err(anyhow!(format!(
+                                "The browser at {} has version {} but {} {} was requested; \
+                                 remove --browser-path to allow a browser download",
+                                original_browser_path,
+                                discovered_version,
+                                self.get_browser_name(),
+                                self.get_browser_version(),
+                            )));
Evidence
PR Compliance ID 389277 requires that a browser-path/version mismatch be surfaced with clear
guidance and an explicit override mechanism. The newly added error message reports the mismatch but
only advises removing --browser-path, without mentioning any explicit override option users can
apply.

Selenium Manager: do not silently ignore --browser-path when --browser-version is specified and mismatches
rust/src/lib.rs[530-538]

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

## Issue description
When `--browser-path` and a specific `--browser-version` mismatch, Selenium Manager now errors, but the error text does not mention any explicit escape hatch users can choose when the mismatch is intentional.

## Issue Context
Compliance requires surfacing the conflict *and* providing an explicit override mechanism with clear logging/guidance.

## Fix Focus Areas
- rust/src/lib.rs[530-538]

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


5. No cross-binding behavior comparison 📘 Rule violation ≡ Correctness ⭐ New
Description
This PR changes Selenium Manager’s user-visible behavior (now erroring on
--browser-path/--browser-version mismatches), which can affect multiple language bindings that
invoke Selenium Manager. There is no in-repo documentation/comment near the change indicating
cross-binding comparison or intentional divergence.
Code

rust/src/lib.rs[R517-520]

+            let original_browser_path = self.get_browser_path().to_string();
            match self.discover_browser_version()? {
                Some(discovered_version) => {
                    if !self.is_safari() {
Evidence
PR Compliance ID 389265 requires cross-binding comparison (or explicit documentation of intended
divergence) when changing user-visible behavior. The diff introduces a new mismatch error path in
Selenium Manager, and other bindings construct Selenium Manager invocations with --browser-version
and --browser-path, so the change can affect them without any documented comparison.

Rule 389265: Compare cross-language bindings when changing user-visible behavior
rust/src/lib.rs[517-540]
java/src/org/openqa/selenium/remote/service/DriverFinder.java[133-150]
py/selenium/webdriver/common/driver_finder.py[81-92]
dotnet/src/webdriver/Manager/SeleniumManager.cs[212-223]

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

## Issue description
A user-visible Selenium Manager behavior change was introduced, but the PR does not document having compared the behavior/expectations across other Selenium language bindings.

## Issue Context
Multiple bindings pass `--browser-path` and `--browser-version` to Selenium Manager; behavior changes can cause inconsistent outcomes across languages unless explicitly validated and documented.

## Fix Focus Areas
- rust/src/lib.rs[517-540]

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


6. Misleading mismatch guidance 🐞 Bug ≡ Correctness ⭐ New
Description
The new mismatch error in discover_local_browser() always tells users to remove --browser-path to
allow a browser download, but Selenium Manager explicitly skips browser downloads for some browser
types (e.g., Safari/WebView2), making that remediation impossible and misleading.
Code

rust/src/lib.rs[R530-538]

+                        if !original_browser_path.is_empty() {
+                            return Err(anyhow!(format!(
+                                "The browser at {} has version {} but {} {} was requested; \
+                                 remove --browser-path to allow a browser download",
+                                original_browser_path,
+                                discovered_version,
+                                self.get_browser_name(),
+                                self.get_browser_version(),
+                            )));
Evidence
The mismatch error unconditionally suggests removing --browser-path to allow a download, but the
download path is explicitly disabled for Safari and WebView2 (among others), so that recommendation
cannot be followed for those browsers.

rust/src/lib.rs[507-541]
rust/src/lib.rs[614-625]

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

### Issue description
`discover_local_browser()` returns an error on browser-path/version mismatch that always includes the guidance “remove --browser-path to allow a browser download”. This guidance is incorrect for browser types where Selenium Manager never downloads a browser (e.g., Safari and WebView2), so the message sends users toward an action that cannot succeed.

### Issue Context
Browser downloading is explicitly skipped for certain browsers in `download_browser_if_necessary()`, but the new mismatch error is emitted earlier and unconditionally includes download-related advice.

### Fix Focus Areas
- rust/src/lib.rs[530-538]
- rust/src/lib.rs[614-625]

### Suggested fix
Adjust the mismatch error message to be conditional:
- If browser downloads are supported for the current manager, keep guidance like “remove --browser-path to allow Selenium Manager to download the requested version”.
- If browser downloads are not supported (`is_safari()` / `is_webview2()` / etc.), change guidance to something actionable (e.g., “fix --browser-path or requested --browser-version so they match” and/or “remove --browser-version to use the detected local version”).

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


View more (6)
7. Ignore skips WebView2 path ✓ Resolved 🐞 Bug ≡ Correctness
Description
When --browser-version ignore is used, discover_local_browser() sets the detected version and
bypasses the existing WebView2 directory-to-msedge(.exe) path rewrite. This can leave
browser_path pointing at a directory instead of the executable for WebView2 on Windows.
Code

rust/src/lib.rs[R528-538]

+                    if self.is_browser_version_ignore() {
+                        if !original_browser_path.is_empty() {
+                            self.get_logger().warn(format!(
+                                "Browser version check bypassed for {} at {}; using detected version {}",
+                                self.get_browser_name(),
+                                original_browser_path,
+                                discovered_version
+                            ));
+                        }
+                        self.set_browser_version(discovered_version);
+                    } else if self.is_browser_version_specific()
Evidence
WebView2’s configured discovery path is a directory, and the existing normalization to the msedge
binary only occurs in the non-ignore branch; the new ignore branch bypasses it.

rust/src/edge.rs[116-190]
rust/src/lib.rs[528-610]

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

## Issue description
The new `is_browser_version_ignore()` early branch sets `browser_version` and does not execute the later WebView2 path normalization block that converts a directory install path into the versioned `msedge` executable path.

## Issue Context
- For WebView2, `get_browser_path_map()` points to a directory (`...\\EdgeWebView\\Application`).
- Existing code rewrites directory paths into `...\\{version}\\msedge(.exe)` after version resolution.
- The new `ignore` branch short-circuits before that rewrite.

## Fix Focus Areas
- rust/src/lib.rs[528-610]
- rust/src/edge.rs[116-190]

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


8. Ignore test missing exit-check ✓ Resolved 🐞 Bug ☼ Reliability
Description
browser_version_ignore_no_detectable_browser_test() executes Selenium Manager and inspects output
but never asserts the command fails with the expected exit code, so it can pass even if the CLI
unexpectedly succeeds or exits with the wrong status. This weakens regression coverage for the
newly-enabled #[test] path.
Code

rust/tests/browser_tests.rs[R186-205]

+#[test]
+#[cfg(unix)]
+fn browser_version_ignore_no_detectable_browser_test() {
+    let silent_browser = create_silent_browser();
+    let mut cmd = get_selenium_manager();
+    let stdout = cmd
+        .args([
+            "--browser",
+            "chrome",
+            "--browser-path",
+            silent_browser.to_str().unwrap(),
+            "--browser-version",
+            "ignore",
+            "--debug",
+        ])
+        .assert()
+        .get_output()
+        .stdout
+        .clone();
+    let stdout_str = std::str::from_utf8(&stdout).unwrap();
Evidence
The test currently captures stdout from .assert().get_output() without asserting success/failure
or exit code, while the runtime code path explicitly returns an error when `--browser-version
ignore is used but no version can be detected, and main.rs exits with DATAERR` on such errors in
normal (non-offline) execution.

rust/tests/browser_tests.rs[186-216]
rust/src/lib.rs[612-636]
rust/src/main.rs[314-360]

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_version_ignore_no_detectable_browser_test()` runs the CLI and reads its captured stdout, but it never asserts the expected process outcome (failure + exit code). Since the product code returns an error when `--browser-version ignore` is used and no detectable local browser version is found, the test should explicitly assert `.failure().code(DATAERR)` to ensure the intended behavior is enforced.

### Issue Context
In `discover_local_browser()` the `ignore` mode returns `Err(anyhow!(...))` when version detection fails, and `main.rs` maps that to a `DATAERR` exit code (non-offline path). The test currently only checks for specific substrings in stdout.

### Fix Focus Areas
- rust/tests/browser_tests.rs[186-216]
- rust/src/lib.rs[612-636]
- rust/src/main.rs[314-360]

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


9. Mismatch check too weak 🐞 Bug ≡ Correctness
Description
browser_path_version_mismatch_test only asserts the output contains the two version strings plus
"ignore", which doesn’t prove the new mismatch guidance text was emitted. This test can pass even if
only the generic "Detected browser" / "Required browser" debug lines are present and the actionable
mismatch message regresses.
Code

rust/tests/browser_tests.rs[R196-207]

+    // The mismatch is always reported, either as ERROR (no cached driver) or WARN (fallback used)
+    assert!(
+        stdout_str.contains("131.0.6778.264"),
+        "Should mention detected version"
+    );
+    assert!(
+        stdout_str.contains("999.0.0.0"),
+        "Should mention requested version"
+    );
+    assert!(
+        stdout_str.contains("ignore"),
+        "Should mention the ignore escape hatch"
Evidence
The test asserts only on version tokens and the substring "ignore". But Selenium Manager always logs
"Required browser" and "Detected browser" messages that include these version strings, so the test
does not uniquely verify the mismatch guidance text is present.

rust/tests/browser_tests.rs[191-208]
rust/src/lib.rs[312-316]
rust/src/lib.rs[521-526]

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_path_version_mismatch_test()` currently checks only for version tokens and the word `ignore`. These checks do not uniquely validate the new user-facing mismatch guidance (e.g., the “remove --browser-path … or set --browser-version ignore …” text), so the test may not catch regressions in the behavior/message this PR intends to enforce.

### Issue Context
Selenium Manager already logs "Required browser: ..." and "Detected browser: ..." debug lines containing the version numbers, so token-based checks are not sufficient to confirm the mismatch error/warn guidance was actually produced.

### Fix Focus Areas
- rust/tests/browser_tests.rs[196-208]

### Suggested change
Replace/augment the current assertions with checks for distinctive substrings from the mismatch guidance, e.g.:
- `"has version"`
- `"remove --browser-path"`
- `"--browser-version ignore"`
This makes the test validate the actual intended UX instead of incidental debug output.

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


10. Mismatch test lacks exit check 📘 Rule violation ☼ Reliability
Description
browser_path_version_mismatch_test() only asserts that certain substrings appear in stdout and
explicitly allows either an ERROR or WARN path, so it does not verify deterministic signaling of the
mismatch (exit code and/or severity). This weakens regression coverage and can allow the mismatch
behavior to change without failing the test.
Code

rust/tests/browser_tests.rs[R196-208]

+    // The mismatch is always reported, either as ERROR (no cached driver) or WARN (fallback used)
+    assert!(
+        stdout_str.contains("131.0.6778.264"),
+        "Should mention detected version"
+    );
+    assert!(
+        stdout_str.contains("999.0.0.0"),
+        "Should mention requested version"
+    );
+    assert!(
+        stdout_str.contains("ignore"),
+        "Should mention the ignore escape hatch"
+    );
Evidence
The testing standard requires appropriate, reliable coverage for behavioral changes. The added test
explicitly tolerates multiple outcomes and only checks for version/keyword substrings, without
asserting exit code or a specific mismatch signal, making the test non-deterministic and potentially
ineffective at catching regressions.

AGENTS.md: Testing Standards: Add/Update Tests, Prefer Small Unit Tests, Avoid Mocks
rust/tests/browser_tests.rs[196-208]

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_path_version_mismatch_test()` does not assert a deterministic outcome (exit status and/or whether the mismatch is an error vs warning). The test comment indicates behavior may vary (ERROR vs WARN), which can make CI results dependent on cache state and reduce the test’s ability to prevent regressions.

## Issue Context
This test is intended to validate the new mismatch handling for `--browser-path` combined with a conflicting `--browser-version`. Without asserting failure/success and the expected signaling mechanism, the test may pass even if the mismatch is no longer surfaced reliably.

## Fix Focus Areas
- rust/tests/browser_tests.rs[196-208]

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


11. Temp fake browser collisions ✓ Resolved 🐞 Bug ☼ Reliability
Description
create_fake_browser() writes to a deterministic path under std::env::temp_dir() and never
removes it, so parallel test execution or repeated runs can race/overwrite the same file and cause
flaky CI failures. The two new tests also create the same fake version string, increasing the chance
of collisions.
Code

rust/tests/browser_tests.rs[R166-174]

+#[cfg(unix)]
+fn create_fake_browser(version: &str) -> std::path::PathBuf {
+    let tmp = std::env::temp_dir().join(format!("fake-chrome-{}", version.replace('.', "-")));
+    let script = format!("#!/bin/sh\necho 'Google Chrome {}'\n", version);
+    std::fs::write(&tmp, script).expect("Unable to write fake browser script");
+    std::fs::set_permissions(&tmp, std::fs::Permissions::from_mode(0o755))
+        .expect("Unable to set executable bit");
+    tmp
+}
Evidence
The helper constructs a deterministic filename in the system temp dir and writes/sets permissions
without cleanup; both tests invoke it with the same version, so they target the same path.

rust/tests/browser_tests.rs[166-174]
rust/tests/browser_tests.rs[176-240]

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

## Issue description
The tests create an executable script at a deterministic temp path (based only on the version string) and do not clean it up. Rust tests can run in parallel, and both tests use the same version, so they can contend for the same file and introduce flaky behavior.

## Issue Context
`create_fake_browser("131.0.6778.264")` is called by multiple tests, producing the same filename each time.

## Fix Focus Areas
- rust/tests/browser_tests.rs[166-240]

### Suggested implementation direction
- Use the `tempfile` crate (e.g., `tempfile::NamedTempFile` or a `TempDir`) to generate a unique executable file per test.
- Keep the temp handle alive for the duration of the test so the file isn’t deleted early.
- Optionally ensure cleanup (automatic with `tempfile`) to avoid leaving artifacts in `/tmp`.

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


12. Ignore test misses success ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
browser_path_version_ignore_test() never asserts the command succeeds (exit code 0), so it could
pass even if Selenium Manager fails after emitting the expected warning text. This weakens
regression coverage for the new ignore behavior.
Code

rust/tests/browser_tests.rs[R214-230]

+fn browser_path_version_ignore_test() {
+    let fake_browser = create_fake_browser("131.0.6778.264");
+    let mut cmd = get_selenium_manager();
+    let stdout = cmd
+        .args([
+            "--browser",
+            "chrome",
+            "--browser-path",
+            fake_browser.to_str().unwrap(),
+            "--browser-version",
+            "ignore",
+            "--debug",
+        ])
+        .assert()
+        .get_output()
+        .stdout
+        .clone();
Evidence
The test calls .assert().get_output() and then checks stdout, but does not include .success() or
an exit code assertion, unlike other tests above in the same file.

rust/tests/browser_tests.rs[212-240]

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

## Issue description
The `browser_path_version_ignore_test` inspects stdout but does not assert `.success()`/`.code(0)`. A failing run could still emit the warning and satisfy the string checks, causing a false-positive test.

## Issue Context
Other tests in this file explicitly assert success or expected failure codes.

## Fix Focus Areas
- rust/tests/browser_tests.rs[212-240]

### Suggested implementation direction
- Add `.success().code(0)` (or `.try_success()` + `assert_output`) to the assertion chain before reading output.
- Optionally assert the output contains the expected warning *and* that execution completes successfully.

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



Informational

13. Stdout-only mismatch asserts 🐞 Bug ☼ Reliability
Description
The mismatch assertions only search stdout_str, so they can miss the mismatch message when
Selenium Manager logs are routed to stderr (e.g., MIXED output mode, or logger init paths that don’t
force stdout targeting). This can make the new assertions fail or become environment-dependent.
Code

rust/tests/browser_tests.rs[R196-207]

+    // The mismatch is always reported, either as ERROR (no cached driver) or WARN (fallback used)
+    assert!(
+        stdout_str.contains("131.0.6778.264"),
+        "Should mention detected version"
+    );
+    assert!(
+        stdout_str.contains("999.0.0.0"),
+        "Should mention requested version"
+    );
+    assert!(
+        stdout_str.contains("ignore"),
+        "Should mention the ignore escape hatch"
Evidence
The test’s assertions are performed against stdout_str only. Selenium Manager explicitly supports
output modes where logs go to stderr (MIXED), and the logger setup selects different targets based
on output type / init path, so stdout-only assertions can miss relevant messages.

rust/tests/browser_tests.rs[191-208]
rust/src/main.rs[85-88]
rust/src/logger.rs[88-131]

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_path_version_mismatch_test()` validates output by searching only `stdout_str`. Selenium Manager can emit logs to stderr under some configurations (notably `--output mixed` routes logs to stderr), so limiting checks to stdout can make the test sensitive to output routing.

### Issue Context
The new assertions added in this PR depend on seeing the mismatch guidance in the captured output.

### Fix Focus Areas
- rust/tests/browser_tests.rs[196-208]

### Suggested change
Capture both streams and assert on the combined output, e.g. concatenate `output.stdout` and `output.stderr` into a single string and search that; alternatively explicitly force `--output LOGGER` and/or clear env that changes logger routing when running the command in the test.

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


Grey Divider

Previous review results

Review updated until commit 0a8c1d4

Results up to commit f93ded1


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Action required
1. Ignore breaks browser download ✓ Resolved 🐞 Bug ≡ Correctness
Description
If --browser-version ignore is used and no local browser is discovered, setup() proceeds to
download_browser() with original_browser_version="ignore", where it is treated as a non-empty
non-stable version and its “major” parses to 0, triggering the minimum-version download guard and
failing with a misleading error. This makes the newly introduced ignore escape hatch unreliable in
environments where local discovery fails or no browser is installed.
Code

rust/src/lib.rs[R528-538]

+                    if self.is_browser_version_ignore() {
+                        if !original_browser_path.is_empty() {
+                            self.get_logger().warn(format!(
+                                "Browser version check bypassed for {} at {}; using detected version {}",
+                                self.get_browser_name(),
+                                original_browser_path,
+                                discovered_version
+                            ));
+                        }
+                        self.set_browser_version(discovered_version);
+                    } else if self.is_browser_version_specific()
Evidence
The PR adds an ignore special-case only in discover_local_browser(); when discovery fails, the
unchanged download path still consumes the original browser_version string, and
download_browser() enforces a numeric minimum using an integer parse of the major version, which
will be 0 for non-numeric values like ignore.

rust/src/lib.rs[508-624]
rust/src/lib.rs[238-266]
rust/src/lib.rs[1400-1409]
rust/src/lib.rs[1971-1976]

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-version ignore` is introduced as a special value, but it is only handled in `discover_local_browser()` when a local browser version is successfully detected. If local discovery returns `None`, the code can still attempt a browser download with `original_browser_version == "ignore"`, which then trips version parsing/min-version download logic and fails with an incorrect/misleading error.

## Issue Context
- `setup()` passes `original_browser_version` (from config) into `download_browser_if_necessary()`.
- `download_browser()` uses `get_major_browser_version()` and parses it as an integer to enforce a minimum browser version for downloads; for `"ignore"` this effectively becomes `0`.

## Fix Focus Areas
- rust/src/lib.rs[508-624]
- rust/src/lib.rs[238-266]
- rust/src/lib.rs[1400-1409]

### Suggested implementation direction
Choose one (but ensure behavior is explicit and user-friendly):
1. **Treat `ignore` as “unset” for download purposes**: when `is_browser_version_ignore()` is true, behave like `browser_version` is empty/stable in download selection (e.g., request latest) OR clear `original_browser_version` before calling `download_browser_if_necessary()`.
2. **Validate and error early**: if `browser-version==ignore` and there is no explicit browser path (and/or no local browser can be discovered), return a clear error explaining that `ignore` is only meaningful when a local browser is available (especially when paired with `--browser-path`).

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



Remediation recommended
2. Temp fake browser collisions ✓ Resolved 🐞 Bug ☼ Reliability
Description
create_fake_browser() writes to a deterministic path under std::env::temp_dir() and never
removes it, so parallel test execution or repeated runs can race/overwrite the same file and cause
flaky CI failures. The two new tests also create the same fake version string, increasing the chance
of collisions.
Code

rust/tests/browser_tests.rs[R166-174]

+#[cfg(unix)]
+fn create_fake_browser(version: &str) -> std::path::PathBuf {
+    let tmp = std::env::temp_dir().join(format!("fake-chrome-{}", version.replace('.', "-")));
+    let script = format!("#!/bin/sh\necho 'Google Chrome {}'\n", version);
+    std::fs::write(&tmp, script).expect("Unable to write fake browser script");
+    std::fs::set_permissions(&tmp, std::fs::Permissions::from_mode(0o755))
+        .expect("Unable to set executable bit");
+    tmp
+}
Evidence
The helper constructs a deterministic filename in the system temp dir and writes/sets permissions
without cleanup; both tests invoke it with the same version, so they target the same path.

rust/tests/browser_tests.rs[166-174]
rust/tests/browser_tests.rs[176-240]

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

## Issue description
The tests create an executable script at a deterministic temp path (based only on the version string) and do not clean it up. Rust tests can run in parallel, and both tests use the same version, so they can contend for the same file and introduce flaky behavior.

## Issue Context
`create_fake_browser("131.0.6778.264")` is called by multiple tests, producing the same filename each time.

## Fix Focus Areas
- rust/tests/browser_tests.rs[166-240]

### Suggested implementation direction
- Use the `tempfile` crate (e.g., `tempfile::NamedTempFile` or a `TempDir`) to generate a unique executable file per test.
- Keep the temp handle alive for the duration of the test so the file isn’t deleted early.
- Optionally ensure cleanup (automatic with `tempfile`) to avoid leaving artifacts in `/tmp`.

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


3. Ignore test misses success ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
browser_path_version_ignore_test() never asserts the command succeeds (exit code 0), so it could
pass even if Selenium Manager fails after emitting the expected warning text. This weakens
regression coverage for the new ignore behavior.
Code

rust/tests/browser_tests.rs[R214-230]

+fn browser_path_version_ignore_test() {
+    let fake_browser = create_fake_browser("131.0.6778.264");
+    let mut cmd = get_selenium_manager();
+    let stdout = cmd
+        .args([
+            "--browser",
+            "chrome",
+            "--browser-path",
+            fake_browser.to_str().unwrap(),
+            "--browser-version",
+            "ignore",
+            "--debug",
+        ])
+        .assert()
+        .get_output()
+        .stdout
+        .clone();
Evidence
The test calls .assert().get_output() and then checks stdout, but does not include .success() or
an exit code assertion, unlike other tests above in the same file.

rust/tests/browser_tests.rs[212-240]

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

## Issue description
The `browser_path_version_ignore_test` inspects stdout but does not assert `.success()`/`.code(0)`. A failing run could still emit the warning and satisfy the string checks, causing a false-positive test.

## Issue Context
Other tests in this file explicitly assert success or expected failure codes.

## Fix Focus Areas
- rust/tests/browser_tests.rs[212-240]

### Suggested implementation direction
- Add `.success().code(0)` (or `.try_success()` + `assert_output`) to the assertion chain before reading output.
- Optionally assert the output contains the expected warning *and* that execution completes successfully.

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


Results up to commit 26efb88


🐞 Bugs (2) 📘 Rule violations (1) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Remediation recommended
1. Mismatch test lacks exit check 📘 Rule violation ☼ Reliability
Description
browser_path_version_mismatch_test() only asserts that certain substrings appear in stdout and
explicitly allows either an ERROR or WARN path, so it does not verify deterministic signaling of the
mismatch (exit code and/or severity). This weakens regression coverage and can allow the mismatch
behavior to change without failing the test.
Code

rust/tests/browser_tests.rs[R196-208]

+    // The mismatch is always reported, either as ERROR (no cached driver) or WARN (fallback used)
+    assert!(
+        stdout_str.contains("131.0.6778.264"),
+        "Should mention detected version"
+    );
+    assert!(
+        stdout_str.contains("999.0.0.0"),
+        "Should mention requested version"
+    );
+    assert!(
+        stdout_str.contains("ignore"),
+        "Should mention the ignore escape hatch"
+    );
Evidence
The testing standard requires appropriate, reliable coverage for behavioral changes. The added test
explicitly tolerates multiple outcomes and only checks for version/keyword substrings, without
asserting exit code or a specific mismatch signal, making the test non-deterministic and potentially
ineffective at catching regressions.

AGENTS.md: Testing Standards: Add/Update Tests, Prefer Small Unit Tests, Avoid Mocks
rust/tests/browser_tests.rs[196-208]

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_path_version_mismatch_test()` does not assert a deterministic outcome (exit status and/or whether the mismatch is an error vs warning). The test comment indicates behavior may vary (ERROR vs WARN), which can make CI results dependent on cache state and reduce the test’s ability to prevent regressions.

## Issue Context
This test is intended to validate the new mismatch handling for `--browser-path` combined with a conflicting `--browser-version`. Without asserting failure/success and the expected signaling mechanism, the test may pass even if the mismatch is no longer surfaced reliably.

## Fix Focus Areas
- rust/tests/browser_tests.rs[196-208]

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


2. Mismatch check too weak 🐞 Bug ≡ Correctness
Description
browser_path_version_mismatch_test only asserts the output contains the two version strings plus
"ignore", which doesn’t prove the new mismatch guidance text was emitted. This test can pass even if
only the generic "Detected browser" / "Required browser" debug lines are present and the actionable
mismatch message regresses.
Code

rust/tests/browser_tests.rs[R196-207]

+    // The mismatch is always reported, either as ERROR (no cached driver) or WARN (fallback used)
+    assert!(
+        stdout_str.contains("131.0.6778.264"),
+        "Should mention detected version"
+    );
+    assert!(
+        stdout_str.contains("999.0.0.0"),
+        "Should mention requested version"
+    );
+    assert!(
+        stdout_str.contains("ignore"),
+        "Should mention the ignore escape hatch"
Evidence
The test asserts only on version tokens and the substring "ignore". But Selenium Manager always logs
"Required browser" and "Detected browser" messages that include these version strings, so the test
does not uniquely verify the mismatch guidance text is present.

rust/tests/browser_tests.rs[191-208]
rust/src/lib.rs[312-316]
rust/src/lib.rs[521-526]

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_path_version_mismatch_test()` currently checks only for version tokens and the word `ignore`. These checks do not uniquely validate the new user-facing mismatch guidance (e.g., the “remove --browser-path … or set --browser-version ignore …” text), so the test may not catch regressions in the behavior/message this PR intends to enforce.

### Issue Context
Selenium Manager already logs "Required browser: ..." and "Detected browser: ..." debug lines containing the version numbers, so token-based checks are not sufficient to confirm the mismatch error/warn guidance was actually produced.

### Fix Focus Areas
- rust/tests/browser_tests.rs[196-208]

### Suggested change
Replace/augment the current assertions with checks for distinctive substrings from the mismatch guidance, e.g.:
- `"has version"`
- `"remove --browser-path"`
- `"--browser-version ignore"`
This makes the test validate the actual intended UX instead of incidental debug output.

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



Informational
3. Stdout-only mismatch asserts 🐞...

@selenium-ci selenium-ci added C-rust Rust code is mostly Selenium Manager B-manager Selenium Manager labels Jun 9, 2026
Comment thread rust/src/lib.rs Outdated
@qodo-code-review

qodo-code-review Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 26efb88

@Delta456 Delta456 force-pushed the selenium_manager branch from f79f7c2 to 02638fb Compare June 9, 2026 19:04
@Delta456 Delta456 requested a review from AutomatedTester June 9, 2026 19:05
@qodo-code-review

qodo-code-review Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

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

Comment thread rust/tests/browser_tests.rs Outdated
@qodo-code-review

qodo-code-review Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 4f3546e

@qodo-code-review

qodo-code-review Bot commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

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

Comment thread rust/src/lib.rs Outdated
Comment on lines 538 to 540
} else if self.is_browser_version_specific()
&& !self.get_browser_version().eq(&discovered_version)
{

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Action required

1. --browser-path major mismatch allowed 📎 Requirement gap ≡ Correctness

The new mismatch error path in discover_local_browser() only triggers when
is_browser_version_specific() is true (i.e., the requested --browser-version contains a .), so
a major-only version like 114/105 can still mismatch the detected version at an explicit
--browser-path without failing fast. In that case, later major-version handling can set
download_browser=true and effectively ignore the user-provided path, violating the requirement to
clearly fail when --browser-path and --browser-version conflict.
Agent Prompt
## Issue description
When `--browser-path` is provided together with a major-only `--browser-version` (e.g., `114`/`105`), a mismatch between the detected browser version at that path and the requested major version does not trigger the new fail-fast mismatch error because it is gated by `is_browser_version_specific()` (versions containing `.`). This can allow later major-version mismatch handling to set `download_browser = true` and effectively ignore the explicit browser path, contrary to the requirement to clearly fail on `--browser-path`/`--browser-version` conflicts.

## Issue Context
Compliance ID 1 requires a clear failure when `--browser-path` and `--browser-version` conflict.
The current mismatch error path only runs when `is_browser_version_specific()` is true (implemented as `version.contains(".")`), so major-only version requests bypass it.
Major-version mismatch handling happens later during major comparisons/reconciliation and can still set `download_browser = true` without checking whether `original_browser_path` / an explicit `--browser-path` was provided.

## Fix Focus Areas
- rust/src/lib.rs[508-610]
- rust/src/lib.rs[855-867]

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

@qodo-code-review

qodo-code-review Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

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

@titusfortner

Copy link
Copy Markdown
Member

I think @bonigarcia objection on Slack is correct here, and I was not. I have a use case in mind for the ignore, but it shouldn't need to be bundled into this bug fix.

@Delta456

Copy link
Copy Markdown
Member Author

I think @bonigarcia objection on Slack is correct here, and I was not. I have a use case in mind for the ignore, but it shouldn't need to be bundled into this bug fix.

Should be fixed now

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 5891a28

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 0a8c1d4

@bonigarcia

Copy link
Copy Markdown
Member

@Delta456 Have you tested this code locally?

I just did it in Windows, and it works the same as before. In the following execution (run using your branch), --browser-path and --browser-version disagree, but Selenium Manager does not error (as requested by @titusfortner)

C:\Users\boni\Documents\dev\selenium\rust>cargo run -- --browser chrome --debug --browser-path C:\Users\boni\.cache\selenium\chrome\win64\149.0.7827.22\chrome.exe --browser-version 150
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.41s
     Running `target\debug\selenium-manager.exe --browser chrome --debug --browser-path C:\Users\boni\.cache\selenium\chrome\win64\149.0.7827.22\chrome.exe --browser-version 150`
[2026-06-19T07:58:48.186Z DEBUG] chromedriver not found in PATH
[2026-06-19T07:58:48.187Z DEBUG] Detected browser: chrome 149.0.7827.22
[2026-06-19T07:58:48.187Z DEBUG] Discovered chrome version (149) different to specified browser version (150)
[2026-06-19T07:58:48.190Z DEBUG] Required browser: chrome 150.0.7871.24
[2026-06-19T07:58:48.190Z DEBUG] chrome 150.0.7871.24 already exists at C:\Users\boni\.cache\selenium\chrome\win64\150.0.7871.24\chrome.exe
[2026-06-19T07:58:48.193Z DEBUG] chrome 150.0.7871.24 is available at C:\Users\boni\.cache\selenium\chrome\win64\150.0.7871.24\chrome.exe
[2026-06-19T07:58:48.204Z DEBUG] Required driver: chromedriver 150.0.7871.24
[2026-06-19T07:58:48.205Z DEBUG] chromedriver 150.0.7871.24 already in the cache
[2026-06-19T07:58:48.207Z INFO ] Driver path: C:\Users\boni\.cache\selenium\chromedriver\win64\150.0.7871.24\chromedriver.exe
[2026-06-19T07:58:48.208Z INFO ] Browser path: C:\Users\boni\.cache\selenium\chrome\win64\150.0.7871.24\chrome.exe

@titusfortner: After thinking more about your original issue request (#17540), I'm pretty convinced this is not a bug. Simply, Selenium Manager gives priority to --browser-version over --browser-path, which is reasonable to me.

But independently of the name (call it a bug or a feature), I don't believe this new behavior (i.e., erroring on disagrees on those flags) is very relevant to users. Probably, the number of users affected by this change is close to zero.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-manager Selenium Manager C-rust Rust code is mostly Selenium Manager

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] SM ignores browser path when version is specified

5 participants