Remove no-op drops and enable parallel test execution by isolating environment

[coderabbitai]

Description

This issue tracks test improvements identified during the review of PR #89.

Tasks

• Remove explicit drop(ninja_path) calls in tests as PathBuf doesn't own external resources
• Remove #[serial] attributes from tests to enable parallel execution
• Modify test fixtures to isolate environment using NINJA_ENV instead of modifying PATH

Context

The tests currently use #[serial] to prevent race conditions when modifying the global PATH environment variable. By using NINJA_ENV within fixtures to specify the ninja binary location, tests can run in parallel safely.

Affected tests in tests/runner_tests.rs:

• run_executes_ninja_without_persisting_file
• run_build_with_emit_keeps_file
• run_build_with_emit_creates_parent_dirs

References

• PR: Create parent dirs for Ninja file #89
• Review comment: Create parent dirs for Ninja file #89 (comment)
• Requested by: @leynos

Benefits

• Faster test execution through parallelisation
• Cleaner test code without unnecessary drops
• Better test isolation

[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

🏁 Script executed:

#!/bin/bash
# First, let's check if the affected test file exists and examine its contents
fd -t f "runner_tests.rs" tests/

# If found, examine the contents to see current state
if [ -f "tests/runner_tests.rs" ]; then
    echo "=== Contents of tests/runner_tests.rs ==="
    cat tests/runner_tests.rs
else
    echo "runner_tests.rs not found, searching for test files in tests directory"
    fd -t f ".*\.rs$" tests/
fi

Length of output: 7735

---

Let me check what environment-related test utilities are available to understand the NINJA_ENV capability better:

Now let me check if GitHub CLI is available and apply the appropriate label:

@leynos The issue is still relevant. After examining the codebase, none of the three main tasks have been completed:

Current State vs Issue Requirements

❌ Remove explicit drop(ninja_path) calls: Still present in 4 tests
❌ Remove #[serial] attributes: Still present on 4 tests
❌ Use NINJA_ENV for isolation: Most tests still modify PATH directly

The affected tests (run_executes_ninja_without_persisting_file, run_build_with_emit_keeps_file, run_build_with_emit_creates_parent_dirs) all still contain the problematic patterns.

Criticality Assessment: Medium

Risk Impact: Low user impact (affects development workflow only)
Blast Radius: Limited to test execution performance and developer experience

The issue doesn't affect end users but impacts:

• Development workflow (slower test execution due to serialisation)
• Code maintainability (unnecessary drop calls)
• Test isolation quality

Impact of Required Changes

Benefits:

• Faster test suite execution through parallelisation
• Cleaner, more maintainable test code
• Better test isolation without global state mutation

Scope: Moderate refactoring of test fixtures in tests/runner_tests.rs

Effort: Low-medium (straightforward changes to existing test infrastructure)

I've applied the medium priority label to reflect the development workflow impact while acknowledging this isn't user-facing functionality.

---

🏁 Script executed:

#!/bin/bash
# Look for test support utilities to understand NINJA_ENV implementation
echo "=== Searching for test support files ==="
fd -t f ".*\.rs$" test_support/

echo -e "\n=== Looking for environment-related functions ==="
rg -n "override_ninja_env|NINJA_ENV" --type rust

Length of output: 3683

---

🏁 Script executed:

#!/bin/bash
# Check the current labels on the issue and apply the medium priority label
echo "=== Current issue state ==="
gh issue view 97 --json labels,state,title

echo -e "\n=== Applying medium priority label ==="
gh issue edit 97 --add-label "medium"

Length of output: 402

---

@leynos The issue is still relevant. After examining the codebase, none of the three main tasks have been completed:

Current State vs Issue Requirements

❌ Remove explicit drop(ninja_path) calls: Still present in 4 tests
❌ Remove #[serial] attributes: Still present on 4 tests
❌ Use NINJA_ENV for isolation: Most tests still modify PATH directly

The affected tests (run_executes_ninja_without_persisting_file, run_build_with_emit_keeps_file, run_build_with_emit_creates_parent_dirs) all still contain the problematic patterns. However, the infrastructure for NINJA_ENV isolation is already implemented - I can see override_ninja_env() utility functions and the NETSUKE_NINJA environment variable support.

Criticality Assessment: Medium

Risk Impact: Low user impact (affects development workflow only)
Blast Radius: Limited to test execution performance and developer experience

The issue doesn't affect end users but impacts:

• Development workflow (slower test execution due to serialisation)
• Code maintainability (unnecessary drop calls)
• Test isolation quality

Impact of Required Changes

Benefits:

• Faster test suite execution through parallelisation
• Cleaner, more maintainable test code
• Better test isolation without global state mutation

Scope: Moderate refactoring of test fixtures in tests/runner_tests.rs

Effort: Low-medium (straightforward changes to existing test infrastructure)

I've applied the medium priority label to reflect the development workflow impact while acknowledging this isn't user-facing functionality.

[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?

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch the issue details
echo "=== Issue #97 Details ==="
gh issue view 97 --json title,body,state,labels

echo -e "\n=== Current state of runner_tests.rs ==="
# Check if the problematic patterns still exist
cat tests/runner_tests.rs | grep -A2 -B2 "drop(ninja_path)" || echo "No drop(ninja_path) found"

echo -e "\n=== Check for #[serial] attributes ==="
cat tests/runner_tests.rs | grep "#\[serial\]" || echo "No #[serial] attributes found"

echo -e "\n=== Check for NINJA_ENV usage in tests ==="
rg -n "override_ninja_env|NINJA_ENV" tests/runner_tests.rs || echo "No NINJA_ENV usage found in runner_tests.rs"

echo -e "\n=== Search for documentation about test isolation ==="
# Check README and docs for any mention of test isolation or NINJA_ENV
fd -t f "README|CONTRIBUTING|TESTING" -e md -e txt

Length of output: 2285

---

🏁 Script executed:

#!/bin/bash
# Check README for any documentation about testing, NINJA_ENV, or test isolation
echo "=== README.md contents ==="
cat README.md

echo -e "\n=== Check for any other documentation files ==="
fd -t f -e md . | head -20

echo -e "\n=== Check if there's developer documentation ==="
fd -t f "DEVELOP|HACKING|TEST" -e md -e txt -e rst

echo -e "\n=== Look at the specific tests mentioned in the issue ==="
grep -n "run_executes_ninja_without_persisting_file\|run_build_with_emit_keeps_file\|run_build_with_emit_creates_parent_dirs" tests/runner_tests.rs

Length of output: 6247

---

🏁 Script executed:

#!/bin/bash
# Let's examine the specific three tests mentioned in the issue to verify their current state
echo "=== Test 1: run_executes_ninja_without_persisting_file ==="
sed -n '105,128p' tests/runner_tests.rs

echo -e "\n=== Test 2: run_build_with_emit_keeps_file ==="
sed -n '130,168p' tests/runner_tests.rs

echo -e "\n=== Test 3: run_build_with_emit_creates_parent_dirs ==="
sed -n '170,208p' tests/runner_tests.rs

echo -e "\n=== Check the one test that DOES use override_ninja_env ==="
sed -n '240,265p' tests/runner_tests.rs

Length of output: 4202

---

@leynos Yes, the issue is still completely relevant. None of the three tasks have been completed.

Current Status

Code Changes: ❌ Not Fixed

All three tests mentioned in the issue still contain the problematic patterns:

run_executes_ninja_without_persisting_file (Line 105):

• ❌ Has #[serial] attribute
• ❌ Uses ninja_in_path() (modifies PATH)
• ❌ Contains explicit drop(ninja_path)

run_build_with_emit_keeps_file (Line 130):

• ❌ Has #[serial] attribute
• ❌ Uses ninja_in_path() (modifies PATH)
• ❌ Contains explicit drop(ninja_path)

run_build_with_emit_creates_parent_dirs (Line 170):

• ❌ Has #[serial] attribute
• ❌ Uses ninja_with_exit_code() (modifies PATH)
• ❌ Contains explicit drop(ninja_path)

Documentation: ❌ No Coverage

The codebase has no documentation about:

• Test isolation practices
• NINJA_ENV usage
• Testing guidelines for contributors

The README.md focuses entirely on user-facing features. None of the docs in docs/ cover developer testing practices or the NINJA_ENV pattern.

Good News ✅

The infrastructure is ready—override_ninja_env() exists and is already used correctly in run_respects_env_override_for_ninja (Line 244). The three flagged tests just need to be migrated to follow that pattern.