test(e2e): migrate test-snapshot-commands.sh to vitest#5346
Open
jyaunches wants to merge 27 commits into
Open
Conversation
Reserve Phase 4 E2E migration work for test-snapshot-commands.sh. Refs #5098
jyaunches
added
area: e2e
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Contributor
|
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:
📝 WalkthroughWalkthroughAdds a new ChangesSnapshot Commands E2E Vitest Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: Dispatch required Vitest E2E scenarios:
Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorBase: Required Vitest E2E scenarios
Optional Vitest E2E scenarios
Relevant changed files
|
Contributor
PR Review AdvisorFindings: 1 needs attention, 5 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Consider writing more tests for
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
Contributor
Vitest E2E Scenario Results — ❌ Some jobs failedRun: 27438097815
|
Contributor
Vitest E2E Scenario Results — ❌ Some jobs failedRun: 27444788389
|
Contributor
Vitest E2E Scenario Results — ❌ Some jobs failedRun: 27446206870
|
Contributor
Vitest E2E Scenario Results — ❌ Some jobs failedRun: 27447293697
|
…shot-commands # Conflicts: # tools/e2e-scenarios/free-standing-jobs.env
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27448496605
|
…shot-commands # Conflicts: # test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts # tools/e2e-scenarios/free-standing-jobs.env
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27452042525
|
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27453710229
|
…shot-commands # Conflicts: # .github/workflows/e2e-vitest-scenarios.yaml # tools/e2e-scenarios/workflow-boundary.mts
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27454075628
|
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27455034736
|
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27455253108
|
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27457008075
|
…shot-commands # Conflicts: # tools/e2e-scenarios/workflow-boundary.mts
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27459326274
|
…shot-commands # Conflicts: # tools/e2e-scenarios/workflow-boundary.mts
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27459963711
|
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27462306048
|
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
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27594969650
|
![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: 5
🤖 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 @.github/workflows/e2e-vitest-scenarios.yaml:
- Around line 2185-2186: The job timeout-minutes is set to 30 minutes, which
matches the Vitest test timeout of 30 minutes. Since setup, npm install, Docker
auth, and artifact upload all consume time within the same job, GitHub Actions
can kill the job before Vitest completes cleanup when the test approaches its
timeout limit. Increase the timeout-minutes value to provide headroom above the
test timeout (for example, change it to 45 minutes) to ensure the job remains
alive long enough for Vitest to finish its cleanup and artifact handling.
In `@test/e2e-scenario/live/snapshot-commands.test.ts`:
- Around line 132-137: The credential leak detection regex pattern
`/nvapi-|sk-|Bearer /` only catches token-shaped values but misses credential
key names. Update this regex pattern in the file body test to detect both
token-shaped values AND common credential/API key variable names such as
NVIDIA_API_KEY, NVIDIA_INFERENCE_API_KEY, OPENAI_API_KEY, and similar provider
key identifiers. The enhanced pattern should flag files containing credential
key definitions regardless of whether the assigned values match token patterns.
- Around line 308-320: The latest-restore test phase only verifies that the
SECOND_MARKER file is correctly restored, but does not check that stale files
like MARKER_FILE (which was deliberately set to BROKEN before the restore) are
actually removed. After the existing expectSandboxFileContent call that
validates SECOND_MARKER content, add another assertion to verify that
MARKER_FILE is absent after the latest restore, using the same absence-checking
pattern that is used for the timestamp-targeted restore phase earlier in the
test.
In `@test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts`:
- Around line 915-917: The test mutation for the checkout action is tightly
coupled to a specific pinned SHA, which causes the test to fail when the pin is
rotated to a new version. Instead of checking for an exact match on the full
`actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10` string in the
condition, modify the check to match any version of the `actions/checkout`
action regardless of the SHA (such as checking if `step.uses` starts with or
contains `actions/checkout`). This ensures the mutation applies consistently
across checkout pin updates.
In `@tools/e2e-scenarios/workflow-boundary.mts`:
- Around line 2469-2473: The validator for the snapshot-commands-vitest job is
missing enforcement of the timeout-minutes property, which should be set to 30
minutes according to the intended contract. Add an explicit validation check
after the existing runs-on check that verifies job["timeout-minutes"] equals 30
and pushes an appropriate error message to the errors array if it does not,
similar to the pattern used for validating the runs-on property.
🪄 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: 4a6b446b-3047-47f5-a50a-51edc018323a
📒 Files selected for processing (4)
.github/workflows/e2e-vitest-scenarios.yamltest/e2e-scenario/live/snapshot-commands.test.tstest/e2e-scenario/support-tests/e2e-scenarios-workflow.test.tstools/e2e-scenarios/workflow-boundary.mts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts (1)
908-912:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTypeScript error: type assertion doesn't allow
timeout-minutesproperty.The static analysis correctly identifies that
"timeout-minutes"cannot index the job type since it only definesenvandsteps. This will fail the build.Proposed fix
const parsedWorkflow = YAML.parse(workflow) as { - jobs: Record<string, { env: Record<string, string>; steps: Array<Record<string, unknown>> }>; + jobs: Record<string, { env: Record<string, string>; steps: Array<Record<string, unknown>>; "timeout-minutes"?: number; [key: string]: unknown }>; }; const snapshotJob = parsedWorkflow.jobs["snapshot-commands-vitest"]; snapshotJob["timeout-minutes"] = 30;🤖 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 `@test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts` around lines 908 - 912, The type assertion for parsedWorkflow is too restrictive and does not account for the timeout-minutes property being assigned to snapshotJob. Modify the type annotation in the YAML.parse call to include timeout-minutes as an optional property in the job object type, or restructure the type to allow additional properties beyond env and steps. This will allow the snapshotJob["timeout-minutes"] = 30 assignment to pass TypeScript type checking without errors.Source: Linters/SAST tools
🤖 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.
Outside diff comments:
In `@test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts`:
- Around line 908-912: The type assertion for parsedWorkflow is too restrictive
and does not account for the timeout-minutes property being assigned to
snapshotJob. Modify the type annotation in the YAML.parse call to include
timeout-minutes as an optional property in the job object type, or restructure
the type to allow additional properties beyond env and steps. This will allow
the snapshotJob["timeout-minutes"] = 30 assignment to pass TypeScript type
checking without errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4e2591e2-87b7-4ae1-b742-e552d25d5f9f
📒 Files selected for processing (4)
.github/workflows/e2e-vitest-scenarios.yamltest/e2e-scenario/live/snapshot-commands.test.tstest/e2e-scenario/support-tests/e2e-scenarios-workflow.test.tstools/e2e-scenarios/workflow-boundary.mts
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/e2e-vitest-scenarios.yaml
Contributor
Vitest E2E Scenario Results — ❌ Some jobs failedRun: 27595607806
|
Contributor
Vitest E2E Scenario Results — ❌ Some jobs failedRun: 27595810674
|
Contributor
Vitest E2E Scenario Results — ❌ Some jobs failedRun: 27596164880
|
Contributor
Vitest E2E Scenario Results — ❌ Some jobs failedRun: 27596480849
|
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27596798746
|
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
2 participants
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
Migrate/retire
test/e2e/test-snapshot-commands.shwith the simplest equivalent Vitest coverage.Related Issues
Refs #5098
Contract mapping
install.sh --non-interactivecreates a live OpenClaw sandbox for snapshot commands.test/e2e-scenario/live/snapshot-commands.test.tsrunsbash install.sh --non-interactivewithNEMOCLAW_SANDBOX_NAME=e2e-snapshot.nemoclaw <name> snapshot createreports a versioned snapshot and writes~/.nemoclaw/rebuild-backups/<name>.Snapshot v<N>...createdand host backup path evidence.nemoclawCLI and host backup filesystem.snapshot listincludes snapshots and parseable timestamp selectors./sandbox/.openclaw/workspace, restores latest, then restores the first timestamp.openshell sandbox execagainst the live sandbox..env/JSON files and help advertises create/list/restore.nemoclaw <name> snapshotoutput.Simplicity check
nightly-e2e.yamljobsnapshot-commands-e2eviae2e-script.yaml, defaultruns-on: ubuntu-latest, Docker/OpenShell,NVIDIA_API_KEY, Docker Hub auth,NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1,NEMOCLAW_NON_INTERACTIVE=1,NEMOCLAW_SANDBOX_NAME=e2e-snapshot, 30 minute timeout.e2e-vitest-scenarios.yamljobsnapshot-commands-vitest,runs-on: ubuntu-latest, Docker/OpenShell,NVIDIA_API_KEY, Docker Hub auth, same sandbox/env and 30 minute timeout.gh workflow run e2e-vitest-scenarios.yaml --repo NVIDIA/NemoClaw --ref e2e-migrate/test-snapshot-commands -f jobs=snapshot-commands-vitest -f pr_number=5346.Verification
npm run build:cliNEMOCLAW_RUN_E2E_SCENARIOS=1 npx vitest run --project e2e-scenarios-live test/e2e-scenario/live/snapshot-commands.test.ts --silent=false --reporter=default(local Docker unavailable, test skips before mutation)npx vitest run --project e2e-vitest-support test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts --silent=false --reporter=defaultnpx vitest run --project cli test/e2e-script-workflow.test.ts --silent=false --reporter=defaultnpm run typecheck:cligit diff --checkorigin/main; support workflow test re-ran locally after refresh.workflow_dispatch,jobs=snapshot-commands-vitest) — passedSummary by CodeRabbit
Tests
New Features
Chores