Refactor PathGuard for injectable env and add mock test#105
Refactor PathGuard for injectable env and add mock test#105
Conversation
Reviewer's GuideThis PR refactors PathGuard to abstract environment mutation behind an Env trait with a real implementation, makes PathGuard generic over this trait (enabling injection), encapsulates the unsafe restore logic in a dedicated method/Drop impl, and adds a mock-based unit test verifying PATH restoration without touching the real environment. Sequence diagram for PATH restoration with mock environment in unit testsequenceDiagram
participant Test as actor Test
participant MockEnv
participant PathGuard
Test->>MockEnv: set_var("PATH", "test_path")
Test->>PathGuard: new("new_path", MockEnv)
PathGuard->>MockEnv: get_var("PATH")
PathGuard->>MockEnv: set_var("PATH", "new_path")
Test-->>PathGuard: drop()
PathGuard->>MockEnv: set_var("PATH", "test_path")
Class diagram for refactored PathGuard and Env traitclassDiagram
class Env {
<<trait>>
+get_var(key: &str) -> Option<String>
+set_var(key: &str, value: &str)
}
class RealEnv {
+get_var(key: &str) -> Option<String>
+set_var(key: &str, value: &str)
}
Env <|.. RealEnv
class PathGuard~E: Env~ {
-env: E
-original_path: Option<String>
+new(path: &str, env: E) -> PathGuard<E>
+drop()
}
PathGuard o-- Env
RealEnv <|-- PathGuard
class MockEnv {
+get_var(key: &str) -> Option<String>
+set_var(key: &str, value: &str)
}
Env <|.. MockEnv
MockEnv <|-- PathGuard
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. Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 41 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
Summary by CodeRabbit
WalkthroughAdd dev-dependency Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test
participant MockEnv as Env (mock)
participant Guard as PathGuard<Env>
Test->>Guard: PathGuard::with_env("/orig", MockEnv)
Test->>Guard: guard.env_mut().set_var("PATH", "/tmp") (unsafe)
Note right of Guard: PATH temporarily "/tmp"
Test-->>Guard: guard drops (scope end)
Guard->>MockEnv: restore -> set_var("PATH", "/orig") (unsafe)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 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 (
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| pub struct RealEnv; | ||
|
|
||
| impl Env for RealEnv { | ||
| #[allow(unsafe_op_in_unsafe_fn, reason = "delegates to std::env")] |
There was a problem hiding this comment.
style: using #[allow] instead of #[expect] violates the custom lint rule requiring narrowly scoped expectations
Context Used: Rule - Only narrowly scoped #[expect(lint, reason = "...")] is allowed. (link)
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@coderabbitai please create new GitHub issues for the following:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@leynos I'll create GitHub issues for both of those suggestions. 🏁 Script executed: Length of output: 1637 @leynos I've successfully created both GitHub issues for your suggestions:
Both issues have been assigned to you and include the relevant context from this PR review. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (3)
tests/support/mod.rs (1)
6-6: Use#[expect]instead of#[allow]Replace
#[allow]with#[expect]to comply with the coding guidelines. The guidelines explicitly forbid#[allow]and require narrowly scoped#[expect(lint, reason = "...")].-#![allow(unexpected_cfgs, reason = "test utilities use custom cfg")] +#![expect(unexpected_cfgs, reason = "test utilities use custom cfg")]tests/support/path_guard.rs (2)
6-6: Use#[expect]instead of#[allow]Replace
#[allow]with#[expect]to comply with the coding guidelines that explicitly forbid#[allow].-#![allow(unexpected_cfgs, reason = "test utilities use custom cfg")] +#![expect(unexpected_cfgs, reason = "test utilities use custom cfg")]
65-66: Remove redundant attributeRemove the
#[allow(dead_code)]attribute.#[cfg_attr(expect, expect(dead_code, reason = "used in env injection tests"))] - #[allow(dead_code, reason = "used in env injection tests")] pub fn env_mut(&mut self) -> &mut E {
📜 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 (4)
Cargo.toml(1 hunks)tests/path_guard_tests.rs(1 hunks)tests/support/mod.rs(2 hunks)tests/support/path_guard.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/path_guard_tests.rstests/support/mod.rstests/support/path_guard.rs
🧬 Code Graph Analysis (2)
tests/path_guard_tests.rs (1)
tests/support/path_guard.rs (4)
set_var(19-19)set_var(28-30)new(48-50)with_env(57-62)
tests/support/path_guard.rs (1)
tests/support/env_lock.rs (1)
acquire(19-21)
🔍 MCP Research (1 server)
Deepwiki:
- Document [Developer Guide]: the listed test dependencies mention rstest, cucumber, and insta as the project's testing frameworks but do not mention mockall (docs referencing Cargo.toml under "Testing Framework" and "Dependency Architecture").
- Document [Testing Framework]: the testing-stack description explicitly details rstest, cucumber-rs, and insta for unit/BDD/snapshot testing; no reference to mockall appears in that page.
🔇 Additional comments (3)
tests/support/path_guard.rs (2)
13-20: Well-designed abstraction for environment mutationThe
Envtrait provides a clean abstraction over environment mutations, properly documenting the safety concerns introduced in Rust 2024. The unsafe marker correctly signals the process-wide state mutation risk.
71-81: Proper restoration logic with appropriate safety documentationThe
restoremethod correctly usesEnvLockto serialise access and includes comprehensive safety documentation explaining the Rust 2024 requirements and mitigation strategy.tests/path_guard_tests.rs (1)
15-34: Well-structured test with proper mockingThe test effectively verifies PATH restoration using a mock environment, avoiding real environment mutations. The sequence enforcement ensures the correct order of operations, and the test properly exercises both the mutation and restoration paths.
| #[cfg_attr(expect, expect(dead_code, reason = "only some tests mutate PATH"))] | ||
| #[allow(dead_code, reason = "only some tests mutate PATH")] |
There was a problem hiding this comment.
Remove redundant attribute
Remove the #[allow(dead_code)] attribute as #[cfg_attr(expect, expect(dead_code))] already handles this conditionally.
#[cfg_attr(expect, expect(dead_code, reason = "only some tests mutate PATH"))]
- #[allow(dead_code, reason = "only some tests mutate PATH")]
/// Create a guard capturing the current `PATH` using the real environment.📝 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.
| #[cfg_attr(expect, expect(dead_code, reason = "only some tests mutate PATH"))] | |
| #[allow(dead_code, reason = "only some tests mutate PATH")] | |
| #[cfg_attr(expect, expect(dead_code, reason = "only some tests mutate PATH"))] | |
| /// Create a guard capturing the current `PATH` using the real environment. |
🤖 Prompt for AI Agents
In tests/support/path_guard.rs around lines 45 to 46, remove the redundant
#[allow(dead_code, reason = "only some tests mutate PATH")] attribute because
the conditional #[cfg_attr(expect, expect(dead_code, reason = "only some tests
mutate PATH"))] already covers the case; delete the #[allow(...)] line and keep
the cfg_attr(expect, expect(...)) attribute only so the dead_code lint is
handled conditionally.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
tests/support/mod.rs (1)
22-23: Remove #[allow(...)] and use #[expect(...)] to comply with lint policyDo not use #[allow]. The project policy permits only narrowly-scoped #[expect(..., reason = "...")]. Apply a single, direct expectation on the item.
Apply this diff:
-#[cfg_attr(expect, expect(dead_code, reason = "used in PATH tests"))] -#[cfg_attr(not(expect), allow(dead_code, reason = "used in PATH tests"))] +#[expect(dead_code, reason = "used in PATH tests")] pub fn fake_ninja(exit_code: i32) -> (TempDir, PathBuf) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
tests/path_guard_tests.rs(1 hunks)tests/support/mod.rs(2 hunks)tests/support/path_guard.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/path_guard_tests.rstests/support/path_guard.rstests/support/mod.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/path_guard_tests.rstests/support/path_guard.rstests/support/mod.rs
🧬 Code Graph Analysis (2)
tests/path_guard_tests.rs (1)
tests/support/path_guard.rs (4)
set_var(21-21)set_var(32-34)new(52-54)with_env(61-66)
tests/support/path_guard.rs (1)
tests/support/env_lock.rs (1)
acquire(19-21)
🔍 MCP Research (1 server)
Deepwiki:
- DeepWiki search result (https://deepwiki.com/search/refactor-pathguard-for-injecta_4f7f8b47-9f7e-4529-903d-a2c381ee606a): repository testing guidance recommends using rstest fixtures for injectable environments and shows example fixture patterns for injecting mocks into tests.
- DeepWiki search result (https://deepwiki.com/search/refactor-pathguard-for-injecta_4f7f8b47-9f7e-4529-903d-a2c381ee606a): mockall is recommended for mocks/stubs in the netsuke tests (examples demonstrate providing mock objects via fixtures).
- DeepWiki search result (https://deepwiki.com/search/refactor-pathguard-for-injecta_4f7f8b47-9f7e-4529-903d-a2c381ee606a): for integration/behavior tests, the repo uses cucumber-rs and wiremock-rs (examples show mounting wiremock MockServer fixtures for HTTP-related tests).
⏰ 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 (9)
tests/support/mod.rs (3)
6-7: Accept the narrowly-scoped lint expectationKeep this crate-level expectation. It documents intent and stays within the “expect with reason” policy.
82-83: Accept the narrowly-scoped lint expectationThe item-level
#[expect(dead_code, ...)]is compliant and scoped correctly.
55-76: Retainmock_path_to: MockEnv andexpect_rawexist
Remove the suggestion to delete this helper.mockable::MockEnvand itsexpect_rawmethod are defined intests/support/env.rs, somock_path_tocompiles without errors.Likely an incorrect or invalid review comment.
tests/support/path_guard.rs (4)
6-7: Accept the crate-level lint expectationKeep this; it explains the custom cfg usage and remains within the “expect with reason” rule.
8-9: Import OsStr/OsString explicitlyThis is the right place to introduce Os types for the public API surface.
43-45: Store Env alongside original PATHThis addition is necessary for injection and is well-scoped.
89-93: LGTM: Drop delegates to restoreThis keeps the RAII guarantee tight and readable.
tests/path_guard_tests.rs (2)
1-5: Good module-level documentationKeep this. It satisfies the requirement for a leading
//!and clarifies test intent.
19-39: LGTM: Verify PATH is set then restored via mock sequence without touching real envThe ordered expectations on
set_varcoupled with the RAII scope validate restoration behaviour cleanly.
| mock! { | ||
| pub Env {} | ||
| impl Env for Env { | ||
| unsafe fn set_var(&mut self, key: &str, val: &OsStr); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Prefer rstest fixtures for reusable mocks per repository testing guidance
Adopt rstest fixtures for the mock environment to align with the repo’s testing standards and reduce duplication as tests grow.
According to the team’s guidance (DeepWiki), tests should use rstest fixtures for injectable environments with mockall. I can sketch a fixture like:
use rstest::fixture;
#[fixture]
fn mock_env() -> MockEnv {
let mut env = MockEnv::new();
// set common expectations here if needed
env
}Would you like me to refactor the tests to use rstest and extract a reusable mock_env fixture?
🤖 Prompt for AI Agents
In tests/path_guard_tests.rs around lines 12 to 17, the mocked Env is declared
inline with mockall; replace that pattern with an rstest fixture so tests can
reuse the mock per the repo guideline. Create a #[fixture] fn mock_env() ->
MockEnv that constructs MockEnv::new(), configures any common expectations, and
returns it; update tests to accept mock_env: MockEnv as a parameter instead of
creating local mocks; add the rstest::fixture import and any necessary mockall
imports to the test module and ensure visibility (pub if needed) so existing
tests compile.
| //! This module provides helpers for creating fake executables along with | ||
| //! logging utilities used in behavioural tests. | ||
|
|
||
| #![allow(unexpected_cfgs, reason = "test utilities use custom cfg")] |
There was a problem hiding this comment.
style: violates custom rule forbidding #[allow] - should use #[expect(unexpected_cfgs, reason = "...")] instead
| #![allow(unexpected_cfgs, reason = "test utilities use custom cfg")] | |
| #![expect(unexpected_cfgs, reason = "test utilities use custom cfg")] |
Context Used: Rule - #[allow] is forbidden. (link)
|
Superseded |
1 participant
Summary
std::env::set_varunsafety and wrap it via a generic Env traitPathGuardTesting
make lintmake testhttps://chatgpt.com/codex/tasks/task_e_689a8d0b78888322aee8b2405a5e74af
Summary by Sourcery
Introduce an Env trait to encapsulate unsafe environment mutations and refactor PathGuard to be generic over an environment implementation, adding constructors for injection and an env_mut accessor; consolidate unsafe std::env::set_var calls into the Env trait and a restore method on drop; add the mockall dependency and a unit test using a mock Env to verify that PATH is correctly restored without touching the real environment.
New Features:
Enhancements:
Build:
Tests: