diff --git a/Cargo.lock b/Cargo.lock index d7f8b449..bfab925c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -884,6 +884,7 @@ dependencies = [ "itoa", "minijinja", "mockable", + "mockall", "rstest", "semver", "serde", diff --git a/Cargo.toml b/Cargo.toml index 048b393d..8c760e58 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -68,6 +68,7 @@ insta = { version = "1", features = ["yaml"] } assert_cmd = "2.0.17" mockable = { version = "0.3", features = ["mock"] } serial_test = "3" +mockall = "0.11" [[test]] name = "cucumber" diff --git a/tests/env_path_tests.rs b/tests/env_path_tests.rs new file mode 100644 index 00000000..6227164c --- /dev/null +++ b/tests/env_path_tests.rs @@ -0,0 +1,81 @@ +//! Tests for scoped manipulation of `PATH` via `prepend_dir_to_path` and +//! `PathGuard`. + +use mockable::Env; +use rstest::rstest; +use serial_test::serial; + +#[path = "support/env.rs"] +mod env; +mod support; +use env::{SystemEnv, mocked_path_env, prepend_dir_to_path}; +use support::env_lock::EnvLock; + +#[rstest] +#[serial] +fn prepend_dir_to_path_sets_and_restores() { + let env = mocked_path_env(); + let original = env.raw("PATH").expect("PATH should be set in mock"); + let dir = tempfile::tempdir().expect("temp dir"); + let guard = prepend_dir_to_path(&env, dir.path()); + let after = std::env::var("PATH").expect("path var"); + let first = std::env::split_paths(&after).next().expect("first path"); + assert_eq!(first, dir.path()); + drop(guard); + let restored = std::env::var("PATH").expect("path var"); + assert_eq!(restored, original); +} + +#[rstest] +#[serial] +fn prepend_dir_to_path_handles_empty_path() { + let original = std::env::var_os("PATH"); + { + let _lock = EnvLock::acquire(); + unsafe { std::env::set_var("PATH", "") }; + } + let env = SystemEnv::new(); + let dir = tempfile::tempdir().expect("temp dir"); + let guard = prepend_dir_to_path(&env, dir.path()); + let after = std::env::var_os("PATH").expect("path var"); + let paths = std::env::split_paths(&after) + .filter(|p| !p.as_os_str().is_empty()) + .collect::>(); + assert_eq!(paths, vec![dir.path().to_path_buf()]); + drop(guard); + assert_eq!(std::env::var_os("PATH"), Some(std::ffi::OsString::new())); + { + let _lock = EnvLock::acquire(); + if let Some(path) = original { + unsafe { std::env::set_var("PATH", path) }; + } else { + unsafe { std::env::remove_var("PATH") }; + } + } +} + +#[rstest] +#[serial] +fn prepend_dir_to_path_handles_missing_path() { + let original = std::env::var_os("PATH"); + { + let _lock = EnvLock::acquire(); + unsafe { std::env::remove_var("PATH") }; + } + let env = SystemEnv::new(); + let dir = tempfile::tempdir().expect("temp dir"); + let guard = prepend_dir_to_path(&env, dir.path()); + let after = std::env::var_os("PATH").expect("PATH should exist after prepend"); + let paths: Vec<_> = std::env::split_paths(&after).collect(); + assert_eq!(paths, vec![dir.path().to_path_buf()]); + drop(guard); + assert!(std::env::var_os("PATH").is_none()); + { + let _lock = EnvLock::acquire(); + if let Some(path) = original { + unsafe { std::env::set_var("PATH", path) }; + } else { + unsafe { std::env::remove_var("PATH") }; + } + } +} diff --git a/tests/path_guard_tests.rs b/tests/path_guard_tests.rs new file mode 100644 index 00000000..2f3008b2 --- /dev/null +++ b/tests/path_guard_tests.rs @@ -0,0 +1,42 @@ +//! Tests for PATH restoration behaviour using mock environments. +//! +//! Verifies that `PathGuard` restores `PATH` without mutating the real +//! process environment. + +#[path = "support/env_lock.rs"] +mod env_lock; +#[path = "support/path_guard.rs"] +mod path_guard; + +use mockall::{Sequence, mock}; +use path_guard::{Env, PathGuard}; +use std::ffi::OsStr; + +mock! { + pub Env {} + impl Env for Env { + unsafe fn set_var(&mut self, key: &str, val: &OsStr); + } +} + +#[test] +fn restores_path_without_touching_real_env() { + let mut env = MockEnv::new(); + let mut seq = Sequence::new(); + env.expect_set_var() + .withf(|k, v| k == "PATH" && v == OsStr::new("/tmp")) + .times(1) + .in_sequence(&mut seq) + .return_const(()); + env.expect_set_var() + .withf(|k, v| k == "PATH" && v == OsStr::new("/orig")) + .times(1) + .in_sequence(&mut seq) + .return_const(()); + { + let mut guard = PathGuard::with_env("/orig".into(), env); + unsafe { + guard.env_mut().set_var("PATH", OsStr::new("/tmp")); + } + } +} diff --git a/tests/runner_tests.rs b/tests/runner_tests.rs index abff32e9..50f54efd 100644 --- a/tests/runner_tests.rs +++ b/tests/runner_tests.rs @@ -6,49 +6,39 @@ use std::path::{Path, PathBuf}; #[path = "support/check_ninja.rs"] mod check_ninja; +#[path = "support/env.rs"] +mod env; mod support; -use support::env_lock::EnvLock; +use env::{SystemEnv, prepend_dir_to_path}; use support::path_guard::PathGuard; -/// Fixture: Put a fake `ninja` (that checks for a build file) on PATH. +/// Fixture: Put a fake `ninja` (that checks for a build file) on `PATH`. +/// +/// In Rust 2024 `std::env::set_var` is `unsafe` because it mutates +/// process-global state. `EnvLock` serialises the mutation and the returned +/// [`PathGuard`] restores the prior value, so the risk is confined to this test. /// /// Returns: (tempdir holding ninja, `ninja_path`, PATH guard) #[fixture] fn ninja_in_path() -> (tempfile::TempDir, PathBuf, PathGuard) { let (ninja_dir, ninja_path) = check_ninja::fake_ninja_check_build_file(); - - // Save PATH and prepend our fake ninja directory. - let original_path = std::env::var_os("PATH").unwrap_or_default(); - let mut paths: Vec<_> = std::env::split_paths(&original_path).collect(); - paths.insert(0, ninja_dir.path().to_path_buf()); - let new_path = std::env::join_paths(paths).expect("join paths"); - let _lock = EnvLock::acquire(); - // Nightly marks set_var unsafe. - unsafe { std::env::set_var("PATH", &new_path) }; - - let guard = PathGuard::new(original_path); + let env = SystemEnv::new(); + let guard = prepend_dir_to_path(&env, ninja_dir.path()); (ninja_dir, ninja_path, guard) } -/// Fixture: Put a fake `ninja` with a specific exit code on PATH. +/// Fixture: Put a fake `ninja` with a specific exit code on `PATH`. /// -/// The default exit code is 0, but can be customised via `#[with(...)]`. +/// The default exit code is 0 but may be customised via `#[with(...)]`. The +/// fixture uses `EnvLock` and [`PathGuard`] to tame the `unsafe` `set_var` call, +/// mirroring [`ninja_in_path`]. /// /// Returns: (tempdir holding ninja, `ninja_path`, PATH guard) #[fixture] fn ninja_with_exit_code(#[default(0)] exit_code: i32) -> (tempfile::TempDir, PathBuf, PathGuard) { let (ninja_dir, ninja_path) = support::fake_ninja(exit_code); - - // Save PATH and prepend our fake ninja directory. - let original_path = std::env::var_os("PATH").unwrap_or_default(); - let mut paths: Vec<_> = std::env::split_paths(&original_path).collect(); - paths.insert(0, ninja_dir.path().to_path_buf()); - let new_path = std::env::join_paths(paths).expect("join paths"); - let _lock = EnvLock::acquire(); - // Nightly marks set_var unsafe. - unsafe { std::env::set_var("PATH", &new_path) }; - - let guard = PathGuard::new(original_path); + let env = SystemEnv::new(); + let guard = prepend_dir_to_path(&env, ninja_dir.path()); (ninja_dir, ninja_path, guard) } diff --git a/tests/steps/process_steps.rs b/tests/steps/process_steps.rs index fe136db0..4e96c71e 100644 --- a/tests/steps/process_steps.rs +++ b/tests/steps/process_steps.rs @@ -1,13 +1,14 @@ //! Step definitions for Ninja process execution. -use crate::{CliWorld, check_ninja, env, support}; +use crate::{ + CliWorld, check_ninja, + env::{self, EnvMut}, + support, +}; use cucumber::{given, then, when}; -use mockable::Env; use netsuke::runner::{self, BuildTargets, NINJA_PROGRAM}; use std::fs; use std::path::{Path, PathBuf}; -use support::env_lock::EnvLock; -use support::path_guard::PathGuard; use tempfile::{NamedTempFile, TempDir}; /// Installs a test-specific ninja binary and updates the `PATH`. @@ -15,17 +16,8 @@ use tempfile::{NamedTempFile, TempDir}; clippy::needless_pass_by_value, reason = "helper owns path for simplicity" )] -fn install_test_ninja(env: &impl Env, world: &mut CliWorld, dir: TempDir, ninja_path: PathBuf) { - let original = env.raw("PATH").unwrap_or_default(); - let guard = PathGuard::new(original.clone().into()); - let new_path = format!("{}:{}", dir.path().display(), original); - // SAFETY: `std::env::set_var` is `unsafe` in Rust 2024 due to global state. - // `EnvLock` serialises mutations and `PathGuard` restores the prior value. - let _lock = EnvLock::acquire(); - unsafe { - std::env::set_var("PATH", &new_path); - } - +fn install_test_ninja(env: &impl EnvMut, world: &mut CliWorld, dir: TempDir, ninja_path: PathBuf) { + let guard = env::prepend_dir_to_path(env, dir.path()); world.path_guard = Some(guard); world.ninja = Some(ninja_path.to_string_lossy().into_owned()); world.temp = Some(dir); diff --git a/tests/support/env.rs b/tests/support/env.rs index 91e9e519..7686663f 100644 --- a/tests/support/env.rs +++ b/tests/support/env.rs @@ -3,9 +3,41 @@ //! Provides fixtures and utilities for managing `PATH` and writing minimal //! manifests. -use mockable::MockEnv; +use mockable::{DefaultEnv, Env, MockEnv}; use rstest::fixture; +use std::ffi::{OsStr, OsString}; use std::io::{self, Write}; +use std::path::Path; + +use crate::support::env_lock::EnvLock; +use crate::support::path_guard::PathGuard; + +/// Alias for the real process environment. +#[allow(dead_code, reason = "re-exported for tests")] +pub type SystemEnv = DefaultEnv; + +/// Environment trait with mutation capabilities. +pub trait EnvMut: Env { + /// Set `key` to `value` within the environment. + /// + /// # Safety + /// + /// Mutating global state is `unsafe` in Rust 2024. Callers must ensure the + /// operation is serialised and rolled back appropriately. + unsafe fn set_var(&self, key: &str, value: &OsStr); +} + +impl EnvMut for DefaultEnv { + unsafe fn set_var(&self, key: &str, value: &OsStr) { + unsafe { std::env::set_var(key, value) }; + } +} + +impl EnvMut for MockEnv { + unsafe fn set_var(&self, key: &str, value: &OsStr) { + unsafe { std::env::set_var(key, value) }; + } +} /// Fixture: capture the original `PATH` via a mocked environment. /// @@ -25,6 +57,7 @@ pub fn mocked_path_env() -> MockEnv { /// Write a minimal manifest to `file`. /// /// The manifest declares a single `hello` target that prints a greeting. +#[allow(dead_code, reason = "used in Cucumber tests")] pub fn write_manifest(file: &mut impl Write) -> io::Result<()> { writeln!( file, @@ -38,3 +71,24 @@ pub fn write_manifest(file: &mut impl Write) -> io::Result<()> { ), ) } + +/// Prepend `dir` to the real `PATH`, returning a guard that restores it. +/// +/// Mutating `PATH` is `unsafe` in Rust 2024 because it alters process globals. +/// `EnvLock` serialises access and `PathGuard` rolls back the change, keeping +/// the unsafety scoped to a single test. +#[allow(dead_code, reason = "used in runner tests")] +pub fn prepend_dir_to_path(env: &impl EnvMut, dir: &Path) -> PathGuard { + let original = env.raw("PATH").ok(); + let original_os = original.clone().map(OsString::from); + let mut paths: Vec<_> = original_os + .as_ref() + .map(|os| std::env::split_paths(os).collect()) + .unwrap_or_default(); + paths.insert(0, dir.to_path_buf()); + let new_path = std::env::join_paths(&paths).expect("Failed to join PATH entries"); + let _lock = EnvLock::acquire(); + // SAFETY: `EnvLock` serialises mutations and the guard restores on drop. + unsafe { env.set_var("PATH", &new_path) }; + PathGuard::new(original_os) +} diff --git a/tests/support/mod.rs b/tests/support/mod.rs index 0dae0142..402d3235 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -17,6 +17,7 @@ use tempfile::TempDir; /// Create a fake Ninja executable that exits with `exit_code`. /// /// Returns the temporary directory and the path to the executable. +#[allow(dead_code, reason = "used in other test crates")] pub fn fake_ninja(exit_code: i32) -> (TempDir, PathBuf) { let dir = TempDir::new().expect("temp dir"); let path = dir.path().join("ninja"); diff --git a/tests/support/path_guard.rs b/tests/support/path_guard.rs index 96c54dcd..662ad5c6 100644 --- a/tests/support/path_guard.rs +++ b/tests/support/path_guard.rs @@ -3,35 +3,88 @@ //! Provides a guard that resets the environment variable on drop so tests do //! not pollute global state. -use std::ffi::OsString; +use std::ffi::{OsStr, OsString}; use super::env_lock::EnvLock; +/// Environment abstraction for setting variables. +pub trait Env { + /// Set `key` to `val` within the environment. + /// + /// # Safety + /// + /// Mutating process globals is `unsafe` in Rust 2024. Callers must ensure + /// access is serialised and state is restored. + unsafe fn set_var(&mut self, key: &str, val: &OsStr); +} + +#[derive(Debug)] +pub struct StdEnv; + +impl Env for StdEnv { + unsafe fn set_var(&mut self, key: &str, val: &OsStr) { + unsafe { std::env::set_var(key, val) }; + } +} + +/// Original `PATH` state captured by `PathGuard`. +#[allow(dead_code, reason = "only some tests mutate PATH")] +#[derive(Debug)] +enum OriginalPath { + Unset, + Set(OsString), +} + /// Guard that restores `PATH` to its original value when dropped. /// /// This uses RAII to ensure the environment is reset even if a test panics. #[allow(dead_code, reason = "only some tests mutate PATH")] #[derive(Debug)] -pub struct PathGuard { - original_path: Option, +pub struct PathGuard { + original: Option, + env: E, } impl PathGuard { + /// Create a guard capturing the current `PATH` using the real environment. + /// + /// Returns a guard that restores the variable when dropped. + #[allow(dead_code, reason = "only some tests mutate PATH")] + pub fn new(original: Option) -> Self { + let state = original.map_or(OriginalPath::Unset, OriginalPath::Set); + Self { + original: Some(state), + env: StdEnv, + } + } +} + +impl PathGuard { + /// Create a guard that uses `env` to restore `PATH`. #[allow(dead_code, reason = "only some tests mutate PATH")] - /// Create a guard capturing the current `PATH`. - pub fn new(original: OsString) -> Self { + pub fn with_env(original: OsString, env: E) -> Self { Self { - original_path: Some(original), + original: Some(OriginalPath::Set(original)), + env, } } + + /// Access the underlying environment. + #[allow(dead_code, reason = "only some tests mutate PATH")] + pub fn env_mut(&mut self) -> &mut E { + &mut self.env + } } -impl Drop for PathGuard { +impl Drop for PathGuard { fn drop(&mut self) { let _lock = EnvLock::acquire(); - if let Some(path) = self.original_path.take() { - // Nightly marks `set_var` unsafe; restoring `PATH` cleans up global state. - unsafe { std::env::set_var("PATH", path) }; + match self.original.take() { + Some(OriginalPath::Set(path)) => { + // Nightly marks `set_var` unsafe; restoring cleans up global state. + unsafe { self.env.set_var("PATH", &path) }; + } + Some(OriginalPath::Unset) | None => unsafe { std::env::remove_var("PATH") }, } } }