From e6554d3cc1d56d03b3d7415159f24ce756c1f84e Mon Sep 17 00:00:00 2001 From: gaurav0107 Date: Mon, 25 May 2026 05:07:35 +0530 Subject: [PATCH 1/3] [rust] honor --browser-version in Selenium Manager Electron driver resolution Fixes #17549. ElectronManager::request_driver_version() always read the redirect from electron/electron/releases/latest, discarding any user-supplied --browser-version. Because Electron release tags equal the chromedriver version that ships with that release, when the user pins a concrete browser version we can use it directly and skip the network roundtrip. Stable / unstable / empty browser versions retain the existing /releases/latest fallback. Also gate the metadata cache write on a non-empty major_browser_version, mirroring the chrome.rs guard, so empty-keyed driver entries no longer get persisted. --- rust/src/electron.rs | 33 +++++++++++++++++++++++---------- rust/tests/electron_tests.rs | 18 ++++++++++++++++++ 2 files changed, 41 insertions(+), 10 deletions(-) diff --git a/rust/src/electron.rs b/rust/src/electron.rs index 8f285a2c3dc68..bfc8fd3b2745c 100644 --- a/rust/src/electron.rs +++ b/rust/src/electron.rs @@ -119,17 +119,30 @@ impl SeleniumManager for ElectronManager { Ok(driver_version) } _ => { - self.assert_online_or_err(OFFLINE_REQUEST_ERR_MSG)?; - - let latest_url = format!( - "{}{}", - self.get_driver_mirror_url_or_default(DRIVER_URL), - LATEST_RELEASE - ); - let driver_version = - read_redirect_from_link(self.get_http_client(), latest_url, self.get_logger())?; + // Electron releases are tagged by Electron version, and the + // chromedriver asset shipped in each release matches that tag. + // When the user pins a concrete browser version (e.g. + // `--browser-version 36.2.1`), that version is the driver + // version we want; resolving via `/releases/latest` would + // discard the user's request and return the latest tag instead. + let browser_version = self.get_browser_version().to_string(); + let driver_version = if !browser_version.is_empty() + && !self.is_browser_version_stable() + && !self.is_browser_version_unstable() + { + browser_version + } else { + self.assert_online_or_err(OFFLINE_REQUEST_ERR_MSG)?; + let latest_url = format!( + "{}{}", + self.get_driver_mirror_url_or_default(DRIVER_URL), + LATEST_RELEASE + ); + read_redirect_from_link(self.get_http_client(), latest_url, self.get_logger())? + }; let driver_ttl = self.get_ttl(); - if driver_ttl > 0 { + if driver_ttl > 0 && !major_browser_version.is_empty() && !driver_version.is_empty() + { metadata.drivers.push(create_driver_metadata( major_browser_version, self.driver_name, diff --git a/rust/tests/electron_tests.rs b/rust/tests/electron_tests.rs index c0e6e48cf92e0..d0f390cdd1dcb 100644 --- a/rust/tests/electron_tests.rs +++ b/rust/tests/electron_tests.rs @@ -37,3 +37,21 @@ fn electron_version_test(#[case] driver_version: String) { .assert(); cmd_assert.success(); } + +#[rstest] +#[case("36.2.1")] +fn electron_browser_version_test(#[case] browser_version: String) { + // Regression for issue #17549: when the user pins --browser-version, the + // resolved driver version must be derived from the requested version, not + // from the /releases/latest redirect. + let mut cmd = get_selenium_manager(); + let cmd_assert = cmd + .args([ + "--browser", + "electron", + "--browser-version", + &browser_version, + ]) + .assert(); + cmd_assert.success(); +} From ff16a22c4272b062545f74ac22980a6c633ce546 Mon Sep 17 00:00:00 2001 From: gaurav0107 Date: Mon, 25 May 2026 05:10:16 +0530 Subject: [PATCH 2/3] [rust] strengthen electron --browser-version regression test Assert the resolved driver path actually contains the requested browser version. Without this, the test only verified that selenium manager exits successfully, which the buggy code path also did (it just downloaded the wrong version). Using --output json + the existing get_driver_path helper makes the regression assertion explicit. --- rust/tests/electron_tests.rs | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/rust/tests/electron_tests.rs b/rust/tests/electron_tests.rs index d0f390cdd1dcb..05529be7dcd56 100644 --- a/rust/tests/electron_tests.rs +++ b/rust/tests/electron_tests.rs @@ -15,7 +15,7 @@ // specific language governing permissions and limitations // under the License. -use crate::common::get_selenium_manager; +use crate::common::{get_driver_path, get_selenium_manager}; use rstest::rstest; @@ -43,15 +43,23 @@ fn electron_version_test(#[case] driver_version: String) { fn electron_browser_version_test(#[case] browser_version: String) { // Regression for issue #17549: when the user pins --browser-version, the // resolved driver version must be derived from the requested version, not - // from the /releases/latest redirect. + // from the /releases/latest redirect. Asserting that the resolved driver + // path contains the requested version protects against silently falling + // back to the latest tag. let mut cmd = get_selenium_manager(); - let cmd_assert = cmd - .args([ - "--browser", - "electron", - "--browser-version", - &browser_version, - ]) - .assert(); - cmd_assert.success(); + cmd.args([ + "--browser", + "electron", + "--browser-version", + &browser_version, + "--output", + "json", + ]); + let driver_path = get_driver_path(&mut cmd); + assert!( + driver_path.contains(&browser_version), + "expected resolved driver path to contain requested version {}, got: {}", + browser_version, + driver_path + ); } From 1f74218cab73bd3faef0279fec8342b3162dcf1f Mon Sep 17 00:00:00 2001 From: Gaurav Dubey Date: Wed, 10 Jun 2026 09:32:46 +0530 Subject: [PATCH 3/3] [rust] address Qodo review: gate Electron pin on specific version, normalize leading v - Move the pinned-version short-circuit ahead of the metadata cache lookup so a cached entry keyed on `major_browser_version` can no longer override an explicit patch pin (e.g. cached `36` overriding `36.2.1`). - Only treat the pin as an exact Electron release tag when it is a *specific* version (contains a dot). Major-only inputs like `36` keep using the `/releases/latest` fallback - Electron has no `v36` tag and emitting one would build a 404 download URL. Regression test added. - Strip a single leading `v` / `V` so inputs like `v36.2.1` no longer collide with the `v` prefix added by `get_driver_url()`. We deliberately do not call `parse_version()` here because it would mangle prerelease suffixes such as `36.0.0-beta.1` that Electron release tags carry. - Add unit tests for `normalize_pinned_version` covering specific versions, leading-v normalization, prerelease preservation, major-only fallback, channel names, and empty/whitespace input. Refs PR #17567 review by titusfortner; addresses the three Qodo flags. --- rust/src/electron.rs | 156 ++++++++++++++++++++++++++++++----- rust/tests/electron_tests.rs | 30 ++++++- 2 files changed, 162 insertions(+), 24 deletions(-) diff --git a/rust/src/electron.rs b/rust/src/electron.rs index bfc8fd3b2745c..b70523f195746 100644 --- a/rust/src/electron.rs +++ b/rust/src/electron.rs @@ -25,7 +25,8 @@ use crate::metadata::{ create_driver_metadata, get_driver_version_from_metadata, get_metadata, write_metadata, }; use crate::{ - LATEST_RELEASE, Logger, OFFLINE_REQUEST_ERR_MSG, SeleniumManager, WINDOWS, create_http_client, + BETA, CANARY, DEV, ESR, LATEST_RELEASE, Logger, NIGHTLY, OFFLINE_REQUEST_ERR_MSG, STABLE, + SeleniumManager, WINDOWS, create_http_client, }; use anyhow::Error; use reqwest::Client; @@ -37,6 +38,46 @@ use std::sync::mpsc::{Receiver, Sender}; pub const ELECTRON_NAME: &str = "electron"; const DRIVER_URL: &str = "https://github.com/electron/electron/releases/"; +/// Returns the user's `--browser-version` as the resolved Electron driver +/// version when - and only when - it is a *specific* version that we can use +/// verbatim as a release tag (e.g. `36.2.1`, `v36.2.1`, `36.0.0-beta.1`). +/// +/// Returns `None` for empty input, channel names (`stable`, `beta`, `dev`, +/// `nightly`, `canary`, `esr`), and major-only inputs like `36` - those must +/// keep using the `/releases/latest` redirect path because Electron has no +/// `v36` tag and emitting one would build a 404 download URL. +/// +/// A leading `v` or `V` is stripped (so `v36.2.1` doesn't double up against +/// the `v` prefix added by `get_driver_url()`); we deliberately do *not* call +/// `parse_version()` here because that strips prerelease suffixes such as +/// `-beta.1`, which Electron release tags legitimately carry. +fn normalize_pinned_version(browser_version: &str) -> Option { + let trimmed = browser_version.trim(); + if trimmed.is_empty() { + return None; + } + if trimmed.eq_ignore_ascii_case(STABLE) + || trimmed.eq_ignore_ascii_case(BETA) + || trimmed.eq_ignore_ascii_case(DEV) + || trimmed.eq_ignore_ascii_case(NIGHTLY) + || trimmed.eq_ignore_ascii_case(CANARY) + || trimmed.eq_ignore_ascii_case(ESR) + { + return None; + } + let stripped = trimmed + .strip_prefix('v') + .or_else(|| trimmed.strip_prefix('V')) + .unwrap_or(trimmed); + // Require a specific version (contains a dot); major-only inputs like + // `36` are not valid Electron release tags and must use the latest + // fallback. + if !stripped.contains('.') { + return None; + } + Some(stripped.to_string()) +} + pub struct ElectronManager { pub browser_name: &'static str, pub driver_name: &'static str, @@ -101,6 +142,27 @@ impl SeleniumManager for ElectronManager { } fn request_driver_version(&mut self) -> Result { + // Electron releases are tagged by Electron version, and the + // chromedriver asset shipped in each release matches that tag. + // When the user pins a *specific* browser version (e.g. + // `--browser-version 36.2.1`), that version is the driver version + // we want; resolving via `/releases/latest` would discard the + // user's request and return the latest tag instead. + // + // This short-circuit must run *before* the metadata cache lookup: + // the cache is keyed only on `major_browser_version`, so a cached + // entry for major `36` would otherwise override an explicit + // `36.2.1` pin and silently return a different patch version. + // + // Major-only inputs like `36` keep using the latest-redirect + // fallback path, because Electron has no `v36` tag - using it + // verbatim would build a 404 download URL. + let browser_version = self.get_browser_version().to_string(); + let normalized_pin = normalize_pinned_version(&browser_version); + if let Some(pinned) = normalized_pin { + return Ok(pinned); + } + let major_browser_version_binding = self.get_major_browser_version(); let major_browser_version = major_browser_version_binding.as_str(); let cache_path = self.get_cache_path()?; @@ -119,27 +181,14 @@ impl SeleniumManager for ElectronManager { Ok(driver_version) } _ => { - // Electron releases are tagged by Electron version, and the - // chromedriver asset shipped in each release matches that tag. - // When the user pins a concrete browser version (e.g. - // `--browser-version 36.2.1`), that version is the driver - // version we want; resolving via `/releases/latest` would - // discard the user's request and return the latest tag instead. - let browser_version = self.get_browser_version().to_string(); - let driver_version = if !browser_version.is_empty() - && !self.is_browser_version_stable() - && !self.is_browser_version_unstable() - { - browser_version - } else { - self.assert_online_or_err(OFFLINE_REQUEST_ERR_MSG)?; - let latest_url = format!( - "{}{}", - self.get_driver_mirror_url_or_default(DRIVER_URL), - LATEST_RELEASE - ); - read_redirect_from_link(self.get_http_client(), latest_url, self.get_logger())? - }; + self.assert_online_or_err(OFFLINE_REQUEST_ERR_MSG)?; + let latest_url = format!( + "{}{}", + self.get_driver_mirror_url_or_default(DRIVER_URL), + LATEST_RELEASE + ); + let driver_version = + read_redirect_from_link(self.get_http_client(), latest_url, self.get_logger())?; let driver_ttl = self.get_ttl(); if driver_ttl > 0 && !major_browser_version.is_empty() && !driver_version.is_empty() { @@ -286,3 +335,66 @@ impl SeleniumManager for ElectronManager { None } } + +#[cfg(test)] +mod tests { + use super::normalize_pinned_version; + + #[test] + fn specific_version_is_returned_verbatim() { + assert_eq!( + normalize_pinned_version("36.2.1"), + Some("36.2.1".to_string()) + ); + } + + #[test] + fn leading_v_is_stripped() { + assert_eq!( + normalize_pinned_version("v36.2.1"), + Some("36.2.1".to_string()) + ); + assert_eq!( + normalize_pinned_version("V36.2.1"), + Some("36.2.1".to_string()) + ); + } + + #[test] + fn prerelease_suffix_is_preserved() { + assert_eq!( + normalize_pinned_version("36.0.0-beta.1"), + Some("36.0.0-beta.1".to_string()) + ); + assert_eq!( + normalize_pinned_version("v36.0.0-beta.1"), + Some("36.0.0-beta.1".to_string()) + ); + } + + #[test] + fn major_only_falls_back() { + // `36` has no Electron tag; must fall back to /releases/latest. + assert_eq!(normalize_pinned_version("36"), None); + assert_eq!(normalize_pinned_version("v36"), None); + } + + #[test] + fn channel_names_fall_back() { + for channel in ["stable", "beta", "dev", "nightly", "canary", "esr"] { + assert_eq!(normalize_pinned_version(channel), None, "{}", channel); + assert_eq!( + normalize_pinned_version(&channel.to_uppercase()), + None, + "{}", + channel + ); + } + } + + #[test] + fn empty_or_whitespace_falls_back() { + assert_eq!(normalize_pinned_version(""), None); + assert_eq!(normalize_pinned_version(" "), None); + } +} diff --git a/rust/tests/electron_tests.rs b/rust/tests/electron_tests.rs index 05529be7dcd56..94da6a528a95f 100644 --- a/rust/tests/electron_tests.rs +++ b/rust/tests/electron_tests.rs @@ -40,12 +40,18 @@ fn electron_version_test(#[case] driver_version: String) { #[rstest] #[case("36.2.1")] +#[case("v36.2.1")] fn electron_browser_version_test(#[case] browser_version: String) { // Regression for issue #17549: when the user pins --browser-version, the // resolved driver version must be derived from the requested version, not // from the /releases/latest redirect. Asserting that the resolved driver // path contains the requested version protects against silently falling // back to the latest tag. + // + // The `v36.2.1` case additionally guards against a doubled `v` prefix: + // `get_driver_url()` already prepends `v` to the driver version, so the + // pinned version must be normalized (leading `v` stripped) before being + // returned. let mut cmd = get_selenium_manager(); cmd.args([ "--browser", @@ -56,10 +62,30 @@ fn electron_browser_version_test(#[case] browser_version: String) { "json", ]); let driver_path = get_driver_path(&mut cmd); + let expected = browser_version.trim_start_matches(['v', 'V']); assert!( - driver_path.contains(&browser_version), + driver_path.contains(expected), "expected resolved driver path to contain requested version {}, got: {}", - browser_version, + expected, driver_path ); + assert!( + !driver_path.contains("vv"), + "resolved driver path must not contain doubled `v` prefix, got: {}", + driver_path + ); +} + +#[test] +fn electron_major_only_browser_version_falls_back_test() { + // Regression: a major-only --browser-version like `36` is not a valid + // Electron release tag (no `v36` exists), so the manager must fall back + // to the /releases/latest redirect instead of constructing a 404 URL. + // We assert the command still succeeds; the resolved driver version is + // whatever `/releases/latest` currently points at, which we do not pin. + let mut cmd = get_selenium_manager(); + let cmd_assert = cmd + .args(["--browser", "electron", "--browser-version", "36"]) + .assert(); + cmd_assert.success(); }