From 8b4e42801ffdea28f93d1e2acdc06ff5c8622dfa Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 12 Aug 2025 01:58:44 +0100 Subject: [PATCH 1/4] Add unit test for PathGuard env restoration --- Cargo.lock | 1 + Cargo.toml | 1 + tests/path_guard_tests.rs | 34 +++++++++++++++++++++ tests/support/mod.rs | 1 + tests/support/path_guard.rs | 60 ++++++++++++++++++++++++++++++++----- 5 files changed, 89 insertions(+), 8 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..924b1c79 --- /dev/null +++ b/tests/path_guard_tests.rs @@ -0,0 +1,34 @@ +mod support; + +use mockall::{Sequence, mock}; +use std::ffi::OsStr; +use support::path_guard::{Env, PathGuard}; + +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/mod.rs b/tests/support/mod.rs index 0dae0142..272ee75d 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 PATH tests")] 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..4bb68bf1 100644 --- a/tests/support/path_guard.rs +++ b/tests/support/path_guard.rs @@ -3,35 +3,79 @@ //! 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 interface allowing `PATH` mutation. +pub trait Env { + /// Set an environment variable. + /// + /// # Safety + /// + /// Mutating process-wide state is `unsafe` in Rust 2024. + unsafe fn set_var(&mut self, key: &str, val: &OsStr); +} + +/// Real environment implementation. +#[derive(Debug, Default)] +pub struct RealEnv; + +impl Env for RealEnv { + #[allow(unsafe_op_in_unsafe_fn, reason = "delegates to std::env")] + unsafe fn set_var(&mut self, key: &str, val: &OsStr) { + std::env::set_var(key, val); + } +} + /// 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 { +pub struct PathGuard { + env: E, original_path: Option, } impl PathGuard { #[allow(dead_code, reason = "only some tests mutate PATH")] - /// Create a guard capturing the current `PATH`. + /// Create a guard capturing the current `PATH` using the real environment. pub fn new(original: OsString) -> Self { + Self::with_env(original, RealEnv) + } +} + +impl PathGuard { + #[allow(dead_code, reason = "only some tests mutate PATH")] + /// Create a guard for `PATH` using a provided environment. + pub fn with_env(original: OsString, env: E) -> Self { Self { + env, original_path: Some(original), } } -} -impl Drop for PathGuard { - fn drop(&mut self) { + /// Access the underlying environment for mutation during a test. + #[allow(dead_code, reason = "used in env injection tests")] + pub fn env_mut(&mut self) -> &mut E { + &mut self.env + } + + fn restore(&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) }; + // SAFETY: `std::env::set_var` is `unsafe` in Rust 2024 because it + // mutates process-wide state. `EnvLock` serialises mutations and + // this guard's RAII drop restores the prior `PATH`, mitigating the + // unsafety. + unsafe { self.env.set_var("PATH", &path) }; } } } + +impl Drop for PathGuard { + fn drop(&mut self) { + self.restore(); + } +} From d3430baeab4665c40740c230c754718e79e7736e Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 12 Aug 2025 02:31:32 +0100 Subject: [PATCH 2/4] chore: conditionally expect dead-code in test utils --- tests/support/mod.rs | 3 +++ tests/support/path_guard.rs | 8 +++++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/tests/support/mod.rs b/tests/support/mod.rs index 272ee75d..a04bf94b 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -3,6 +3,8 @@ //! This module provides helpers for creating fake executables along with //! logging utilities used in behavioural tests. +#![allow(unexpected_cfgs, reason = "test utilities use custom cfg")] + pub mod env_lock; pub mod path_guard; @@ -17,6 +19,7 @@ use tempfile::TempDir; /// Create a fake Ninja executable that exits with `exit_code`. /// /// Returns the temporary directory and the path to the executable. +#[cfg_attr(expect, expect(dead_code, reason = "used in PATH tests"))] #[allow(dead_code, reason = "used in PATH tests")] pub fn fake_ninja(exit_code: i32) -> (TempDir, PathBuf) { let dir = TempDir::new().expect("temp dir"); diff --git a/tests/support/path_guard.rs b/tests/support/path_guard.rs index 4bb68bf1..f1c5880e 100644 --- a/tests/support/path_guard.rs +++ b/tests/support/path_guard.rs @@ -3,6 +3,8 @@ //! Provides a guard that resets the environment variable on drop so tests do //! not pollute global state. +#![allow(unexpected_cfgs, reason = "test utilities use custom cfg")] + use std::ffi::{OsStr, OsString}; use super::env_lock::EnvLock; @@ -22,7 +24,7 @@ pub trait Env { pub struct RealEnv; impl Env for RealEnv { - #[allow(unsafe_op_in_unsafe_fn, reason = "delegates to std::env")] + #[expect(unsafe_op_in_unsafe_fn, reason = "delegates to std::env")] unsafe fn set_var(&mut self, key: &str, val: &OsStr) { std::env::set_var(key, val); } @@ -31,6 +33,7 @@ impl Env for RealEnv { /// Guard that restores `PATH` to its original value when dropped. /// /// This uses RAII to ensure the environment is reset even if a test panics. +#[cfg_attr(expect, expect(dead_code, reason = "only some tests mutate PATH"))] #[allow(dead_code, reason = "only some tests mutate PATH")] #[derive(Debug)] pub struct PathGuard { @@ -39,6 +42,7 @@ pub struct PathGuard { } impl PathGuard { + #[cfg_attr(expect, expect(dead_code, reason = "only some tests mutate PATH"))] #[allow(dead_code, reason = "only some tests mutate PATH")] /// Create a guard capturing the current `PATH` using the real environment. pub fn new(original: OsString) -> Self { @@ -47,6 +51,7 @@ impl PathGuard { } impl PathGuard { + #[cfg_attr(expect, expect(dead_code, reason = "only some tests mutate PATH"))] #[allow(dead_code, reason = "only some tests mutate PATH")] /// Create a guard for `PATH` using a provided environment. pub fn with_env(original: OsString, env: E) -> Self { @@ -57,6 +62,7 @@ impl PathGuard { } /// Access the underlying environment for mutation during a test. + #[cfg_attr(expect, expect(dead_code, reason = "used in env injection tests"))] #[allow(dead_code, reason = "used in env injection tests")] pub fn env_mut(&mut self) -> &mut E { &mut self.env From 1c50f2305fc3524b9b6aa5d8fae3a9eff84c1d36 Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 12 Aug 2025 21:05:37 +0100 Subject: [PATCH 3/4] Gate PATH helpers' dead code behind cfg_attr --- tests/path_guard_tests.rs | 5 +++++ tests/support/mod.rs | 4 ++-- tests/support/path_guard.rs | 20 +++++++++++++------- 3 files changed, 20 insertions(+), 9 deletions(-) diff --git a/tests/path_guard_tests.rs b/tests/path_guard_tests.rs index 924b1c79..27cbaab2 100644 --- a/tests/path_guard_tests.rs +++ b/tests/path_guard_tests.rs @@ -1,3 +1,8 @@ +//! Tests for PATH restoration behaviour using mock environments. +//! +//! Verifies that `PathGuard` restores `PATH` without mutating the real +//! process environment. + mod support; use mockall::{Sequence, mock}; diff --git a/tests/support/mod.rs b/tests/support/mod.rs index a04bf94b..eef0de99 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -3,7 +3,7 @@ //! This module provides helpers for creating fake executables along with //! logging utilities used in behavioural tests. -#![allow(unexpected_cfgs, reason = "test utilities use custom cfg")] +#![expect(unexpected_cfgs, reason = "test utilities use custom cfg")] pub mod env_lock; pub mod path_guard; @@ -20,7 +20,7 @@ use tempfile::TempDir; /// /// Returns the temporary directory and the path to the executable. #[cfg_attr(expect, expect(dead_code, reason = "used in PATH tests"))] -#[allow(dead_code, reason = "used in PATH tests")] +#[cfg_attr(not(expect), allow(dead_code, reason = "used in PATH tests"))] 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 f1c5880e..59b16bdc 100644 --- a/tests/support/path_guard.rs +++ b/tests/support/path_guard.rs @@ -3,13 +3,15 @@ //! Provides a guard that resets the environment variable on drop so tests do //! not pollute global state. -#![allow(unexpected_cfgs, reason = "test utilities use custom cfg")] +#![expect(unexpected_cfgs, reason = "test utilities use custom cfg")] use std::ffi::{OsStr, OsString}; use super::env_lock::EnvLock; /// Environment interface allowing `PATH` mutation. +#[cfg_attr(expect, expect(dead_code, reason = "used in PATH tests"))] +#[cfg_attr(not(expect), allow(dead_code, reason = "used in PATH tests"))] pub trait Env { /// Set an environment variable. /// @@ -20,6 +22,8 @@ pub trait Env { } /// Real environment implementation. +#[cfg_attr(expect, expect(dead_code, reason = "used in PATH tests"))] +#[cfg_attr(not(expect), allow(dead_code, reason = "used in PATH tests"))] #[derive(Debug, Default)] pub struct RealEnv; @@ -34,7 +38,7 @@ impl Env for RealEnv { /// /// This uses RAII to ensure the environment is reset even if a test panics. #[cfg_attr(expect, expect(dead_code, reason = "only some tests mutate PATH"))] -#[allow(dead_code, reason = "only some tests mutate PATH")] +#[cfg_attr(not(expect), allow(dead_code, reason = "only some tests mutate PATH"))] #[derive(Debug)] pub struct PathGuard { env: E, @@ -42,18 +46,18 @@ pub struct PathGuard { } impl PathGuard { - #[cfg_attr(expect, expect(dead_code, reason = "only some tests mutate PATH"))] - #[allow(dead_code, reason = "only some tests mutate PATH")] /// Create a guard capturing the current `PATH` using the real environment. + #[cfg_attr(expect, expect(dead_code, reason = "only some tests mutate PATH"))] + #[cfg_attr(not(expect), allow(dead_code, reason = "only some tests mutate PATH"))] pub fn new(original: OsString) -> Self { Self::with_env(original, RealEnv) } } impl PathGuard { - #[cfg_attr(expect, expect(dead_code, reason = "only some tests mutate PATH"))] - #[allow(dead_code, reason = "only some tests mutate PATH")] /// Create a guard for `PATH` using a provided environment. + #[cfg_attr(expect, expect(dead_code, reason = "only some tests mutate PATH"))] + #[cfg_attr(not(expect), allow(dead_code, reason = "only some tests mutate PATH"))] pub fn with_env(original: OsString, env: E) -> Self { Self { env, @@ -63,11 +67,13 @@ impl PathGuard { /// Access the underlying environment for mutation during a test. #[cfg_attr(expect, expect(dead_code, reason = "used in env injection tests"))] - #[allow(dead_code, reason = "used in env injection tests")] + #[cfg_attr(not(expect), allow(dead_code, reason = "used in env injection tests"))] pub fn env_mut(&mut self) -> &mut E { &mut self.env } + #[cfg_attr(expect, expect(dead_code, reason = "only used via Drop impl"))] + #[cfg_attr(not(expect), allow(dead_code, reason = "only used via Drop impl"))] fn restore(&mut self) { let _lock = EnvLock::acquire(); if let Some(path) = self.original_path.take() { From bf17dd2aa98fc6d1f6661cbc0baea8759b807b04 Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 12 Aug 2025 21:48:20 +0100 Subject: [PATCH 4/4] Gate PATH helpers with cfg(test) --- tests/path_guard_tests.rs | 7 +++++-- tests/support/mod.rs | 5 ++--- tests/support/path_guard.rs | 6 ++---- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/path_guard_tests.rs b/tests/path_guard_tests.rs index 27cbaab2..2f3008b2 100644 --- a/tests/path_guard_tests.rs +++ b/tests/path_guard_tests.rs @@ -3,11 +3,14 @@ //! Verifies that `PathGuard` restores `PATH` without mutating the real //! process environment. -mod support; +#[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; -use support::path_guard::{Env, PathGuard}; mock! { pub Env {} diff --git a/tests/support/mod.rs b/tests/support/mod.rs index eef0de99..e2646b1f 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -3,7 +3,7 @@ //! This module provides helpers for creating fake executables along with //! logging utilities used in behavioural tests. -#![expect(unexpected_cfgs, reason = "test utilities use custom cfg")] +#![allow(unexpected_cfgs, reason = "test utilities use custom cfg")] pub mod env_lock; pub mod path_guard; @@ -19,8 +19,7 @@ use tempfile::TempDir; /// Create a fake Ninja executable that exits with `exit_code`. /// /// Returns the temporary directory and the path to the executable. -#[cfg_attr(expect, expect(dead_code, reason = "used in PATH tests"))] -#[cfg_attr(not(expect), allow(dead_code, reason = "used in PATH tests"))] +#[cfg(test)] 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 59b16bdc..9c7e40d3 100644 --- a/tests/support/path_guard.rs +++ b/tests/support/path_guard.rs @@ -10,8 +10,7 @@ use std::ffi::{OsStr, OsString}; use super::env_lock::EnvLock; /// Environment interface allowing `PATH` mutation. -#[cfg_attr(expect, expect(dead_code, reason = "used in PATH tests"))] -#[cfg_attr(not(expect), allow(dead_code, reason = "used in PATH tests"))] +#[cfg(test)] pub trait Env { /// Set an environment variable. /// @@ -22,8 +21,7 @@ pub trait Env { } /// Real environment implementation. -#[cfg_attr(expect, expect(dead_code, reason = "used in PATH tests"))] -#[cfg_attr(not(expect), allow(dead_code, reason = "used in PATH tests"))] +#[cfg(test)] #[derive(Debug, Default)] pub struct RealEnv;