test(snapshot): cover create and list flows#5481
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe test file for the sandbox snapshot action is updated to expand Vitest mock coverage. New mocks are introduced for ChangesSandbox Snapshot Test Expansion
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the branch is 96%. Coverage data for the branch is not yet available. Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the branch is 46%. Coverage data for the branch is not yet available. Show a code coverage summary of the most covered files.
Updated |
Contributor
E2E Advisor RecommendationRequired E2E: None Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
|
Contributor
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: None Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorBase: Required Vitest E2E scenarios
Optional Vitest E2E scenarios
Relevant changed files
|
Contributor
PR Review AdvisorFindings: 0 needs attention, 1 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
This is an automated advisory review. A human maintainer must make the final merge decision. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/actions/sandbox/snapshot.test.ts (1)
103-116:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEnsure mocked
isShieldsDownis restored in afinallyblock.Line 104 mutates a shared mocked export and restores it at Line 115. If an assertion throws before Line 115, later tests can inherit
undefinedand fail for the wrong reason.Suggested patch
it("refuses snapshot creation before backup when the shields gate helper is unavailable", async () => { const consoleError = vi.spyOn(console, "error").mockImplementation(() => {}); const shields = await import("../../shields"); - vi.mocked(shields).isShieldsDown = undefined as never; - const { runSandboxSnapshot } = await import("./snapshot"); - - await expect(runSandboxSnapshot("alpha", { kind: "create" })).rejects.toMatchObject({ - exitCode: 1, - }); - - expect(backupSandboxStateMock).not.toHaveBeenCalled(); - expect(consoleError.mock.calls.flat().join("\n")).toContain( - "Cannot verify shields state. Refusing to create snapshot.", - ); - vi.mocked(shields).isShieldsDown = isShieldsDownMock as never; + const originalIsShieldsDown = vi.mocked(shields).isShieldsDown; + vi.mocked(shields).isShieldsDown = undefined as never; + try { + const { runSandboxSnapshot } = await import("./snapshot"); + await expect(runSandboxSnapshot("alpha", { kind: "create" })).rejects.toMatchObject({ + exitCode: 1, + }); + expect(backupSandboxStateMock).not.toHaveBeenCalled(); + expect(consoleError.mock.calls.flat().join("\n")).toContain( + "Cannot verify shields state. Refusing to create snapshot.", + ); + } finally { + vi.mocked(shields).isShieldsDown = originalIsShieldsDown; + } });🤖 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 `@src/lib/actions/sandbox/snapshot.test.ts` around lines 103 - 116, The test mutates the shared `shields.isShieldsDown` mock at the beginning and restores it at the end, but if any assertion fails before the restoration line, the mock state is not cleaned up, causing subsequent tests to fail. Wrap the test logic (from the mock assignment through the assertions) in a try-finally block, moving the restoration of `vi.mocked(shields).isShieldsDown = isShieldsDownMock as never;` into the finally block to ensure it always executes regardless of whether assertions pass or fail.
🤖 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 `@src/lib/actions/sandbox/snapshot.test.ts`:
- Around line 148-174: The test fixture for the snapshot list includes timestamp
data for both snapshots (2026-06-01T00:00:00.000Z and 2026-06-02T00:00:00.000Z),
but the test expectations do not validate that these timestamps are actually
rendered in the console output. Add explicit expect assertions to verify that
both timestamp values are included in the output, similar to the existing
assertions that check for version names, snapshot names, and paths. This will
ensure that any regression in timestamp rendering will be caught by the test.
---
Outside diff comments:
In `@src/lib/actions/sandbox/snapshot.test.ts`:
- Around line 103-116: The test mutates the shared `shields.isShieldsDown` mock
at the beginning and restores it at the end, but if any assertion fails before
the restoration line, the mock state is not cleaned up, causing subsequent tests
to fail. Wrap the test logic (from the mock assignment through the assertions)
in a try-finally block, moving the restoration of
`vi.mocked(shields).isShieldsDown = isShieldsDownMock as never;` into the
finally block to ensure it always executes regardless of whether assertions pass
or fail.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d4bd4345-ce4a-48da-b95b-4e495ccdc1d7
📒 Files selected for processing (1)
src/lib/actions/sandbox/snapshot.test.ts
Comment on lines
+148
to
174
| it("renders a stable snapshot list with versions, names, timestamps, and paths", async () => { | ||
| const consoleLog = vi.spyOn(console, "log").mockImplementation(() => {}); | ||
| listBackupsMock.mockReturnValue([ | ||
| { | ||
| snapshotVersion: 1, | ||
| name: "initial", | ||
| timestamp: "2026-06-01T00:00:00.000Z", | ||
| backupPath: "/tmp/alpha/v1", | ||
| }, | ||
| { | ||
| snapshotVersion: 2, | ||
| name: null, | ||
| timestamp: "2026-06-02T00:00:00.000Z", | ||
| backupPath: "/tmp/alpha/v2", | ||
| }, | ||
| ]); | ||
| const { runSandboxSnapshot } = await import("./snapshot"); | ||
|
|
||
| await runSandboxSnapshot("alpha", { kind: "list" }); | ||
|
|
||
| const output = consoleLog.mock.calls.flat().join("\n"); | ||
| expect(output).toContain("Snapshots for 'alpha'"); | ||
| expect(output).toContain("v1"); | ||
| expect(output).toContain("initial"); | ||
| expect(output).toContain("/tmp/alpha/v2"); | ||
| expect(output).toContain("2 snapshot(s). Restore with:"); | ||
| }); |
Contributor
There was a problem hiding this comment.
Add explicit timestamp assertions in the snapshot-list test.
The fixtures include timestamps (Lines 154 and 160), but the expectations never validate timestamp rendering. A regression in timestamp output could slip through unnoticed.
Suggested patch
expect(output).toContain("Snapshots for 'alpha'");
expect(output).toContain("v1");
expect(output).toContain("initial");
+ expect(output).toContain("2026-06-01");
+ expect(output).toContain("2026-06-02");
expect(output).toContain("/tmp/alpha/v2");
expect(output).toContain("2 snapshot(s). Restore with:");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("renders a stable snapshot list with versions, names, timestamps, and paths", async () => { | |
| const consoleLog = vi.spyOn(console, "log").mockImplementation(() => {}); | |
| listBackupsMock.mockReturnValue([ | |
| { | |
| snapshotVersion: 1, | |
| name: "initial", | |
| timestamp: "2026-06-01T00:00:00.000Z", | |
| backupPath: "/tmp/alpha/v1", | |
| }, | |
| { | |
| snapshotVersion: 2, | |
| name: null, | |
| timestamp: "2026-06-02T00:00:00.000Z", | |
| backupPath: "/tmp/alpha/v2", | |
| }, | |
| ]); | |
| const { runSandboxSnapshot } = await import("./snapshot"); | |
| await runSandboxSnapshot("alpha", { kind: "list" }); | |
| const output = consoleLog.mock.calls.flat().join("\n"); | |
| expect(output).toContain("Snapshots for 'alpha'"); | |
| expect(output).toContain("v1"); | |
| expect(output).toContain("initial"); | |
| expect(output).toContain("/tmp/alpha/v2"); | |
| expect(output).toContain("2 snapshot(s). Restore with:"); | |
| }); | |
| it("renders a stable snapshot list with versions, names, timestamps, and paths", async () => { | |
| const consoleLog = vi.spyOn(console, "log").mockImplementation(() => {}); | |
| listBackupsMock.mockReturnValue([ | |
| { | |
| snapshotVersion: 1, | |
| name: "initial", | |
| timestamp: "2026-06-01T00:00:00.000Z", | |
| backupPath: "/tmp/alpha/v1", | |
| }, | |
| { | |
| snapshotVersion: 2, | |
| name: null, | |
| timestamp: "2026-06-02T00:00:00.000Z", | |
| backupPath: "/tmp/alpha/v2", | |
| }, | |
| ]); | |
| const { runSandboxSnapshot } = await import("./snapshot"); | |
| await runSandboxSnapshot("alpha", { kind: "list" }); | |
| const output = consoleLog.mock.calls.flat().join("\n"); | |
| expect(output).toContain("Snapshots for 'alpha'"); | |
| expect(output).toContain("v1"); | |
| expect(output).toContain("initial"); | |
| expect(output).toContain("2026-06-01"); | |
| expect(output).toContain("2026-06-02"); | |
| expect(output).toContain("/tmp/alpha/v2"); | |
| expect(output).toContain("2 snapshot(s). Restore with:"); | |
| }); |
🤖 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 `@src/lib/actions/sandbox/snapshot.test.ts` around lines 148 - 174, The test
fixture for the snapshot list includes timestamp data for both snapshots
(2026-06-01T00:00:00.000Z and 2026-06-02T00:00:00.000Z), but the test
expectations do not validate that these timestamps are actually rendered in the
console output. Add explicit expect assertions to verify that both timestamp
values are included in the output, similar to the existing assertions that check
for version names, snapshot names, and paths. This will ensure that any
regression in timestamp rendering will be caught by the test.
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
Adds focused snapshot command coverage for successful snapshot creation and list rendering. This improves coverage for the snapshot lifecycle without changing runtime behavior.
Changes
src/lib/actions/sandbox/snapshot.test.tsto cover named snapshot creation after liveness and shields checks pass.Type of Change
Verification
npx prek run --from-ref main --to-ref HEADpassesnpx vitest run src/lib/actions/sandbox/snapshot.test.ts --project clinpm run typecheck:clinpm testpasses (broad runtime changes only)npm run docsbuilds without warnings (doc changes only)Note: commit and push hooks were skipped for this test-only branch because the broad local hook suite has unrelated pre-existing CLI failures in this checkout; targeted tests and CLI typecheck passed.
Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit