diff --git a/docs/test-isolation-with-ninja-env.md b/docs/test-isolation-with-ninja-env.md new file mode 100644 index 00000000..80285de8 --- /dev/null +++ b/docs/test-isolation-with-ninja-env.md @@ -0,0 +1,63 @@ +# Test isolation with `NINJA_ENV` + +Netsuke resolves the Ninja binary from the `NINJA_ENV` environment variable +before falling back to `ninja` on `PATH`. Tests should override `NINJA_ENV` +instead of mutating `PATH` so they can execute in parallel without stepping on +each other's environment. + +## Why prefer `NINJA_ENV` + +- Mutating `PATH` is global and risks races when tests run concurrently. +- `override_ninja_env` scopes changes via an `EnvGuard`, restoring the previous + value even if the test fails. +- Keeping `PATH` untouched avoids coupling to the developer's shell setup. +- `override_ninja_env` also holds a process-wide lock for the guard's lifetime, + preventing parallel tests from interleaving `NINJA_ENV` mutations. + +## Fixture pattern + +Use a fixture to create a fake Ninja executable and point `NINJA_ENV` at it. +The fixture keeps the temporary directory alive for the test duration and +automatically restores the environment on drop. + +```rust +use rstest::fixture; +use test_support::env::{NinjaEnvGuard, SystemEnv, override_ninja_env}; +use test_support::fake_ninja; + +#[fixture] +fn ninja_in_env() -> anyhow::Result<(tempfile::TempDir, NinjaEnvGuard)> { + let (ninja_dir, ninja_path) = fake_ninja(0)?; + let env = SystemEnv::new(); + let guard = override_ninja_env(&env, ninja_path.as_path()); + Ok((ninja_dir, guard)) +} +``` + +Inject the fixture into tests that need a controlled Ninja binary: + +```rust +#[rstest] +fn run_build_uses_fake_ninja( + (_, _guard): (tempfile::TempDir, NinjaEnvGuard), +) { + // run the command-line interface (CLI) here; the guard restores NINJA_ENV + // on drop +} +``` + +## Dos and don'ts + +- Do keep the guard alive until after the CLI invocation so `NINJA_ENV` stays + set. +- Do avoid explicit `drop` calls for `PathBuf` values; they do not own external + resources. +- Don't add `#[serial]` purely to protect `PATH` mutations; prefer the fixture + above to keep tests parallel-friendly. + +## Precedence over `PATH` + +`NINJA_ENV` should override any `ninja` found on `PATH`. When asserting this in +tests, place a failing fake Ninja on `PATH` with `prepend_dir_to_path` and set +`NINJA_ENV` to a working fake Ninja via `override_ninja_env`. The test should +pass only if `NINJA_ENV` is respected. diff --git a/test_support/src/env.rs b/test_support/src/env.rs index b6e3b53f..f652c600 100644 --- a/test_support/src/env.rs +++ b/test_support/src/env.rs @@ -13,7 +13,11 @@ use std::{ path::Path, }; -use crate::{env_guard::EnvGuard, env_lock::EnvLock, path_guard::PathGuard}; +use crate::{ + env_guard::{EnvGuard, StdEnv}, + env_lock::EnvLock, + path_guard::PathGuard, +}; /// Alias for the real process environment. pub type SystemEnv = DefaultEnv; @@ -207,17 +211,26 @@ pub fn prepend_dir_to_path(env: &impl EnvMut, dir: &Path) -> Result { Ok(PathGuard::new(original_os)) } -/// Guard that restores `NINJA_ENV` to its previous value on drop. -#[derive(Debug)] +/// Guard that restores `NINJA_ENV` to its previous value and holds `EnvLock` for +/// its entire lifetime to prevent parallel mutation races. +/// +/// Invariants (mirroring `PathGuard`): +/// - `override_ninja_env` acquires `EnvLock` once and stores it in `_lock`. +/// - `inner` is created with `lock_on_drop = false`, so it restores the value +/// without attempting to re-lock. +/// - Field order matters: `inner` drops before `_lock`, ensuring the restore +/// runs while the lock is still held. pub struct NinjaEnvGuard { inner: EnvGuard, + _lock: EnvLock, } /// Override the `NINJA_ENV` variable with `path`, returning a guard that resets it. /// /// In Rust 2024 `std::env::set_var` is `unsafe` because it mutates process-global -/// state. `EnvLock` serialises the mutation and the guard restores the prior -/// value, confining the unsafety to the scope of the guard. +/// state. `EnvLock` serialises the mutation and the guard retains the lock for +/// its entire lifetime, restoring the prior value on drop. Holding the lock +/// prevents parallel tests from interleaving writes to `NINJA_ENV`. /// /// # Examples /// @@ -236,12 +249,15 @@ pub struct NinjaEnvGuard { /// assert!(std::env::var(NINJA_ENV).is_err()); /// ``` pub fn override_ninja_env(env: &impl EnvMut, path: &Path) -> NinjaEnvGuard { - let _lock = EnvLock::acquire(); + let lock = EnvLock::acquire(); let original = env.raw(NINJA_ENV).ok().map(OsString::from); - // SAFETY: `EnvLock` serialises the mutation and the guard restores on drop. + // SAFETY: `EnvLock` is held for the guard's lifetime; `with_env_and_lock` + // restores the original value on drop without re-locking, avoiding parallel + // races while the guard is alive. unsafe { env.set_var(NINJA_ENV, path.as_os_str()) }; NinjaEnvGuard { - inner: EnvGuard::new(NINJA_ENV, original), + inner: EnvGuard::with_env_and_lock(NINJA_ENV, original, StdEnv::default(), false), + _lock: lock, } } diff --git a/tests/runner_tests.rs b/tests/runner_tests.rs index cb7cdddf..3b64cd23 100644 --- a/tests/runner_tests.rs +++ b/tests/runner_tests.rs @@ -4,47 +4,61 @@ use anyhow::{Context, Result, bail, ensure}; use netsuke::cli::{BuildArgs, Cli, Commands}; use netsuke::runner::{BuildTargets, run, run_ninja}; use rstest::{fixture, rstest}; -use serial_test::serial; use std::path::{Path, PathBuf}; use test_support::{ check_ninja, - env::{SystemEnv, override_ninja_env, prepend_dir_to_path}, + env::{NinjaEnvGuard, SystemEnv, override_ninja_env, prepend_dir_to_path}, fake_ninja, - path_guard::PathGuard, }; -/// Fixture: Put a fake `ninja` (that checks for a build file) on `PATH`. +/// Fixture: point `NINJA_ENV` at a fake `ninja` that validates `-f` files. /// -/// 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. +/// Using `NINJA_ENV` avoids mutating `PATH`, letting tests run in parallel +/// without trampling each other's environment. /// -/// Returns: (tempdir holding ninja, `ninja_path`, PATH guard) +/// Returns: (tempdir holding ninja, `NINJA_ENV` guard) #[fixture] -fn ninja_in_path() -> Result<(tempfile::TempDir, PathBuf, PathGuard)> { +fn ninja_in_env() -> Result<(tempfile::TempDir, PathBuf, NinjaEnvGuard)> { let (ninja_dir, ninja_path) = check_ninja::fake_ninja_check_build_file()?; let env = SystemEnv::new(); - let guard = prepend_dir_to_path(&env, ninja_dir.path())?; + let guard = override_ninja_env(&env, ninja_path.as_path()); Ok((ninja_dir, ninja_path, guard)) } -/// Fixture: Put a fake `ninja` with a specific exit code on `PATH`. +/// Fixture: point `NINJA_ENV` at a fake `ninja` with a configurable exit code. /// -/// 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) +/// Returns: (tempdir holding ninja, `NINJA_ENV` guard) #[fixture] fn ninja_with_exit_code( #[default(0u8)] exit_code: u8, -) -> Result<(tempfile::TempDir, PathBuf, PathGuard)> { +) -> Result<(tempfile::TempDir, PathBuf, NinjaEnvGuard)> { let (ninja_dir, ninja_path) = fake_ninja(exit_code)?; let env = SystemEnv::new(); - let guard = prepend_dir_to_path(&env, ninja_dir.path())?; + let guard = override_ninja_env(&env, ninja_path.as_path()); Ok((ninja_dir, ninja_path, guard)) } +/// Shared setup for tests that rely on `NINJA_ENV`. +/// +/// Returns the fake ninja directory, temp project directory, constructed CLI, +/// and the guard keeping `NINJA_ENV` set for the test duration. +fn setup_ninja_env_test() -> Result<( + tempfile::TempDir, + PathBuf, + tempfile::TempDir, + Cli, + NinjaEnvGuard, +)> { + let (ninja_dir, ninja_path, guard) = ninja_in_env()?; + let (temp, manifest_path) = create_test_manifest()?; + let cli = Cli { + file: manifest_path.clone(), + directory: Some(temp.path().to_path_buf()), + ..Cli::default() + }; + Ok((ninja_dir, ninja_path, temp, cli, guard)) +} + /// Create a temporary project with a Netsukefile from `minimal.yml`. fn create_test_manifest() -> Result<(tempfile::TempDir, PathBuf)> { let temp = tempfile::tempdir().context("create temp dir for test manifest")?; @@ -101,15 +115,8 @@ fn run_ninja_not_found() -> Result<()> { } #[rstest] -#[serial] fn run_executes_ninja_without_persisting_file() -> Result<()> { - let (_ninja_dir, ninja_path, _guard) = ninja_in_path()?; - let (temp, manifest_path) = create_test_manifest()?; - let cli = Cli { - file: manifest_path.clone(), - directory: Some(temp.path().to_path_buf()), - ..Cli::default() - }; + let (_ninja_dir, _ninja_path, temp, cli, _guard) = setup_ninja_env_test()?; run(&cli).context("expected run to succeed without emit path")?; @@ -118,17 +125,13 @@ fn run_executes_ninja_without_persisting_file() -> Result<()> { !temp.path().join("build.ninja").exists(), "build.ninja should not persist when emit path unset" ); - - // Drop the fake ninja artefacts. PATH is restored by guard drop. - drop(ninja_path); Ok(()) } #[cfg(unix)] -#[serial] #[rstest] fn run_build_with_emit_keeps_file() -> Result<()> { - let (_ninja_dir, ninja_path, _guard) = ninja_in_path()?; + let (_ninja_dir, _ninja_path, _guard) = ninja_in_env()?; let (temp, manifest_path) = create_test_manifest()?; let emit_path = temp.path().join("emitted.ninja"); let cli = Cli { @@ -158,17 +161,13 @@ fn run_build_with_emit_keeps_file() -> Result<()> { !temp.path().join("build.ninja").exists(), "build.ninja should not remain when emit path provided" ); - - // Drop the fake ninja artefacts. PATH is restored by guard drop. - drop(ninja_path); Ok(()) } #[cfg(unix)] -#[serial] #[rstest] fn run_build_with_emit_creates_parent_dirs() -> Result<()> { - let (_ninja_dir, ninja_path, _guard) = ninja_with_exit_code(0)?; + let (_ninja_dir, _ninja_path, _guard) = ninja_with_exit_code(0)?; let (temp, manifest_path) = create_test_manifest()?; let nested_dir = temp.path().join("nested").join("dir"); let emit_path = nested_dir.join("emitted.ninja"); @@ -189,9 +188,6 @@ fn run_build_with_emit_creates_parent_dirs() -> Result<()> { run(&cli).context("expected run to succeed with nested emit path")?; ensure!(emit_path.exists(), "emit path should be created"); ensure!(nested_dir.exists(), "nested directory should be created"); - - // Drop the fake ninja artefacts. PATH is restored by guard drop. - drop(ninja_path); Ok(()) } @@ -242,11 +238,13 @@ fn run_manifest_subcommand_accepts_relative_manifest_path() -> Result<()> { } #[test] -#[serial] fn run_respects_env_override_for_ninja() -> Result<()> { - let (temp_dir, ninja_path) = fake_ninja(0u8)?; + let (_temp_dir_env, ninja_env_path) = fake_ninja(0u8)?; + let (temp_dir_path, _ninja_path_on_path) = fake_ninja(1u8)?; let env = SystemEnv::new(); - let guard = override_ninja_env(&env, &ninja_path); + let _path_guard = + prepend_dir_to_path(&env, temp_dir_path.path()).context("prepend failing ninja to PATH")?; + let _env_guard = override_ninja_env(&env, &ninja_env_path); let (temp, manifest_path) = create_test_manifest()?; let cli = Cli { file: manifest_path.clone(), @@ -254,9 +252,34 @@ fn run_respects_env_override_for_ninja() -> Result<()> { ..Cli::default() }; - run(&cli).context("expected run to use overridden NINJA_ENV")?; - drop(guard); - drop(ninja_path); - drop(temp_dir); + run(&cli).context("expected run to prefer NINJA_ENV over PATH entry")?; + Ok(()) +} + +#[rstest] +fn run_succeeds_with_checking_ninja_env() -> Result<()> { + let (_ninja_dir, ninja_path, _temp, cli, _guard) = setup_ninja_env_test()?; + + run(&cli).context("expected run to succeed using NINJA_ENV check binary")?; + ensure!(ninja_path.exists(), "fake ninja should remain present"); + Ok(()) +} + +#[rstest] +fn run_fails_with_failing_ninja_env() -> Result<()> { + let (_ninja_dir, _ninja_path, _guard) = ninja_with_exit_code(7)?; + let (temp, manifest_path) = create_test_manifest()?; + let cli = Cli { + file: manifest_path.clone(), + directory: Some(temp.path().to_path_buf()), + ..Cli::default() + }; + + let err = run(&cli).expect_err("expected run to fail when NINJA_ENV ninja exits non-zero"); + let messages: Vec = err.chain().map(ToString::to_string).collect(); + ensure!( + messages.iter().any(|m| m.contains("ninja exited")), + "error should report ninja exit status, got: {messages:?}" + ); Ok(()) }