Avoid clobbering manifests in test helper#159
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR enhances the test helper to prevent overwriting existing manifest files by switching from File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Reviews pausedUse the following commands to manage reviews:
Summary by CodeRabbit
WalkthroughSwitch the manifest write path in test_support/src/lib.rs to use persist_noclobber instead of persist within ensure_manifest_exists. This change adds a clarifying comment. No signatures or overall flow otherwise changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor T as Test/Caller
participant E as ensure_manifest_exists
participant FS as Filesystem
participant TF as TempFile
T->>E: ensure_manifest_exists(manifest_path)
E->>FS: Check if manifest_path exists
alt Not exists
E->>FS: Create parent dir(s)
E->>TF: Create temp file with manifest content
E->>FS: persist_noclobber(temp -> manifest_path)
FS-->>E: OK or AlreadyExists
alt AlreadyExists
E-->>T: Return manifest_path (existing)
else OK
E-->>T: Return manifest_path (created)
end
else Exists
E-->>T: Return manifest_path
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
Pre-merge checks✅ Passed checks (5 passed)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test_support/src/lib.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use en-GB-oxendict spelling and grammar in code comments (exceptions allowed for external API names)
Function documentation must include clear examples; test documentation should omit redundant examples
Keep file size manageable: no Rust source file longer than 400 lines; split large switches/dispatch tables and move big test data to external files
Disallow Clippy warnings; fix warnings in code rather than silencing them
Extract helper functions for long functions; maintain separation of concerns and CQRS
Group many related parameters into meaningful structs
If returning a large error type, consider using Arc to reduce data copied/returned
Each Rust module must begin with a module-level //! comment stating purpose and utility
Document public APIs with Rustdoc (///) so cargo doc can generate documentation
Prefer immutable data; avoid unnecessary mut bindings
Handle errors with Result instead of panicking where feasible
Avoid unsafe code unless absolutely necessary and document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions when conditional criteria have more than two branches
Do not silence lints except as a last resort; suppressions must be tightly scoped and include a clear reason
Prefer expect over allow for lint management
Use conditional compilation (#[cfg]/#[cfg_attr]) for functions unused under specific feature sets
Prefer .expect() over .unwrap()
Use concat!() for long string literals instead of escaping newlines with backslashes
Prefer single-line function bodies when appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums (derive std::error::Error via thiserror) for inspectable conditions
Use an opaque eyre::Report only at the application boundary for human-readable logs; do not expose in public APIs
Never export eyre::Report from libraries; convert to domain error enums at API boundaries and to eyre only in main/top-level async tas...
Files:
test_support/src/lib.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.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
test_support/src/lib.rs
🔍 Remote MCP Ref
Additional context for reviewing PR #159 (Avoid clobbering manifests)
- Change implemented: tests now use NamedTempFile::persist_noclobber when writing manifests to avoid overwriting an existing file created concurrently.
- Behavior of persist_noclobber: it fails if the destination path already exists (so it prevents accidental clobbering). Verify the PR checks/handles the returned error appropriately in tests.
- Alternatives noted in the linked issue (#58): drop the temp file and use std::fs::write, or use persist_noclobber — PR chooses persist_noclobber (matches the issue recommendation).
- Reviewer checklist (concise, actionable):
- Ensure persist_noclobber errors are handled/propagated (no silent unwrap that could mask failures).
- Confirm tests include or could simulate the “target exists” case to assert the new behavior.
- Check for any OS-specific rename/atomicity implications (persist_noclobber behavior can differ by platform); consider using explicit write+fsync if atomic semantics are required.
- Note: some remote fetch attempts were partially blocked during research (limited retrievals). If you want full upstream diffs or exact docs pages fetched, I can re-run targeted fetches.
Sources:
- tempfile docs (NamedTempFile::persist_noclobber).
- Issue/PR search for netsuke and issue #58 (proposal and alternatives).
⏰ 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). (2)
- GitHub Check: Sourcery review
- GitHub Check: build-test (stable)
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Break ensure_manifest_exists into smaller helpers to reduce nested control flow while preserving behaviour.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
1 participant
Summary
persist_noclobberTesting
make fmtmake check-fmtmake lintmake testcloses #58
https://chatgpt.com/codex/tasks/task_e_68c6158d7d648322b540014697a08b5d
Summary by Sourcery
Bug Fixes: