From d9b58b873f3ebc6dc4dd09523943b3d730836c03 Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 2 Dec 2025 19:46:12 +0000 Subject: [PATCH 1/6] refactor(tests): use NINJA_ENV for ninja binary in tests instead of PATH Replace test fixtures that put a fake ninja executable on PATH with fixtures that set NINJA_ENV environment variable instead. This avoids mutating PATH, enabling tests to run in parallel without interference. Add documentation explaining benefits of using NINJA_ENV and provide fixture patterns for controlled ninja execution during tests. Co-authored-by: terragon-labs[bot] --- docs/test-isolation-with-ninja-env.md | 53 ++++++++++++++++++++++++++ tests/runner_tests.rs | 54 +++++++++------------------ 2 files changed, 70 insertions(+), 37 deletions(-) create mode 100644 docs/test-isolation-with-ninja-env.md diff --git a/docs/test-isolation-with-ninja-env.md b/docs/test-isolation-with-ninja-env.md new file mode 100644 index 00000000..47007eb8 --- /dev/null +++ b/docs/test-isolation-with-ninja-env.md @@ -0,0 +1,53 @@ +# 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. + +## 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 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. diff --git a/tests/runner_tests.rs b/tests/runner_tests.rs index cb7cdddf..1e44104b 100644 --- a/tests/runner_tests.rs +++ b/tests/runner_tests.rs @@ -8,41 +8,35 @@ 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}, 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, 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())?; - Ok((ninja_dir, ninja_path, guard)) + let guard = override_ninja_env(&env, ninja_path.as_path()); + Ok((ninja_dir, 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, NinjaEnvGuard)> { let (ninja_dir, ninja_path) = fake_ninja(exit_code)?; let env = SystemEnv::new(); - let guard = prepend_dir_to_path(&env, ninja_dir.path())?; - Ok((ninja_dir, ninja_path, guard)) + let guard = override_ninja_env(&env, ninja_path.as_path()); + Ok((ninja_dir, guard)) } /// Create a temporary project with a Netsukefile from `minimal.yml`. @@ -101,9 +95,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 (_ninja_dir, _guard) = ninja_in_env()?; let (temp, manifest_path) = create_test_manifest()?; let cli = Cli { file: manifest_path.clone(), @@ -118,17 +111,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, _guard) = ninja_in_env()?; let (temp, manifest_path) = create_test_manifest()?; let emit_path = temp.path().join("emitted.ninja"); let cli = Cli { @@ -158,17 +147,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, _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 +174,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(()) } @@ -244,7 +226,7 @@ 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, ninja_path) = fake_ninja(0u8)?; let env = SystemEnv::new(); let guard = override_ninja_env(&env, &ninja_path); let (temp, manifest_path) = create_test_manifest()?; @@ -256,7 +238,5 @@ fn run_respects_env_override_for_ninja() -> Result<()> { run(&cli).context("expected run to use overridden NINJA_ENV")?; drop(guard); - drop(ninja_path); - drop(temp_dir); Ok(()) } From 962d0e87591b6a3e22c7845192380e38ae907327 Mon Sep 17 00:00:00 2001 From: Leynos Date: Wed, 3 Dec 2025 12:23:32 +0000 Subject: [PATCH 2/6] test(runner): add test to verify NINJA_ENV overrides PATH Add a test that ensures the NINJA_ENV environment variable takes precedence over any ninja executable on PATH. This involves placing a failing fake ninja on PATH and a working fake ninja via NINJA_ENV, verifying that the latter is respected during builds. Also update documentation to clarify NINJA_ENV precedence behavior. Co-authored-by: terragon-labs[bot] --- docs/test-isolation-with-ninja-env.md | 7 +++++++ tests/runner_tests.rs | 14 +++++++------- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/docs/test-isolation-with-ninja-env.md b/docs/test-isolation-with-ninja-env.md index 47007eb8..b9206e8e 100644 --- a/docs/test-isolation-with-ninja-env.md +++ b/docs/test-isolation-with-ninja-env.md @@ -51,3 +51,10 @@ fn run_build_uses_fake_ninja( 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/tests/runner_tests.rs b/tests/runner_tests.rs index 1e44104b..253cd0d3 100644 --- a/tests/runner_tests.rs +++ b/tests/runner_tests.rs @@ -4,11 +4,10 @@ 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::{NinjaEnvGuard, SystemEnv, override_ninja_env}, + env::{NinjaEnvGuard, SystemEnv, override_ninja_env, prepend_dir_to_path}, fake_ninja, }; @@ -224,11 +223,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(), @@ -236,7 +237,6 @@ fn run_respects_env_override_for_ninja() -> Result<()> { ..Cli::default() }; - run(&cli).context("expected run to use overridden NINJA_ENV")?; - drop(guard); + run(&cli).context("expected run to prefer NINJA_ENV over PATH entry")?; Ok(()) } From bfd54eb298df4cb23b16bd322ca695eab04a21a8 Mon Sep 17 00:00:00 2001 From: Leynos Date: Wed, 3 Dec 2025 20:10:02 +0000 Subject: [PATCH 3/6] fix(env): hold EnvLock while overriding NINJA_ENV to prevent races The NinjaEnvGuard now retains the EnvLock for its entire lifetime, ensuring that parallel mutations of the NINJA_ENV environment variable are serialized. This prevents race conditions when tests override NINJA_ENV concurrently. The guard restores the prior value on drop and holds the lock to prevent interleaving writes, confining unsafety to the guard's scope. Co-authored-by: terragon-labs[bot] --- test_support/src/env.rs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/test_support/src/env.rs b/test_support/src/env.rs index b6e3b53f..ce1755f7 100644 --- a/test_support/src/env.rs +++ b/test_support/src/env.rs @@ -207,17 +207,19 @@ 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. 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 +238,13 @@ 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. unsafe { env.set_var(NINJA_ENV, path.as_os_str()) }; NinjaEnvGuard { - inner: EnvGuard::new(NINJA_ENV, original), + inner: EnvGuard::new_unlocked(NINJA_ENV, original), + _lock: lock, } } From dadad61b5a780df86ee02129463abc65ff44fb29 Mon Sep 17 00:00:00 2001 From: Leynos Date: Wed, 3 Dec 2025 21:40:13 +0000 Subject: [PATCH 4/6] test(runner): add tests for NINJA_ENV environment variable handling Added integration tests verifying behavior when using the NINJA_ENV environment variable. Tests cover the successful execution of the CLI with a fake ninja binary and failure when the ninja binary exits with a non-zero status. Also improved safety comments and refined NinjaEnvGuard implementation in env.rs. Co-authored-by: terragon-labs[bot] --- docs/test-isolation-with-ninja-env.md | 3 ++- test_support/src/env.rs | 12 ++++++--- tests/runner_tests.rs | 37 +++++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 4 deletions(-) diff --git a/docs/test-isolation-with-ninja-env.md b/docs/test-isolation-with-ninja-env.md index b9206e8e..9e7d6128 100644 --- a/docs/test-isolation-with-ninja-env.md +++ b/docs/test-isolation-with-ninja-env.md @@ -39,7 +39,8 @@ Inject the fixture into tests that need a controlled Ninja binary: fn run_build_uses_fake_ninja( (_, _guard): (tempfile::TempDir, NinjaEnvGuard), ) { - // run the CLI here; the guard restores NINJA_ENV on drop + // run the command-line interface (CLI) here; the guard restores NINJA_ENV + // on drop } ``` diff --git a/test_support/src/env.rs b/test_support/src/env.rs index ce1755f7..7886a2d8 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; @@ -240,10 +244,12 @@ pub struct NinjaEnvGuard { pub fn override_ninja_env(env: &impl EnvMut, path: &Path) -> NinjaEnvGuard { 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_unlocked(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 253cd0d3..c248051d 100644 --- a/tests/runner_tests.rs +++ b/tests/runner_tests.rs @@ -240,3 +240,40 @@ fn run_respects_env_override_for_ninja() -> Result<()> { 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, _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() + }; + + run(&cli).context("expected run to succeed using NINJA_ENV check binary")?; + ensure!( + ninja_dir.path().join("ninja").exists(), + "fake ninja should remain present" + ); + Ok(()) +} + +#[rstest] +fn run_fails_with_failing_ninja_env() -> Result<()> { + let (_ninja_dir, _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(()) +} From 907453cd897c2b3dbe873396d232879afd502b9c Mon Sep 17 00:00:00 2001 From: Leynos Date: Wed, 3 Dec 2025 21:56:52 +0000 Subject: [PATCH 5/6] refactor(tests): extract setup function for tests using NINJA_ENV Refactored repeated setup code in runner_tests.rs into a shared helper function `setup_ninja_env_test`. This new function consolidates the creation of a fake ninja directory, temporary project directory, CLI construction, and environment guard setup for tests that rely on the `NINJA_ENV` environment variable. The change improves code reuse and readability in the test suite. Co-authored-by: terragon-labs[bot] --- tests/runner_tests.rs | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/tests/runner_tests.rs b/tests/runner_tests.rs index c248051d..e31f76eb 100644 --- a/tests/runner_tests.rs +++ b/tests/runner_tests.rs @@ -38,6 +38,21 @@ fn ninja_with_exit_code( Ok((ninja_dir, 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, tempfile::TempDir, Cli, NinjaEnvGuard)> { + let (ninja_dir, 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, 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")?; @@ -95,13 +110,7 @@ fn run_ninja_not_found() -> Result<()> { #[rstest] fn run_executes_ninja_without_persisting_file() -> Result<()> { - let (_ninja_dir, _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() - }; + let (_ninja_dir, temp, cli, _guard) = setup_ninja_env_test()?; run(&cli).context("expected run to succeed without emit path")?; @@ -243,13 +252,7 @@ fn run_respects_env_override_for_ninja() -> Result<()> { #[rstest] fn run_succeeds_with_checking_ninja_env() -> Result<()> { - let (ninja_dir, _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() - }; + let (ninja_dir, _temp, cli, _guard) = setup_ninja_env_test()?; run(&cli).context("expected run to succeed using NINJA_ENV check binary")?; ensure!( From 275c7fde0f774b0d35c9f99bcb57d5e8850f1838 Mon Sep 17 00:00:00 2001 From: Leynos Date: Thu, 4 Dec 2025 01:05:42 +0000 Subject: [PATCH 6/6] refactor(tests): pass ninja_path in fixtures to better manage NINJA_ENV tests - Updated ninja_in_env and ninja_with_exit_code fixtures to return PathBuf for ninja_path alongside NinjaEnvGuard. - Modified setup_ninja_env_test and tests to use the ninja_path for existence checks and improved clarity. - Enhanced NinjaEnvGuard docs to clarify lock acquisition and restore order. - Added documentation on EnvLock preventing parallel test NINJA_ENV mutation races. This refactor clarifies test fixtures and ensures robust handling of environment variables in tests. Co-authored-by: terragon-labs[bot] --- docs/test-isolation-with-ninja-env.md | 2 ++ test_support/src/env.rs | 7 ++++++ tests/runner_tests.rs | 35 +++++++++++++++------------ 3 files changed, 28 insertions(+), 16 deletions(-) diff --git a/docs/test-isolation-with-ninja-env.md b/docs/test-isolation-with-ninja-env.md index 9e7d6128..80285de8 100644 --- a/docs/test-isolation-with-ninja-env.md +++ b/docs/test-isolation-with-ninja-env.md @@ -11,6 +11,8 @@ each other's environment. - `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 diff --git a/test_support/src/env.rs b/test_support/src/env.rs index 7886a2d8..f652c600 100644 --- a/test_support/src/env.rs +++ b/test_support/src/env.rs @@ -213,6 +213,13 @@ pub fn prepend_dir_to_path(env: &impl EnvMut, dir: &Path) -> Result { /// 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, diff --git a/tests/runner_tests.rs b/tests/runner_tests.rs index e31f76eb..3b64cd23 100644 --- a/tests/runner_tests.rs +++ b/tests/runner_tests.rs @@ -18,11 +18,11 @@ use test_support::{ /// /// Returns: (tempdir holding ninja, `NINJA_ENV` guard) #[fixture] -fn ninja_in_env() -> Result<(tempfile::TempDir, NinjaEnvGuard)> { +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 = override_ninja_env(&env, ninja_path.as_path()); - Ok((ninja_dir, guard)) + Ok((ninja_dir, ninja_path, guard)) } /// Fixture: point `NINJA_ENV` at a fake `ninja` with a configurable exit code. @@ -31,26 +31,32 @@ fn ninja_in_env() -> Result<(tempfile::TempDir, NinjaEnvGuard)> { #[fixture] fn ninja_with_exit_code( #[default(0u8)] exit_code: u8, -) -> Result<(tempfile::TempDir, NinjaEnvGuard)> { +) -> Result<(tempfile::TempDir, PathBuf, NinjaEnvGuard)> { let (ninja_dir, ninja_path) = fake_ninja(exit_code)?; let env = SystemEnv::new(); let guard = override_ninja_env(&env, ninja_path.as_path()); - Ok((ninja_dir, guard)) + 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, tempfile::TempDir, Cli, NinjaEnvGuard)> { - let (ninja_dir, guard) = ninja_in_env()?; +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, temp, cli, guard)) + Ok((ninja_dir, ninja_path, temp, cli, guard)) } /// Create a temporary project with a Netsukefile from `minimal.yml`. @@ -110,7 +116,7 @@ fn run_ninja_not_found() -> Result<()> { #[rstest] fn run_executes_ninja_without_persisting_file() -> Result<()> { - let (_ninja_dir, temp, cli, _guard) = setup_ninja_env_test()?; + let (_ninja_dir, _ninja_path, temp, cli, _guard) = setup_ninja_env_test()?; run(&cli).context("expected run to succeed without emit path")?; @@ -125,7 +131,7 @@ fn run_executes_ninja_without_persisting_file() -> Result<()> { #[cfg(unix)] #[rstest] fn run_build_with_emit_keeps_file() -> Result<()> { - let (_ninja_dir, _guard) = ninja_in_env()?; + 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 { @@ -161,7 +167,7 @@ fn run_build_with_emit_keeps_file() -> Result<()> { #[cfg(unix)] #[rstest] fn run_build_with_emit_creates_parent_dirs() -> Result<()> { - let (_ninja_dir, _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"); @@ -252,19 +258,16 @@ fn run_respects_env_override_for_ninja() -> Result<()> { #[rstest] fn run_succeeds_with_checking_ninja_env() -> Result<()> { - let (ninja_dir, _temp, cli, _guard) = setup_ninja_env_test()?; + 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_dir.path().join("ninja").exists(), - "fake ninja should remain present" - ); + ensure!(ninja_path.exists(), "fake ninja should remain present"); Ok(()) } #[rstest] fn run_fails_with_failing_ninja_env() -> Result<()> { - let (_ninja_dir, _guard) = ninja_with_exit_code(7)?; + 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(),