Conversation
Reviewer's GuideExtract common test utilities into a standalone test_support crate, remove redundant annotations, and update all existing tests to import and adapt to the new crate’s refined API. Entity relationship diagram for test_support crate moduleserDiagram
ENV ||--o| ENVLOCK : manages
PATH_GUARD ||--o| ENV : uses
FAKE_NINJA ||--o| ENV : simulates
Class diagram for test_support crate modulesclassDiagram
class Env {
+fn set_var()
+fn get_var()
+fn remove_var()
}
class PathGuard {
+fn new()
+fn guard_path()
}
class EnvLock {
+fn lock_env()
+fn unlock_env()
}
class FakeNinja {
+fn run()
+fn output()
}
EnvLock --> Env : manages
PathGuard --> Env : uses
FakeNinja --> Env : simulates
test_support : Env
test_support : PathGuard
test_support : EnvLock
test_support : FakeNinja
File-Level Changes
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
WalkthroughAdd a new test_support crate and register it as a dev-dependency; move test helpers (PathGuard, EnvLock, fake_ninja, env helpers) into that crate and update tests to import and use those shared utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test
participant TS as test_support::fake_ninja
participant FS as Filesystem
Test->>TS: call fake_ninja(exit_code: u8)
TS->>FS: create TempDir
TS->>FS: write platform-specific executable (ninja / ninja.cmd)
TS->>FS: set permissions (Unix) / write batch (Windows)
TS-->>Test: return (TempDir, PathBuf)
Test->>Test: add dir to PATH or install into test world
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the "Integrations" page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs⚙️ CodeRabbit Configuration File
Files:
🔍 Additional research (Deepwiki)Summary of additional context found (relevant to reviewing this PR):
Tools used:🔇 Additional comments (11)
✨ 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 (
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![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 - here's some feedback:
- Consider moving the
test_supportpath dependency into[dev-dependencies]instead of[dependencies], since it’s only used by tests.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider moving the `test_support` path dependency into `[dev-dependencies]` instead of `[dependencies]`, since it’s only used by tests.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?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: 11
🔭 Outside diff range comments (5)
test_support/src/env.rs (2)
72-90: Add a short usage example to the public APIAugment the docs for
prepend_dir_to_pathwith a minimal example to meet the documentation guidelines and make intent explicit.Example to add to the doc comment:
/// # Examples /// ``` /// use test_support::env::{SystemEnv, prepend_dir_to_path}; /// let env = SystemEnv::new(); /// let dir = tempfile::tempdir().expect("temp dir"); /// let _guard = prepend_dir_to_path(&env, dir.path()); /// // PATH now starts with `dir` and will be restored when `_guard` is dropped. /// ```
1-5: Ensure doctest access to helpers per org guidanceDoctests for the main crate typically cannot rely on a separate dev-only crate. Add a small
#[cfg(doctest)]bridge in the primary crate to re-export the minimal helpers needed (e.g.,PathGuard,prepend_dir_to_path), or expose doctest-only helpers as per the Deepwiki guidance. This keeps doctest ergonomics while retaining a dedicated test-support crate.Reference: Deepwiki recommendations on promoting test support and doctest gating.
test_support/src/path_guard.rs (2)
46-56: Fix misleading doc forPathGuard::newThe function does not capture
PATH; it accepts a previously captured value. Correct the docs to avoid confusion.impl PathGuard { - /// Create a guard capturing the current `PATH` using the real environment. - /// - /// Returns a guard that restores the variable when dropped. + /// Create a guard that will restore a previously captured `PATH`. + /// + /// Pass `original` as the current value of `PATH` (e.g., from + /// `std::env::var_os("PATH")`). The guard restores this value when dropped. pub fn new(original: Option<OsString>) -> Self { let state = original.map_or(OriginalPath::Unset, OriginalPath::Set); Self { original: Some(state), env: StdEnv, } } }
10-19: Route environment removal via the Env abstractionAvoid calling
std::env::remove_vardirectly. ExtendEnvwithremove_varso Drop uses the same abstraction and centralises unsafety and locking policy.-/// Environment abstraction for setting variables. +/// Environment abstraction for mutating variables. pub trait Env { /// Set `key` to `val` within the environment. @@ - unsafe fn set_var(&mut self, key: &str, val: &OsStr); + unsafe fn set_var(&mut self, key: &str, val: &OsStr); + /// Remove `key` from the environment. + /// + /// # Safety + /// + /// Mutating process globals is `unsafe` in Rust 2024. + unsafe fn remove_var(&mut self, key: &str); } @@ impl Env for StdEnv { unsafe fn set_var(&mut self, key: &str, val: &OsStr) { unsafe { std::env::set_var(key, val) }; } + unsafe fn remove_var(&mut self, key: &str) { + unsafe { std::env::remove_var(key) } + } } @@ impl<E: Env> Drop for PathGuard<E> { fn drop(&mut self) { let _lock = EnvLock::acquire(); match self.original.take() { Some(OriginalPath::Set(path)) => { // Nightly marks `set_var` unsafe; restoring cleans up global state. unsafe { self.env.set_var("PATH", &path) }; } - Some(OriginalPath::Unset) | None => unsafe { std::env::remove_var("PATH") }, + Some(OriginalPath::Unset) | None => unsafe { self.env.remove_var("PATH") }, } } }Also applies to: 24-28, 74-85
tests/runner_tests.rs (1)
207-242: Serialise NINJA_ENV mutations with EnvLockGuard both the override and restoration with
EnvLockto match the project’s approach to process-global state and avoid test flakiness under parallel execution.- unsafe { - std::env::set_var(NINJA_ENV, &ninja_path); - } + { + let _lock = EnvLock::acquire(); + unsafe { std::env::set_var(NINJA_ENV, &ninja_path); } + } @@ - unsafe { - if let Some(val) = original { - std::env::set_var(NINJA_ENV, val); - } else { - std::env::remove_var(NINJA_ENV); - } - } + { + let _lock = EnvLock::acquire(); + unsafe { + if let Some(val) = original { + std::env::set_var(NINJA_ENV, val); + } else { + std::env::remove_var(NINJA_ENV); + } + } + }
♻️ Duplicate comments (1)
test_support/src/lib.rs (1)
18-34: Fix Unix-only behaviour; add Windows support and strengthen diagnosticsThe current implementation writes a POSIX shell script named “ninja” and only sets mode on Unix. On Windows, this won’t be executable, which will break tests. Implement a Windows batch variant (ninja.cmd) or gate the function to Unix. Also improve expect() messages to aid debugging, and document Panics/Platform/Examples in Rustdoc. This echoes previous feedback on expect messages.
Apply this diff:
-/// Create a fake Ninja executable that exits with `exit_code`. -/// -/// Returns the temporary directory and the path to the executable. +/// Create a fake Ninja executable that exits with `exit_code`. +/// +/// Returns the temporary directory and the full path to the created executable. +/// +/// # Panics +/// Panics if the temporary directory cannot be created, the script cannot be written, +/// or (on Unix) executable permissions cannot be set. +/// +/// # Platform +/// - Unix: creates a POSIX shell script named `ninja` with mode 0o755. +/// - Windows: creates a batch file named `ninja.cmd` that exits with the given code. +/// +/// # Examples +/// ```no_run +/// let (dir, path) = test_support::fake_ninja(42); +/// assert!(path.exists()); +/// drop(dir); // cleans up the temporary directory +/// ``` pub fn fake_ninja(exit_code: i32) -> (TempDir, PathBuf) { - let dir = TempDir::new().expect("temp dir"); - let path = dir.path().join("ninja"); - let mut file = File::create(&path).expect("script"); - writeln!(file, "#!/bin/sh\nexit {exit_code}").expect("write script"); - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - let mut perms = fs::metadata(&path).expect("meta").permissions(); - perms.set_mode(0o755); - fs::set_permissions(&path, perms).expect("perms"); - } - (dir, path) + let dir = TempDir::new().expect("create temporary directory for fake ninja"); + + #[cfg(unix)] + let path = dir.path().join("ninja"); + #[cfg(windows)] + let path = dir.path().join("ninja.cmd"); + + let mut file = File::create(&path).expect("create fake ninja program"); + + #[cfg(unix)] + { + writeln!(file, "#!/bin/sh\nexit {exit_code}") + .expect("write shell script for fake ninja"); + use std::os::unix::fs::PermissionsExt; + let mut perms = fs::metadata(&path) + .expect("read permissions for fake ninja") + .permissions(); + perms.set_mode(0o755); + fs::set_permissions(&path, perms) + .expect("set executable permissions for fake ninja"); + } + + #[cfg(windows)] + { + // Use CRLF line endings for cmd.exe + writeln!(file, "@echo off\r\nexit /B {exit_code}\r") + .expect("write batch script for fake ninja"); + } + + (dir, path) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
Cargo.toml(1 hunks)test_support/Cargo.toml(1 hunks)test_support/src/env.rs(1 hunks)test_support/src/lib.rs(1 hunks)test_support/src/path_guard.rs(1 hunks)tests/assert_cmd_tests.rs(2 hunks)tests/cucumber.rs(2 hunks)tests/env_path_tests.rs(1 hunks)tests/path_guard_tests.rs(1 hunks)tests/runner_tests.rs(3 hunks)tests/steps/process_steps.rs(2 hunks)tests/support/mod.rs(0 hunks)
💤 Files with no reviewable changes (1)
- tests/support/mod.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/steps/process_steps.rstests/assert_cmd_tests.rstest_support/src/env.rstests/env_path_tests.rstests/path_guard_tests.rstest_support/src/lib.rstest_support/src/path_guard.rstests/cucumber.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/steps/process_steps.rstests/assert_cmd_tests.rstest_support/src/env.rstests/env_path_tests.rstests/path_guard_tests.rstest_support/src/lib.rstest_support/src/path_guard.rstests/cucumber.rstests/runner_tests.rs
Cargo.toml
📄 CodeRabbit Inference Engine (AGENTS.md)
Cargo.toml: Use explicit version ranges in Cargo.toml and keep dependencies up-to-date.
Mandate caret requirements for all dependencies. All crate versions specified in Cargo.toml must use SemVer-compatible caret requirements (e.g., some-crate = "1.2.3").
Prohibit unstable version specifiers. The use of wildcard (*) or open-ended inequality (>=) version requirements is strictly forbidden. Tilde requirements (~) should only be used where a dependency must be locked to patch-level updates for a specific, documented reason.
Files:
Cargo.toml
🧬 Code Graph Analysis (5)
tests/steps/process_steps.rs (1)
test_support/src/lib.rs (1)
fake_ninja(21-34)
tests/assert_cmd_tests.rs (2)
test_support/src/lib.rs (1)
fake_ninja(21-34)tests/steps/process_steps.rs (1)
fake_ninja(28-32)
tests/env_path_tests.rs (1)
test_support/src/env.rs (2)
mocked_path_env(46-53)prepend_dir_to_path(77-90)
test_support/src/lib.rs (3)
tests/env_path_tests.rs (1)
std(39-41)tests/steps/process_steps.rs (1)
fake_ninja(28-32)test_support/src/path_guard.rs (1)
new(50-56)
tests/runner_tests.rs (3)
test_support/src/env.rs (1)
prepend_dir_to_path(77-90)test_support/src/lib.rs (1)
fake_ninja(21-34)tests/steps/process_steps.rs (1)
fake_ninja(28-32)
🔍 MCP Research (1 server)
Deepwiki:
- Recommendation to promote test helpers into the crate by placing them under src/ and guarding with #[cfg(test)] for unit tests; includes an example showing a #[cfg(test)] mod with rstest fixtures usable by unit tests. (https://deepwiki.com/search/promote-test-support-to-crate_0ff71952-d058-4a23-bd5b-4cc13730eae0)
- Guidance to expose helpers for doctests using #[cfg(doctest)] (example module doctest_helpers) so helpers are available only when running cargo test --doc. (https://deepwiki.com/search/promote-test-support-to-crate_0ff71952-d058-4a23-bd5b-4cc13730eae0)
⏰ 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 (10)
test_support/Cargo.toml (1)
1-9: Validate test-support crate aligns with our test-helper architecture
- test-support crate is imported exclusively by integration tests under tests/ (runner_tests.rs, path_guard_tests.rs, env_path_tests.rs, cucumber.rs, assert_cmd_tests.rs, steps/process_steps.rs)
- No #[cfg(doctest)]-guarded helpers found
Deepwiki prescribes in-crate fixtures behind #[cfg(test)] for unit tests; a separate crate is acceptable for sharing helpers across crates. Confirm this dedicated crate is the intended approach. If doctests require helpers, introduce a minimal helper module in the primary crate behind #[cfg(doctest)].
Cargo.toml (2)
72-72: LGTM: Wire up test_support as a dev-dependency.The path dependency is correctly scoped under dev-dependencies and won’t affect release builds.
63-77: No stale legacy support references remain
- Confirmed absence of
mod support,use support::…,crate::support,tests/supportorsupport::fake_ninjaacross the repository.- All tests now import exclusively from the
test_supportcrate.tests/assert_cmd_tests.rs (2)
10-10: LGTM: Import fake_ninja from test_support.The import is correct and scoped to the test.
29-29: LGTM: Use fake_ninja to seed PATH with a controlled ninja.The setup ensures deterministic execution for the build test.
test_support/src/env.rs (1)
12-12: Consolidate crate imports: LGTMKeep this combined import; it reduces churn across intra-crate moves and preserves clarity.
tests/env_path_tests.rs (1)
7-10: Switch tests to test_support imports: LGTMThe rewire removes local test modules and uses the dedicated
test_supportcrate. This aligns the suite and avoids duplication.tests/cucumber.rs (1)
24-25: World state type change: LGTMUsing
Option<PathGuard>inCliWorldis appropriate for per-scenario PATH isolation.test_support/src/path_guard.rs (1)
8-8: Module path update for EnvLock: LGTMThe move to
crate::env_lock::EnvLockimproves clarity and decouples from parent-module layout.tests/runner_tests.rs (1)
36-41: Switch to test_support::fake_ninja: LGTMThe fixture now uses the unified helper; this removes duplication and keeps the intent clear.
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
tests/runner_tests.rs (1)
206-241: Acquire EnvLock when setting/restoring NINJA_ENV.Guard the
set_var/restore withEnvLockso mutations are serialised and scoped to this test.Apply this diff:
#[test] #[serial] fn run_respects_env_override_for_ninja() { - let (temp_dir, ninja_path) = fake_ninja(0); + let (temp_dir, ninja_path) = fake_ninja(0); let original = std::env::var_os(NINJA_ENV); + let _lock = EnvLock::acquire(); unsafe { std::env::set_var(NINJA_ENV, &ninja_path); } @@ assert!(result.is_ok()); unsafe { if let Some(val) = original { std::env::set_var(NINJA_ENV, val); } else { std::env::remove_var(NINJA_ENV); } } drop(ninja_path); drop(temp_dir); }
♻️ Duplicate comments (7)
tests/path_guard_tests.rs (1)
8-8: Import PathGuard from crate root; keep Env trait scoped locally [done].Leverage the crate’s re-export and keep the trait import local to this module for clarity. Change aligns with prior guidance.
tests/cucumber.rs (1)
4-4: Use the crate root re-export for PathGuard [done].Shorten the path and keep types consistent across the test suite. Matches the new test_support API.
Also applies to: 24-25
tests/runner_tests.rs (1)
6-10: Import EnvLock to serialise NINJA_ENV mutations.Mutating process environment without the lock introduces test flakiness under parallel runs and violates your unsafe contract. Pull
EnvLockinto scope.Apply this diff:
use test_support::{ PathGuard, check_ninja, env::{SystemEnv, prepend_dir_to_path}, fake_ninja, + env_lock::EnvLock, };tests/steps/process_steps.rs (2)
9-13: Import fake_ninja alongside check_ninja/env [done].Avoid mixed FQ/imported usage and keep imports cohesive.
29-31: Use the imported fake_ninja symbol [done].Call the locally imported
fake_ninjafor consistency.test_support/src/lib.rs (2)
1-6: Broaden crate-level documentation and state test-only intent.Clarify scope, primary utilities, and platform notes in the crate docs.
Apply this diff:
-//! Test-only utilities for integration and unit tests. -//! -//! This crate offers helpers for crafting fake executables, manipulating the -//! environment, and guarding global state so tests can exercise process -//! interactions without touching the host system. +//! Test-support crate for Netsuke. +//! +//! This crate provides test-only utilities: +//! - creating fake executables for process-related tests +//! - manipulating PATH safely (PathGuard) +//! - serialising environment mutation across tests (EnvLock) +//! +//! All items are intended for use in tests within this workspace; do not use in production code. +//! +//! Platform notes: fake executables are implemented for Unix and Windows.
12-12: Document the re-export for discoverability.Attach Rustdoc to explain why
PathGuardis re-exported at the crate root.Apply this diff:
+/// Re-export of `PathGuard` for crate-level ergonomics in tests. pub use path_guard::PathGuard;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
test_support/Cargo.toml(1 hunks)test_support/src/env.rs(1 hunks)test_support/src/lib.rs(1 hunks)tests/cucumber.rs(2 hunks)tests/path_guard_tests.rs(1 hunks)tests/runner_tests.rs(3 hunks)tests/steps/process_steps.rs(2 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:
test_support/src/lib.rstests/path_guard_tests.rstest_support/src/env.rstests/cucumber.rstests/steps/process_steps.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:
test_support/src/lib.rstests/path_guard_tests.rstest_support/src/env.rstests/cucumber.rstests/steps/process_steps.rstests/runner_tests.rs
🧬 Code Graph Analysis (3)
test_support/src/lib.rs (2)
tests/env_path_tests.rs (1)
std(39-41)test_support/src/path_guard.rs (1)
new(50-56)
tests/steps/process_steps.rs (1)
test_support/src/lib.rs (1)
fake_ninja(22-49)
tests/runner_tests.rs (2)
test_support/src/env.rs (1)
prepend_dir_to_path(75-88)test_support/src/lib.rs (1)
fake_ninja(22-49)
🔍 Additional research (Deepwiki)
Summary of relevant findings for PR #114 ("Promote test support to crate"):
-
The repository docs/wiki show the project already uses a shared test-helper pattern and a strict CI/testing policy; the PR adds a new test_support crate (test_support/Cargo.toml + src/) and updates tests to import helpers from test_support instead of local tests/support modules.,
-
Key changes to review:
- Workspace Cargo.toml now adds test_support as a path dev-dependency; test_support/Cargo.toml sets publish = false and edition = "2024". Verify workspace membership and that path is correct for CI/test matrix.
- test_support exposes modules/env helpers: path_guard, env, env_lock and a fake_ninja function; tests were updated to use test_support::{...} and to remove tests/support/mod.rs (which previously provided these helpers). Confirm public API/visibility and re-exports (pub use path_guard::PathGuard) match updated import sites.
- Some #[allow(dead_code,...)] and rstest fixture attributes were removed in moved files (env.rs, path_guard.rs). Check for resulting compiler warnings/failing clippy rules or test fixture behavior changes—repo enforces strict lint/test gates.
- New dependencies in test_support: tempfile, mockable (with "mock" feature), rstest as dev-dep. Confirm versions are acceptable and don’t conflict with workspace dev-dependencies.
- Author ran make fmt, make lint, make test. Still verify CI (format, clippy, tests including cucumber/insta snapshots) passes with the relocated helpers.
Files/areas to spot-check in review:
- Workspace Cargo.toml + test_support/Cargo.toml (path, publish=false, edition, dependency versions).
- test_support/src/lib.rs exports and fake_ninja implementation (cross-platform script creation and tempdir handling).
- All updated tests imports/usages (cucumber.rs, runner_tests.rs, env_path_tests.rs, path_guard_tests.rs, assert_cmd_tests.rs, steps/*) for compile and fixture behavior.
- Removal of tests/support/mod.rs — ensure no remaining references to the old module and no lost helper behavior (e.g., mock_path_to or fake_ninja_pwd if still used).
- Lint/test impacts from removed #[allow(...)] and removed rstest fixture attributes.
Tools consulted:
- Repository wiki/pages listing and full content used as context:,
⏰ 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 (5)
test_support/src/env.rs (1)
11-11: Co-locate crate imports; keep the API tidy [LGTM].Importing
EnvLockandPathGuardfromcrate::…is correct for intra-crate use and matches the moved module structure.test_support/Cargo.toml (3)
5-5: Prevent publication: confirmed.Keep
publish = false; this correctly prevents accidental publication of the internal test-support crate.
11-12: Keep rstest scoped to dev-dependencies.Leave
rstestunder[dev-dependencies]to avoid leaking test macros transitively.
4-4: Confirm CI nightly toolchain supports Rust 2024 edition
- rust-toolchain.toml pins channel to nightly-2025-06-26 with rustfmt and clippy.
- Both Cargo.toml at the workspace root and test_support/Cargo.toml declare edition = "2024".
- test_support appears only under
[dev-dependencies].No further changes required.
test_support/src/lib.rs (1)
7-10: Export test helpers without cfg gating.Do not wrap these
pub modexports with#[cfg(test)]; dependent crates import this crate as a dev-dependency, and cfg-gating here would hide the modules from them.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
tests/runner_tests.rs (2)
209-241: Serialise NINJA_ENV mutations with EnvLock.Guard both the set and restore operations to avoid races with other tests that may read or write the environment concurrently.
Apply this diff to protect the unsafe environment mutations:
#[test] #[serial] fn run_respects_env_override_for_ninja() { - let (temp_dir, ninja_path) = fake_ninja(0u8); + let (temp_dir, ninja_path) = fake_ninja(0u8); let original = std::env::var_os(NINJA_ENV); - unsafe { - std::env::set_var(NINJA_ENV, &ninja_path); - } + { + let _lock = EnvLock::acquire(); + unsafe { + std::env::set_var(NINJA_ENV, &ninja_path); + } + } @@ - unsafe { - if let Some(val) = original { - std::env::set_var(NINJA_ENV, val); - } else { - std::env::remove_var(NINJA_ENV); - } - } + { + let _lock = EnvLock::acquire(); + unsafe { + if let Some(val) = original { + std::env::set_var(NINJA_ENV, val); + } else { + std::env::remove_var(NINJA_ENV); + } + } + } drop(ninja_path); drop(temp_dir); }
6-10: Import EnvLock to serialise environment mutations.Pull EnvLock into scope; you mutate NINJA_ENV later and should guard those unsafe calls.
Apply this diff to the use list:
use test_support::{ PathGuard, check_ninja, env::{SystemEnv, prepend_dir_to_path}, + env_lock::EnvLock, fake_ninja, };test_support/src/lib.rs (2)
18-18: Document the re-export for discoverability.Explain why PathGuard is re-exported at the crate root so it shows up in generated docs.
Apply this diff:
-pub use path_guard::PathGuard; +/// Re-export PathGuard for crate-level ergonomics in tests. +pub use path_guard::PathGuard;
13-17: Gate helper modules for test-only builds.Constrain public helper modules to test/doc-test builds to prevent accidental use in non-test contexts.
Apply this diff:
-pub mod check_ninja; -pub mod env; -pub mod env_lock; -pub mod path_guard; +#[cfg(any(test, doctest))] +pub mod check_ninja; +#[cfg(any(test, doctest))] +pub mod env; +#[cfg(any(test, doctest))] +pub mod env_lock; +#[cfg(any(test, doctest))] +pub mod path_guard;Follow up: Ensure the crate remains listed only under [dev-dependencies] in workspace Cargo.toml.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
test_support/src/lib.rs(1 hunks)tests/assert_cmd_tests.rs(2 hunks)tests/runner_tests.rs(3 hunks)tests/steps/process_steps.rs(2 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:
test_support/src/lib.rstests/runner_tests.rstests/assert_cmd_tests.rstests/steps/process_steps.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:
test_support/src/lib.rstests/runner_tests.rstests/assert_cmd_tests.rstests/steps/process_steps.rs
🧬 Code Graph Analysis (4)
test_support/src/lib.rs (2)
tests/env_path_tests.rs (1)
std(39-41)test_support/src/path_guard.rs (1)
new(50-56)
tests/runner_tests.rs (2)
test_support/src/env.rs (1)
prepend_dir_to_path(75-88)test_support/src/lib.rs (1)
fake_ninja(43-98)
tests/assert_cmd_tests.rs (1)
test_support/src/lib.rs (1)
fake_ninja(43-98)
tests/steps/process_steps.rs (1)
test_support/src/lib.rs (1)
fake_ninja(43-98)
🪛 GitHub Actions: CI
tests/steps/process_steps.rs
[error] 27-33: Rust formatting check failed: cargo fmt --check detected formatting changes in tests/steps/process_steps.rs. Run 'cargo fmt' to fix formatting.
🔍 Additional research (Context7, Deepwiki)
Summary of relevant context found
-
PR goal: promote local test helpers into a new test_support crate and update tests to use it (workspace Cargo.toml updated to add test_support; author ran make fmt / make lint / make test).
-
New test_support crate:
- test_support/Cargo.toml added (package test_support, deps: tempfile, mockable with "mock" feature; dev-dep rstest).
- Modules added/re-exported: path_guard, env, env_lock, check_ninja; pub fn fake_ninja(exit_code: u8) implemented (platform-specific executable creation, returns (TempDir, PathBuf)).
-
Tests updated to import from test_support (replaced local tests/support module):
- Many test files switched to use test_support::{..., fake_ninja, PathGuard, env::{...}, env_lock::EnvLock, check_ninja, ...}. Calls adjusted to use fake_ninja(0u8) etc. tests/support/mod.rs was removed. PathGuard types re-exported at crate root. #[allow(dead_code)] attributes removed from moved code.
-
Notable API/behavioral notes:
- Public test helpers surface preserved (fake_ninja, PathGuard, mocked_path_env, prepend_dir_to_path) though some lint/test-utility attributes were removed; function signatures largely unchanged except fake_ninja now takes/returns u8 for exit code.
-
Limitations: attempted request for recent commits / PR 114 diffs returned that git/PR details/diffs are not accessible from the tool (cannot fetch commit list or file diffs). Use the repo context above for file-level changes.
🔇 Additional comments (7)
tests/assert_cmd_tests.rs (2)
10-10: Adopt shared helper crate for fake_ninja.Import fake_ninja from test_support to centralise test utilities and reduce duplication. Good move.
29-29: Use u8 for exit code to match helper API.Passing 0u8 matches fake_ninja’s signature and keeps platform semantics precise. The PATH override is correctly scoped to the child process via Command::env.
tests/runner_tests.rs (1)
35-40: Fixture migration to fake_ninja(u8) is correct.Using the shared helper with a u8 exit code aligns with the new API and keeps the fixtures small and focused.
tests/steps/process_steps.rs (2)
3-3: Keep world type import local.Importing CliWorld at the top-level improves readability of step signatures. No issues.
9-13: Unify test_support imports.Import fake_ninja alongside check_ninja and env to avoid mixed FQ/imported usage within the module.
test_support/src/lib.rs (2)
1-11: Provide clear crate-level docs for test-only scope and platform notes.The crate docs set expectations, list utilities, and call out platform behaviour. Good coverage.
43-98: Implement platform-correct fake_ninja with descriptive panics.The helper now models exit codes as u8, emits a .cmd on Windows and a POSIX script on Unix, and surfaces context-rich panic messages. This is robust and test-friendly.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@sourcery-ai review |
1 participant
Summary
test_supportcrateallow(unused_code)attributes from test utilitiesTesting
make fmtmake lintmake testhttps://chatgpt.com/codex/tasks/task_e_689bbf62cf80832287838e7140381734
Summary by Sourcery
Extract test utilities into a dedicated test_support crate and update the test suite to use it, simplifying imports, adjusting helper signatures, and ensuring safe environment mutation concurrency.
New Features:
Enhancements: