Conversation
Reviewer's GuideCentralize environment variable handling by extracting the EnvVarGuard and its helper functions into a shared test support module and updating both the crate’s config module and test suites to use this unified implementation. Class diagram for centralized EnvVarGuard and helpersclassDiagram
class EnvVarGuard {
- key: String
- original: Option<String>
+ set(key: &str, val: &str) -> Self
+ drop()
}
class env_guard {
+ set_env_var(key: &str, val: &str)
+ remove_env_var(key: &str)
}
EnvVarGuard <.. env_guard : uses
%% Show re-export in support module
class support {
+ EnvVarGuard
+ set_env_var(key: &str, val: &str)
+ remove_env_var(key: &str)
}
support <.. env_guard : re-exports
support <.. EnvVarGuard : re-exports
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 46 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 (1)
Summary by CodeRabbit
WalkthroughRefactor environment variable management in tests by extracting the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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/support/env_guard.rs:12` </location>
<code_context>
+ ));
}
- impl EnvVarGuard {
- fn set(key: &str, val: &str) -> Self {
- let original = std::env::var(key).ok();
</code_context>
<issue_to_address>
Consider adding a test for nested EnvVarGuard usage.
Testing nested EnvVarGuards for the same key will verify correct restoration of the original value when guards overlap.
Suggested implementation:
```rust
impl Drop for EnvVarGuard {
fn drop(&mut self) {
```
```rust
#[cfg(test)]
mod tests {
use super::EnvVarGuard;
use std::env;
#[test]
fn test_nested_env_var_guard() {
let key = "ENV_GUARD_TEST_NESTED";
// Ensure the variable is unset before starting
env::remove_var(key);
// Set an initial value
env::set_var(key, "initial");
assert_eq!(env::var(key).unwrap(), "initial");
// First guard sets to "first"
let guard1 = EnvVarGuard::set(key, "first");
assert_eq!(env::var(key).unwrap(), "first");
{
// Nested guard sets to "second"
let guard2 = EnvVarGuard::set(key, "second");
assert_eq!(env::var(key).unwrap(), "second");
// guard2 drops here
}
// After inner guard drops, should be restored to "first"
assert_eq!(env::var(key).unwrap(), "first");
// guard1 drops here
drop(guard1);
// Should be restored to "initial"
assert_eq!(env::var(key).unwrap(), "initial");
// Clean up
env::remove_var(key);
}
}
```
</issue_to_address>
### Comment 2
<location> `tests/support/env_guard.rs:34` </location>
<code_context>
+}
+
+/// Safely set an environment variable for tests.
+pub fn set_env_var(key: &str, value: &str) {
+ // Safety: tests execute serially so no concurrent access occurs.
+ unsafe { std::env::set_var(key, value) };
+}
+
</code_context>
<issue_to_address>
Missing tests for set_env_var and remove_env_var helpers.
Direct unit tests are recommended for these helpers, given their use of unsafe blocks.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
}
=======
}
#[cfg(test)]
mod tests {
use super::*;
#[test]
fn test_set_env_var_sets_variable() {
let key = "TEST_ENV_SET_VAR";
let value = "test_value";
// Ensure the variable is not set before
std::env::remove_var(key);
assert!(std::env::var(key).is_err());
set_env_var(key, value);
assert_eq!(std::env::var(key).unwrap(), value);
// Clean up
std::env::remove_var(key);
}
#[test]
fn test_remove_env_var_removes_variable() {
let key = "TEST_ENV_REMOVE_VAR";
let value = "to_remove";
std::env::set_var(key, value);
assert_eq!(std::env::var(key).unwrap(), value);
remove_env_var(key);
assert!(std::env::var(key).is_err());
}
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `tests/steps/config_steps.rs:8` </location>
<code_context>
use std::path::PathBuf;
use tempfile::TempDir;
+use crate::support::env_guard::{EnvVarGuard, remove_env_var};
use comenqd::config::Config;
</code_context>
<issue_to_address>
Consider adding a test for environment variable removal when not set.
Add a test to verify that calling remove_env_var on an unset key does not cause errors or unexpected behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/support/env_guard.rs (1)
45-103: Comprehensive test coverage addresses previous review feedback.The test module provides excellent coverage of all scenarios including edge cases and nested guard behaviour. The use of
#[serial_test::serial]properly handles test isolation for environment variable manipulation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/support/env_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.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/env_guard.rs
🧬 Code Graph Analysis (1)
tests/support/env_guard.rs (1)
tests/steps/config_steps.rs (2)
set_env_var(83-85)drop(123-127)
🔇 Additional comments (4)
tests/support/env_guard.rs (4)
1-4: Module documentation meets requirements.The module documentation correctly uses
//!comments and clearly explains the module's purpose and utility as required by the coding guidelines.
6-10: Well-designed RAII struct.The
EnvVarGuardstruct correctly implements the RAII pattern with appropriate field types to handle both set and unset environment variables.
12-22: Correct RAII implementation.The
setmethod properly captures the original environment variable state before modification and constructs the guard correctly for restoration on drop.
24-31: Proper cleanup implementation.The
Dropimplementation correctly restores the environment variable to its original state, handling both previously set and unset variables appropriately.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/support/env_guard.rs (1)
33-46: Remove unnecessary unsafe blocks.
std::env::set_varandstd::env::remove_varare safe functions that don't require unsafe blocks. The concurrency concerns are addressed by the#[serial_test::serial]attribute on tests, not by unsafe code.Apply this diff to fix the unnecessary unsafe usage:
/// Set an environment variable for tests. -/// -/// The nightly compiler marks `std::env::set_var` as `unsafe`. -/// Tests run serially so using it is acceptable here. pub fn set_env_var(key: &str, value: &str) { - unsafe { std::env::set_var(key, value) }; + std::env::set_var(key, value); } /// Remove an environment variable for tests. -/// -/// `std::env::remove_var` is also `unsafe` on nightly. pub fn remove_env_var(key: &str) { - unsafe { std::env::remove_var(key) }; + std::env::remove_var(key); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/support/env_guard.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
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.
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Useconcat!()to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums. Derivestd::error::Error(via thethiserrorcrate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary. Useeyre::Reportfor human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library. Convert to domain enums at API boundaries, and toeyreonly in the mainmain()entrypoint or top-level async task.
Files:
tests/support/env_guard.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.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/env_guard.rs
🧬 Code Graph Analysis (1)
tests/support/env_guard.rs (1)
tests/steps/config_steps.rs (2)
set_env_var(83-85)drop(123-127)
🔇 Additional comments (4)
tests/support/env_guard.rs (4)
1-10: Excellent module documentation and struct design.The module-level documentation clearly explains the purpose and utility, and the
EnvVarGuardstruct is well-designed with appropriate fields for tracking the key and original value.
12-22: Solid RAII implementation.The
setmethod correctly captures the original environment variable value before setting the new one, ensuring proper restoration on drop.
24-31: Correct restoration logic in Drop.The Drop implementation properly restores the environment variable to its original state, handling both cases where the variable previously existed or was unset.
48-49: Proper test module structure.The test module is correctly configured with
#[cfg(test)]following Rust conventions.
1 participant
Summary
Testing
make fmtmake lintmake testmake markdownlinthttps://chatgpt.com/codex/tasks/task_e_6888f7ea4f2083228cdd174537e3132b
Summary by Sourcery
Centralize environment variable guard logic for tests into a shared support module and update existing tests to use the common implementation
Enhancements: