diff --git a/tests/cucumber.rs b/tests/cucumber.rs index 76e7885d..b4b7c5c3 100644 --- a/tests/cucumber.rs +++ b/tests/cucumber.rs @@ -19,21 +19,14 @@ pub struct CliWorld { pub run_error: Option, /// Temporary directory handle for test isolation. pub temp: Option, - /// Original `PATH` value restored after each scenario. - pub original_path: Option, -} - -impl Drop for CliWorld { - fn drop(&mut self) { - if let Some(path) = self.original_path.take() { - // SAFETY: nightly marks `set_var` as unsafe; restore path for isolation. - unsafe { - std::env::set_var("PATH", path); - } - } - } + /// Guard that restores `PATH` after each scenario. + pub path_guard: Option, } +#[path = "support/check_ninja.rs"] +mod check_ninja; +#[path = "support/env.rs"] +mod env; mod steps; mod support; diff --git a/tests/runner_tests.rs b/tests/runner_tests.rs index b2a5b69b..abff32e9 100644 --- a/tests/runner_tests.rs +++ b/tests/runner_tests.rs @@ -2,44 +2,27 @@ use netsuke::cli::{BuildArgs, Cli, Commands}; use netsuke::runner::{BuildTargets, NINJA_ENV, run, run_ninja}; use rstest::{fixture, rstest}; use serial_test::serial; -use std::ffi::OsString; use std::path::{Path, PathBuf}; +#[path = "support/check_ninja.rs"] +mod check_ninja; mod support; - -/// Guard that restores PATH to its original value when dropped. -/// -/// Using a simple guard avoids heap allocation and guarantees teardown on -/// early returns or panics. -struct PathGuard { - original: OsString, -} - -impl PathGuard { - fn new(original: OsString) -> Self { - Self { original } - } -} - -impl Drop for PathGuard { - fn drop(&mut self) { - // Nightly marks set_var unsafe. - unsafe { std::env::set_var("PATH", &self.original) }; - } -} +use support::env_lock::EnvLock; +use support::path_guard::PathGuard; /// Fixture: Put a fake `ninja` (that checks for a build file) on PATH. /// /// Returns: (tempdir holding ninja, `ninja_path`, PATH guard) #[fixture] fn ninja_in_path() -> (tempfile::TempDir, PathBuf, PathGuard) { - let (ninja_dir, ninja_path) = support::fake_ninja_check_build_file(); + 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) }; @@ -61,6 +44,7 @@ fn ninja_with_exit_code(#[default(0)] exit_code: i32) -> (tempfile::TempDir, Pat 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) }; diff --git a/tests/steps/process_steps.rs b/tests/steps/process_steps.rs index bffcb331..fe136db0 100644 --- a/tests/steps/process_steps.rs +++ b/tests/steps/process_steps.rs @@ -1,10 +1,13 @@ //! Step definitions for Ninja process execution. -use crate::{CliWorld, support}; +use crate::{CliWorld, check_ninja, env, 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`. @@ -12,17 +15,18 @@ use tempfile::{NamedTempFile, TempDir}; clippy::needless_pass_by_value, reason = "helper owns path for simplicity" )] -fn install_test_ninja(world: &mut CliWorld, dir: TempDir, ninja_path: PathBuf) { - let original = world - .original_path - .get_or_insert_with(|| std::env::var_os("PATH").unwrap_or_default()); - - let new_path = format!("{}:{}", dir.path().display(), original.to_string_lossy()); - // SAFETY: nightly marks `set_var` as unsafe; override path for test isolation. +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); } + world.path_guard = Some(guard); world.ninja = Some(ninja_path.to_string_lossy().into_owned()); world.temp = Some(dir); } @@ -31,14 +35,16 @@ fn install_test_ninja(world: &mut CliWorld, dir: TempDir, ninja_path: PathBuf) { #[given(expr = "a fake ninja executable that exits with {int}")] fn fake_ninja(world: &mut CliWorld, code: i32) { let (dir, path) = support::fake_ninja(code); - install_test_ninja(world, dir, path); + let env = env::mocked_path_env(); + install_test_ninja(&env, world, dir, path); } /// Creates a fake ninja executable that validates the build file path. #[given("a fake ninja executable that checks for the build file")] fn fake_ninja_check(world: &mut CliWorld) { - let (dir, path) = support::fake_ninja_check_build_file(); - install_test_ninja(world, dir, path); + let (dir, path) = check_ninja::fake_ninja_check_build_file(); + let env = env::mocked_path_env(); + install_test_ninja(&env, world, dir, path); } /// Sets up a scenario where no ninja executable is available. @@ -50,7 +56,8 @@ fn fake_ninja_check(world: &mut CliWorld) { fn no_ninja(world: &mut CliWorld) { let dir = TempDir::new().expect("temp dir"); let path = dir.path().join("ninja"); - install_test_ninja(world, dir, path); + let env = env::mocked_path_env(); + install_test_ninja(&env, world, dir, path); } /// Updates the CLI to use the temporary directory created for the fake ninja. @@ -92,7 +99,7 @@ fn run(world: &mut CliWorld) { if !manifest_path.exists() { let mut file = NamedTempFile::new_in(dir.path()).expect("Failed to create temporary manifest file"); - support::write_manifest(&mut file).expect("Failed to write manifest content"); + env::write_manifest(&mut file).expect("Failed to write manifest content"); file.persist(&manifest_path) .expect("Failed to persist manifest file"); } diff --git a/tests/support/check_ninja.rs b/tests/support/check_ninja.rs new file mode 100644 index 00000000..11d9cbb3 --- /dev/null +++ b/tests/support/check_ninja.rs @@ -0,0 +1,36 @@ +//! Helpers for validating build file paths via fake Ninja binaries. + +use std::fs::{self, File}; +use std::io::Write; +use std::path::PathBuf; +use tempfile::TempDir; + +/// Create a fake Ninja that validates the build file path provided via `-f`. +/// +/// The script exits with status `1` if the file is missing or not a regular +/// file, otherwise `0`. +pub fn fake_ninja_check_build_file() -> (TempDir, PathBuf) { + let dir = TempDir::new().expect("temp dir"); + let path = dir.path().join("ninja"); + let mut file = File::create(&path).expect("script"); + writeln!( + file, + concat!( + "#!/bin/sh\n", + "if [ \"$1\" = \"-f\" ] && [ ! -f \"$2\" ]; then\n", + " echo 'missing build file: $2' >&2\n", + " exit 1\n", + "fi\n", + "exit 0" + ), + ) + .expect("write script"); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let mut perms = fs::metadata(&path).expect("meta").permissions(); + perms.set_mode(0o755); + fs::set_permissions(&path, perms).expect("perms"); + } + (dir, path) +} diff --git a/tests/support/env.rs b/tests/support/env.rs new file mode 100644 index 00000000..91e9e519 --- /dev/null +++ b/tests/support/env.rs @@ -0,0 +1,40 @@ +//! Helpers for environment manipulation in process tests. +//! +//! Provides fixtures and utilities for managing `PATH` and writing minimal +//! manifests. + +use mockable::MockEnv; +use rstest::fixture; +use std::io::{self, Write}; + +/// Fixture: capture the original `PATH` via a mocked environment. +/// +/// Returns a `MockEnv` that yields the current `PATH` when queried. Tests can +/// modify the real environment while the mock continues to expose the initial +/// value. +#[fixture] +pub fn mocked_path_env() -> MockEnv { + let original = std::env::var("PATH").unwrap_or_default(); + let mut env = MockEnv::new(); + env.expect_raw() + .withf(|k| k == "PATH") + .returning(move |_| Ok(original.clone())); + env +} + +/// Write a minimal manifest to `file`. +/// +/// The manifest declares a single `hello` target that prints a greeting. +pub fn write_manifest(file: &mut impl Write) -> io::Result<()> { + writeln!( + file, + concat!( + "netsuke_version: \"1.0.0\"\n", + "targets:\n", + " - name: hello\n", + " recipe:\n", + " kind: command\n", + " command: \"echo hi\"\n" + ), + ) +} diff --git a/tests/support/env_lock.rs b/tests/support/env_lock.rs new file mode 100644 index 00000000..cf22b3c2 --- /dev/null +++ b/tests/support/env_lock.rs @@ -0,0 +1,22 @@ +//! Serialise environment mutations across tests. +//! +//! The `EnvLock` guard ensures that changes to global state like `PATH` are +//! synchronised, preventing interference between concurrently running tests. + +use std::sync::{Mutex, MutexGuard}; + +#[allow(dead_code, reason = "only some tests mutate PATH")] +/// Global mutex protecting environment changes. +static ENV_LOCK: Mutex<()> = Mutex::new(()); + +#[allow(dead_code, reason = "only some tests mutate PATH")] +/// RAII guard that holds the global environment lock. +pub struct EnvLock(MutexGuard<'static, ()>); + +impl EnvLock { + #[allow(dead_code, reason = "only some tests mutate PATH")] + /// Acquire the global lock serialising environment mutations. + pub fn acquire() -> Self { + Self(ENV_LOCK.lock().expect("env lock")) + } +} diff --git a/tests/support/mod.rs b/tests/support/mod.rs index 8a7ed244..0dae0142 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -1,25 +1,22 @@ //! Test utilities for process management. //! -//! This module provides helpers for creating fake executables and -//! generating minimal manifests used in behavioural tests. +//! This module provides helpers for creating fake executables along with +//! logging utilities used in behavioural tests. + +pub mod env_lock; +pub mod path_guard; + +#[expect(unused_imports, reason = "re-export for selective test crates")] +pub use path_guard::PathGuard; -use mockable::MockEnv; use std::fs::{self, File}; use std::io::{self, Write}; use std::path::{Path, PathBuf}; -use std::sync::{Arc, Mutex}; use tempfile::TempDir; -use tracing::Level; -use tracing_subscriber::fmt; /// Create a fake Ninja executable that exits with `exit_code`. /// /// Returns the temporary directory and the path to the executable. -#[allow( - unfulfilled_lint_expectations, - reason = "used only in some test crates" -)] -#[expect(dead_code, reason = "used in CLI behaviour tests")] pub fn fake_ninja(exit_code: i32) -> (TempDir, PathBuf) { let dir = TempDir::new().expect("temp dir"); let path = dir.path().join("ninja"); @@ -37,17 +34,6 @@ pub fn fake_ninja(exit_code: i32) -> (TempDir, PathBuf) { /// Set up `env` so `PATH` resolves only to `dir`. /// -/// # Examples -/// ```ignore -/// let (dir, _) = fake_ninja(0); -/// let mut env = MockEnv::new(); -/// mock_path_to(&mut env, dir.path()); -/// ``` -#[allow( - unfulfilled_lint_expectations, - reason = "used only in some test crates" -)] -#[expect(dead_code, reason = "used in PATH tests")] /// Build a valid `PATH` string that contains exactly one entry pointing to /// `dir` and configure the mock to return it. This avoids lossy conversions /// and makes the UTF-8 requirement explicit to callers. @@ -55,17 +41,25 @@ pub fn fake_ninja(exit_code: i32) -> (TempDir, PathBuf) { /// Note: `MockEnv::raw` returns a `String`, so callers must accept UTF-8. This /// helper returns an error if the constructed `PATH` cannot be represented as /// UTF-8. -pub fn mock_path_to(env: &mut MockEnv, dir: &Path) -> std::io::Result<()> { +/// +/// # Examples +/// ```ignore +/// let (dir, _) = fake_ninja(0); +/// let mut env = MockEnv::new(); +/// mock_path_to(&mut env, dir.path()); +/// ``` +#[expect(dead_code, reason = "used in PATH tests")] +pub fn mock_path_to(env: &mut mockable::MockEnv, dir: &Path) -> io::Result<()> { // Join using the platform-appropriate separator while ensuring exactly one // element is present in the PATH value. let joined = std::env::join_paths([dir.as_os_str()]) - .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidInput, e))?; + .map_err(|e| io::Error::new(io::ErrorKind::InvalidInput, e))?; // `MockEnv::raw` expects a `String`. Propagate if the single-entry PATH is // not valid UTF-8 to keep the contract explicit. let path = joined.into_string().map_err(|os| { - std::io::Error::new( - std::io::ErrorKind::InvalidData, + io::Error::new( + io::ErrorKind::InvalidData, format!("non-UTF-8 PATH entry: {}", os.to_string_lossy()), ) })?; @@ -77,98 +71,10 @@ pub fn mock_path_to(env: &mut MockEnv, dir: &Path) -> std::io::Result<()> { Ok(()) } -/// Create a fake Ninja that validates the build file path provided via `-f`. -/// -/// The script exits with status `1` if the file is missing or not a regular -/// file, otherwise `0`. -#[allow( - unfulfilled_lint_expectations, - reason = "used only in some test crates" -)] -#[expect(dead_code, reason = "used in build file validation tests")] -pub fn fake_ninja_check_build_file() -> (TempDir, PathBuf) { - let dir = TempDir::new().expect("temp dir"); - let path = dir.path().join("ninja"); - let mut file = File::create(&path).expect("script"); - writeln!( - file, - concat!( - "#!/bin/sh\n", - "if [ \"$1\" = \"-f\" ] && [ ! -f \"$2\" ]; then\n", - " echo 'missing build file: $2' >&2\n", - " exit 1\n", - "fi\n", - "exit 0" - ) - ) - .expect("write script"); - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - let mut perms = fs::metadata(&path).expect("meta").permissions(); - perms.set_mode(0o755); - fs::set_permissions(&path, perms).expect("perms"); - } - (dir, path) -} - -#[allow( - unfulfilled_lint_expectations, - reason = "compiled only for logging tests" -)] -#[expect(dead_code, reason = "compiled as its own crate during linting")] -#[derive(Clone)] -struct BufferWriter { - buf: Arc>>, -} - -impl Write for BufferWriter { - fn write(&mut self, data: &[u8]) -> io::Result { - self.buf.lock().expect("lock").write(data) - } - - fn flush(&mut self) -> io::Result<()> { - self.buf.lock().expect("lock").flush() - } -} - -/// Capture logs emitted within the provided closure. -/// -/// # Examples -/// -/// ```ignore -/// use tracing::Level; -/// let output = capture_logs(Level::INFO, || tracing::info!("hello")); -/// assert!(output.contains("hello")); -/// ``` -#[allow( - unfulfilled_lint_expectations, - reason = "compiled only for logging tests" -)] -#[expect(dead_code, reason = "compiled as its own crate during linting")] -pub fn capture_logs(level: Level, f: F) -> String -where - F: FnOnce(), -{ - let buf = Arc::new(Mutex::new(Vec::new())); - let writer = BufferWriter { - buf: Arc::clone(&buf), - }; - let subscriber = fmt() - .with_max_level(level) - .without_time() - .with_writer(move || writer.clone()) - .finish(); - tracing::subscriber::with_default(subscriber, f); - let locked = buf.lock().expect("lock"); - String::from_utf8(locked.clone()).expect("utf8") -} - /// Create a fake Ninja executable that writes its current directory to the file /// specified as the first argument. /// /// Returns the temporary directory and the path to the executable. -#[allow(unfulfilled_lint_expectations, reason = "used only in directory tests")] #[expect(dead_code, reason = "used only in directory tests")] pub fn fake_ninja_pwd() -> (TempDir, PathBuf) { let dir = TempDir::new().expect("temp dir"); @@ -185,27 +91,3 @@ pub fn fake_ninja_pwd() -> (TempDir, PathBuf) { } (dir, path) } - -/// Write a minimal manifest to `file`. -/// -/// The manifest declares a single `hello` target that prints a greeting. -/// This must be `allow` as `expect` will trigger an unfulfilled warning -/// despite the lint violation arising. -#[allow( - unfulfilled_lint_expectations, - reason = "shared test utility not used in all crates" -)] -#[expect(dead_code, reason = "shared test utility not used in all crates")] -pub fn write_manifest(file: &mut impl Write) -> io::Result<()> { - writeln!( - file, - concat!( - "netsuke_version: \"1.0.0\"\n", - "targets:\n", - " - name: hello\n", - " recipe:\n", - " kind: command\n", - " command: \"echo hi\"\n" - ), - ) -} diff --git a/tests/support/path_guard.rs b/tests/support/path_guard.rs new file mode 100644 index 00000000..96c54dcd --- /dev/null +++ b/tests/support/path_guard.rs @@ -0,0 +1,37 @@ +//! Restore `PATH` after tests mutate it. +//! +//! Provides a guard that resets the environment variable on drop so tests do +//! not pollute global state. + +use std::ffi::OsString; + +use super::env_lock::EnvLock; + +/// 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, +} + +impl PathGuard { + #[allow(dead_code, reason = "only some tests mutate PATH")] + /// Create a guard capturing the current `PATH`. + pub fn new(original: OsString) -> Self { + Self { + original_path: Some(original), + } + } +} + +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) }; + } + } +}