refactor: add which method to Worker trait

Before this change, the `which` crate was used
inside the experiments themselves to lookup the
location of binaries in the $PATH of the target
system.

This worked ok, but left the test suite susceptible
to the $PATH of the machine running the tests.

In this commit, a `which` method is added to the
`Worker` trait. In the real-world `System` impl,
this is a simple call again to the `which` crate.

In the `MockSystem`, provision has been added to
indicate that certain mocked files should be
returned when the `MockSystem::which` function is
called.

Author: jnsgruk

src/experiments/sudors.rs

@@ -2,7 +2,6 @@ use crate::utils::Worker;
 use anyhow::Result;
 use std::path::{Path, PathBuf};
 use tracing::info;
-use which::which;
 
 const PACKAGE: &str = "sudo-rs";
 const FIRST_SUPPORTED_RELEASE: &str = "24.04";
@@ -49,8 +48,8 @@ impl<'a> SudoRsExperiment<'a> {
         self.system.install_package(PACKAGE)?;
 
         for f in Self::sudors_files() {
-            let filename = f.file_name().unwrap();
-            let existing = match which(filename) {
+            let filename = f.file_name().unwrap().to_str().unwrap();
+            let existing = match self.system.which(filename) {
                 Ok(path) => path,
                 Err(_) => Path::new("/usr/bin").join(filename),
             };
@@ -63,8 +62,8 @@ impl<'a> SudoRsExperiment<'a> {
     /// Disable the experiment by removing the package and restoring the original files.
     pub fn disable(&self) -> Result<()> {
         for f in Self::sudors_files() {
-            let filename = f.file_name().unwrap();
-            let existing = match which(filename) {
+            let filename = f.file_name().unwrap().to_str().unwrap();
+            let existing = match self.system.which(filename) {
                 Ok(path) => path,
                 Err(_) => Path::new("/usr/bin").join(filename),
             };
@@ -168,12 +167,12 @@ mod tests {
     fn sudors_compatible_runner() -> MockSystem {
         let runner = MockSystem::default();
         runner.mock_files(vec![
-            ("/usr/lib/cargo/bin/sudo", ""),
-            ("/usr/lib/cargo/bin/su", ""),
-            ("/usr/lib/cargo/bin/visudo", ""),
-            ("/usr/bin/sudo", ""),
-            ("/usr/bin/su", ""),
-            ("/usr/sbin/visudo", ""),
+            ("/usr/lib/cargo/bin/sudo", "", false),
+            ("/usr/lib/cargo/bin/su", "", false),
+            ("/usr/lib/cargo/bin/visudo", "", false),
+            ("/usr/bin/sudo", "", true),
+            ("/usr/bin/su", "", true),
+            ("/usr/sbin/visudo", "", true),
         ]);
         runner
     }

src/experiments/uutils.rs

@@ -2,7 +2,6 @@ use crate::utils::Worker;
 use anyhow::Result;
 use std::path::{Path, PathBuf};
 use tracing::info;
-use which::which;
 
 /// An experiment to install and configure a Rust-based replacement for a system utility.
 pub struct UutilsExperiment<'a> {
@@ -66,8 +65,8 @@ impl<'a> UutilsExperiment<'a> {
         let files = self.system.list_files(self.bin_directory.clone())?;
 
         for f in files {
-            let filename = f.file_name().unwrap();
-            let existing = match which(filename) {
+            let filename = f.file_name().unwrap().to_str().unwrap();
+            let existing = match self.system.which(filename) {
                 Ok(path) => path,
                 Err(_) => Path::new("/usr/bin").join(filename),
             };
@@ -88,8 +87,8 @@ impl<'a> UutilsExperiment<'a> {
         let files = self.system.list_files(self.bin_directory.clone())?;
 
         for f in files {
-            let filename = f.file_name().unwrap();
-            let existing = match which(filename) {
+            let filename = f.file_name().unwrap().to_str().unwrap();
+            let existing = match self.system.which(filename) {
                 Ok(path) => path,
                 Err(_) => Path::new("/usr/bin").join(filename),
             };
@@ -209,10 +208,10 @@ mod tests {
     fn coreutils_compatible_runner() -> MockSystem {
         let runner = MockSystem::default();
         runner.mock_files(vec![
-            ("/usr/lib/cargo/bin/coreutils/date", ""),
-            ("/usr/lib/cargo/bin/coreutils/sort", ""),
-            ("/usr/bin/sort", ""),
-            ("/usr/bin/date", ""),
+            ("/usr/lib/cargo/bin/coreutils/date", "", false),
+            ("/usr/lib/cargo/bin/coreutils/sort", "", false),
+            ("/usr/bin/sort", "", true),
+            ("/usr/bin/date", "", true),
         ]);
         runner
     }
@@ -231,10 +230,10 @@ mod tests {
     fn findutils_compatible_runner() -> MockSystem {
         let runner = MockSystem::default();
         runner.mock_files(vec![
-            ("/usr/lib/cargo/bin/findutils/find", ""),
-            ("/usr/lib/cargo/bin/findutils/xargs", ""),
-            ("/usr/bin/find", ""),
-            ("/usr/bin/xargs", ""),
+            ("/usr/lib/cargo/bin/findutils/find", "", false),
+            ("/usr/lib/cargo/bin/findutils/xargs", "", false),
+            ("/usr/bin/find", "", true),
+            ("/usr/bin/xargs", "", true),
         ]);
         runner
     }

src/main.rs

@@ -125,7 +125,7 @@ fn main() -> Result<()> {
 }
 
 /// Enables selected experiments
-fn enable<'a>(system: &'a impl Worker, experiments: Vec<Experiment>, yes: bool) -> Result<()> {
+fn enable(system: &impl Worker, experiments: Vec<Experiment>, yes: bool) -> Result<()> {
     confirm_or_exit(yes);
 
     info!("Updating apt package cache");
@@ -147,17 +147,17 @@ fn disable(experiments: Vec<Experiment<'_>>, yes: bool) -> Result<()> {
 }
 
 /// Get selected experiments from the command line arguments.
-fn selected_experiments<'a>(
+fn selected_experiments(
     all: bool,
     selected: Vec<String>,
-    system: &'a impl Worker,
-) -> Vec<Experiment<'a>> {
+    system: &impl Worker,
+) -> Vec<Experiment<'_>> {
     let all_experiments = all_experiments(system);
     let default_experiments = default_experiments();
 
     match all {
         true => {
-            if selected.len() > 0 && !vecs_eq(selected, default_experiments) {
+            if !selected.is_empty() && !vecs_eq(selected, default_experiments) {
                 warn!("Ignoring --experiments flag as --all is set");
             }
 

src/utils/worker.rs

@@ -6,6 +6,7 @@ use std::{
 use anyhow::Result;
 use std::fs;
 use tracing::{debug, trace, warn};
+use which::which;
 
 use super::{Command, Distribution};
 
@@ -30,6 +31,9 @@ pub trait Worker {
     /// List files in a directory, returning an error if the directory does not exist.
     fn list_files(&self, directory: PathBuf) -> Result<Vec<PathBuf>>;
 
+    /// Find the path to a binary in the system's PATH.
+    fn which(&self, binary_name: &str) -> Result<PathBuf>;
+
     /// Install a package using the system package manager.
     fn install_package(&self, package: &str) -> Result<()> {
         let cmd = Command::build("apt-get", &["install", "-y", package]);
@@ -122,6 +126,11 @@ impl Worker for System {
         Ok(files)
     }
 
+    /// Find the path to a binary in the system's PATH.
+    fn which(&self, binary_name: &str) -> Result<PathBuf> {
+        Ok(which(binary_name)?)
+    }
+
     /// Replace a file with a symlink. If the target file already exists, it will be backed up
     /// before being replaced.
     fn replace_file_with_symlink(&self, source: PathBuf, target: PathBuf) -> Result<()> {

src/utils/worker_mock.rs

@@ -9,8 +9,10 @@ pub mod tests {
     pub struct MockSystem {
         /// Tracks the commands executed by the Worker
         pub commands: RefCell<Vec<String>>,
-        /// Mock files that the Worker's file-related methods can see/act upon
-        pub files: RefCell<HashMap<PathBuf, String>>,
+        /// Mock files that the Worker's file-related methods can see/act upon.
+        /// The String is the contents, and the bool indicates if this file takes
+        /// priority in the simulated $PATH, and returned by the `MockSystem::which` function.
+        pub files: RefCell<HashMap<PathBuf, (String, bool)>>,
         /// A list of packages that should report as "installed" on the mock system
         pub installed_packages: RefCell<Vec<String>>,
         /// List of symlinks created by the worker
@@ -46,14 +48,14 @@ pub mod tests {
 
             s.mock_command("lsb_release -is", distribution.id.as_str());
             s.mock_command("lsb_release -rs", distribution.release.as_str());
-            return s;
+            s
         }
 
-        pub fn mock_files(&self, files: Vec<(&str, &str)>) {
-            for (path, contents) in files {
+        pub fn mock_files(&self, files: Vec<(&str, &str, bool)>) {
+            for (path, contents, primary) in files {
                 self.files
                     .borrow_mut()
-                    .insert(PathBuf::from(path), contents.to_string());
+                    .insert(PathBuf::from(path), (contents.to_string(), primary));
             }
         }
 
@@ -102,6 +104,15 @@ pub mod tests {
             Ok(files)
         }
 
+        fn which(&self, binary_name: &str) -> Result<PathBuf> {
+            for (filename, file) in self.files.borrow().iter() {
+                if filename.file_name().unwrap().to_str().unwrap() == binary_name && file.1 {
+                    return Ok(filename.clone());
+                }
+            }
+            anyhow::bail!("{} not found in mocked filesystem", binary_name);
+        }
+
         fn replace_file_with_symlink(&self, source: PathBuf, target: PathBuf) -> Result<()> {
             if self.files.borrow().contains_key(&target) {
                 self.backup_file(target.clone())?;