Scope EnvLock in NINJA_ENV tests#117
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ 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 (
|
Reviewer's guide (collapsed on small PRs)Reviewer's GuideScoped EnvLock usage and automated NINJA_ENV state capture/restore in ninja_env tests to ensure isolation without manual drops. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
aa0dee7
into
codex/add-envlock-and-mockable-env-for-ninja_env
There was a problem hiding this comment.
Hey there - 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:47` </location>
<code_context>
assert!(std::env::var(NINJA_ENV).is_err());
// Restore original global state for isolation
</code_context>
<issue_to_address>
Consider asserting the value of NINJA_ENV after restoration.
This will verify that the restoration logic correctly resets NINJA_ENV to its prior state.
Suggested implementation:
```rust
// Restore original global state for isolation
{
let _lock = EnvLock::acquire();
if let Some(val) = before {
```
```rust
if let Some(val) = before {
// Assert that NINJA_ENV is restored to its original value
assert_eq!(std::env::var(NINJA_ENV).as_deref(), Some(val.as_str()));
} else {
// Assert that NINJA_ENV is not set after restoration
assert!(std::env::var(NINJA_ENV).is_err());
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -46,13 +47,14 @@ fn override_ninja_env_unset_removes_variable() { | |||
| assert!(std::env::var(NINJA_ENV).is_err()); | |||
There was a problem hiding this comment.
suggestion (testing): Consider asserting the value of NINJA_ENV after restoration.
This will verify that the restoration logic correctly resets NINJA_ENV to its prior state.
Suggested implementation:
// Restore original global state for isolation
{
let _lock = EnvLock::acquire();
if let Some(val) = before { if let Some(val) = before {
// Assert that NINJA_ENV is restored to its original value
assert_eq!(std::env::var(NINJA_ENV).as_deref(), Some(val.as_str()));
} else {
// Assert that NINJA_ENV is not set after restoration
assert!(std::env::var(NINJA_ENV).is_err());
}* Add guard for NINJA_ENV overrides * Quote NINJA_ENV in doc comment * Centralise NINJA_ENV and cover unset restoration * Remove manual NINJA_ENV cleanup in tests * Add crate for shared NINJA_ENV constant * Scope EnvLock in NINJA_ENV tests (#117) * Apply review suggestions for ninja env crate (#118) * Document NINJA_ENV and restore via OsString (#120) * Refine ninja env docs and tests (#121)
Summary
Testing
make fmtmake lintmake testhttps://chatgpt.com/codex/tasks/task_e_689e8659b5d08322b5929aaf6d6307f9
Summary by Sourcery
Improve NINJA_ENV tests by scoping EnvLock in inner blocks, eliminating manual drop calls, and ensuring the original environment variable is captured and restored automatically.
Enhancements:
Tests: