From 01ce896ddf68ac5cedf9ce5e6d7782890dc08def Mon Sep 17 00:00:00 2001 From: Boni Garcia Date: Mon, 12 Aug 2024 16:36:42 +0200 Subject: [PATCH 1/8] [rust] Selenium Manager errors when browser-path is wrong (#13352) --- rust/src/config.rs | 2 ++ rust/src/lib.rs | 14 +++++++++++++ rust/src/main.rs | 41 ++++++++++++++++++++----------------- rust/tests/browser_tests.rs | 14 +++++++++++++ 4 files changed, 52 insertions(+), 19 deletions(-) diff --git a/rust/src/config.rs b/rust/src/config.rs index a3a2699bee6f1..7c57688b4acf2 100644 --- a/rust/src/config.rs +++ b/rust/src/config.rs @@ -42,6 +42,7 @@ pub const CACHE_PATH_KEY: &str = "cache-path"; pub struct ManagerConfig { pub cache_path: String, + pub fallback_driver_from_cache: bool, pub browser_version: String, pub driver_version: String, pub browser_path: String, @@ -97,6 +98,7 @@ impl ManagerConfig { ManagerConfig { cache_path, + fallback_driver_from_cache: true, browser_version: StringKey(vec!["browser-version", &browser_version_label], "") .get_value(), driver_version: StringKey(vec!["driver-version", &driver_version_label], "") diff --git a/rust/src/lib.rs b/rust/src/lib.rs index d7a0fc0878951..7d9bf60e5fbd3 100644 --- a/rust/src/lib.rs +++ b/rust/src/lib.rs @@ -1065,6 +1065,12 @@ pub trait SeleniumManager { if let Some(path) = self.detect_browser_path() { browser_path = path_to_string(&path); } + } else if !Path::new(&browser_path).exists() { + self.set_fallback_driver_from_cache(false); + return Err(anyhow!(format_one_arg( + "Browser path does not exist: {}", + &browser_path, + ))); } let escaped_browser_path = self.get_escaped_path(browser_path.to_string()); @@ -1466,6 +1472,14 @@ pub trait SeleniumManager { self.get_config_mut().avoid_stats = true; } } + + fn is_fallback_driver_from_cache(&self) -> bool { + self.get_config().fallback_driver_from_cache + } + + fn set_fallback_driver_from_cache(&mut self, fallback_driver_from_cache: bool) { + self.get_config_mut().fallback_driver_from_cache = fallback_driver_from_cache; + } } // ---------------------------------------------------------- diff --git a/rust/src/main.rs b/rust/src/main.rs index 4bf799e781396..537a0257ab152 100644 --- a/rust/src/main.rs +++ b/rust/src/main.rs @@ -243,25 +243,28 @@ fn main() { }) .unwrap_or_else(|err| { let log = selenium_manager.get_logger(); - if let Some(best_driver_from_cache) = - selenium_manager.find_best_driver_from_cache().unwrap() - { - log.debug_or_warn( - format!( - "There was an error managing {} ({}); using driver found in the cache", - selenium_manager.get_driver_name(), - err - ), - selenium_manager.is_offline(), - ); - log_driver_and_browser_path( - log, - &best_driver_from_cache, - &selenium_manager.get_browser_path_or_latest_from_cache(), - selenium_manager.get_receiver(), - ); - flush_and_exit(OK, log, Some(err)); - } else if selenium_manager.is_offline() { + if selenium_manager.is_fallback_driver_from_cache() { + if let Some(best_driver_from_cache) = + selenium_manager.find_best_driver_from_cache().unwrap() + { + log.debug_or_warn( + format!( + "There was an error managing {} ({}); using driver found in the cache", + selenium_manager.get_driver_name(), + err + ), + selenium_manager.is_offline(), + ); + log_driver_and_browser_path( + log, + &best_driver_from_cache, + &selenium_manager.get_browser_path_or_latest_from_cache(), + selenium_manager.get_receiver(), + ); + flush_and_exit(OK, log, Some(err)); + } + } + if selenium_manager.is_offline() { log.warn(&err); flush_and_exit(OK, log, Some(err)); } else { diff --git a/rust/tests/browser_tests.rs b/rust/tests/browser_tests.rs index 5254e3f0d277b..dd69101ee1fcf 100644 --- a/rust/tests/browser_tests.rs +++ b/rust/tests/browser_tests.rs @@ -151,3 +151,17 @@ fn browser_path_test(#[case] os: String, #[case] browser: String, #[case] browse assert!(!stdout.contains("WARN")); } } + +#[test] +fn invalid_browser_path_test() { + let mut cmd = get_selenium_manager(); + cmd.args([ + "--browser", + "chrome", + "--browser-path", + "/bad/path/google-chrome-wrong", + ]) + .assert() + .code(DATAERR) + .failure(); +} From 15d02632b15b55f53cfdf0bafb72075b10358d0b Mon Sep 17 00:00:00 2001 From: Boni Garcia Date: Mon, 12 Aug 2024 20:27:21 +0200 Subject: [PATCH 2/8] [rust] Remove test data with incorrect browser path in macOS --- rust/tests/browser_tests.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/rust/tests/browser_tests.rs b/rust/tests/browser_tests.rs index dd69101ee1fcf..71db49d0ece40 100644 --- a/rust/tests/browser_tests.rs +++ b/rust/tests/browser_tests.rs @@ -133,11 +133,6 @@ fn invalid_geckodriver_version_test() { "chrome", r"/Applications/Google\ Chrome.app/Contents/MacOS/Google\ Chrome" )] -#[case( - "macos", - "chrome", - r"/Applications/Google Chrome.app/Contents/MacOS/Google Chrome" -)] fn browser_path_test(#[case] os: String, #[case] browser: String, #[case] browser_path: String) { if OS.eq(&os) { let mut cmd = get_selenium_manager(); From 57370a8c9ebb5f5684d1f0efad970e19e933760d Mon Sep 17 00:00:00 2001 From: Boni Garcia Date: Mon, 12 Aug 2024 23:06:24 +0200 Subject: [PATCH 3/8] Revert "[rust] Remove test data with incorrect browser path in macOS" This reverts commit 79c22d63c9823c9b6ccc9c8a8145426c6978b45e. --- rust/tests/browser_tests.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/rust/tests/browser_tests.rs b/rust/tests/browser_tests.rs index 71db49d0ece40..dd69101ee1fcf 100644 --- a/rust/tests/browser_tests.rs +++ b/rust/tests/browser_tests.rs @@ -133,6 +133,11 @@ fn invalid_geckodriver_version_test() { "chrome", r"/Applications/Google\ Chrome.app/Contents/MacOS/Google\ Chrome" )] +#[case( + "macos", + "chrome", + r"/Applications/Google Chrome.app/Contents/MacOS/Google Chrome" +)] fn browser_path_test(#[case] os: String, #[case] browser: String, #[case] browser_path: String) { if OS.eq(&os) { let mut cmd = get_selenium_manager(); From a001ae5460d02f03f2601c63a9654ebf611d32ca Mon Sep 17 00:00:00 2001 From: Boni Garcia Date: Mon, 12 Aug 2024 23:31:36 +0200 Subject: [PATCH 4/8] [rust] Escape browser path before checking existence --- rust/src/lib.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/rust/src/lib.rs b/rust/src/lib.rs index 7d9bf60e5fbd3..c26022709dceb 100644 --- a/rust/src/lib.rs +++ b/rust/src/lib.rs @@ -1061,18 +1061,19 @@ pub trait SeleniumManager { cmd_version_arg: &str, ) -> Result, Error> { let mut browser_path = self.get_browser_path().to_string(); + let mut escaped_browser_path = self.get_escaped_path(&browser_path); if browser_path.is_empty() { if let Some(path) = self.detect_browser_path() { browser_path = path_to_string(&path); + escaped_browser_path = self.get_escaped_path(&browser_path); } - } else if !Path::new(&browser_path).exists() { + } else if !Path::new(&escaped_browser_path).exists() { self.set_fallback_driver_from_cache(false); return Err(anyhow!(format_one_arg( "Browser path does not exist: {}", &browser_path, ))); } - let escaped_browser_path = self.get_escaped_path(browser_path.to_string()); let mut commands = Vec::new(); if WINDOWS.is(self.get_os()) { @@ -1122,7 +1123,7 @@ pub trait SeleniumManager { if browser_path.is_empty() { match self.detect_browser_path() { Some(path) => { - browser_path = self.get_escaped_path(path_to_string(&path)); + browser_path = self.get_escaped_path(&path_to_string(&path)); } _ => return Ok(None), } @@ -1318,9 +1319,9 @@ pub trait SeleniumManager { canon_path } - fn get_escaped_path(&self, string_path: String) -> String { - let mut escaped_path = string_path.clone(); - let path = Path::new(&string_path); + fn get_escaped_path(&self, string_path: &str) -> String { + let mut escaped_path = string_path.to_string(); + let path = Path::new(string_path); if path.exists() { escaped_path = self.canonicalize_path(path.to_path_buf()); @@ -1331,7 +1332,7 @@ pub trait SeleniumManager { Command::new_single(format_one_arg(ESCAPE_COMMAND, escaped_path.as_str())); escaped_path = run_shell_command("bash", "-c", escape_command).unwrap_or_default(); if escaped_path.is_empty() { - escaped_path = string_path.clone(); + escaped_path = string_path.to_string(); } } } From 5ec697f4b9b174ef09dfeb9f51efb963809fa16c Mon Sep 17 00:00:00 2001 From: Boni Garcia Date: Tue, 13 Aug 2024 00:25:12 +0200 Subject: [PATCH 5/8] Revert "[rust] Escape browser path before checking existence" This reverts commit b876e2233ace321a41303a60cd1906aa071ed617. --- rust/src/lib.rs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/rust/src/lib.rs b/rust/src/lib.rs index c26022709dceb..7d9bf60e5fbd3 100644 --- a/rust/src/lib.rs +++ b/rust/src/lib.rs @@ -1061,19 +1061,18 @@ pub trait SeleniumManager { cmd_version_arg: &str, ) -> Result, Error> { let mut browser_path = self.get_browser_path().to_string(); - let mut escaped_browser_path = self.get_escaped_path(&browser_path); if browser_path.is_empty() { if let Some(path) = self.detect_browser_path() { browser_path = path_to_string(&path); - escaped_browser_path = self.get_escaped_path(&browser_path); } - } else if !Path::new(&escaped_browser_path).exists() { + } else if !Path::new(&browser_path).exists() { self.set_fallback_driver_from_cache(false); return Err(anyhow!(format_one_arg( "Browser path does not exist: {}", &browser_path, ))); } + let escaped_browser_path = self.get_escaped_path(browser_path.to_string()); let mut commands = Vec::new(); if WINDOWS.is(self.get_os()) { @@ -1123,7 +1122,7 @@ pub trait SeleniumManager { if browser_path.is_empty() { match self.detect_browser_path() { Some(path) => { - browser_path = self.get_escaped_path(&path_to_string(&path)); + browser_path = self.get_escaped_path(path_to_string(&path)); } _ => return Ok(None), } @@ -1319,9 +1318,9 @@ pub trait SeleniumManager { canon_path } - fn get_escaped_path(&self, string_path: &str) -> String { - let mut escaped_path = string_path.to_string(); - let path = Path::new(string_path); + fn get_escaped_path(&self, string_path: String) -> String { + let mut escaped_path = string_path.clone(); + let path = Path::new(&string_path); if path.exists() { escaped_path = self.canonicalize_path(path.to_path_buf()); @@ -1332,7 +1331,7 @@ pub trait SeleniumManager { Command::new_single(format_one_arg(ESCAPE_COMMAND, escaped_path.as_str())); escaped_path = run_shell_command("bash", "-c", escape_command).unwrap_or_default(); if escaped_path.is_empty() { - escaped_path = string_path.to_string(); + escaped_path = string_path.clone(); } } } From f3444e75463db7ba220afc2fc2b613a04c7ce310 Mon Sep 17 00:00:00 2001 From: Boni Garcia Date: Tue, 13 Aug 2024 00:27:58 +0200 Subject: [PATCH 6/8] [rust] Remove test data with incorrect browser path in macOS --- rust/tests/browser_tests.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/rust/tests/browser_tests.rs b/rust/tests/browser_tests.rs index dd69101ee1fcf..10fd138491029 100644 --- a/rust/tests/browser_tests.rs +++ b/rust/tests/browser_tests.rs @@ -128,11 +128,6 @@ fn invalid_geckodriver_version_test() { r"C:\Program Files\Google\Chrome\Application\chrome.exe" )] #[case("linux", "chrome", "/usr/bin/google-chrome")] -#[case( - "macos", - "chrome", - r"/Applications/Google\ Chrome.app/Contents/MacOS/Google\ Chrome" -)] #[case( "macos", "chrome", From a968a40e5afdd6d9de90cc190e2d2e380b5b9ee7 Mon Sep 17 00:00:00 2001 From: Boni Garcia Date: Tue, 13 Aug 2024 01:41:13 +0200 Subject: [PATCH 7/8] [rust] Force window-sys crate version in Windows --- rust/Cargo.Bazel.lock | 12 ++++++++++-- rust/Cargo.lock | 1 + rust/Cargo.toml | 3 +++ 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/rust/Cargo.Bazel.lock b/rust/Cargo.Bazel.lock index b3c69fc570b52..ed4bd24fd3f0b 100644 --- a/rust/Cargo.Bazel.lock +++ b/rust/Cargo.Bazel.lock @@ -1,5 +1,5 @@ { - "checksum": "4ef6c22ec363acd77306d3758c75001ec6e6164f8f964457ff9af16470c5cf13", + "checksum": "d3b4f76b61f6e1c21a4bd8b7905b2b82a1dee313ae0632e04a1c1a7a2e49d414", "crates": { "addr2line 0.21.0": { "name": "addr2line", @@ -11951,7 +11951,14 @@ "target": "zip" } ], - "selects": {} + "selects": { + "cfg(target_os = \"windows\")": [ + { + "id": "windows-sys 0.59.0", + "target": "windows_sys" + } + ] + } }, "deps_dev": { "common": [ @@ -19699,6 +19706,7 @@ "toml 0.8.19", "walkdir 2.5.0", "which 6.0.2", + "windows-sys 0.59.0", "zip 2.1.6" ], "direct_dev_deps": [ diff --git a/rust/Cargo.lock b/rust/Cargo.lock index 75a8c93872f40..9ced73f9b82a8 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -1892,6 +1892,7 @@ dependencies = [ "toml", "walkdir", "which", + "windows-sys 0.59.0", "zip", ] diff --git a/rust/Cargo.toml b/rust/Cargo.toml index 1182fb58fa4eb..282eb98ee46dc 100644 --- a/rust/Cargo.toml +++ b/rust/Cargo.toml @@ -36,6 +36,9 @@ anyhow = { version = "1.0.86", default-features = false, features = ["backtrace" apple-flat-package = "0.18.0" which = "6.0.2" +[target.'cfg(target_os = "windows")'.dependencies] +windows-sys = "0.59.0" + [dev-dependencies] assert_cmd = "2.0.16" rstest = "0.19.0" From 3b93e38460b35201c1397b029556951783fe76ef Mon Sep 17 00:00:00 2001 From: Boni Garcia Date: Tue, 13 Aug 2024 02:08:16 +0200 Subject: [PATCH 8/8] Revert "[rust] Force window-sys crate version in Windows" This reverts commit a968a40e5afdd6d9de90cc190e2d2e380b5b9ee7. --- rust/Cargo.Bazel.lock | 12 ++---------- rust/Cargo.lock | 1 - rust/Cargo.toml | 3 --- 3 files changed, 2 insertions(+), 14 deletions(-) diff --git a/rust/Cargo.Bazel.lock b/rust/Cargo.Bazel.lock index ed4bd24fd3f0b..b3c69fc570b52 100644 --- a/rust/Cargo.Bazel.lock +++ b/rust/Cargo.Bazel.lock @@ -1,5 +1,5 @@ { - "checksum": "d3b4f76b61f6e1c21a4bd8b7905b2b82a1dee313ae0632e04a1c1a7a2e49d414", + "checksum": "4ef6c22ec363acd77306d3758c75001ec6e6164f8f964457ff9af16470c5cf13", "crates": { "addr2line 0.21.0": { "name": "addr2line", @@ -11951,14 +11951,7 @@ "target": "zip" } ], - "selects": { - "cfg(target_os = \"windows\")": [ - { - "id": "windows-sys 0.59.0", - "target": "windows_sys" - } - ] - } + "selects": {} }, "deps_dev": { "common": [ @@ -19706,7 +19699,6 @@ "toml 0.8.19", "walkdir 2.5.0", "which 6.0.2", - "windows-sys 0.59.0", "zip 2.1.6" ], "direct_dev_deps": [ diff --git a/rust/Cargo.lock b/rust/Cargo.lock index 9ced73f9b82a8..75a8c93872f40 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -1892,7 +1892,6 @@ dependencies = [ "toml", "walkdir", "which", - "windows-sys 0.59.0", "zip", ] diff --git a/rust/Cargo.toml b/rust/Cargo.toml index 282eb98ee46dc..1182fb58fa4eb 100644 --- a/rust/Cargo.toml +++ b/rust/Cargo.toml @@ -36,9 +36,6 @@ anyhow = { version = "1.0.86", default-features = false, features = ["backtrace" apple-flat-package = "0.18.0" which = "6.0.2" -[target.'cfg(target_os = "windows")'.dependencies] -windows-sys = "0.59.0" - [dev-dependencies] assert_cmd = "2.0.16" rstest = "0.19.0"