You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR allows SM to manage full browser versions, such as Chrome 131.0.6725.0, Firefox 121.0.1, or Edge 129.0.2792.79. Previously, the latest stable browser version for each browser (e.g., Chrome 131.0.6778.3) was managed. Now, any specific browser version should be managed (if available). For instance:
./selenium-manager --debug --browser chrome --browser-version 131.0.6725.0
[2024-10-18T11:58:20.151Z DEBUG] chromedriver not found in PATH
[2024-10-18T11:58:20.152Z DEBUG] chrome detected at C:\Program Files\Google\Chrome\Application\chrome.exe
[2024-10-18T11:58:20.154Z DEBUG] Running command: wmic datafile where name='C:\\Program Files\\Google\\Chrome\\Application\\chrome.exe' get Version /value
[2024-10-18T11:58:20.233Z DEBUG] Output: "\r\r\n\r\r\nVersion=129.0.6668.103\r\r\n\r\r\n\r\r\n\r"
[2024-10-18T11:58:20.238Z DEBUG] Detected browser: chrome 129.0.6668.103
[2024-10-18T11:58:20.238Z DEBUG] Discovered chrome version (129) different to specified browser version (131)
[2024-10-18T11:58:20.244Z DEBUG] Required browser: chrome 131.0.6725.0
[2024-10-18T11:58:20.245Z DEBUG] Discovering versions from https://googlechromelabs.github.io/chrome-for-testing/known-good-versions-with-downloads.json
[2024-10-18T11:58:20.459Z DEBUG] Downloading chrome 131.0.6725.0 from https://storage.googleapis.com/chrome-for-testing-public/131.0.6725.0/win64/chrome-win64.zip
[2024-10-18T11:58:39.928Z DEBUG] chrome 131.0.6725.0 is available at C:\Users\boni\.cache\selenium\chrome\win64\131.0.6725.0\chrome.exe
[2024-10-18T11:58:39.933Z DEBUG] Required driver: chromedriver 131.0.6778.3
[2024-10-18T11:58:39.934Z DEBUG] chromedriver 131.0.6778.3 already in the cache
[2024-10-18T11:58:39.935Z INFO ] Driver path: C:\Users\boni\.cache\selenium\chromedriver\win64\131.0.6778.3\chromedriver.exe
[2024-10-18T11:58:39.937Z INFO ] Browser path: C:\Users\boni\.cache\selenium\chrome\win64\131.0.6725.0\chrome.exe
Here are some key observations to aid the review process:
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review
Error Handling The error handling for unavailable downloads has been simplified. Verify if this change maintains proper error reporting and doesn't suppress important information.
Version Specificity New logic for handling specific browser versions has been introduced. Ensure that this change correctly differentiates between specific and non-specific version requests.
Metadata Handling Changes in metadata handling for browser version discovery. Verify that the new logic correctly updates and uses the metadata for both specific and non-specific version requests.
-if self.is_version_specific(original_browser_version) {- browser_version = original_browser_version.to_string();-} else {- // Browser version is checked in the local metadata- match get_browser_version_from_metadata(- &metadata.browsers,- self.get_browser_name(),- &major_browser_version,- ) {- Some(version) => {- self.get_logger().trace(format!(- "Browser with valid TTL. Getting {} version from metadata",- self.get_browser_name()- ));- browser_version = version;- self.set_browser_version(browser_version.clone());- }- _ => {- // If not in metadata, discover version using online metadata- if self.is_browser_version_stable() || self.is_browser_version_empty() {- browser_version = self- .request_latest_browser_version_from_online(original_browser_version)?;- } else {- browser_version = self- .request_fixed_browser_version_from_online(original_browser_version)?;- }- self.set_browser_version(browser_version.clone());+browser_version = self.get_and_update_browser_version(+ original_browser_version,+ &metadata,+ &major_browser_version,+ cache_path,+)?;- let browser_ttl = self.get_ttl();- if browser_ttl > 0- && !self.is_browser_version_empty()- && !self.is_browser_version_stable()- {- metadata.browsers.push(create_browser_metadata(- self.get_browser_name(),- &major_browser_version,- &browser_version,- browser_ttl,- ));- write_metadata(&metadata, self.get_logger(), cache_path);- }- }- }-}-
Apply this suggestion
Suggestion importance[1-10]: 8
Why: Extracting the version checking and updating logic into a separate method would significantly improve code organization and readability. This refactoring would make the code more modular and easier to maintain.
8
Enhancement
✅ Improve variable naming for better code readabilitySuggestion Impact:The variable inter_versions was renamed to iter_versions, which aligns with the suggestion's intent to improve readability, although the exact name suggested was not used.
code diff:
- let inter_versions = all_versions.versions.into_iter();+ let iter_versions = all_versions.versions.into_iter();
let filtered_versions: Vec<Version> = if self.is_browser_version_specific() {
- inter_versions+ iter_versions
.filter(|r| r.version.eq(browser_version.as_str()))
.collect()
} else {
- inter_versions+ iter_versions
Consider using a more descriptive variable name instead of inter_versions. For example, all_browser_versions would better convey its purpose.
Why: The suggestion to rename inter_versions to all_browser_versions enhances code readability by making the variable's purpose clearer. This is a straightforward improvement that aligns with good naming conventions.
7
Improve method naming for better clarity of its purpose
Consider using a more descriptive name for the is_version_specific method, such as is_full_version, to better convey its purpose of checking if the version contains a dot.
Why: Renaming is_version_specific to is_full_version provides a clearer understanding of the method's purpose. This change enhances code clarity, although the original name is not misleading.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Description
This PR allows SM to manage full browser versions, such as Chrome 131.0.6725.0, Firefox 121.0.1, or Edge 129.0.2792.79. Previously, the latest stable browser version for each browser (e.g., Chrome 131.0.6778.3) was managed. Now, any specific browser version should be managed (if available). For instance:
Motivation and Context
Requested in #13419.
Types of changes
Checklist
PR Type
Enhancement, Tests
Description
Changes walkthrough 📝
chrome.rs
Support specific browser version filtering in ChromeManagerrust/src/chrome.rs
lib.rs
Implement specific browser version handling in SeleniumManagerrust/src/lib.rs
browser_download_tests.rs
Add tests for specific browser version downloadsrust/tests/browser_download_tests.rs