From c2a603f05aef0a0e331d7c7e059eb210bf3904b0 Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 12 Aug 2025 01:58:15 +0100 Subject: [PATCH 1/7] Test PATH guard helper --- tests/env_path_tests.rs | 25 ++++++++++++++++++++++++ tests/runner_tests.rs | 43 ++++++++++++++++------------------------- tests/support/env.rs | 26 ++++++++++++++++++++++++- tests/support/mod.rs | 1 + 4 files changed, 68 insertions(+), 27 deletions(-) create mode 100644 tests/env_path_tests.rs diff --git a/tests/env_path_tests.rs b/tests/env_path_tests.rs new file mode 100644 index 00000000..5aa1dac0 --- /dev/null +++ b/tests/env_path_tests.rs @@ -0,0 +1,25 @@ +use mockable::Env; +use rstest::rstest; +use serial_test::serial; + +#[path = "support/env.rs"] +mod env; +mod support; +use env::{mocked_path_env, prepend_dir_to_path}; + +#[rstest] +#[serial] +fn prepend_dir_to_path_sets_and_restores() { + let env = mocked_path_env(); + let original = env.raw("PATH").expect("PATH missing 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(&std::ffi::OsString::from(&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); +} diff --git a/tests/runner_tests.rs b/tests/runner_tests.rs index abff32e9..5569f678 100644 --- a/tests/runner_tests.rs +++ b/tests/runner_tests.rs @@ -1,3 +1,4 @@ +use mockable::DefaultEnv; use netsuke::cli::{BuildArgs, Cli, Commands}; use netsuke::runner::{BuildTargets, NINJA_ENV, run, run_ninja}; use rstest::{fixture, rstest}; @@ -6,49 +7,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::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 = DefaultEnv::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 = DefaultEnv::new(); + let guard = prepend_dir_to_path(&env, ninja_dir.path()); (ninja_dir, ninja_path, guard) } diff --git a/tests/support/env.rs b/tests/support/env.rs index 91e9e519..bd8ea7be 100644 --- a/tests/support/env.rs +++ b/tests/support/env.rs @@ -3,9 +3,14 @@ //! Provides fixtures and utilities for managing `PATH` and writing minimal //! manifests. -use mockable::MockEnv; +use mockable::{Env, MockEnv}; use rstest::fixture; +use std::ffi::OsString; use std::io::{self, Write}; +use std::path::Path; + +use crate::support::env_lock::EnvLock; +use crate::support::path_guard::PathGuard; /// Fixture: capture the original `PATH` via a mocked environment. /// @@ -25,6 +30,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 +44,21 @@ pub fn write_manifest(file: &mut impl Write) -> io::Result<()> { ), ) } + +/// Prepend `dir` to the real `PATH`, returning a guard that restores it. +/// +/// `std::env::set_var` is `unsafe` in Rust 2024 because it mutates 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 Env, dir: &Path) -> PathGuard { + let original = env.raw("PATH").unwrap_or_default(); + let original_os: OsString = original.clone().into(); + let mut paths: Vec<_> = std::env::split_paths(&original_os).collect(); + paths.insert(0, dir.to_path_buf()); + let new_path = std::env::join_paths(paths).expect("join paths"); + let _lock = EnvLock::acquire(); + // SAFETY: protected by `EnvLock` and reverted by the returned `PathGuard`. + unsafe { std::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"); From b8fbb4d908b0b68d8890b6dd83db1d23ded7471d Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 12 Aug 2025 10:14:34 +0100 Subject: [PATCH 2/7] Clarify env fixtures and test empty PATH --- tests/env_path_tests.rs | 32 ++++++++++++++++++++++++++++++-- tests/runner_tests.rs | 6 +++--- tests/support/env.rs | 3 ++- 3 files changed, 35 insertions(+), 6 deletions(-) diff --git a/tests/env_path_tests.rs b/tests/env_path_tests.rs index 5aa1dac0..36ace748 100644 --- a/tests/env_path_tests.rs +++ b/tests/env_path_tests.rs @@ -1,4 +1,4 @@ -use mockable::Env; +use mockable::{DefaultEnv as SystemEnv, Env}; use rstest::rstest; use serial_test::serial; @@ -6,12 +6,13 @@ use serial_test::serial; mod env; mod support; use env::{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 missing in mock"); + 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"); @@ -23,3 +24,30 @@ fn prepend_dir_to_path_sets_and_restores() { 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); + { + 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/runner_tests.rs b/tests/runner_tests.rs index 5569f678..b4f571a0 100644 --- a/tests/runner_tests.rs +++ b/tests/runner_tests.rs @@ -1,4 +1,4 @@ -use mockable::DefaultEnv; +use mockable::DefaultEnv as SystemEnv; use netsuke::cli::{BuildArgs, Cli, Commands}; use netsuke::runner::{BuildTargets, NINJA_ENV, run, run_ninja}; use rstest::{fixture, rstest}; @@ -23,7 +23,7 @@ use support::path_guard::PathGuard; #[fixture] fn ninja_in_path() -> (tempfile::TempDir, PathBuf, PathGuard) { let (ninja_dir, ninja_path) = check_ninja::fake_ninja_check_build_file(); - let env = DefaultEnv::new(); + let env = SystemEnv::new(); let guard = prepend_dir_to_path(&env, ninja_dir.path()); (ninja_dir, ninja_path, guard) } @@ -38,7 +38,7 @@ fn ninja_in_path() -> (tempfile::TempDir, PathBuf, PathGuard) { #[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); - let env = DefaultEnv::new(); + let env = SystemEnv::new(); let guard = prepend_dir_to_path(&env, ninja_dir.path()); (ninja_dir, ninja_path, guard) } diff --git a/tests/support/env.rs b/tests/support/env.rs index bd8ea7be..06dd02ef 100644 --- a/tests/support/env.rs +++ b/tests/support/env.rs @@ -58,7 +58,8 @@ pub fn prepend_dir_to_path(env: &impl Env, dir: &Path) -> PathGuard { paths.insert(0, dir.to_path_buf()); let new_path = std::env::join_paths(paths).expect("join paths"); let _lock = EnvLock::acquire(); - // SAFETY: protected by `EnvLock` and reverted by the returned `PathGuard`. + // Mockable's `Env` trait cannot mutate variables, so call directly. + // SAFETY: `EnvLock` serialises mutations and the guard restores on drop. unsafe { std::env::set_var("PATH", &new_path) }; PathGuard::new(original_os) } From 71ddea44cd3acda1e6e55bd4f02dae20f143584b Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 12 Aug 2025 21:05:30 +0100 Subject: [PATCH 3/7] Handle missing PATH in helper --- tests/env_path_tests.rs | 27 +++++++++++++++++++++++++++ tests/steps/process_steps.rs | 9 ++++++--- tests/support/env.rs | 11 +++++++---- tests/support/path_guard.rs | 23 +++++++++++++++++------ 4 files changed, 57 insertions(+), 13 deletions(-) diff --git a/tests/env_path_tests.rs b/tests/env_path_tests.rs index 36ace748..d4bdd348 100644 --- a/tests/env_path_tests.rs +++ b/tests/env_path_tests.rs @@ -42,6 +42,33 @@ fn prepend_dir_to_path_handles_empty_path() { .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 { diff --git a/tests/steps/process_steps.rs b/tests/steps/process_steps.rs index fe136db0..f0386e5d 100644 --- a/tests/steps/process_steps.rs +++ b/tests/steps/process_steps.rs @@ -16,9 +16,12 @@ use tempfile::{NamedTempFile, TempDir}; 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); + let original = env.raw("PATH").ok(); + let guard = PathGuard::new(original.clone().map(Into::into)); + let new_path = original.as_ref().map_or_else( + || dir.path().display().to_string(), + |orig| format!("{}:{}", dir.path().display(), orig), + ); // 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(); diff --git a/tests/support/env.rs b/tests/support/env.rs index 06dd02ef..b7db3699 100644 --- a/tests/support/env.rs +++ b/tests/support/env.rs @@ -52,11 +52,14 @@ pub fn write_manifest(file: &mut impl Write) -> io::Result<()> { /// keeping the unsafety scoped to a single test. #[allow(dead_code, reason = "used in runner tests")] pub fn prepend_dir_to_path(env: &impl Env, dir: &Path) -> PathGuard { - let original = env.raw("PATH").unwrap_or_default(); - let original_os: OsString = original.clone().into(); - let mut paths: Vec<_> = std::env::split_paths(&original_os).collect(); + let original = env.raw("PATH").ok(); + let original_os = original.clone().map(OsString::from); + let mut paths: Vec<_> = original_os + .clone() + .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("join paths"); + let new_path = std::env::join_paths(&paths).expect("failed to join PATH entries"); let _lock = EnvLock::acquire(); // Mockable's `Env` trait cannot mutate variables, so call directly. // SAFETY: `EnvLock` serialises mutations and the guard restores on drop. diff --git a/tests/support/path_guard.rs b/tests/support/path_guard.rs index 96c54dcd..2c026e6c 100644 --- a/tests/support/path_guard.rs +++ b/tests/support/path_guard.rs @@ -7,21 +7,29 @@ use std::ffi::OsString; use super::env_lock::EnvLock; +#[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, + original: Option, } impl PathGuard { #[allow(dead_code, reason = "only some tests mutate PATH")] /// Create a guard capturing the current `PATH`. - pub fn new(original: OsString) -> Self { + pub fn new(original: Option) -> Self { + let state = original.map_or(OriginalPath::Unset, OriginalPath::Set); Self { - original_path: Some(original), + original: Some(state), } } } @@ -29,9 +37,12 @@ impl 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 { std::env::set_var("PATH", path) }; + } + Some(OriginalPath::Unset) | None => unsafe { std::env::remove_var("PATH") }, } } } From 768ca4672a0598ebc8a588db60dcc102baf40d89 Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 12 Aug 2025 21:48:19 +0100 Subject: [PATCH 4/7] Refactor PATH helpers to use mutable env trait --- tests/env_path_tests.rs | 7 ++++-- tests/runner_tests.rs | 3 +-- tests/steps/process_steps.rs | 25 ++++++------------- tests/support/env.rs | 48 +++++++++++++++++++++++++++--------- tests/support/path_guard.rs | 3 ++- 5 files changed, 52 insertions(+), 34 deletions(-) diff --git a/tests/env_path_tests.rs b/tests/env_path_tests.rs index d4bdd348..d8928f09 100644 --- a/tests/env_path_tests.rs +++ b/tests/env_path_tests.rs @@ -1,11 +1,14 @@ -use mockable::{DefaultEnv as SystemEnv, Env}; +//! 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::{mocked_path_env, prepend_dir_to_path}; +use env::{SystemEnv, mocked_path_env, prepend_dir_to_path}; use support::env_lock::EnvLock; #[rstest] diff --git a/tests/runner_tests.rs b/tests/runner_tests.rs index b4f571a0..50f54efd 100644 --- a/tests/runner_tests.rs +++ b/tests/runner_tests.rs @@ -1,4 +1,3 @@ -use mockable::DefaultEnv as SystemEnv; use netsuke::cli::{BuildArgs, Cli, Commands}; use netsuke::runner::{BuildTargets, NINJA_ENV, run, run_ninja}; use rstest::{fixture, rstest}; @@ -10,7 +9,7 @@ mod check_ninja; #[path = "support/env.rs"] mod env; mod support; -use env::prepend_dir_to_path; +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`. diff --git a/tests/steps/process_steps.rs b/tests/steps/process_steps.rs index f0386e5d..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,20 +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").ok(); - let guard = PathGuard::new(original.clone().map(Into::into)); - let new_path = original.as_ref().map_or_else( - || dir.path().display().to_string(), - |orig| format!("{}:{}", dir.path().display(), orig), - ); - // 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 b7db3699..7686663f 100644 --- a/tests/support/env.rs +++ b/tests/support/env.rs @@ -3,15 +3,42 @@ //! Provides fixtures and utilities for managing `PATH` and writing minimal //! manifests. -use mockable::{Env, MockEnv}; +use mockable::{DefaultEnv, Env, MockEnv}; use rstest::fixture; -use std::ffi::OsString; +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. /// /// Returns a `MockEnv` that yields the current `PATH` when queried. Tests can @@ -47,22 +74,21 @@ pub fn write_manifest(file: &mut impl Write) -> io::Result<()> { /// Prepend `dir` to the real `PATH`, returning a guard that restores it. /// -/// `std::env::set_var` is `unsafe` in Rust 2024 because it mutates process -/// globals. `EnvLock` serialises access and `PathGuard` rolls back the change, -/// keeping the unsafety scoped to a single test. +/// 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 Env, dir: &Path) -> PathGuard { +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 - .clone() - .map(|os| std::env::split_paths(&os).collect()) + .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 new_path = std::env::join_paths(&paths).expect("Failed to join PATH entries"); let _lock = EnvLock::acquire(); - // Mockable's `Env` trait cannot mutate variables, so call directly. // SAFETY: `EnvLock` serialises mutations and the guard restores on drop. - unsafe { std::env::set_var("PATH", &new_path) }; + unsafe { env.set_var("PATH", &new_path) }; PathGuard::new(original_os) } diff --git a/tests/support/path_guard.rs b/tests/support/path_guard.rs index 2c026e6c..eb631314 100644 --- a/tests/support/path_guard.rs +++ b/tests/support/path_guard.rs @@ -7,6 +7,7 @@ use std::ffi::OsString; use super::env_lock::EnvLock; +/// Original `PATH` state captured by `PathGuard`. #[allow(dead_code, reason = "only some tests mutate PATH")] #[derive(Debug)] enum OriginalPath { @@ -24,8 +25,8 @@ pub struct PathGuard { } impl PathGuard { - #[allow(dead_code, reason = "only some tests mutate PATH")] /// Create a guard capturing the current `PATH`. + #[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 { From 604e0ef1996de74517da939d637f45f9c71223e2 Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 12 Aug 2025 22:43:08 +0100 Subject: [PATCH 5/7] Clarify path guard usage Explain restoration behaviour in PathGuard::new and streamline path splitting in tests. --- tests/env_path_tests.rs | 4 +--- tests/support/path_guard.rs | 2 ++ 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/env_path_tests.rs b/tests/env_path_tests.rs index d8928f09..6227164c 100644 --- a/tests/env_path_tests.rs +++ b/tests/env_path_tests.rs @@ -19,9 +19,7 @@ fn prepend_dir_to_path_sets_and_restores() { 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(&std::ffi::OsString::from(&after)) - .next() - .expect("first path"); + 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"); diff --git a/tests/support/path_guard.rs b/tests/support/path_guard.rs index eb631314..e9970503 100644 --- a/tests/support/path_guard.rs +++ b/tests/support/path_guard.rs @@ -26,6 +26,8 @@ pub struct PathGuard { impl PathGuard { /// Create a guard capturing the current `PATH`. + /// + /// 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); From a7cb79acd5cc45d236a558e31821186962002f9c Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 12 Aug 2025 22:43:15 +0100 Subject: [PATCH 6/7] Allow PathGuard to use injected env --- Cargo.lock | 1 + Cargo.toml | 1 + tests/path_guard_tests.rs | 42 ++++++++++++++++++++++++++++++++ tests/support/path_guard.rs | 48 ++++++++++++++++++++++++++++++++----- 4 files changed, 86 insertions(+), 6 deletions(-) create mode 100644 tests/path_guard_tests.rs 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/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/support/path_guard.rs b/tests/support/path_guard.rs index e9970503..ed09b10e 100644 --- a/tests/support/path_guard.rs +++ b/tests/support/path_guard.rs @@ -3,10 +3,30 @@ //! 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)] @@ -20,30 +40,46 @@ enum OriginalPath { /// 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 { +pub struct PathGuard { original: Option, + env: E, } impl PathGuard { - /// Create a guard capturing the current `PATH`. + /// 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")] + pub fn with_env(original: OsString, env: E) -> Self { Self { - original: Some(state), + 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(); match self.original.take() { Some(OriginalPath::Set(path)) => { // Nightly marks `set_var` unsafe; restoring cleans up global state. - unsafe { std::env::set_var("PATH", path) }; + unsafe { self.env.set_var("PATH", &path) }; } Some(OriginalPath::Unset) | None => unsafe { std::env::remove_var("PATH") }, } From fde9d5fa7b0c57ff782c3e290c1d9dd74babc9c2 Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Tue, 12 Aug 2025 22:53:46 +0100 Subject: [PATCH 7/7] Apply formatting --- tests/support/path_guard.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/support/path_guard.rs b/tests/support/path_guard.rs index ed09b10e..662ad5c6 100644 --- a/tests/support/path_guard.rs +++ b/tests/support/path_guard.rs @@ -52,7 +52,10 @@ impl PathGuard { #[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 } + Self { + original: Some(state), + env: StdEnv, + } } }