Conversation
Reviewer's GuideThis PR adds a NinjaEnvGuard for safely overriding NINJA_ENV with guaranteed restoration via EnvLock and mockable::Env, refactors existing runner tests to use this guard instead of manual unsafe mutations, and introduces dedicated tests to verify environment restoration. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughSummarise the test helpers and tests that add a guard-based mechanism to temporarily override and restore the NINJA_ENV environment variable during tests, plus small test-support annotation and Debug derive changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant EnvLock
participant Env as MockEnv
participant NinjaEnvGuard
Test->>EnvLock: acquire()
Test->>Env: raw(NINJA_ENV) -> original?
Test->>NinjaEnvGuard: override_ninja_env(lock, env, "new")
NinjaEnvGuard->>Env: unsafe set NINJA_ENV="new"
Test->>Env: observe "new"
Note over Test,NinjaEnvGuard: scope exit / guard dropped
NinjaEnvGuard->>Env: restore original or remove NINJA_ENV
Test->>Env: observe restored state
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `tests/ninja_env_tests.rs:9` </location>
<code_context>
+mod support;
+
+#[test]
+fn override_ninja_env_restores_original() {
+ let _lock = EnvLock::acquire();
+ let mut env = MockEnv::new();
+ env.expect_raw()
+ .withf(|k| k == NINJA_ENV)
+ .returning(|_| Ok("orig".to_string()));
+
+ {
+ let _guard = override_ninja_env(&env, "new");
+ assert_eq!(std::env::var(NINJA_ENV).as_deref(), Ok("new"));
+ }
+ assert_eq!(std::env::var(NINJA_ENV).as_deref(), Ok("orig"));
+ // Clean up to avoid leaking environment state. `remove_var` is `unsafe`
+ // on Rust 2024; the lock above serialises this mutation.
+ unsafe { std::env::remove_var(NINJA_ENV) };
+}
</code_context>
<issue_to_address>
Consider adding tests for edge cases: when NINJA_ENV is unset and when it is set to an empty string.
The current test only checks when NINJA_ENV has a value. Please add cases for when NINJA_ENV is unset and when it is set to an empty string to fully verify guard behavior.
</issue_to_address>
### Comment 2
<location> `tests/support/ninja_env.rs:30` </location>
<code_context>
+/// let env = DefaultEnv::new();
+/// let _guard = override_ninja_env(&env, "/usr/bin/ninja");
+/// ```
+#[allow(dead_code, reason = "only some tests override NINJA_ENV")]
+pub fn override_ninja_env(env: &impl Env, value: &str) -> NinjaEnvGuard {
+ let original = env.raw(NINJA_ENV).ok();
+ // Callers must hold [`EnvLock`](super::env_lock::EnvLock) to serialise this
</code_context>
<issue_to_address>
Consider documenting thread-safety expectations for override_ninja_env.
Explicitly stating the thread-safety requirement in the documentation will help prevent future misuse, especially in tests where EnvLock may not be held.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
/// Set `NINJA_ENV` to `value`, returning a guard that restores the previous
/// value when dropped.
///
/// # Examples
/// ```ignore
/// use mockable::DefaultEnv;
/// use tests::support::ninja_env::override_ninja_env;
/// let env = DefaultEnv::new();
/// let _guard = override_ninja_env(&env, "/usr/bin/ninja");
/// ```
=======
/// Set `NINJA_ENV` to `value`, returning a guard that restores the previous
/// value when dropped.
///
/// # Thread Safety
///
/// This function is **not thread-safe**. Callers must hold [`EnvLock`](super::env_lock::EnvLock)
/// to serialize this mutation. Failing to do so may result in undefined behavior or
/// race conditions, especially in tests or concurrent environments.
///
/// # Examples
/// ```ignore
/// use mockable::DefaultEnv;
/// use tests::support::ninja_env::override_ninja_env;
/// let env = DefaultEnv::new();
/// let _guard = override_ninja_env(&env, "/usr/bin/ninja");
/// ```
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| fn override_ninja_env_restores_original() { | ||
| let _lock = EnvLock::acquire(); | ||
| let mut env = MockEnv::new(); | ||
| env.expect_raw() | ||
| .withf(|k| k == NINJA_ENV) | ||
| .returning(|_| Ok("orig".to_string())); | ||
|
|
||
| { | ||
| let _guard = override_ninja_env(&env, "new"); | ||
| assert_eq!(std::env::var(NINJA_ENV).as_deref(), Ok("new")); |
There was a problem hiding this comment.
suggestion (testing): Consider adding tests for edge cases: when NINJA_ENV is unset and when it is set to an empty string.
The current test only checks when NINJA_ENV has a value. Please add cases for when NINJA_ENV is unset and when it is set to an empty string to fully verify guard behavior.
| /// Set `NINJA_ENV` to `value`, returning a guard that restores the previous | ||
| /// value when dropped. | ||
| /// | ||
| /// # Examples | ||
| /// ```ignore | ||
| /// use mockable::DefaultEnv; | ||
| /// use tests::support::ninja_env::override_ninja_env; | ||
| /// let env = DefaultEnv::new(); | ||
| /// let _guard = override_ninja_env(&env, "/usr/bin/ninja"); | ||
| /// ``` |
There was a problem hiding this comment.
suggestion: Consider documenting thread-safety expectations for override_ninja_env.
Explicitly stating the thread-safety requirement in the documentation will help prevent future misuse, especially in tests where EnvLock may not be held.
| /// Set `NINJA_ENV` to `value`, returning a guard that restores the previous | |
| /// value when dropped. | |
| /// | |
| /// # Examples | |
| /// ```ignore | |
| /// use mockable::DefaultEnv; | |
| /// use tests::support::ninja_env::override_ninja_env; | |
| /// let env = DefaultEnv::new(); | |
| /// let _guard = override_ninja_env(&env, "/usr/bin/ninja"); | |
| /// ``` | |
| /// Set `NINJA_ENV` to `value`, returning a guard that restores the previous | |
| /// value when dropped. | |
| /// | |
| /// # Thread Safety | |
| /// | |
| /// This function is **not thread-safe**. Callers must hold [`EnvLock`](super::env_lock::EnvLock) | |
| /// to serialize this mutation. Failing to do so may result in undefined behavior or | |
| /// race conditions, especially in tests or concurrent environments. | |
| /// | |
| /// # Examples | |
| /// ```ignore | |
| /// use mockable::DefaultEnv; | |
| /// use tests::support::ninja_env::override_ninja_env; | |
| /// let env = DefaultEnv::new(); | |
| /// let _guard = override_ninja_env(&env, "/usr/bin/ninja"); | |
| /// ``` |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -0,0 +1,50 @@ | |||
| //! Override and restore `NINJA_ENV` for tests. | |||
| //! | |||
| //! Provides a helper that sets the `NETSUKE_NINJA` environment variable while | |||
There was a problem hiding this comment.
syntax: Documentation references NETSUKE_NINJA but the actual constant is NINJA_ENV. Should be consistent with the variable name used throughout the codebase.
| //! Provides a helper that sets the `NETSUKE_NINJA` environment variable while | |
| //! Provides a helper that sets the `NINJA_ENV` environment variable while |
| env.expect_raw() | ||
| .withf(|k| k == NINJA_ENV) | ||
| .returning(|_| Ok("orig".to_string())); |
There was a problem hiding this comment.
style: Mock expectation is set but the guard implementation calls env.raw() only once during construction. Consider verifying the mock was called exactly once.
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
tests/support/ninja_env.rs (1)
8-10: Confirm the NINJA_ENV symbol path.This module depends on netsuke::runner::NINJA_ENV. If that symbol moved, adjust the import or add a re-export. See the verification requested in tests/ninja_env_tests.rs Line 2.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
tests/ninja_env_tests.rs(1 hunks)tests/runner_tests.rs(3 hunks)tests/support/mod.rs(2 hunks)tests/support/ninja_env.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Comment why, not what. Explain assumptions, edge cases, trade-offs, or complexity. Don't echo the obvious.
Use functions and composition. Avoid repetition by extracting reusable logic. Prefer generators or comprehensions, and declarative code to imperative repetition when readable.
Small, meaningful functions. Functions must be small, clear in purpose, single responsibility, and obey command/query segregation.
Name things precisely. Use clear, descriptive variable and function names. For booleans, prefer names with is, has, or should.
Every module must begin with a module level (//! ) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Place function attributes after doc comments.
Do not use return in single-line functions.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type instead of panicking where feasible.
Prefer .expect() over .unwrap().
Use concat!() to combine long string literals rather than escaping newlines with a backslash.
Prefer single line versions of functions where appropriate.
Clippy warnings MUST be disallowed.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Keep file size manageable. No single code file may be longer than 400 lines. Long switch statements or dispatch tables should be broken up by feature and constituents colocated with targets. Large blocks of test data should be moved to external data files.
Illustrate with clear examples. Function documentation must include clear examples demonstrating the usage and outcome of the function. Test documentation should omit examples where the example serves only to reiterate the test logic.
Prefer semantic error enums. Derive std::error::Error (via the thiserror crate) for any condition the caller might inspect, retry, or map to an HTTP status.
...
Files:
tests/ninja_env_tests.rstests/support/mod.rstests/support/ninja_env.rstests/runner_tests.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Files:
tests/ninja_env_tests.rstests/support/mod.rstests/support/ninja_env.rstests/runner_tests.rs
🧬 Code Graph Analysis (2)
tests/ninja_env_tests.rs (2)
tests/support/ninja_env.rs (1)
override_ninja_env(31-38)tests/support/env_lock.rs (1)
acquire(19-21)
tests/runner_tests.rs (4)
tests/steps/process_steps.rs (1)
run(88-129)src/runner.rs (5)
run(105-126)run_ninja(348-421)new(57-59)new(74-76)new(87-89)tests/support/ninja_env.rs (1)
override_ninja_env(31-38)tests/support/env_lock.rs (1)
acquire(19-21)
🔍 MCP Research (1 server)
Deepwiki:
- Document docs/netsuke-design.md: the project’s env(var_name) Jinja function reads environment variables from the system (uses minijinja for templating). (https://deepwiki.com/search/guard-ninjaenv-overrides-with_1d5072b3-00ac-4971-9436-0fa1c81de94c)
- Minijinja integration: the codebase configures custom functions/filters on minijinja::Environment at startup, providing a natural injection point for swapping the env-reader implementation used by env(). (https://deepwiki.com/search/guard-ninjaenv-overrides-with_1d5072b3-00ac-4971-9436-0fa1c81de94c)
- Testing guidance (rstest pattern): recommended approach in the repository notes is to abstract environment access behind a trait, implement a real system reader and a mock implementation, and inject the mock into the minijinja environment for tests to control values like NINJA_ENV. (https://deepwiki.com/search/guard-ninjaenv-overrides-with_1d5072b3-00ac-4971-9436-0fa1c81de94c)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test
🔇 Additional comments (6)
tests/support/mod.rs (1)
7-7: Export the ninja_env test helper.Expose the new test helper module to call-sites. This aligns with the PR objective of centralising env override logic behind a guard.
tests/runner_tests.rs (2)
1-1: Import the guard and Env for controlled overrides.Importing DefaultEnv, run, and the new override_ninja_env helper achieves the goal of mockable, lock-guarded env overrides in tests.
Also applies to: 3-3, 12-12
225-231: Hold EnvLock while the guard drops; drop order is correct.Create _lock before _guard so that _guard drops first, while the lock is still held. This satisfies the safety requirement for the unsafe env mutation in the guard’s Drop.
tests/ninja_env_tests.rs (2)
10-24: Exercise guard-based override with proper locking.Acquire EnvLock before overriding and rely on the guard to restore on scope exit; then clean up under lock. This matches the documented safety model.
2-2: Retain import—NINJA_ENV remains publicly exported at netsuke::runner::NINJA_ENV
rg confirmspub const NINJA_ENV: &str = "NETSUKE_NINJA";in src/runner.rs (line 23). The existinguse netsuke::runner::NINJA_ENV;in tests will continue to work.tests/support/ninja_env.rs (1)
1-7: Provide a focused doc for the test-only NINJA_ENV guard.Document the safety model (EnvLock) and the dependency on a mockable Env. This aligns with the Deepwiki guidance to abstract environment access behind a trait.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (8)
tests/ninja_env_tests.rs (4)
1-8: Serialise these tests to avoid cross-test environment races.Even with EnvLock, other tests might mutate the environment without taking the lock. Add serial_test::serial and mark these tests serial to be defensive.
Apply this diff:
use mockable::MockEnv; use netsuke::runner::NINJA_ENV; use std::env::VarError; use support::env_lock::EnvLock; use support::ninja_env::override_ninja_env; +use serial_test::serial; + mod support;
9-10: Mark this test serial to prevent collisions with non-locked env mutations.Apply this diff:
-#[test] +#[test] +#[serial] fn override_ninja_env_restores_original() {
28-29: Mark this test serial to prevent collisions with non-locked env mutations.Apply this diff:
-#[test] +#[test] +#[serial] fn override_ninja_env_removes_when_unset() {
45-46: Mark this test serial to prevent collisions with non-locked env mutations.Apply this diff:
-#[test] +#[test] +#[serial] fn override_ninja_env_restores_empty() {tests/support/ninja_env.rs (4)
31-33: Fix the example import path.Use the test crate’s support module path to avoid confusion.
Apply this diff:
/// ```ignore /// use mockable::DefaultEnv; -/// use tests::support::ninja_env::override_ninja_env; +/// use support::ninja_env::override_ninja_env; /// let env = DefaultEnv::new(); /// let _guard = override_ninja_env(&env, "/usr/bin/ninja"); /// ```
35-36: Replace forbidden #[allow(dead_code)] with a narrowly scoped #[expect(dead_code)].Do not silence lints with allow. Use expect with a clear reason.
Apply this diff:
-#[allow(dead_code, reason = "only some tests override NINJA_ENV")] +#[expect(dead_code, reason = "unused in some test configurations; helper is intentionally provided for tests")] pub fn override_ninja_env(env: &impl Env, value: &str) -> NinjaEnvGuard {
38-41: Justify the unsafe set_var with a clear SAFETY contract.State precisely why this is sound while EnvLock is held.
Apply this diff:
- // Callers must hold [`EnvLock`](super::env_lock::EnvLock) to serialise this - // mutation. `set_var` is `unsafe` on Rust 2024 and the guard restores the - // prior value on drop. + // SAFETY: Callers hold [`EnvLock`](super::env_lock::EnvLock), which serialises + // process-global environment mutation across tests (Rust 2024 marks env mutation + // unsafe). With the lock held, setting `NINJA_ENV` here is safe; the guard restores + // the prior value on Drop. unsafe { std::env::set_var(NINJA_ENV, value) };
47-50: Use the conventional SAFETY: heading and reference the lock explicitly.Make the safety contract explicit at the restoration site as well.
Apply this diff:
- // Safety: callers hold [`EnvLock`] for the guard's lifetime, so these - // `set_var`/`remove_var` calls are serialised. Both functions are - // `unsafe` on Rust 2024. + // SAFETY: The guard is intended to be dropped while + // [`EnvLock`](super::env_lock::EnvLock) is still held, serialising these + // process-wide mutations. Both functions are `unsafe` on Rust 2024.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
tests/ninja_env_tests.rs(1 hunks)tests/support/ninja_env.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Comment why, not what. Explain assumptions, edge cases, trade-offs, or complexity. Don't echo the obvious.
Use functions and composition. Avoid repetition by extracting reusable logic. Prefer generators or comprehensions, and declarative code to imperative repetition when readable.
Small, meaningful functions. Functions must be small, clear in purpose, single responsibility, and obey command/query segregation.
Name things precisely. Use clear, descriptive variable and function names. For booleans, prefer names with is, has, or should.
Every module must begin with a module level (//! ) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Place function attributes after doc comments.
Do not use return in single-line functions.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type instead of panicking where feasible.
Prefer .expect() over .unwrap().
Use concat!() to combine long string literals rather than escaping newlines with a backslash.
Prefer single line versions of functions where appropriate.
Clippy warnings MUST be disallowed.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Keep file size manageable. No single code file may be longer than 400 lines. Long switch statements or dispatch tables should be broken up by feature and constituents colocated with targets. Large blocks of test data should be moved to external data files.
Illustrate with clear examples. Function documentation must include clear examples demonstrating the usage and outcome of the function. Test documentation should omit examples where the example serves only to reiterate the test logic.
Prefer semantic error enums. Derive std::error::Error (via the thiserror crate) for any condition the caller might inspect, retry, or map to an HTTP status.
...
Files:
tests/support/ninja_env.rstests/ninja_env_tests.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Files:
tests/support/ninja_env.rstests/ninja_env_tests.rs
🧬 Code Graph Analysis (1)
tests/ninja_env_tests.rs (2)
tests/support/ninja_env.rs (1)
override_ninja_env(36-43)tests/support/env_lock.rs (1)
acquire(19-21)
🔍 MCP Research (1 server)
Deepwiki:
-
The project exposes a custom Jinja function env(var_name: &str) -> Result<String, Error> that reads environment variables via minijinja::Environment; making environment access testable requires replacing direct std::env::var calls with an injectable, mockable interface. (https://deepwiki.com/search/guard-ninjaenv-overrides-with_46c218b2-e886-4652-98c6-78cbb6908cc2)
-
Suggested mocking strategy: define a trait like EnvironmentReader { fn get_var(&self, name: &str) -> Option; } and have the env Jinja function use an instance of that trait so tests can supply a mock implementation. (https://deepwiki.com/search/guard-ninjaenv-overrides-with_46c218b2-e886-4652-98c6-78cbb6908cc2)
-
Implementation considerations note that env is part of the "Essential Custom Functions" for minijinja integration, and should be implemented in safe Rust while providing an injectable interface to meet security and testability requirements. (https://deepwiki.com/search/guard-ninjaenv-overrides-with_46c218b2-e886-4652-98c6-78cbb6908cc2)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test
🔇 Additional comments (3)
tests/ninja_env_tests.rs (2)
13-16: Verify the mock is called exactly once — LGTM.The expectation .times(1) aligns with the guard’s behaviour (single raw() read during construction).
2-2: Resolve the inconsistency with the PR summary regarding NINJA_ENV imports.The summary claims tests no longer import netsuke::runner::NINJA_ENV, but this file does. Either adjust the summary or re-export/use a local constant to match the claim.
Would you like me to switch these tests to use a local constant or a test-support re-export to align with the summary?
tests/support/ninja_env.rs (1)
8-10: Comply with the injectable environment standard — LGTM.Using mockable::Env to obtain the prior value aligns with the organisational requirement to make environment access injectable and testable.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tests/ninja_env_tests.rs (1)
14-18: Serialise this test to prevent process-wide env racesEnvironment is process-global. Even with
EnvLock, other tests might mutate env without the lock. Serialise this test to eliminate flakiness.Apply this diff:
#[rstest] +#[serial] #[case(Some("orig"))] #[case(None)] #[case(Some(""))] fn override_ninja_env_restores(#[case] original: Option<&'static str>) {Ensure
serial_testis in[dev-dependencies].
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/ninja_env_tests.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Comment why, not what. Explain assumptions, edge cases, trade-offs, or complexity. Don't echo the obvious.
Use functions and composition. Avoid repetition by extracting reusable logic. Prefer generators or comprehensions, and declarative code to imperative repetition when readable.
Small, meaningful functions. Functions must be small, clear in purpose, single responsibility, and obey command/query segregation.
Name things precisely. Use clear, descriptive variable and function names. For booleans, prefer names with is, has, or should.
Every module must begin with a module level (//! ) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Place function attributes after doc comments.
Do not use return in single-line functions.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type instead of panicking where feasible.
Prefer .expect() over .unwrap().
Use concat!() to combine long string literals rather than escaping newlines with a backslash.
Prefer single line versions of functions where appropriate.
Clippy warnings MUST be disallowed.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Keep file size manageable. No single code file may be longer than 400 lines. Long switch statements or dispatch tables should be broken up by feature and constituents colocated with targets. Large blocks of test data should be moved to external data files.
Illustrate with clear examples. Function documentation must include clear examples demonstrating the usage and outcome of the function. Test documentation should omit examples where the example serves only to reiterate the test logic.
Prefer semantic error enums. Derive std::error::Error (via the thiserror crate) for any condition the caller might inspect, retry, or map to an HTTP status.
...
Files:
tests/ninja_env_tests.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Files:
tests/ninja_env_tests.rs
🔍 MCP Research (1 server)
Deepwiki:
- docs/rust-testing-with-rstest-fixtures.md — Recommends using rstest fixtures to provide mock implementations for external dependencies (example shows mocking a database) and states the same pattern can be applied to mock an Env used by minijinja's env(var_name) function for tests (DeepWiki search result linked: https://deepwiki.com/search/guard-ninjaenv-overrides-with_aa8c5b07-0b4a-4e36-8b86-a9eb3f2c0b81)
- Developer Guide (leynos/netsuke) — Mentions "Mock Fixtures" and using rstest fixtures to inject controlled test doubles; notes there is no direct code example for making the minijinja env function's underlying environment access mockable, only conceptual guidance
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test
🔇 Additional comments (2)
tests/ninja_env_tests.rs (2)
19-34: LGTM: Mock interaction is precise and matches the guard behaviourMock
env.raw()is asserted withtimes(1)and keyed correctly toNINJA_ENV. This aligns with the implementation that callsenv.raw()exactly once during construction. Usingreturn_onceavoids accidental reuse.
36-39: LGTM: Hold the lock while overriding and assert the in-scope valueThe guard holds
EnvLockwhile setting the var to "new", and you assert viastd::env::varimmediately. Using.as_deref()keeps the assertion concise.
| use mockable::MockEnv; | ||
| use netsuke::runner::NINJA_ENV; | ||
| use rstest::rstest; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add module-level docs and import serial_test
Add a module-level //! doc comment per guidelines and import the serial attribute used below.
Apply this diff:
+//! Tests verifying guard-based override of NINJA_ENV. Exercises override_ninja_env and NinjaEnvGuard
+//! semantics with EnvLock and a mock Env, ensuring restoration to the prior state.
use mockable::MockEnv;
use netsuke::runner::NINJA_ENV;
use rstest::rstest;
+use serial_test::serial;📝 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.
| use mockable::MockEnv; | |
| use netsuke::runner::NINJA_ENV; | |
| use rstest::rstest; | |
| //! Tests verifying guard-based override of NINJA_ENV. Exercises override_ninja_env and NinjaEnvGuard | |
| //! semantics with EnvLock and a mock Env, ensuring restoration to the prior state. | |
| use mockable::MockEnv; | |
| use netsuke::runner::NINJA_ENV; | |
| use rstest::rstest; | |
| use serial_test::serial; |
🤖 Prompt for AI Agents
In tests/ninja_env_tests.rs around lines 1 to 3, the file is missing a
module-level doc comment and the `serial` attribute import used by tests; add a
top-of-file `//!` module doc comment (briefly describing the test module's
purpose) and add the import `use serial_test::serial;` alongside the existing
imports so the `serial` attribute is available to the tests.
| #[expect( | ||
| unused, | ||
| reason = "support module exports helpers unused in these tests" | ||
| )] | ||
| mod support; | ||
|
|
There was a problem hiding this comment.
Replace lint-group unused with a narrowly scoped lint
Using the lint group unused violates the “no lint groups” rule. Scope this to the specific lint you need (here: dead_code) and keep the scope tight.
Apply this diff:
-#[expect(
- unused,
- reason = "support module exports helpers unused in these tests"
-)]
+#[expect(
+ dead_code,
+ reason = "support module exposes helpers not used in this test crate"
+)]
mod support;If some submodules export additional helpers, prefer placing the narrow #[expect(dead_code, reason = "...")] directly on those specific items in tests/support/*.rs instead of suppressing at the module import site.
📝 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.
| #[expect( | |
| unused, | |
| reason = "support module exports helpers unused in these tests" | |
| )] | |
| mod support; | |
| #[expect( | |
| dead_code, | |
| reason = "support module exposes helpers not used in this test crate" | |
| )] | |
| mod support; |
🤖 Prompt for AI Agents
In tests/ninja_env_tests.rs around lines 8 to 13, the module import uses the
lint group `unused`; replace that with a narrowly scoped lint by changing the
attribute to expect `dead_code` instead of `unused`, and if possible move that
`#[expect(dead_code, reason = "...")]` onto the specific unused items in
tests/support/*.rs rather than suppressing at the module level so only the exact
dead code is allowed.
| match original { | ||
| Some(val) => assert_eq!(std::env::var(NINJA_ENV).as_deref(), Ok(val)), | ||
| None => assert!(std::env::var(NINJA_ENV).is_err()), | ||
| } | ||
|
|
||
| let _cleanup = EnvLock::acquire(); | ||
| // SAFETY: `EnvLock` serialises this mutation; see above for details. | ||
| unsafe { std::env::remove_var(NINJA_ENV) }; | ||
| } |
There was a problem hiding this comment.
Hold EnvLock for restoration assertion and cleanup to eliminate a race window
Acquire EnvLock before asserting the post-guard state and keep it through cleanup. This removes a race window between guard drop and the assertion/cleanup where other tests could mutate env.
Apply this diff:
- match original {
+ let _lock = EnvLock::acquire();
+ match original {
Some(val) => assert_eq!(std::env::var(NINJA_ENV).as_deref(), Ok(val)),
None => assert!(std::env::var(NINJA_ENV).is_err()),
}
- let _cleanup = EnvLock::acquire();
- // SAFETY: `EnvLock` serialises this mutation; see above for details.
+ // SAFETY: `EnvLock` is held via `_lock`, serialising this mutation; see above for details.
unsafe { std::env::remove_var(NINJA_ENV) };📝 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.
| match original { | |
| Some(val) => assert_eq!(std::env::var(NINJA_ENV).as_deref(), Ok(val)), | |
| None => assert!(std::env::var(NINJA_ENV).is_err()), | |
| } | |
| let _cleanup = EnvLock::acquire(); | |
| // SAFETY: `EnvLock` serialises this mutation; see above for details. | |
| unsafe { std::env::remove_var(NINJA_ENV) }; | |
| } | |
| let _lock = EnvLock::acquire(); | |
| match original { | |
| Some(val) => assert_eq!(std::env::var(NINJA_ENV).as_deref(), Ok(val)), | |
| None => assert!(std::env::var(NINJA_ENV).is_err()), | |
| } | |
| // SAFETY: `EnvLock` is held via `_lock`, serialising this mutation; see above for details. | |
| unsafe { std::env::remove_var(NINJA_ENV) }; | |
| } |
🤖 Prompt for AI Agents
In tests/ninja_env_tests.rs around lines 41 to 49, the test drops the EnvLock
before asserting/restoring the environment which creates a race; acquire EnvLock
before making the post-guard assertion and keep the returned guard stored (e.g.,
let _cleanup = EnvLock::acquire();) until after the assertion and cleanup code
so the lock serialises both the check and the removal/restoration of NINJA_ENV,
and perform the unsafe std::env::remove_var(NINJA_ENV) while the lock is still
held.
|
Superseded |
1 participant
Summary
Testing
make lintmake testhttps://chatgpt.com/codex/tasks/task_e_689a8cb37e3c83229e25c6278d6128fb
Summary by Sourcery
Introduce a mockable environment guard to safely override and restore the NINJA_ENV variable in tests.
New Features:
Enhancements:
Tests: