selenium manager: check invalid browser version

[Delta456]

User description

Thanks for contributing to Selenium! A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines. Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Returns an error instead of downloading the possible browser version

Motivation and Context

Fix the issue https://github.com/SeleniumHQ/selenium/issues/13418

Types of changes

• [ ] Bug fix (non-breaking change which fixes an issue)
• [x] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• [x] I have read the contributing document.
• [ ] My change requires a change to the documentation.
• [ ] I have updated the documentation accordingly.
• [x] I have added tests to cover my changes.
• [x] All new and existing tests passed.

---

PR Type

Bug fix, Tests

---

Description

• Implemented error handling in SeleniumManager to return an error when an invalid browser version is provided, preventing unnecessary downloads.
• Added a test case to verify the behavior when an invalid browser version is specified, ensuring the system fails gracefully.

---

Changes walkthrough 📝

	Relevant files
Error handling	lib.rs Add error handling for invalid browser version                      rust/src/lib.rs Added error handling for invalid browser version input. Prevented downloading of browser when version mismatch occurs. Set fallback driver from cache to false. +5/-1
Tests	browser_tests.rs Add test for invalid browser version handling                        rust/tests/browser_tests.rs Added a new test for invalid browser version. Ensured the test checks for failure with incorrect version input. +9/-0

---

💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

[qodo-code-review[bot]]

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Key issues to review Error Handling The new error handling for invalid browser versions might prevent legitimate use cases. Consider adding a warning instead of an error for minor version mismatches.

[qodo-code-review[bot]]

PR Code Suggestions ✨

Category	Suggestion	Score
Test improvement	Verify the error message content in the test assertion Enhance the test by checking the error message content. This ensures that not only does the command fail, but it fails with the expected error message. rust/tests/browser_tests.rs [167-170] cmd.args(["--browser", "chrome", "--browser-version", "bad-version"]) .assert() .code(DATAERR) - .failure(); + .failure() + .stderr(predicate::str::contains("Invalid version bad-version provided")); [ ] Apply this suggestion Suggestion importance[1-10]: 9 Why: Enhancing the test to check the error message content ensures that the failure is not only detected but also matches the expected error message, improving the test's robustness and reliability.	9
Error handling	Use a custom error type for more specific error handling Consider using a more specific error type instead of anyhow!. Create a custom error enum for different error scenarios in your application, which will make error handling more robust and informative. rust/src/lib.rs [491-494] -return Err(anyhow!(format!( - "Invalid version {} provided", - major_browser_version -))); +return Err(BrowserError::InvalidVersion(major_browser_version.to_string())); [ ] Apply this suggestion Suggestion importance[1-10]: 8 Why: Using a custom error type instead of anyhow! can improve error handling by making it more specific and informative, which is beneficial for debugging and maintaining the code.	8
Logging	Log the error message before returning it Consider logging the error before returning it. This will help with debugging and provide more context in log files. rust/src/lib.rs [490-494] self.set_fallback_driver_from_cache(false); -return Err(anyhow!(format!( - "Invalid version {} provided", - major_browser_version -))); +let error_msg = format!("Invalid version {} provided", major_browser_version); +self.get_logger().error(&error_msg); +return Err(anyhow!(error_msg)); [ ] Apply this suggestion Suggestion importance[1-10]: 7 Why: Logging the error message before returning it provides additional context in log files, which can be helpful for debugging and understanding the error's occurrence.	7

[bonigarcia]

This implementation is not correct since it will make Selenium Manager to fail even for valid browser versions (e.g., Chrome 127). See for example:

cargo run -- --debug --browser chrome --browser-version 127

[2024-09-19T13:58:36.355Z DEBUG] chromedriver not found in PATH
[2024-09-19T13:58:36.357Z DEBUG] chrome detected at C:\Program Files\Google\Chrome\Application\chrome.exe
[2024-09-19T13:58:36.358Z DEBUG] Running command: wmic datafile where name='C:\\Program Files\\Google\\Chrome\\Application\\chrome.exe' get Version /value
[2024-09-19T13:58:36.521Z DEBUG] Output: "\r\r\n\r\r\nVersion=128.0.6613.138\r\r\n\r\r\n\r\r\n\r"
[2024-09-19T13:58:36.528Z DEBUG] Detected browser: chrome 128.0.6613.138
[2024-09-19T13:58:36.528Z DEBUG] Discovered chrome version (128) different to specified browser version (127)
[2024-09-19T13:58:36.529Z ERROR] Invalid version 127 provided

Also, the error message can be enhanced a bit:

                        return Err(anyhow!(format!(
                            "Invalid {} version provided: {}",
                            self.get_browser_name(),
                            major_browser_version
                        )));

[Delta456]

Oops look like I didn't consider all test cases. I will take a look 😅

[bonigarcia]

I reviewed this PR. I'm sorry, but it is not correct.

First, adding a call to request_fixed_browser_version_from_online like that is not a good idea. You can discover by yourself by running all the existing tests with:

cargo test

Also, the two new test cases are not adequate. First, invalid_browser_version_test() is redundant. See here. Second, the logic assessed by valid_browser_version_pass_test() is already covered by this test.

#13418 should be done with the following changes in lib.rs:

         // Download browser if necessary
         match self.download_browser_if_necessary(&original_browser_version) {
             Ok(_) => {}
-            Err(err) => self.check_error_with_driver_in_path(&use_driver_in_path, err)?,
+            Err(err) => {
+                self.set_fallback_driver_from_cache(false);
+                self.check_error_with_driver_in_path(&use_driver_in_path, err)?
+            }
         }

[Delta456]

@bonigarcia You are right. I have committed the code changes as suggested by you. Please let me know if something is missing.