Skip to content

Conversation

bonigarcia
Copy link
Member

@bonigarcia bonigarcia commented Sep 24, 2025

User description

🔗 Related Issues

Fixes #15517.

💥 What does this PR do?

This PR makes Selenium Manager to honor the full browser version when specified even when a matching major browser version is already installed.

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • Bug fix (backwards compatible)

PR Type

Bug fix


Description

  • Honor full browser version when specified, even if major version exists

  • Restructure browser version comparison logic with conditional checks

  • Add specific version validation before fallback to major version matching


Diagram Walkthrough

flowchart LR
  A["Browser Version Request"] --> B["Check if Specific Version"]
  B --> C["Compare with Discovered Version"]
  C --> D["Download if Mismatch"]
  C --> E["Use Discovered if Match"]
  B --> F["Fallback to Major Version Logic"]
Loading

File Walkthrough

Relevant files
Bug fix
lib.rs
Browser version matching logic enhancement                             

rust/src/lib.rs

  • Add specific browser version check before major version comparison
  • Restructure conditional logic to prioritize exact version matching
  • Move WebView2 path handling inside conditional block
+55/-47 

@selenium-ci selenium-ci added C-rust Rust code is mostly Selenium Manager B-manager Selenium Manager labels Sep 24, 2025
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

5678 - Partially compliant

Compliant requirements:

Non-compliant requirements:

  • Investigate and resolve ConnectFailure when creating multiple ChromeDriver instances.
  • Determine root cause for first instance success and subsequent failures.
  • Provide fix/workaround specific to ChromeDriver/Ubuntu environment described.
  • Validation and documentation of resolution steps.

Requires further human verification:

  • Reproduce multi-instance ChromeDriver behavior on Ubuntu 16.04 with specified versions.
  • Runtime validation that connection failures no longer occur.

1234 - Partially compliant

Compliant requirements:

Non-compliant requirements:

  • Ensure click() triggers JavaScript in link href in Firefox.
  • Provide fix and verification for Firefox 42 regression.

Requires further human verification:

  • Manual/browser testing to confirm alert fires on click() across affected versions.
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Possible Logic Bug

When is_browser_version_specific() and the discovered version differs, download_browser is set to true but browser_version may remain unset; ensure version is set appropriately before WebView2 path construction and downstream usage.

if self.is_browser_version_specific()
    && !self.get_browser_version().eq(&discovered_version)
{
    download_browser = true;
} else {
    let discovered_major_browser_version = self
        .get_major_version(&discovered_version)
        .unwrap_or_default();

    if self.is_browser_version_stable() || self.is_browser_version_unstable() {
        let online_browser_version = self.request_browser_version()?;
        if online_browser_version.is_some() {
            let major_online_browser_version =
                self.get_major_version(&online_browser_version.unwrap())?;
            if discovered_major_browser_version
                .eq(&major_online_browser_version)
            {
                self.get_logger().debug(format!(
                    "Discovered online {} version ({}) is the same as the detected local {} version",
                    self.get_browser_name(),
                    discovered_major_browser_version,
                    self.get_browser_name(),
                ));
                self.set_browser_version(discovered_version);
            } else {
                self.get_logger().debug(format!(
                    "Discovered online {} version ({}) is different to the detected local {} version ({})",
                    self.get_browser_name(),
                    major_online_browser_version,
                    self.get_browser_name(),
                    discovered_major_browser_version,
                ));
                download_browser = true;
            }
        } else {
            self.set_browser_version(discovered_version);
        }
    } else if !major_browser_version.is_empty()
        && !self.is_browser_version_unstable()
        && !major_browser_version.eq(&discovered_major_browser_version)
    {
        self.get_logger().debug(format!(
            "Discovered {} version ({}) different to specified browser version ({})",
            self.get_browser_name(),
            discovered_major_browser_version,
            major_browser_version,
        ));
        download_browser = true;
    } else {
        self.set_browser_version(discovered_version);
    }
    if self.is_webview2() && PathBuf::from(self.get_browser_path()).is_dir() {
        let browser_path = format!(
            r"{}\{}\msedge{}",
            self.get_browser_path(),
            &self.get_browser_version(),
            get_binary_extension(self.get_os())
        );
        self.set_browser_path(browser_path);
    }
}
Unwrap Risk

Calling online_browser_version.unwrap() after is_some() is correct, but consider avoiding double computation and potential future refactors by binding once to reduce risk of accidental unwrap on None.

let online_browser_version = self.request_browser_version()?;
if online_browser_version.is_some() {
    let major_online_browser_version =
        self.get_major_version(&online_browser_version.unwrap())?;
    if discovered_major_browser_version

Copy link
Contributor

PR Code Suggestions ✨

No code suggestions found for the PR.

@bonigarcia bonigarcia merged commit 1347c78 into trunk Sep 24, 2025
22 checks passed
@bonigarcia bonigarcia deleted the sm_browser_version_honor branch September 24, 2025 14:06
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 Review effort 2/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[🐛 Bug]: Selenium Manager doesn't use exact browser version unless force-browser-download is enabled

2 participants