Skip to content

test(e2e): migrate Spark install smoke to Vitest - #5131

Closed
cv wants to merge 18 commits into
mainfrom
codex/e2e-migrate-spark-install
Closed

test(e2e): migrate Spark install smoke to Vitest#5131
cv wants to merge 18 commits into
mainfrom
codex/e2e-migrate-spark-install

Conversation

@cv

@cv cv commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator

Summary

Migrates the manual DGX Spark install smoke from a standalone bash script into the live Vitest E2E project. The replacement stays explicitly gated because it performs a real host install, while removing the stale bash entry point.

Related Issue

Part of #5098.

Changes

  • Add test/e2e-scenario/live/spark-install.test.ts with the same Linux, Docker, non-interactive install, public-install, PATH refresh, nemoclaw, and openshell checks.
  • Gate the destructive host install behind NEMOCLAW_E2E_SPARK_INSTALL=1 in addition to the live E2E project opt-in.
  • Delete test/e2e/test-spark-install.sh.

Type of Change

  • Code change (feature, bug fix, or refactor)
  • Code change with doc updates
  • Doc only (prose changes, no code sample modifications)
  • Doc only (includes code sample changes)

Verification

  • npx prek run --all-files passes
  • npm test passes
  • Tests added or updated for new or changed behavior
  • No secrets, API keys, or credentials committed
  • Docs updated for user-facing behavior changes
  • npm run docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: Carlos Villela cvillela@nvidia.com

@cv cv self-assigned this Jun 10, 2026
@copy-pr-bot

copy-pr-bot Bot commented Jun 10, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: c49c9494-c845-4d2a-ac6a-39bbf9c01486

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/e2e-migrate-spark-install

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: None
Optional E2E: openshell-version-pin-vitest

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/codex/e2e-simplify-migration-tracking
Head: HEAD
Confidence: high

Required E2E

  • None. No merge-blocking E2E is required because this PR only migrates E2E test code and removes the legacy bash test entry point; it does not change NemoClaw runtime behavior, installer implementation, onboarding logic, sandbox lifecycle, credential handling, security boundaries, network policy, inference routing, deployment, or user-facing assistant flows.

Optional E2E

  • openshell-version-pin-vitest (low): Optional confidence check for the same free-standing live Vitest scenario harness pattern under test/e2e-scenario/live. It does not validate the new Spark installer scenario directly, but can catch broad project/fixture breakage.

New E2E recommendations

  • spark-install-vitest-dispatch (medium): The new test is a free-standing live Vitest file and is guarded by NEMOCLAW_E2E_SPARK_INSTALL=1. Existing E2E / Vitest Scenarios workflow only runs registry-scenarios.test.ts plus openshell-version-pin.test.ts, so this migrated Spark install smoke appears to lack a dedicated dispatchable workflow job.
    • Suggested test: Add a manual, opt-in spark-install-vitest job in .github/workflows/e2e-vitest-scenarios.yaml that sets NEMOCLAW_E2E_SPARK_INSTALL=1, NEMOCLAW_NON_INTERACTIVE=1, and NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1, then runs test/e2e-scenario/live/spark-install.test.ts on an appropriate Linux/DGX Spark-capable runner.

@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

E2E Scenario Advisor Recommendation

Required scenario E2E: e2e-scenarios-all
Optional scenario E2E: None

Dispatch required scenario E2E:

  • gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref>

Workflow run

Full scenario advisor summary

E2E Scenario Advisor

Base: origin/codex/e2e-simplify-migration-tracking
Head: HEAD
Confidence: medium

Required scenario E2E

  • e2e-scenarios-all: A new live Vitest scenario E2E file was added under test/e2e-scenario/live/. It is not a trusted-main live-supported typed registry scenario ID, so a targeted dispatch cannot be recommended; use the scenario fan-out as the conservative valid workflow dispatch.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref>

Optional scenario E2E

  • None.

Relevant changed files

  • test/e2e-scenario/live/spark-install.test.ts

@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor

Findings: 0 needs attention, 3 worth checking, 0 nice ideas
Since last review: 0 prior items resolved, 2 still apply, 0 new items found

Review findings

🛠️ Needs attention

  • None.

🔎 Worth checking

  • Source-of-truth review needed: test/e2e-scenario/live/spark-install.test.ts: The advisor marked localized patch analysis as needs_followup.
    • Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
    • Evidence: `runSparkInstallTest` gates on `NEMOCLAW_E2E_SPARK_INSTALL`, but workflow/docs references for `spark-install.test.ts` were not found.
  • Spark install smoke is gated but not wired or documented as the replacement entry point (test/e2e-scenario/live/spark-install.test.ts:15): The PR deletes the standalone `test/e2e/test-spark-install.sh` entry point and adds a free-standing live Vitest test that only runs when `NEMOCLAW_E2E_SPARK_INSTALL=1`. The maintained `e2e-vitest-scenarios.yaml` workflow explicitly runs `registry-scenarios.test.ts` and the existing `openshell-version-pin.test.ts` free-standing job, but it does not reference `spark-install.test.ts`; docs also do not provide the replacement command. This leaves the migrated smoke difficult to discover and makes the PR-body fidelity claim about updated workflow coverage unsupported by the diff.
    • Recommendation: Either add a safe manual workflow job/step for `test/e2e-scenario/live/spark-install.test.ts` with pinned actions, `persist-credentials: false`, artifact allowlisting, and no untrusted URL input, or update the E2E migration/docs surface to state that Spark install is intentionally manual-only and provide the exact replacement Vitest invocation and required environment variables.
    • Evidence: `runSparkInstallTest` uses `test.skip` unless `NEMOCLAW_E2E_SPARK_INSTALL === "1"`; workflow inspection found entries for `registry-scenarios.test.ts` and `openshell-version-pin.test.ts` but no `spark-install.test.ts`; `spark-install.md` does not mention the Vitest replacement.
  • Public install mode still allows an arbitrary curl-piped installer URL (test/e2e-scenario/live/spark-install.test.ts:21): When `NEMOCLAW_E2E_PUBLIC_INSTALL=1`, the test reads `NEMOCLAW_INSTALL_SCRIPT_URL` and pipes `curl -fsSL "$url"` directly into `bash`. Quoting avoids simple shell metacharacter injection, but there is no scheme or trusted-origin validation before executing remote content on the host. This preserves the legacy behavior behind opt-in, but it remains an installer-trust and workflow-boundary risk if the test is exposed through a workflow or run with a mistaken or compromised environment.
    • Recommendation: Constrain the override to trusted HTTPS installer origins, or explicitly document it as trusted-operator-only and ensure any workflow that runs this test never accepts the URL from untrusted dispatch inputs.
    • Evidence: `installCommand()` builds `url="${NEMOCLAW_INSTALL_SCRIPT_URL:-https://www.nvidia.com/nemoclaw.sh}"\` followed by `curl -fsSL "$url" | NEMOCLAW_NON_INTERACTIVE=1 NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1 bash`.

🌱 Nice ideas

  • None.
Consider writing more tests for
  • **Runtime validation** — Assert the Spark install smoke remains skipped unless `NEMOCLAW_E2E_SPARK_INSTALL=1`.. The code change is test-only, but it replaces a direct host installer entry point and preserves a destructive public installer path. Runtime execution is intentionally manual; support-level validation would improve confidence in the skip and workflow boundary without running the installer in normal tests.
  • **Runtime validation** — Assert the Spark install smoke is either present in a safe manual workflow job or documented as manual-only with the exact Vitest command and required env.. The code change is test-only, but it replaces a direct host installer entry point and preserves a destructive public installer path. Runtime execution is intentionally manual; support-level validation would improve confidence in the skip and workflow boundary without running the installer in normal tests.
  • **Runtime validation** — If URL validation is added, reject non-HTTPS and untrusted `NEMOCLAW_INSTALL_SCRIPT_URL` values while allowing the default trusted installer URL.. The code change is test-only, but it replaces a direct host installer entry point and preserves a destructive public installer path. Runtime execution is intentionally manual; support-level validation would improve confidence in the skip and workflow boundary without running the installer in normal tests.
  • **Acceptance clause:** Part of Epic: Migrate legacy bash E2E into the Vitest E2E system #5098. — add test evidence or identify existing coverage. The deterministic context did not include issue Epic: Migrate legacy bash E2E into the Vitest E2E system #5098 body or comments. Repository docs identify Epic: Migrate legacy bash E2E into the Vitest E2E system #5098 as the direct legacy bash-suite migration epic, but no issue clauses were available to map beyond the PR-body reference.
  • **Acceptance clause:** Legacy E2E deletion evidence: Fidelity verification: focused checks listed in this PR plus the updated workflow coverage exercise the replacement Vitest path. — add test evidence or identify existing coverage. The focused checks are present in `spark-install.test.ts`, but no updated workflow coverage for `spark-install.test.ts` was found in `.github/workflows/e2e-vitest-scenarios.yaml` or workflow-boundary support tests.
  • **test/e2e-scenario/live/spark-install.test.ts** — Partial: the code-level `test.skip` gate exists, but there is no support test or workflow-boundary test proving the smoke is skipped by default or intentionally exposed only through a safe manual entry point.. `runSparkInstallTest` gates on `NEMOCLAW_E2E_SPARK_INSTALL`, but workflow/docs references for `spark-install.test.ts` were not found.
Since last review details

Current findings:

  • Source-of-truth review needed: test/e2e-scenario/live/spark-install.test.ts: The advisor marked localized patch analysis as needs_followup.
    • Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
    • Evidence: `runSparkInstallTest` gates on `NEMOCLAW_E2E_SPARK_INSTALL`, but workflow/docs references for `spark-install.test.ts` were not found.
  • Spark install smoke is gated but not wired or documented as the replacement entry point (test/e2e-scenario/live/spark-install.test.ts:15): The PR deletes the standalone `test/e2e/test-spark-install.sh` entry point and adds a free-standing live Vitest test that only runs when `NEMOCLAW_E2E_SPARK_INSTALL=1`. The maintained `e2e-vitest-scenarios.yaml` workflow explicitly runs `registry-scenarios.test.ts` and the existing `openshell-version-pin.test.ts` free-standing job, but it does not reference `spark-install.test.ts`; docs also do not provide the replacement command. This leaves the migrated smoke difficult to discover and makes the PR-body fidelity claim about updated workflow coverage unsupported by the diff.
    • Recommendation: Either add a safe manual workflow job/step for `test/e2e-scenario/live/spark-install.test.ts` with pinned actions, `persist-credentials: false`, artifact allowlisting, and no untrusted URL input, or update the E2E migration/docs surface to state that Spark install is intentionally manual-only and provide the exact replacement Vitest invocation and required environment variables.
    • Evidence: `runSparkInstallTest` uses `test.skip` unless `NEMOCLAW_E2E_SPARK_INSTALL === "1"`; workflow inspection found entries for `registry-scenarios.test.ts` and `openshell-version-pin.test.ts` but no `spark-install.test.ts`; `spark-install.md` does not mention the Vitest replacement.
  • Public install mode still allows an arbitrary curl-piped installer URL (test/e2e-scenario/live/spark-install.test.ts:21): When `NEMOCLAW_E2E_PUBLIC_INSTALL=1`, the test reads `NEMOCLAW_INSTALL_SCRIPT_URL` and pipes `curl -fsSL "$url"` directly into `bash`. Quoting avoids simple shell metacharacter injection, but there is no scheme or trusted-origin validation before executing remote content on the host. This preserves the legacy behavior behind opt-in, but it remains an installer-trust and workflow-boundary risk if the test is exposed through a workflow or run with a mistaken or compromised environment.
    • Recommendation: Constrain the override to trusted HTTPS installer origins, or explicitly document it as trusted-operator-only and ensure any workflow that runs this test never accepts the URL from untrusted dispatch inputs.
    • Evidence: `installCommand()` builds `url="${NEMOCLAW_INSTALL_SCRIPT_URL:-https://www.nvidia.com/nemoclaw.sh}"\` followed by `curl -fsSL "$url" | NEMOCLAW_NON_INTERACTIVE=1 NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1 bash`.

Workflow run details

This is an automated advisory review. A human maintainer must make the final merge decision.

@wscurran wscurran added area: ci CI workflows, checks, release automation, or GitHub Actions area: e2e End-to-end tests, nightly failures, or validation infrastructure platform: dgx-spark Affects DGX Spark hardware or workflows refactor PR restructures code without intended behavior change labels Jun 10, 2026
@wscurran

Copy link
Copy Markdown
Contributor

Base automatically changed from codex/e2e-simplify-migration-tracking to main June 10, 2026 20:53
@jyaunches

Copy link
Copy Markdown
Contributor

Closing this draft codex migration PR as superseded by the simplified #5098 per-script migration process. The branch carries broad/stale framework-era diff noise; if this test is still needed, we'll salvage the useful core in a fresh/simple one-script PR using nemoclaw-e2e-legacy-migrate.

@jyaunches jyaunches closed this Jun 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: ci CI workflows, checks, release automation, or GitHub Actions area: e2e End-to-end tests, nightly failures, or validation infrastructure platform: dgx-spark Affects DGX Spark hardware or workflows refactor PR restructures code without intended behavior change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants