Snapshot bump lockfile output and publish stale-lockfile errors (#81) - #119
Open
leynos wants to merge 1 commit into
Open
Snapshot bump lockfile output and publish stale-lockfile errors (#81)#119leynos wants to merge 1 commit into
leynos wants to merge 1 commit into
Conversation
PR #75 introduced text-based CLI output for lockfile operations that was verified only by substring matching: the (lockfile) suffix in lading bump result messages and the multi-line stale-lockfile error raised by lading publish. Add syrupy snapshot coverage for all four cases: a bump message with a root Cargo.lock, a bump message including a nested tests/ui_lints/Cargo.lock, and the PublishPreflightError text for one and for multiple stale lockfiles including their repair commands. Closes #81
|
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:
WalkthroughAdd Syrupy snapshot tests covering ChangesLockfile message snapshot coverage
Suggested labels: Suggested reviewers: Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 20✅ Passed checks (20 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideAdds deterministic syrupy snapshot tests to validate CLI lockfile-related output formats for bump and publish preflight operations, covering root vs nested Cargo.lock handling and single vs multiple stale lockfile errors. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
leynos
marked this pull request as ready for review
July 27, 2026 21:57
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The tests rely on private helpers (
bump._format_result_message,_build_stale_lockfile_message), which may change frequently; consider routing through the public CLI interface or adding a thin public wrapper to reduce brittleness. - The stale lockfile tests are very similar; you could use a parametrized test (single vs multiple lockfiles) to reduce duplication while keeping snapshot coverage intact.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The tests rely on private helpers (`bump._format_result_message`, `_build_stale_lockfile_message`), which may change frequently; consider routing through the public CLI interface or adding a thin public wrapper to reduce brittleness.
- The stale lockfile tests are very similar; you could use a parametrized test (single vs multiple lockfiles) to reduce duplication while keeping snapshot coverage intact.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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_lockfile_message_snapshots.py`:
- Line 40: Update every snapshot assertion comparing snapshot with
_result_message(changes) in the lockfile message tests to include a concise,
case-specific assertion message identifying whether the scenario is root,
nested, single-lockfile, or multiple-lockfile. Apply this consistently to all
four assertions while preserving their existing comparisons.
- Around line 33-70: The four repeated snapshot tests should be consolidated
into parametrized pytest cases, grouped in a focused test class where
appropriate. Parameterize the bump-message scenarios with stable IDs such as
root and nested, and the stale-lockfile scenarios with single and multiple;
preserve each existing input setup, snapshot assertion, and test behavior.
🪄 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: 1a96614f-053d-4344-834b-3367bd58a6d9
📒 Files selected for processing (2)
tests/unit/__snapshots__/test_lockfile_message_snapshots.ambrtests/unit/test_lockfile_message_snapshots.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)
Comment on lines
+33
to
+70
| def test_bump_message_with_root_lockfile(snapshot: SnapshotAssertion) -> None: | ||
| """A workspace-root Cargo.lock is listed with the (lockfile) suffix.""" | ||
| changes = bump.BumpChanges( | ||
| manifests=(_WORKSPACE_ROOT / "Cargo.toml",), | ||
| lockfiles=(_WORKSPACE_ROOT / "Cargo.lock",), | ||
| ) | ||
|
|
||
| assert snapshot == _result_message(changes) | ||
|
|
||
|
|
||
| def test_bump_message_with_nested_lockfile(snapshot: SnapshotAssertion) -> None: | ||
| """A nested Cargo.lock renders relative to the workspace root.""" | ||
| changes = bump.BumpChanges( | ||
| manifests=(_WORKSPACE_ROOT / "Cargo.toml",), | ||
| lockfiles=( | ||
| _WORKSPACE_ROOT / "Cargo.lock", | ||
| _WORKSPACE_ROOT / "tests" / "ui_lints" / "Cargo.lock", | ||
| ), | ||
| ) | ||
|
|
||
| assert snapshot == _result_message(changes) | ||
|
|
||
|
|
||
| def test_stale_lockfile_error_single(snapshot: SnapshotAssertion) -> None: | ||
| """A single stale lockfile lists one repair command.""" | ||
| message = _build_stale_lockfile_message([_WORKSPACE_ROOT / "Cargo.lock"]) | ||
|
|
||
| assert snapshot == message | ||
|
|
||
|
|
||
| def test_stale_lockfile_error_multiple(snapshot: SnapshotAssertion) -> None: | ||
| """Multiple stale lockfiles each list their own repair command.""" | ||
| message = _build_stale_lockfile_message([ | ||
| _WORKSPACE_ROOT / "Cargo.lock", | ||
| _WORKSPACE_ROOT / "tests" / "ui_lints" / "Cargo.lock", | ||
| ]) | ||
|
|
||
| assert snapshot == message |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Parameterize the repeated snapshot cases.
Replace the two bump tests and two stale-lockfile tests with parametrized cases, retaining stable IDs such as root, nested, single, and multiple. Group related cases in a focused pytest class where appropriate.
🤖 Prompt for 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.
In `@tests/unit/test_lockfile_message_snapshots.py` around lines 33 - 70, The four
repeated snapshot tests should be consolidated into parametrized pytest cases,
grouped in a focused test class where appropriate. Parameterize the bump-message
scenarios with stable IDs such as root and nested, and the stale-lockfile
scenarios with single and multiple; preserve each existing input setup, snapshot
assertion, and test behavior.
Sources: Coding guidelines, Path instructions
| lockfiles=(_WORKSPACE_ROOT / "Cargo.lock",), | ||
| ) | ||
|
|
||
| assert snapshot == _result_message(changes) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add messages to every snapshot assertion.
Change each bare assertion to include a concise case-specific message identifying the root, nested, single, or multiple-lockfile scenario.
Also applies to: 53-53, 60-60, 70-70
🤖 Prompt for 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.
In `@tests/unit/test_lockfile_message_snapshots.py` at line 40, Update every
snapshot assertion comparing snapshot with _result_message(changes) in the
lockfile message tests to include a concise, case-specific assertion message
identifying whether the scenario is root, nested, single-lockfile, or
multiple-lockfile. Apply this consistently to all four assertions while
preserving their existing comparisons.
Source: Path instructions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #81
PR #75 introduced text-based CLI output for lockfile operations that was verified only by substring matching. New
tests/unit/test_lockfile_message_snapshots.pyadds syrupy snapshot coverage for all four cases:Cargo.lock((lockfile)suffix)tests/ui_lints/Cargo.lockPublishPreflightErrormessage for a single stale lockfile (with repair command)PublishPreflightErrormessage for multiple stale lockfilesSnapshots use a fixed
/wsworkspace root so they stay deterministic.Testing
make check-fmt,make lint,make typecheck, andmake test(559 passed) all green.coderabbit review --agent: 0 findings.🤖 Generated with Claude Code
Summary by Sourcery
Add snapshot-based tests to lock in CLI output for lockfile bump and stale lockfile publish preflight messages.
Tests: