Add Hypothesis tests for bump_lockfiles path resolution (#93) - #129
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
SummaryAdds Hypothesis property-based tests for Test coverage
Formatting, linting, type checking, and the full test suite pass with 697 tests. WalkthroughAdd Hypothesis property tests for ChangesLockfile path validation
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 20✅ Passed checks (20 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideAdds Hypothesis-based property tests to validate bump_lockfiles manifest path resolution invariants, including normalization, deduplication, ordering, and rejection of paths escaping the workspace root. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
cdf28ae to
516fa42
Compare
516fa42 to
b3a9016
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
b3a9016 to
18ee5ba
Compare
18ee5ba to
6e4ad75
Compare
b241742 to
2dc6390
Compare
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
2dc6390 to
df8de67
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
df8de67 to
6bf17fc
Compare
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
The path normalisation, deduplication, and ordering invariants in bump_lockfiles manifest resolution were exercised only by fixed parametrised cases. Add property tests covering the four invariants from issue #93: arbitrary in-workspace manifest strings (including redundant "." segments) always produce a normalised sibling Cargo.lock path; every spelling of the workspace root manifest deduplicates to exactly one root entry; the workspace root Cargo.lock is always the first element regardless of input order; and any manifest path escaping the workspace root raises LockfileRegenerationError. The escape test anchors the workspace in a subdirectory and excludes the degenerate suffix that would legitimately resolve back inside. Closes #93
Respond to reviewer comments on the manifest-path resolution property tests: - Add diagnostic messages to every assertion in test_inside_manifests_resolve_to_sibling_lockfiles so a Hypothesis counter-example names the violated invariant. Convert the bare relative_to() containment call into an explicit is_relative_to assertion with a message. - Rename the module-level strategies to UPPER_SNAKE_CASE constants (_SEGMENT, _INSIDE_DIR) to match the naming convention for module-level values. - Strengthen the ordering invariant: compare the full resolved tuple against an ordered expected tuple built with dict.fromkeys (root Cargo.lock first, then per-manifest siblings in execution order, duplicates removed) instead of a set comparison, which permitted reordered nested lockfiles to pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
6bf17fc to
fb13d90
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/test_bump_lockfiles.py`:
- Around line 289-295: Extend the _INSIDE_DIR Hypothesis strategy to generate
safe ".." parent-traversal segments alongside _SEGMENT and "." entries, while
keeping generated paths within the workspace after normalization. Preserve the
existing bounds and ensure cases such as crate/../Cargo.toml are exercised
without allowing traversal above the workspace root.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2019179c-15d3-44a0-978b-3a6e01cbde43
📒 Files selected for processing (1)
tests/unit/test_bump_lockfiles.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/cmd-mox(auto-detected)leynos/cuprum(auto-detected)leynos/shared-actions(auto-detected)
Extend the _INSIDE_DIR Hypothesis strategy so generated in-workspace manifest paths also include ".." parent-traversal segments alongside real and "." segments. A composite strategy tracks the running segment balance and only emits ".." while a prior real segment remains to cancel it, so paths such as crate/../Cargo.toml are exercised while never ascending above the workspace root after normalisation. The existing list bounds (0-4 segments) are preserved, and resolve_lockfile_paths continues to accept every generated manifest. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
Summary
Closes #93
Property tests covering the four invariants of manifest-path resolution in
bump_lockfiles:.segments) always produce a normalised siblingCargo.lockpath inside the workspace.Cargo.toml,./Cargo.toml, repeated) produces exactly one root entry.Cargo.lockis always the first element, regardless of input order; resolved paths are unique.LockfileRegenerationError. The escape test anchors the workspace in a subdirectory and excludes (viaassume) the degenerate suffix that would legitimately resolve back inside.Testing
make check-fmt,make lint,make typecheck, andmake test(697 passed) all green after rebasing onto currentmain.🤖 Generated with Claude Code
Summary by Sourcery
Add property-based tests to verify manifest path resolution behavior in bump_lockfiles, including normalization, deduplication, ordering, and rejection of paths escaping the workspace root.
Tests:
References