Skip to content

Document NINJA_ENV usage and restore via OsString #120

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
leynos merged 1 commit into from
Aug 15, 2025
+13 −3
Merged

Document NINJA_ENV usage and restore via OsString #120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 11 additions & 1 deletion ninja_env/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,16 @@
#![forbid(unsafe_code)]

//! Shared environment constants for netsuke tests.
//! Shared environment constants used across netsuke crates (library, tests,
//! and helpers).

/// Environment variable override for the Ninja executable.
///
/// # Examples
Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai Bot Aug 15, 2025

Choose a reason for hiding this comment

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

issue (review_instructions): Add behavioural and unit tests for the new or changed feature documentation example.

You have added an example for using the NINJA_ENV constant, but there are no corresponding behavioural or unit tests in the codebase to verify this functionality. Add tests that set, check, and remove the environment variable as shown in the example to ensure the feature works as documented.

Review instructions:

Path patterns: **/*

Instructions:
For any new feature or change to an existing feature, both behavioural and unit tests are required.

///
/// ```
/// use ninja_env::NINJA_ENV;
/// std::env::set_var(NINJA_ENV, "/usr/bin/ninja");
/// assert_eq!(std::env::var(NINJA_ENV).unwrap(), "/usr/bin/ninja");
/// std::env::remove_var(NINJA_ENV);
/// ```
pub const NINJA_ENV: &str = "NETSUKE_NINJA";
4 changes: 2 additions & 2 deletions tests/ninja_env_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ fn override_ninja_env_sets_and_restores() {
#[rstest]
#[serial]
fn override_ninja_env_unset_removes_variable() {
let before = std::env::var(NINJA_ENV).ok();
Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai Bot Aug 15, 2025

Choose a reason for hiding this comment

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

suggestion (testing): Good update to use var_os for capturing environment variable.

Consider adding a test where NINJA_ENV is set to a non-UTF8 value to confirm correct handling.

let before = std::env::var_os(NINJA_ENV);
{
let _lock = EnvLock::acquire();
// SAFETY: `EnvLock` serialises mutations during setup.
Expand All @@ -47,7 +47,7 @@ fn override_ninja_env_unset_removes_variable() {
let after = std::env::var(NINJA_ENV).expect("NINJA_ENV should be set after override");
assert_eq!(after, target.to_string_lossy().as_ref());
}
assert!(std::env::var(NINJA_ENV).is_err());
Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai Bot Aug 15, 2025

Choose a reason for hiding this comment

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

suggestion (testing): Assertion updated to use var_os for checking unset variable.

Consider adding an assertion to verify that the environment variable is restored to its original value after the test, ensuring proper test isolation.

assert!(std::env::var_os(NINJA_ENV).is_none());

// Restore original global state for isolation
{
Expand Down