From dd606f392dfe3bb0570ab40db2f074cd73e251ca Mon Sep 17 00:00:00 2001 From: Leynos Date: Mon, 11 Aug 2025 19:38:53 +0100 Subject: [PATCH 1/7] Refactor test ninja env handling --- tests/steps/process_steps.rs | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/tests/steps/process_steps.rs b/tests/steps/process_steps.rs index bffcb331..78b5a0c9 100644 --- a/tests/steps/process_steps.rs +++ b/tests/steps/process_steps.rs @@ -2,6 +2,7 @@ use crate::{CliWorld, support}; use cucumber::{given, then, when}; +use mockable::{Env, MockEnv}; use netsuke::runner::{self, BuildTargets, NINJA_PROGRAM}; use std::fs; use std::path::{Path, PathBuf}; @@ -12,13 +13,14 @@ 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) { +fn install_test_ninja(env: &impl Env, 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()); + .get_or_insert_with(|| env.raw("PATH").unwrap_or_default().into()); let new_path = format!("{}:{}", dir.path().display(), original.to_string_lossy()); - // SAFETY: nightly marks `set_var` as unsafe; override path for test isolation. + // SAFETY: `std::env::set_var` is `unsafe` in Rust 2024 due to global state; restoring + // the original `PATH` after the test mitigates this risk. unsafe { std::env::set_var("PATH", &new_path); } @@ -27,18 +29,29 @@ fn install_test_ninja(world: &mut CliWorld, dir: TempDir, ninja_path: PathBuf) { world.temp = Some(dir); } +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 +} + /// Creates a fake ninja executable that exits with the given status code. #[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 = 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 env = mocked_path_env(); + install_test_ninja(&env, world, dir, path); } /// Sets up a scenario where no ninja executable is available. @@ -50,7 +63,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 = mocked_path_env(); + install_test_ninja(&env, world, dir, path); } /// Updates the CLI to use the temporary directory created for the fake ninja. From 2c4539737f4c0816bef5d43eea98a8e723495f15 Mon Sep 17 00:00:00 2001 From: Leynos Date: Mon, 11 Aug 2025 20:17:28 +0100 Subject: [PATCH 2/7] Refactor test ninja setup with path guard --- tests/cucumber.rs | 15 ++------------- tests/runner_tests.rs | 23 +---------------------- tests/steps/process_steps.rs | 30 ++++++++++-------------------- tests/support/mod.rs | 24 ++++++++++++++++++++++++ tests/support/path_guard.rs | 33 +++++++++++++++++++++++++++++++++ 5 files changed, 70 insertions(+), 55 deletions(-) create mode 100644 tests/support/path_guard.rs diff --git a/tests/cucumber.rs b/tests/cucumber.rs index 76e7885d..84cffa73 100644 --- a/tests/cucumber.rs +++ b/tests/cucumber.rs @@ -19,19 +19,8 @@ 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, } mod steps; diff --git a/tests/runner_tests.rs b/tests/runner_tests.rs index b2a5b69b..771d0fb6 100644 --- a/tests/runner_tests.rs +++ b/tests/runner_tests.rs @@ -2,31 +2,10 @@ 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}; 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::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 78b5a0c9..50cea4eb 100644 --- a/tests/steps/process_steps.rs +++ b/tests/steps/process_steps.rs @@ -2,7 +2,7 @@ use crate::{CliWorld, support}; use cucumber::{given, then, when}; -use mockable::{Env, MockEnv}; +use mockable::Env; use netsuke::runner::{self, BuildTargets, NINJA_PROGRAM}; use std::fs; use std::path::{Path, PathBuf}; @@ -14,35 +14,25 @@ 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 = world - .original_path - .get_or_insert_with(|| env.raw("PATH").unwrap_or_default().into()); - - let new_path = format!("{}:{}", dir.path().display(), original.to_string_lossy()); - // SAFETY: `std::env::set_var` is `unsafe` in Rust 2024 due to global state; restoring - // the original `PATH` after the test mitigates this risk. + let original = env.raw("PATH").unwrap_or_default(); + let guard = support::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; `PathGuard` + // restores the prior value when dropped. 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); } -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 -} - /// Creates a fake ninja executable that exits with the given status code. #[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); - let env = mocked_path_env(); + let env = support::mocked_path_env(); install_test_ninja(&env, world, dir, path); } @@ -50,7 +40,7 @@ fn fake_ninja(world: &mut CliWorld, code: i32) { #[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(); - let env = mocked_path_env(); + let env = support::mocked_path_env(); install_test_ninja(&env, world, dir, path); } @@ -63,7 +53,7 @@ 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"); - let env = mocked_path_env(); + let env = support::mocked_path_env(); install_test_ninja(&env, world, dir, path); } diff --git a/tests/support/mod.rs b/tests/support/mod.rs index 8a7ed244..29672676 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -4,6 +4,7 @@ //! generating minimal manifests used in behavioural tests. use mockable::MockEnv; +use rstest::fixture; use std::fs::{self, File}; use std::io::{self, Write}; use std::path::{Path, PathBuf}; @@ -12,6 +13,10 @@ use tempfile::TempDir; use tracing::Level; use tracing_subscriber::fmt; +mod path_guard; +#[allow(unused_imports, reason = "re-export for selective test crates")] +pub use path_guard::PathGuard; + /// Create a fake Ninja executable that exits with `exit_code`. /// /// Returns the temporary directory and the path to the executable. @@ -77,6 +82,25 @@ pub fn mock_path_to(env: &mut MockEnv, dir: &Path) -> std::io::Result<()> { Ok(()) } +/// 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] +#[allow( + unfulfilled_lint_expectations, + reason = "only used in process step tests" +)] +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 +} + /// 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 diff --git a/tests/support/path_guard.rs b/tests/support/path_guard.rs new file mode 100644 index 00000000..4a7b16aa --- /dev/null +++ b/tests/support/path_guard.rs @@ -0,0 +1,33 @@ +use std::ffi::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( + unfulfilled_lint_expectations, + reason = "used only in select test crates", +)] +#[expect(dead_code, reason = "constructed only in PATH tests")] +#[derive(Debug)] +pub struct PathGuard { + original: OsString, +} + +#[allow( + unfulfilled_lint_expectations, + reason = "used only in select test crates", +)] +impl PathGuard { + /// Create a guard capturing the current `PATH`. + #[expect(dead_code, reason = "constructed only in PATH tests")] + pub fn new(original: OsString) -> Self { + Self { original } + } +} + +impl Drop for PathGuard { + fn drop(&mut self) { + // Nightly marks `set_var` unsafe; restoring `PATH` cleans up global state. + unsafe { std::env::set_var("PATH", &self.original) }; + } +} From 1ff8261122899d19391e5a89fd308beab6fd18de Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 12 Aug 2025 00:43:22 +0100 Subject: [PATCH 3/7] refactor path guard linting --- tests/support/path_guard.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/tests/support/path_guard.rs b/tests/support/path_guard.rs index 4a7b16aa..b4577100 100644 --- a/tests/support/path_guard.rs +++ b/tests/support/path_guard.rs @@ -1,3 +1,8 @@ +//! 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; /// Guard that restores `PATH` to its original value when dropped. @@ -5,7 +10,7 @@ use std::ffi::OsString; /// This uses RAII to ensure the environment is reset even if a test panics. #[allow( unfulfilled_lint_expectations, - reason = "used only in select test crates", + reason = "used only in select test crates" )] #[expect(dead_code, reason = "constructed only in PATH tests")] #[derive(Debug)] @@ -13,9 +18,10 @@ pub struct PathGuard { original: OsString, } +#[cfg_attr(test, expect(dead_code, reason = "constructed only in PATH tests"))] #[allow( unfulfilled_lint_expectations, - reason = "used only in select test crates", + reason = "used only in select test crates" )] impl PathGuard { /// Create a guard capturing the current `PATH`. From a3eec3ed9f6e484effac5b792364f03004c245af Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 12 Aug 2025 00:43:27 +0100 Subject: [PATCH 4/7] Guard dead code expectations in test helpers --- tests/support/mod.rs | 26 +++++++++++++++++++------- tests/support/path_guard.rs | 4 ++-- 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/tests/support/mod.rs b/tests/support/mod.rs index 29672676..b42dcf64 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -24,7 +24,7 @@ pub use path_guard::PathGuard; unfulfilled_lint_expectations, reason = "used only in some test crates" )] -#[expect(dead_code, reason = "used in CLI behaviour tests")] +#[cfg_attr(test, 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"); @@ -52,7 +52,7 @@ pub fn fake_ninja(exit_code: i32) -> (TempDir, PathBuf) { unfulfilled_lint_expectations, reason = "used only in some test crates" )] -#[expect(dead_code, reason = "used in PATH tests")] +#[cfg_attr(test, 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. @@ -109,7 +109,10 @@ pub fn mocked_path_env() -> MockEnv { unfulfilled_lint_expectations, reason = "used only in some test crates" )] -#[expect(dead_code, reason = "used in build file validation tests")] +#[cfg_attr( + test, + 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"); @@ -140,7 +143,10 @@ pub fn fake_ninja_check_build_file() -> (TempDir, PathBuf) { unfulfilled_lint_expectations, reason = "compiled only for logging tests" )] -#[expect(dead_code, reason = "compiled as its own crate during linting")] +#[cfg_attr( + test, + expect(dead_code, reason = "compiled as its own crate during linting") +)] #[derive(Clone)] struct BufferWriter { buf: Arc>>, @@ -169,7 +175,10 @@ impl Write for BufferWriter { unfulfilled_lint_expectations, reason = "compiled only for logging tests" )] -#[expect(dead_code, reason = "compiled as its own crate during linting")] +#[cfg_attr( + test, + expect(dead_code, reason = "compiled as its own crate during linting") +)] pub fn capture_logs(level: Level, f: F) -> String where F: FnOnce(), @@ -193,7 +202,7 @@ where /// /// 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")] +#[cfg_attr(test, expect(dead_code, reason = "used only in directory tests"))] pub fn fake_ninja_pwd() -> (TempDir, PathBuf) { let dir = TempDir::new().expect("temp dir"); let path = dir.path().join("ninja"); @@ -219,7 +228,10 @@ pub fn fake_ninja_pwd() -> (TempDir, PathBuf) { unfulfilled_lint_expectations, reason = "shared test utility not used in all crates" )] -#[expect(dead_code, reason = "shared test utility not used in all crates")] +#[cfg_attr( + test, + expect(dead_code, reason = "shared test utility not used in all crates") +)] pub fn write_manifest(file: &mut impl Write) -> io::Result<()> { writeln!( file, diff --git a/tests/support/path_guard.rs b/tests/support/path_guard.rs index b4577100..832b442c 100644 --- a/tests/support/path_guard.rs +++ b/tests/support/path_guard.rs @@ -12,7 +12,7 @@ use std::ffi::OsString; unfulfilled_lint_expectations, reason = "used only in select test crates" )] -#[expect(dead_code, reason = "constructed only in PATH tests")] +#[cfg_attr(test, expect(dead_code, reason = "constructed only in PATH tests"))] #[derive(Debug)] pub struct PathGuard { original: OsString, @@ -25,7 +25,7 @@ pub struct PathGuard { )] impl PathGuard { /// Create a guard capturing the current `PATH`. - #[expect(dead_code, reason = "constructed only in PATH tests")] + #[cfg_attr(test, expect(dead_code, reason = "constructed only in PATH tests"))] pub fn new(original: OsString) -> Self { Self { original } } From 29284b1a859fbcf14375b60c946acd8b44bdad53 Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 12 Aug 2025 00:43:33 +0100 Subject: [PATCH 5/7] Factor shared test env and ninja helpers --- tests/cucumber.rs | 8 +- tests/runner_tests.rs | 8 +- tests/steps/process_steps.rs | 14 +-- tests/support/check_ninja.rs | 36 +++++++ tests/support/env.rs | 40 ++++++++ tests/support/mod.rs | 177 ++--------------------------------- tests/support/path_guard.rs | 11 --- 7 files changed, 104 insertions(+), 190 deletions(-) create mode 100644 tests/support/check_ninja.rs create mode 100644 tests/support/env.rs diff --git a/tests/cucumber.rs b/tests/cucumber.rs index 84cffa73..f220e3bf 100644 --- a/tests/cucumber.rs +++ b/tests/cucumber.rs @@ -20,9 +20,15 @@ pub struct CliWorld { /// Temporary directory handle for test isolation. pub temp: Option, /// Guard that restores `PATH` after each scenario. - pub path_guard: Option, + pub path_guard: Option, } +#[path = "support/check_ninja.rs"] +mod check_ninja; +#[path = "support/env.rs"] +mod env; +#[path = "support/path_guard.rs"] +mod path_guard; mod steps; mod support; diff --git a/tests/runner_tests.rs b/tests/runner_tests.rs index 771d0fb6..1d151ad7 100644 --- a/tests/runner_tests.rs +++ b/tests/runner_tests.rs @@ -4,15 +4,19 @@ use rstest::{fixture, rstest}; use serial_test::serial; use std::path::{Path, PathBuf}; +#[path = "support/check_ninja.rs"] +mod check_ninja; +#[path = "support/path_guard.rs"] +mod path_guard; mod support; -use support::PathGuard; +use 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(); diff --git a/tests/steps/process_steps.rs b/tests/steps/process_steps.rs index 50cea4eb..3cfe34bc 100644 --- a/tests/steps/process_steps.rs +++ b/tests/steps/process_steps.rs @@ -1,6 +1,6 @@ //! Step definitions for Ninja process execution. -use crate::{CliWorld, support}; +use crate::{CliWorld, check_ninja, env, path_guard, support}; use cucumber::{given, then, when}; use mockable::Env; use netsuke::runner::{self, BuildTargets, NINJA_PROGRAM}; @@ -15,7 +15,7 @@ use tempfile::{NamedTempFile, TempDir}; )] 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 = support::PathGuard::new(original.clone().into()); + let guard = path_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; `PathGuard` // restores the prior value when dropped. @@ -32,15 +32,15 @@ fn install_test_ninja(env: &impl Env, world: &mut CliWorld, dir: TempDir, ninja_ #[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); - let env = support::mocked_path_env(); + 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(); - let env = support::mocked_path_env(); + let (dir, path) = check_ninja::fake_ninja_check_build_file(); + let env = env::mocked_path_env(); install_test_ninja(&env, world, dir, path); } @@ -53,7 +53,7 @@ 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"); - let env = support::mocked_path_env(); + let env = env::mocked_path_env(); install_test_ninja(&env, world, dir, path); } @@ -96,7 +96,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/mod.rs b/tests/support/mod.rs index b42dcf64..5266ff4d 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -1,30 +1,16 @@ //! 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. -use mockable::MockEnv; -use rstest::fixture; 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; - -mod path_guard; -#[allow(unused_imports, reason = "re-export for selective test crates")] -pub use path_guard::PathGuard; /// 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" -)] -#[cfg_attr(test, 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"); @@ -48,11 +34,7 @@ pub fn fake_ninja(exit_code: i32) -> (TempDir, PathBuf) { /// let mut env = MockEnv::new(); /// mock_path_to(&mut env, dir.path()); /// ``` -#[allow( - unfulfilled_lint_expectations, - reason = "used only in some test crates" -)] -#[cfg_attr(test, expect(dead_code, reason = "used in PATH tests"))] +#[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. @@ -60,17 +42,17 @@ 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<()> { +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()), ) })?; @@ -82,127 +64,11 @@ pub fn mock_path_to(env: &mut MockEnv, dir: &Path) -> std::io::Result<()> { Ok(()) } -/// 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] -#[allow( - unfulfilled_lint_expectations, - reason = "only used in process step tests" -)] -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 -} - -/// 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" -)] -#[cfg_attr( - test, - 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" -)] -#[cfg_attr( - test, - 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" -)] -#[cfg_attr( - test, - 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")] -#[cfg_attr(test, expect(dead_code, 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"); let path = dir.path().join("ninja"); @@ -218,30 +84,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" -)] -#[cfg_attr( - test, - 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 index 832b442c..007c1b0a 100644 --- a/tests/support/path_guard.rs +++ b/tests/support/path_guard.rs @@ -8,24 +8,13 @@ use std::ffi::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( - unfulfilled_lint_expectations, - reason = "used only in select test crates" -)] -#[cfg_attr(test, expect(dead_code, reason = "constructed only in PATH tests"))] #[derive(Debug)] pub struct PathGuard { original: OsString, } -#[cfg_attr(test, expect(dead_code, reason = "constructed only in PATH tests"))] -#[allow( - unfulfilled_lint_expectations, - reason = "used only in select test crates" -)] impl PathGuard { /// Create a guard capturing the current `PATH`. - #[cfg_attr(test, expect(dead_code, reason = "constructed only in PATH tests"))] pub fn new(original: OsString) -> Self { Self { original } } From 55e4f0a144cd8343e25900e02ed353df1ea61cb3 Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 12 Aug 2025 01:22:55 +0100 Subject: [PATCH 6/7] Move expect after doc comments --- tests/support/mod.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/tests/support/mod.rs b/tests/support/mod.rs index 5266ff4d..9ab4354c 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -28,13 +28,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()); -/// ``` -#[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. @@ -42,6 +35,14 @@ 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. +/// +/// # 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. From dcf9e1e6465ea423761f79572e448b57db49859c Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 12 Aug 2025 01:23:00 +0100 Subject: [PATCH 7/7] Add test EnvLock --- tests/cucumber.rs | 4 +--- tests/runner_tests.rs | 7 ++++--- tests/steps/process_steps.rs | 11 +++++++---- tests/support/env_lock.rs | 22 ++++++++++++++++++++++ tests/support/mod.rs | 6 ++++++ tests/support/path_guard.rs | 17 +++++++++++++---- 6 files changed, 53 insertions(+), 14 deletions(-) create mode 100644 tests/support/env_lock.rs diff --git a/tests/cucumber.rs b/tests/cucumber.rs index f220e3bf..b4b7c5c3 100644 --- a/tests/cucumber.rs +++ b/tests/cucumber.rs @@ -20,15 +20,13 @@ pub struct CliWorld { /// Temporary directory handle for test isolation. pub temp: Option, /// Guard that restores `PATH` after each scenario. - pub path_guard: Option, + pub path_guard: Option, } #[path = "support/check_ninja.rs"] mod check_ninja; #[path = "support/env.rs"] mod env; -#[path = "support/path_guard.rs"] -mod path_guard; mod steps; mod support; diff --git a/tests/runner_tests.rs b/tests/runner_tests.rs index 1d151ad7..abff32e9 100644 --- a/tests/runner_tests.rs +++ b/tests/runner_tests.rs @@ -6,10 +6,9 @@ use std::path::{Path, PathBuf}; #[path = "support/check_ninja.rs"] mod check_ninja; -#[path = "support/path_guard.rs"] -mod path_guard; mod support; -use path_guard::PathGuard; +use support::env_lock::EnvLock; +use support::path_guard::PathGuard; /// Fixture: Put a fake `ninja` (that checks for a build file) on PATH. /// @@ -23,6 +22,7 @@ fn ninja_in_path() -> (tempfile::TempDir, PathBuf, PathGuard) { 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) }; @@ -44,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 3cfe34bc..fe136db0 100644 --- a/tests/steps/process_steps.rs +++ b/tests/steps/process_steps.rs @@ -1,11 +1,13 @@ //! Step definitions for Ninja process execution. -use crate::{CliWorld, check_ninja, env, path_guard, 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`. @@ -15,10 +17,11 @@ use tempfile::{NamedTempFile, TempDir}; )] 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 = path_guard::PathGuard::new(original.clone().into()); + 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; `PathGuard` - // restores the prior value when dropped. + // 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); } 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 9ab4354c..0dae0142 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -3,6 +3,12 @@ //! 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 std::fs::{self, File}; use std::io::{self, Write}; use std::path::{Path, PathBuf}; diff --git a/tests/support/path_guard.rs b/tests/support/path_guard.rs index 007c1b0a..96c54dcd 100644 --- a/tests/support/path_guard.rs +++ b/tests/support/path_guard.rs @@ -5,24 +5,33 @@ 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: OsString, + 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 } + Self { + original_path: Some(original), + } } } impl Drop for PathGuard { fn drop(&mut self) { - // Nightly marks `set_var` unsafe; restoring `PATH` cleans up global state. - unsafe { std::env::set_var("PATH", &self.original) }; + 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) }; + } } }