Refactor ninja_with_exit_code fixture to use NINJA_ENV instead of PATH

[coderabbitai]

Description

The ninja_with_exit_code fixture in tests/runner_tests.rs currently manipulates the global PATH environment variable to make a fake ninja executable available. This should be refactored to use the NINJA_ENV environment variable instead, mirroring the approach used in other test fixtures.

Context

This refactoring was suggested during the review of PR #89: #89 (comment)

Current Implementation

The fixture currently:

• Saves and modifies the PATH environment variable
• Prepends the fake ninja directory to PATH
• Returns a cleanup closure that restores the original PATH

Proposed Change

The fixture should instead:

• Use NINJA_ENV to point directly to the fake ninja executable
• Save and restore the original NINJA_ENV value
• Avoid PATH manipulation entirely

Benefits

• Cleaner test isolation (no global PATH modification)
• Consistency with other test fixtures in the codebase
• More predictable test behaviour

Implementation Notes

• The fixture is located at lines 34-56 in tests/runner_tests.rs
• Should follow the same pattern as the run_respects_env_override_for_ninja test
• Maintain the same return tuple structure (tempfile::TempDir, PathBuf, Cleanup)

Requested by: @leynos

[leynos]

@coderabbitai looking at the codebase, is this issue still relevant? If the issue has been fixed in the code, does the documentation reflect this?

If it is still relevant, please provide an assessment of the issue criticality (critical, high, medium or low), based on risk of the issue impacting the user and the anticipated blast radius.

Once a decision has been made, apply the appropriate label.

If the issue is documentation ONLY (i.e., no code changes needed), apply the documentation label in addition to the appropriate criticality label.

Please also provide an impact of the required changes.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

I'll analyze the current codebase to determine if this issue is still relevant and assess its criticality.

Based on my analysis of the codebase, I can provide you with a comprehensive assessment:

Issue Relevance: STILL RELEVANT

The ninja_with_exit_code fixture in tests/runner_tests.rs (lines 34-56) still uses the old PATH manipulation approach and has not been refactored to use NINJA_ENV. Here's what I found:

Current Implementation Issues:

1. PATH Manipulation: The fixture still saves and modifies the global PATH environment variable
2. Inconsistency: Other parts of the codebase properly use NINJA_ENV (like in run_respects_env_override_for_ninja test)
3. Test Isolation: Global PATH modification affects test isolation

Criticality Assessment: LOW

• User Impact: This is a test-only issue with no impact on end users
• Blast Radius: Limited to test suite maintenance and developer experience
• Risk: Minimal - affects test reliability and maintainability only

Impact of Required Changes:

• Scope: Code changes only in test fixtures
• Files Affected: tests/runner_tests.rs (lines 34-56)
• Effort: Low - straightforward refactoring following existing patterns
• Benefits: Improved test isolation and consistency with existing patterns

@leynos The issue remains relevant and requires code changes to align the test fixture with the established NINJA_ENV pattern used elsewhere in the codebase. I've applied the low criticality label as this affects only test maintenance with no user-facing impact.

---

🏁 Script executed:

#!/bin/bash
# First, let's examine the current implementation of the ninja_with_exit_code fixture
echo "=== Examining tests/runner_tests.rs ==="
if [ -f "tests/runner_tests.rs" ]; then
    cat tests/runner_tests.rs
else
    echo "File tests/runner_tests.rs not found"
fi

Length of output: 7676

---

🏁 Script executed:

#!/bin/bash
# Let's also search for any references to NINJA_ENV in the codebase to understand the pattern
echo "=== Searching for NINJA_ENV usage ==="
rg -n "NINJA_ENV" --type rust

echo -e "\n=== Searching for PATH manipulation in tests ==="
rg -n -A3 -B3 "PATH.*env" tests/ --type rust

echo -e "\n=== Looking for ninja_with_exit_code fixture usage ==="
rg -n "ninja_with_exit_code" --type rust

Length of output: 3276

---

🏁 Script executed:

#!/bin/bash
# Let's check if there's a run_respects_env_override_for_ninja test mentioned in the issue
echo "=== Searching for run_respects_env_override_for_ninja test ==="
rg -n "run_respects_env_override_for_ninja" --type rust -A10 -B2

Length of output: 1061

---

🏁 Script executed:

#!/bin/bash
# Get current labels on the issue to understand what's already applied
echo "=== Current issue labels ==="
gh issue view 96 --json labels --jq '.labels[].name'

Length of output: 121

---

🏁 Script executed:

#!/bin/bash
# Apply the low criticality label to the issue
gh issue edit 96 --add-label "low"

Length of output: 78

---

@leynos I've completed my analysis of the codebase. Here's my comprehensive assessment:

Issue Status: STILL RELEVANT

The ninja_with_exit_code fixture has not been refactored and still uses the problematic PATH manipulation approach:

#[fixture]
fn ninja_with_exit_code(#[default(0u8)] exit_code: u8) -> (tempfile::TempDir, PathBuf, PathGuard) {
    let (ninja_dir, ninja_path) = fake_ninja(exit_code);
    let env = SystemEnv::new();
    let guard = prepend_dir_to_path(&env, ninja_dir.path());  // ← Still manipulating PATH
    (ninja_dir, ninja_path, guard)
}

Meanwhile, the proper NINJA_ENV pattern is already established and working correctly in the run_respects_env_override_for_ninja test:

let (temp_dir, ninja_path) = fake_ninja(0u8);
let env = SystemEnv::new();
let guard = override_ninja_env(&env, &ninja_path);  // ← Proper NINJA_ENV usage

Criticality Assessment: LOW ✅ (Label Applied)

Rationale:

• User Impact: Zero - this affects only test infrastructure
• Blast Radius: Single fixture in test suite
• Risk: Minimal - impacts test maintainability and consistency only

Required Changes Impact:

Scope: Code changes only (not documentation)

• Files: tests/runner_tests.rs (lines 36-40)
• Pattern: Replace prepend_dir_to_path with override_ninja_env
• Effort: Low - straightforward refactoring following established pattern
• Benefits:
  • Improved test isolation (no global PATH modification)
  • Consistency with other test fixtures
  • Cleaner test architecture

The refactoring should be straightforward since all the infrastructure (override_ninja_env, fake_ninja, proper guard pattern) is already in place and well-tested.