Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions tests/cli_world_tests.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
//! Tests for environment restoration in `CliWorld`.

mod world;
use mockable::MockEnv;
use world::CliWorld;

#[test]
fn drop_restores_path() {
Comment on lines +7 to +8
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Serialise this test to avoid global PATH races

This test mutates the process-wide PATH and can race with others. Serialise it.

Either:

  • Add serial_test and annotate:
+use serial_test::serial;
@@
-#[test]
+#[test]
+#[serial]
 fn drop_restores_path() {

Add to dev-dependencies:

  • serial_test = "3"
    Or:
  • Configure CI to run env-mutating tests with --test-threads=1.

MCP Context Analysis:

📊 Deepwiki:


🏁 Script executed:

#!/bin/bash
set -e

echo "Locate the drop_restores_path test file"
rg --files-with-matches "fn drop_restores_path" -n .

echo "Show context around the test definition"
rg -n "fn drop_restores_path" -A10 -B5 tests/cli_world_tests.rs

echo "Search for environment variable modifications in the test"
rg -nE "env::(set_var|remove_var)" -n tests/cli_world_tests.rs

Length of output: 1167


🏁 Script executed:

#!/bin/bash
set -e

echo "Locate the Drop implementation for CliWorld"
rg --files-with-matches 'impl Drop for CliWorld' -n .

echo "Display the Drop implementation context"
file=$(rg --files-with-matches 'impl Drop for CliWorld' -n . | head -1)
rg -n 'impl Drop for CliWorld' -A20 -B5 "$file"

Length of output: 910


Serialise tests that restore PATH to prevent inter-test races

The drop_restores_path test invokes CliWorld’s Drop impl (tests/world.rs:67–75), which uses unsafe { std::env::set_var("PATH", …) }. That mutates the process-wide PATH and races with parallel tests. Apply one of the following fixes:

• Add serial_test to dev-dependencies and annotate:

 // tests/cli_world_tests.rs
+use serial_test::serial;
 #[test]
+#[serial]
 fn drop_restores_path() {
     …
 }

• Or configure CI to run environment-mutating tests single-threaded:

cargo test -- --test-threads=1
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
#[test]
fn drop_restores_path() {
// at the top of tests/cli_world_tests.rs
+ use serial_test::serial;
#[test]
+ #[serial]
fn drop_restores_path() {
// … existing test code …
}
🤖 Prompt for AI Agents
In tests/cli_world_tests.rs around lines 7 to 8, the drop_restores_path test
mutates the process-wide PATH via CliWorld::Drop and races with other tests; fix
by making the test run serially: either add the serial_test crate to
dev-dependencies and annotate the test with #[serial] (or #[serial(env)] if you
want a named serial group for environment-affecting tests), or ensure CI runs
tests single-threaded by invoking cargo test with -- --test-threads=1 (update CI
pipeline/test job to include that flag). Ensure you choose one approach and
apply the annotation or CI change consistently for all tests that mutate
process-wide environment.

let original = std::env::var("PATH").unwrap_or_default();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Decide whether an absent PATH should fail the test

Using unwrap_or_default hides environments without PATH and may weaken the assertion. If the repo assumes PATH is always set under CI, prefer:

let original = std::env::var("PATH").expect("PATH must be set for this test");
🤖 Prompt for AI Agents
In tests/cli_world_tests.rs around line 9, the test uses
std::env::var("PATH").unwrap_or_default(), which silently continues when PATH is
missing and weakens the assertion; change this to
std::env::var("PATH").expect("PATH must be set for this test") so the test fails
fast and loudly when PATH is absent, providing a clear failure message for CI or
local runs.

{
let mut env = MockEnv::new();
let original_clone = original.clone();
env.expect_raw()
.withf(|key| key == "PATH")
.returning(move |_| Ok(original_clone.clone()));
let mut world = CliWorld::default();
world.env = Box::new(env);
world.original_path = Some(world.env.raw("PATH").expect("retrieve PATH").into());
unsafe {
std::env::set_var("PATH", "temp-path");
}
Comment thread
leynos marked this conversation as resolved.
}
assert_eq!(
std::env::var("PATH").expect("read PATH after drop"),
original
);
}
38 changes: 4 additions & 34 deletions tests/cucumber.rs
Original file line number Diff line number Diff line change
@@ -1,38 +1,8 @@
//! Cucumber test runner and world state.
//! Cucumber test runner.

use cucumber::World;

/// Shared state for Cucumber scenarios.
#[derive(Debug, Default, World)]
pub struct CliWorld {
pub cli: Option<netsuke::cli::Cli>,
pub cli_error: Option<String>,
pub manifest: Option<netsuke::ast::NetsukeManifest>,
pub manifest_error: Option<String>,
pub build_graph: Option<netsuke::ir::BuildGraph>,
/// Generated Ninja file content.
pub ninja: Option<String>,
/// Status of the last process execution (true for success, false for
/// failure).
pub run_status: Option<bool>,
/// Error message from the last failed process execution.
pub run_error: Option<String>,
/// Temporary directory handle for test isolation.
pub temp: Option<tempfile::TempDir>,
/// Original `PATH` value restored after each scenario.
pub original_path: Option<std::ffi::OsString>,
}

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);
}
}
}
}
mod world;
use cucumber::World as _;
pub use world::CliWorld;

mod steps;
mod support;
Expand Down
11 changes: 8 additions & 3 deletions tests/steps/process_steps.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
use crate::{CliWorld, support};
use cucumber::{given, then, when};
use netsuke::runner::{self, BuildTargets, NINJA_PROGRAM};
use std::ffi::OsString;
use std::fs;
use std::path::{Path, PathBuf};
use tempfile::{NamedTempFile, TempDir};
Expand All @@ -13,9 +14,13 @@ use tempfile::{NamedTempFile, TempDir};
reason = "helper owns path for simplicity"
)]
fn install_test_ninja(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());
let original = world.original_path.get_or_insert_with(|| {
world
.env
.raw("PATH")
.map(OsString::from)
.unwrap_or_default()
});
Comment on lines 16 to +23
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Avoid lossy conversions when handling PATH

Stop converting OsString to UTF-8 via to_string_lossy when manipulating PATH. The change above to join_paths keeps raw OS encodings intact, which is crucial on non-UTF-8 systems.

🤖 Prompt for AI Agents
In tests/steps/process_steps.rs around lines 16 to 23, the code converts PATH
entries through UTF-8 lossy conversion; instead preserve raw OS strings by
avoiding to_string_lossy and using OsString/PathBuf directly: call
world.env.raw("PATH") -> Option<OsString> (or use std::env::split_paths on
OsString via OsStr) and when modifying/setting PATH use std::env::join_paths or
world.env.set with a Vec<OsString>/Iterator of PathBufs so the PATH is
reconstructed with join_paths (or equivalent) to retain original non-UTF-8
encodings rather than converting to UTF-8 strings.


let new_path = format!("{}:{}", dir.path().display(), original.to_string_lossy());
// SAFETY: nightly marks `set_var` as unsafe; override path for test isolation.
Expand Down
76 changes: 76 additions & 0 deletions tests/world.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
//! Shared test world for Cucumber scenarios.

use cucumber::World;
use mockable::{Env, MockEnv};

/// Shared state for Cucumber scenarios.
#[derive(World)]
pub struct CliWorld {
pub cli: Option<netsuke::cli::Cli>,
pub cli_error: Option<String>,
pub manifest: Option<netsuke::ast::NetsukeManifest>,
pub manifest_error: Option<String>,
pub build_graph: Option<netsuke::ir::BuildGraph>,
/// Generated Ninja file content.
pub ninja: Option<String>,
/// Status of the last process execution (true for success, false for
/// failure).
pub run_status: Option<bool>,
/// Error message from the last failed process execution.
pub run_error: Option<String>,
/// Temporary directory handle for test isolation.
pub temp: Option<tempfile::TempDir>,
/// Mockable environment access.
pub env: Box<dyn Env>,
/// Original `PATH` value restored after each scenario.
pub original_path: Option<std::ffi::OsString>,
}

impl std::fmt::Debug for CliWorld {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.debug_struct("CliWorld")
.field("cli", &self.cli)
.field("cli_error", &self.cli_error)
.field("manifest", &self.manifest)
.field("manifest_error", &self.manifest_error)
.field("build_graph", &self.build_graph)
.field("ninja", &self.ninja)
.field("run_status", &self.run_status)
.field("run_error", &self.run_error)
.field("temp", &self.temp)
.field("env", &"<env>")
.field("original_path", &self.original_path)
.finish()
}
}

impl Default for CliWorld {
fn default() -> Self {
let mut env = MockEnv::new();
env.expect_raw().returning(|key| std::env::var(key));
Self {
cli: None,
cli_error: None,
manifest: None,
manifest_error: None,
build_graph: None,
ninja: None,
run_status: None,
run_error: None,
temp: None,
env: Box::new(env),
original_path: None,
}
}
}

impl Drop for CliWorld {
fn drop(&mut self) {
if let Some(path) = self.original_path.take() {
// SAFETY: Rust 2024 marks `set_var` as unsafe. Dropping `CliWorld`
// reinstates the original `PATH`, ensuring scenarios cannot leak
// environment changes into subsequent tests.
unsafe { std::env::set_var("PATH", path) }
}
Comment on lines +69 to +74
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Remove unnecessary unsafe and correct the misleading comment

std::env::set_var is safe on stable; the unsafe block triggers the unused_unsafe lint and violates the “avoid unsafe unless necessary” guideline. Remove the unsafe and fix the comment.

Apply this diff:

-        if let Some(path) = self.original_path.take() {
-            // SAFETY: Rust 2024 marks `set_var` as unsafe. Dropping `CliWorld`
-            // reinstates the original `PATH`, ensuring scenarios cannot leak
-            // environment changes into subsequent tests.
-            unsafe { std::env::set_var("PATH", path) }
-        }
+        if let Some(path) = self.original_path.take() {
+            // Restore the original PATH for this scenario to avoid cross-scenario leakage.
+            std::env::set_var("PATH", path);
+        }

Verify the claim about std::env::set_var being unsafe and find any other occurrences of this pattern:


🏁 Script executed:

#!/bin/bash
# 1) Confirm whether set_var is ever unsafe on stable/nightly in Rust docs
# 2) Locate all uses of set_var wrapped in `unsafe` to clean them up

# 1) Web-check is requested below.
# 2) Repo-wide search:
rg -n -S $'unsafe\\s*\\{\\s*std::env::set_var' -A 2

Length of output: 680


🌐 Web query:

Is std::env::set_var marked unsafe on stable Rust or in the 2024 edition (or nightly)?

💡 Result:

Short answer: Yes—std::env::set_var is marked unsafe in the Rust 2024 edition (including on nightly when using edition 2024), but it remains safe in earlier editions on stable and nightly. On Windows it is always sound to call, but the API is still unsafe in 2024 for cross-platform consistency and because the unsoundness exists on other OSes.[6][2]

Key points

  • In Rust 2024, std::env::{set_var, remove_var} are newly marked as unsafe to reflect long-known unsoundness of mutating the process environment in multithreaded programs on non-Windows platforms.[6]
  • Prior editions (2015/2018/2021) do not require unsafe to call these functions; the edition boundary avoids a breaking change for existing code.[6]
  • The function docs explicitly state: it is safe in single-threaded programs; always safe on Windows; in multithreaded programs on other OSes, you must ensure no other threads are reading or writing the environment (a requirement that is practically impossible to uphold), so the only safe option there is to avoid set_var/remove_var.[2]
  • Tooling and ecosystem notes:
    • The Rust Edition Guide documents the change under “Newly unsafe functions.”[6]
    • Some IDEs or linters may surface this as unsafe even when the crate is still in 2021 edition; that’s a tooling issue, not the language rule for older editions.[1]
    • Rust compiler/internal code and community discussions acknowledge and track the shift to unsafe in 2024.[5][7][9][10][8]

What this means for you

  • If your crate’s edition is 2021 (or earlier): calling env::set_var does not require unsafe, even on current stable or nightly.[6]
  • If your crate’s edition is 2024: you must wrap calls in an unsafe block and satisfy the safety preconditions. Prefer alternatives like Command::env when spawning child processes, or confine mutations to single-threaded phases (e.g., before starting threads or within build scripts) where you can justify soundness.[6][2]

Citations:


MCP Context Analysis:

📊 Deepwiki:


🏁 Script executed:

#!/bin/bash
# Determine the Rust edition for the main crate
rg -n 'edition' Cargo.toml

Length of output: 45


Retain unsafe and revise the SAFETY comment

std::env::set_var is newly marked unsafe in the Rust 2024 edition. Keep the unsafe block and update the comment to justify safety in our single-threaded teardown:

Locations:

  • tests/world.rs (around line 73)
  • tests/runner_tests.rs (lines 27, 44, 65)

Example diff for tests/world.rs:

-        if let Some(path) = self.original_path.take() {
-            // SAFETY: Rust 2024 marks `set_var` as unsafe. Dropping `CliWorld`
-            // reinstates the original `PATH`, ensuring scenarios cannot leak
-            // environment changes into subsequent tests.
-            unsafe { std::env::set_var("PATH", path) }
-        }
+        if let Some(path) = self.original_path.take() {
+            // SAFETY: `set_var` is unsafe in Rust 2024 due to potential data races
+            // on global environment. This runs in single-threaded teardown,
+            // restoring PATH to avoid cross-scenario leakage.
+            unsafe { std::env::set_var("PATH", path) }
+        }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if let Some(path) = self.original_path.take() {
// SAFETY: Rust 2024 marks `set_var` as unsafe. Dropping `CliWorld`
// reinstates the original `PATH`, ensuring scenarios cannot leak
// environment changes into subsequent tests.
unsafe { std::env::set_var("PATH", path) }
}
if let Some(path) = self.original_path.take() {
// SAFETY: `set_var` is unsafe in Rust 2024 due to potential data races
// on the global environment. This runs in single-threaded teardown,
// restoring PATH to avoid cross-scenario leakage.
unsafe { std::env::set_var("PATH", path) }
}
🤖 Prompt for AI Agents
In tests/world.rs around lines 69 to 74 (and also update tests/runner_tests.rs
at lines 27, 44, 65), retain the unsafe block around std::env::set_var and
revise the SAFETY comments to justify why the unsafe call is sound: state that
teardown runs single-threaded in tests (no concurrent env access), we restore
the original PATH taken from self.original_path, and no other code observes a
partially-updated environment; update each comment to explicitly assert
single-threaded teardown and restoration semantics as the reason the unsafe
set_var is safe.

}
}
Comment on lines +67 to +76
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Serialise environment mutation to avoid cross-test races

