Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions rust/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,31 @@ fn get_env_name(suffix: &str) -> String {
concat(ENV_PREFIX, suffix_uppercase.as_str())
}

#[cfg(test)]
mod env_name_tests {
use super::*;

#[test]
fn get_env_name_simple_key() {
assert_eq!(get_env_name("browser"), "SE_BROWSER");
}

#[test]
fn get_env_name_dashes_become_underscores() {
assert_eq!(get_env_name("browser-version"), "SE_BROWSER_VERSION");
}

#[test]
fn get_env_name_mixed_case_uppercased() {
assert_eq!(get_env_name("Cache-Path"), "SE_CACHE_PATH");
}

#[test]
fn get_env_name_empty_suffix() {
assert_eq!(get_env_name(""), "SE_");
}
}

fn get_config() -> Result<Table, Error> {
let cache_path = read_cache_path();
let config_path = Path::new(&cache_path).to_path_buf().join(CONFIG_FILE);
Expand Down
3 changes: 2 additions & 1 deletion rust/src/edge.rs
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,8 @@ impl SeleniumManager for EdgeManager {
read_version_from_link(self.get_http_client(), &driver_url, self.get_logger())?;

let driver_ttl = self.get_ttl();
if driver_ttl > 0 && !major_browser_version.is_empty() {
if driver_ttl > 0 && !major_browser_version.is_empty() && !driver_version.is_empty()
{
Comment on lines +256 to +257

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

2. String passed to helper 🐞 Bug ≡ Correctness

EdgeManager::request_driver_version passes major_browser_version (a String) into
should_cache_driver_version, which requires &str, causing a compile-time type mismatch and
breaking the build.
Agent Prompt
### Issue description
`should_cache_driver_version(driver_ttl, major_browser_version, &driver_version)` passes a `String` where the helper requires `&str`, which is a compile error in Rust.

### Issue Context
In `edge.rs`, `major_browser_version` is a `String` returned from `get_major_browser_version()`, and the helper signature is `(&str, &str)` for the version parameters.

### Fix Focus Areas
- rust/src/edge.rs[252-264]

### Suggested change
Update the call to explicitly pass a string slice, e.g.:
- `should_cache_driver_version(driver_ttl, major_browser_version.as_str(), &driver_version)`
  (or `should_cache_driver_version(driver_ttl, &major_browser_version, &driver_version)` relying on deref coercion)

Keep the rest of the logic unchanged.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

metadata.drivers.push(create_driver_metadata(
major_browser_version.as_str(),
self.driver_name,
Comment thread
qodo-code-review[bot] marked this conversation as resolved.
Outdated
Expand Down
3 changes: 2 additions & 1 deletion rust/src/firefox.rs
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,8 @@ impl SeleniumManager for FirefoxManager {
};

let driver_ttl = self.get_ttl();
if driver_ttl > 0 && !major_browser_version.is_empty() {
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,
Expand Down
37 changes: 37 additions & 0 deletions rust/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1972,3 +1972,40 @@ fn get_index_version(full_version: &str, index: usize) -> Result<String, Error>
.ok_or(anyhow!(format!("Wrong version: {}", full_version)))?
.to_string())
}

#[cfg(test)]
mod index_version_tests {
use super::*;

#[test]
fn get_index_version_major() {
assert_eq!(get_index_version("120.0.6099.109", 0).unwrap(), "120");
}

#[test]
fn get_index_version_minor() {
assert_eq!(get_index_version("120.0.6099.109", 1).unwrap(), "0");
}

#[test]
fn get_index_version_patch() {
assert_eq!(get_index_version("120.0.6099.109", 2).unwrap(), "6099");
}

#[test]
fn get_index_version_single_component() {
assert_eq!(get_index_version("115", 0).unwrap(), "115");
}

#[test]
fn get_index_version_out_of_bounds_errors() {
let result = get_index_version("115", 1);
assert!(result.is_err());
}

#[test]
fn get_index_version_empty_string_errors() {
let result = get_index_version("", 0);
assert!(result.is_err());
}
}
274 changes: 274 additions & 0 deletions rust/tests/cache_unit_tests.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,274 @@
// Licensed to the Software Freedom Conservancy (SFC) under one
// or more contributor license agreements. See the NOTICE file
// distributed with this work for additional information
// regarding copyright ownership. The SFC licenses this file
// to you under the Apache License, Version 2.0 (the
// "License"); you may not use this file except in compliance
// with the License. You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing,
// software is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR
// CONDITIONS OF ANY KIND, either express or implied. See the License
// for the specific language governing permissions and limitations
// under the License.

use selenium_manager::files::{collect_files_from_cache, find_latest_from_cache};
use selenium_manager::get_manager_by_browser;
use selenium_manager::SeleniumManager;

use rstest::rstest;
use std::fs;
use std::path::PathBuf;
use tempfile::tempdir;
use walkdir::WalkDir;

fn platform_label() -> (&'static str, &'static str, &'static str) {
if cfg!(windows) {
("win64", "windows", ".exe")
} else if cfg!(target_os = "macos") {
("mac-x64", "macos", "")
} else {
("linux64", "linux", "")
}
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in commit 015b4b3create_driver_in_cache now uses the manager's actual get_platform_label() and get_os() instead of hardcoded values, so tests work correctly on Apple Silicon (mac-arm64) and 32-bit Windows (win32).

Comment thread
qodo-code-review[bot] marked this conversation as resolved.
Outdated

fn create_driver_in_cache(
cache: &PathBuf,
driver_name: &str,
version: &str,
) {
let (platform, os_dir, ext) = platform_label();
let dir = cache
.join(driver_name)
.join(os_dir)
.join(platform)
.join(version);
fs::create_dir_all(&dir).unwrap();
fs::write(
dir.join(format!("{}{}", driver_name, ext)),
b"fake binary",
)
.unwrap();
}

fn create_driver_in_cache_custom(
cache: &PathBuf,
driver_name: &str,
os_label: &str,
arch_label: &str,
version: &str,
) {
let dir = cache
.join(driver_name)
.join(os_label)
.join(arch_label)
.join(version);
fs::create_dir_all(&dir).unwrap();
let ext = if os_label == "windows" { ".exe" } else { "" };
fs::write(
dir.join(format!("{}{}", driver_name, ext)),
b"fake binary",
)
.unwrap();
}

#[test]
fn find_latest_from_cache_returns_none_when_empty() {
let tmp = tempdir().unwrap();
let cache = tmp.path().to_path_buf();
let result = find_latest_from_cache(&cache, |_| true).unwrap();
assert!(result.is_none());
}

#[test]
fn find_latest_from_cache_returns_matching_file() {
let tmp = tempdir().unwrap();
let cache = tmp.path().to_path_buf();
create_driver_in_cache(&cache, "chromedriver", "120.0.6099.109");

let result = find_latest_from_cache(&cache, |entry| {
entry
.file_name()
.to_str()
.map(|s| s.contains("chromedriver"))
.unwrap_or(false)
})
.unwrap();

assert!(result.is_some());
let path = result.unwrap();
assert!(path.exists());
assert!(path.to_str().unwrap().contains("chromedriver"));
}

#[test]
fn find_latest_from_cache_returns_last_when_multiple_matches() {
let tmp = tempdir().unwrap();
let cache = tmp.path().to_path_buf();
create_driver_in_cache(&cache, "chromedriver", "115.0.5790.110");
create_driver_in_cache(&cache, "chromedriver", "120.0.6099.109");

let result = find_latest_from_cache(&cache, |entry| {
entry
.file_name()
.to_str()
.map(|s| s.contains("chromedriver"))
.unwrap_or(false)
})
.unwrap();

assert!(result.is_some());
let path = result.unwrap();
assert!(
path.to_str().unwrap().contains("120.0.6099.109"),
"expected last sorted entry to be 120.x, got: {:?}",
path
);
}

#[test]
fn find_latest_from_cache_lexical_sort_misorders_across_digit_boundaries() {
let tmp = tempdir().unwrap();
let cache = tmp.path().to_path_buf();
create_driver_in_cache_custom(&cache, "chromedriver", "linux", "x86_64", "9.0.0");
create_driver_in_cache_custom(&cache, "chromedriver", "linux", "x86_64", "10.0.0");

let result = find_latest_from_cache(&cache, |entry| {
entry
.file_name()
.to_str()
.map(|s| s.contains("chromedriver"))
.unwrap_or(false)
})
.unwrap()
.unwrap();

assert!(
result.to_str().unwrap().contains("9.0.0"),
"lexical sort puts '9' after '10'; this test documents the latent bug"
);
}

#[test]
fn collect_files_from_cache_returns_empty_when_no_match() {
let tmp = tempdir().unwrap();
let cache = tmp.path().to_path_buf();
create_driver_in_cache(&cache, "chromedriver", "120.0.6099.109");

let result = collect_files_from_cache(&cache, |_| false);
assert!(result.is_empty());
}

#[test]
fn collect_files_from_cache_returns_all_matches() {
let tmp = tempdir().unwrap();
let cache = tmp.path().to_path_buf();
create_driver_in_cache(&cache, "chromedriver", "115.0.5790.110");
create_driver_in_cache(&cache, "chromedriver", "120.0.6099.109");

let result = collect_files_from_cache(&cache, |entry| {
entry
.file_name()
.to_str()
.map(|s| s.contains("chromedriver"))
.unwrap_or(false)
});

assert_eq!(result.len(), 2);
}

#[test]
fn is_driver_and_matches_browser_version_matches_major() {
let tmp = tempdir().unwrap();
let cache = tmp.path().to_path_buf();
create_driver_in_cache(&cache, "chromedriver", "120.0.6099.109");

let mut manager = get_manager_by_browser("chrome".to_string()).unwrap();
manager.set_browser_version("120".to_string());
let entries: Vec<_> = WalkDir::new(&cache)
.into_iter()
.filter_map(|e| e.ok())
.filter(|e| e.file_type().is_file())
.collect();

let matching: Vec<_> = entries
.iter()
.filter(|e| manager.is_driver_and_matches_browser_version(e))
.collect();

assert_eq!(matching.len(), 1);
}

#[test]
fn is_driver_and_matches_browser_version_empty_major_matches_all() {
let tmp = tempdir().unwrap();
let cache = tmp.path().to_path_buf();
create_driver_in_cache(&cache, "chromedriver", "115.0.5790.110");
create_driver_in_cache(&cache, "chromedriver", "120.0.6099.109");

let manager = get_manager_by_browser("chrome".to_string()).unwrap();
let entries: Vec<_> = WalkDir::new(&cache)
.into_iter()
.filter_map(|e| e.ok())
.filter(|e| e.file_type().is_file())
.collect();

let matching: Vec<_> = entries
.iter()
.filter(|e| manager.is_driver_and_matches_browser_version(e))
.collect();

assert_eq!(
matching.len(),
2,
"starts_with(\"\") matches every cached driver — documents the latent bug"
);
}

#[test]
fn find_best_driver_from_cache_returns_matching_version() {
let tmp = tempdir().unwrap();
let cache = tmp.path().to_path_buf();
create_driver_in_cache(&cache, "chromedriver", "115.0.5790.110");
create_driver_in_cache(&cache, "chromedriver", "120.0.6099.109");

let mut manager = get_manager_by_browser("chrome".to_string()).unwrap();
manager.set_cache_path(cache.to_str().unwrap().to_string());
manager.set_browser_version("120".to_string());

let result = manager.find_best_driver_from_cache().unwrap();
assert!(result.is_some());
let path = result.unwrap();
assert!(path.to_str().unwrap().contains("120.0.6099.109"));
}

#[test]
fn find_best_driver_from_cache_falls_back_to_latest_when_no_match() {
let tmp = tempdir().unwrap();
let cache = tmp.path().to_path_buf();
create_driver_in_cache(&cache, "chromedriver", "115.0.5790.110");

let mut manager = get_manager_by_browser("chrome".to_string()).unwrap();
manager.set_cache_path(cache.to_str().unwrap().to_string());
manager.set_browser_version("130".to_string());

let result = manager.find_best_driver_from_cache().unwrap();
assert!(result.is_some());
let path = result.unwrap();
assert!(path.to_str().unwrap().contains("chromedriver"));
}

#[test]
fn find_best_driver_from_cache_returns_none_when_cache_empty() {
let tmp = tempdir().unwrap();
let cache = tmp.path().to_path_buf();

let mut manager = get_manager_by_browser("chrome".to_string()).unwrap();
manager.set_cache_path(cache.to_str().unwrap().to_string());
manager.set_browser_version("120".to_string());

let result = manager.find_best_driver_from_cache().unwrap();
assert!(result.is_none());
}
Loading
Loading