test(e2e): migrate test-spark-install.sh to vitest - #5608
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:
📝 WalkthroughWalkthroughA new ChangesSpark Install Vitest E2E
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 unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most covered files.
Updated |
Vitest E2E Scenario Results — ❌ Some jobs failedRun: 27982893726
|
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
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
|
Vitest E2E Scenario Results — ❌ Some jobs failedRun: 27983056821
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@test/e2e-scenario/live/spark-install.test.ts`:
- Around line 157-158: The expect statements on lines 157 and 158 use the
nullish coalescing operator `?? "1"` which provides a default value when the
environment variables are undefined, causing the test to pass even when the
required environment variables are not actually set. Remove the `?? "1"` default
from both the `process.env.NEMOCLAW_NON_INTERACTIVE` and
`process.env.NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE` assertions so that the test
directly validates these required environment variables are explicitly present
and set to "1" rather than defaulting them, thus properly enforcing the
required-env contract.
- Around line 52-67: The buildInstallerInvocation function contains conditional
branches (checking process.env.NEMOCLAW_E2E_PUBLIC_INSTALL and similar
environment variable conditions at the other mentioned locations) that violate
the test-conditionals growth guardrail. Refactor this function to use a
branch-free style by extracting the conditional logic outside the function or
using configuration injection to determine which installer invocation to build,
rather than using if/else statements within the test setup code. This applies to
all three locations mentioned in the guardrail violation (the
buildInstallerInvocation function and the other conditional blocks at lines 139
and 176).
- Around line 167-197: The install command fails because the parent directory of
installLog (which may be a path like .../logs/install.log) does not exist before
the bash command attempts to write to it. Create the parent directory of
installLog before calling host.command with the installer script to ensure the
directory path exists when the output is redirected. Use Node's file system
utilities to extract the directory path from installLog and create it with the
recursive option enabled.
🪄 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: b3d52df6-d570-4dd5-afe6-76ac5d11026f
📒 Files selected for processing (2)
.github/workflows/e2e-vitest-scenarios.yamltest/e2e-scenario/live/spark-install.test.ts
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Vitest E2E Scenario Results — ❌ Some jobs failedRun: 27983149394
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
PR Review Advisor — No blocking findingsMerge posture: No blocking advisor findings Action checklist
Test follow-ups to resolve or justifyIf these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.
This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision. |
Vitest E2E Scenario Results — ✅ All requested jobs passedRun: 27983458976
|
Vitest E2E Scenario Results — ✅ All requested jobs passedRun: 27983972303
|
Vitest E2E Scenario Results — ✅ All requested jobs passedRun: 27986019551
|
Vitest E2E Scenario Results — ✅ All requested jobs passedRun: 27986651890
|
Vitest E2E Scenario Results — ✅ All requested jobs passedRun: 27988254354
|
Vitest E2E Scenario Results — ✅ All requested jobs passedRun: 27988660786
|
Vitest E2E Scenario Results — ✅ All requested jobs passedRun: 27989632569
|
wscurran
added
area: ci
3 participants
Summary
Migrate
test/e2e/test-spark-install.shwith the simplest equivalent live Vitest coverage.Related Issues
Refs #5098
Assertion parity
/workspaceor script-relative checkout withinstall.sh; otherwise exits 1test/e2e-scenario/live/spark-install.test.tsassertsREPO_ROOT/install.shexists and runs from that rootcoveredprocess.platform === "linux"; workflow job runs onubuntu-latestcovereddocker infomust exit 0host.command("docker", ["info"])andexpect(exitCode).toBe(0)coveredNEMOCLAW_NON_INTERACTIVE=1is requiredNEMOCLAW_NON_INTERACTIVE=1; installer command prefixes itcoveredNEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1is requiredNEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1; installer command prefixes itcoveredbash install.sh --non-interactiveand asserts the command does not includesetup-sparkcoveredNEMOCLAW_E2E_PUBLIC_INSTALL=1usescurl -fsSL $NEMOCLAW_INSTALL_SCRIPT_URL \| ... bashbuildInstallerInvocation()selects/asserts the curl-pipe command and URL/default when that env is setcoveredexpect(install.exitCode).toBe(0)withexitDetail(..., installLog)tailing the logcovered.bashrc/nvm.sh/~/.local/binrefresh,command -v nemoclawsucceedssourceInstalledPathProbe()runs same refresh and asserts exit 0/stdout containsnemoclawcoveredcommand -v openshellsucceedsopenshellcoverednemoclaw --helpexits 0nemoclaw --help >/dev/nulland asserts exit 0coveredscenario.json,installer.json, shell artifacts, and workflow uploadse2e-artifacts/vitest/spark-install/strongerAll legacy assertions are covered or intentionally stronger in Vitest. No row is
missing,partial, orcandidate only.Contract mapping
test/e2e-scenario/live/spark-install.test.tscovers A1-A12.bash install.sh --non-interactive, optionalcurl -fsSL ... | bash, install log file, profile/PATH refresh, realnemoclawandopenshellcommands.Simplicity check
e2e-vitest-scenarios.yamljobspark-install-vitestonubuntu-latestwith Docker.gh workflow run e2e-vitest-scenarios.yaml --repo NVIDIA/NemoClaw --ref e2e-migrate-test-spark-install -f jobs=spark-install-vitest -f pr_number=<PR>Pre-push parity gate
test/e2e/test-spark-install.sh121200yes — Linux + Docker manual legacy contract mapped to ubuntu-latest + Docker selective Vitest jobAdvisor follow-up
host.commandredaction and retainedinstall.logis written throughwriteRedactedInstallLog; helper test proves sentinel API key redaction in both retained log and failure detail.https://www.nvidia.com/nemoclaw.shand enablesset -euo pipefail.e2e-spark-install-test-owned prefix before cleanup/install.shouldRunInstallerIntegration()and removed unused import.Verification
NEMOCLAW_RUN_E2E_SCENARIOS=1 npx vitest run --project e2e-scenarios-live test/e2e-scenario/live/spark-install.test.ts --reporter=default(skips on local macOS)npx tsx -e "import { validateE2eVitestScenariosWorkflowBoundary } from './tools/e2e-scenarios/workflow-boundary.mts'; const errors = validateE2eVitestScenariosWorkflowBoundary(); console.log(errors.join('\\n')); process.exit(errors.length ? 1 : 0);"npx vitest run --project e2e-vitest-support test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts --reporter=defaultnpx vitest run --project e2e-vitest-support test/e2e-scenario/support-tests/spark-install-helpers.test.ts --reporter=defaultnpm run build:cligit diff --checknpx prek run --all-files --stage pre-push --skip tsc-plugin --skip tsc-js --skip tsc-cli --skip version-tag-sync --skip test-cli --skip test-plugin --skip source-shape-test-budget --skip test-file-size-budget --skip test-skills-yamlqueued, validates hardening commitfad3eaf32)success,spark-install-vitestpassed onubuntu-latest+ Docker)Summary by CodeRabbit
Release Notes
Tests
CI/CD Updates