Mutating process-wide environment variables is inherently racy under parallel test execution. Ensure Cucumber scenarios and any unit/integration tests that set PATH run serially, or guard set_var in Drop with a global test-only mutex to avoid interference.

Would you like a small Env lock helper (tests-only) using a static Mutex or once_cell to guard PATH mutations?


MCP Context Analysis:

📊 Deepwiki:


Run a quick search to inspect environment mutations and test definitions:


🏁 Script executed:

#!/bin/bash
set -eo pipefail

echo "1. Locate the Drop impl for CliWorld:"
rg "impl Drop for CliWorld" -n .

echo "2. Find all PATH modifications via std::env::set_var:"
rg "std::env::set_var" -n .

echo "3. List all test functions in the tests/ directory:"
rg "#\\[test\\]" -n tests

echo "4. Check for Cucumber runner usage in tests:"
rg "cucumber" -n tests

Length of output: 2445


Guard all PATH mutations under a global test mutex

  • Create a test-only helper at tests/helpers/env_lock.rs:

    use std::sync::Mutex;
    
    static ENV_LOCK: Mutex<()> = Mutex::new(());
    
    pub struct EnvLock(std::sync::MutexGuard<'static, ()>);
    
    impl EnvLock {
        pub fn acquire() -> Self {
            EnvLock(ENV_LOCK.lock().unwrap())
        }
    }
  • Wrap every std::env::set_var("PATH", …) with a guard:

    fn drop(&mut self) {
    +   let _lock = EnvLock::acquire();
        if let Some(path) = self.original_path.take() {
            // SAFETY: Rust 2024 marks `set_var` as unsafe. …
            unsafe { std::env::set_var("PATH", path) }
        }
    }
  • Apply the same guard in:
    • tests/runner_tests.rs (lines 27, 44, 65)
    • tests/cli_world_tests.rs
    • tests/steps/*

Alternatively, annotate all tests that touch PATH with #[serial] from the serial_test crate. Pick one approach and apply it consistently.

🤖 Prompt for AI Agents
In tests/world.rs around lines 67 to 76, PATH mutations are not guarded which
can leak env changes across tests; create a test-only helper at
tests/helpers/env_lock.rs that provides a static Mutex and an EnvLock::acquire()
guard, then wrap every std::env::set_var("PATH", ...) and the drop that restores
PATH inside the EnvLock guard (acquire before calling set_var and hold until the
restore), and apply the same guard in tests/runner_tests.rs (lines 27, 44, 65),
tests/cli_world_tests.rs, and tests/steps/*; alternatively, choose to annotate
all tests that touch PATH with #[serial] from serial_test and apply that
consistently across these files.

Loading