chore(test): migrate root test suite from node:test to vitest#653
Open
cv wants to merge 4 commits intoNVIDIA:mainfrom
Open
chore(test): migrate root test suite from node:test to vitest#653cv wants to merge 4 commits intoNVIDIA:mainfrom
cv wants to merge 4 commits intoNVIDIA:mainfrom
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:
📝 WalkthroughWalkthroughReplaces Node's built-in test runner with Vitest: adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/setup-sandbox-name.test.js (1)
18-20: Consider usingtoContain()for string inclusion checks.The pattern
expect(content.includes('...')).toBeTruthy()works, but Vitest providestoContain()for more readable string assertions with better error messages on failure.♻️ Example refactor
it("accepts sandbox name as $1 with default", () => { - expect(content.includes('SANDBOX_NAME="${1:-nemoclaw}"')).toBeTruthy(); + expect(content).toContain('SANDBOX_NAME="${1:-nemoclaw}"'); });This pattern could also apply to lines 47-48.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/setup-sandbox-name.test.js` around lines 18 - 20, Replace the boolean inclusion assertions with Vitest's clearer string matcher: in the test case it("accepts sandbox name as $1 with default", ...) change the assertion from expect(content.includes('SANDBOX_NAME="${1:-nemoclaw}"')).toBeTruthy() to using expect(content).toContain('SANDBOX_NAME="${1:-nemoclaw}"'); do the same refactor for the other similar assertions referenced around the test (the lines near the second occurrence at lines ~47-48) so all string inclusion checks use toContain() instead of includes(...).toBeTruthy().test/onboard-readiness.test.js (1)
12-14: Consider usingtoBeFalsy()instead of negating insideexpect().The pattern
expect(!condition).toBeTruthy()works but is less idiomatic thanexpect(condition).toBeFalsy(). This applies to multiple assertions in this file (lines 13, 17-18, 29, 37, 41-43, 54, 66-68, 83, 116-118, 146-156, 167).♻️ Example refactor
it("rejects NotReady sandbox", () => { - expect(!isSandboxReady("my-assistant NotReady init failed", "my-assistant")).toBeTruthy(); + expect(isSandboxReady("my-assistant NotReady init failed", "my-assistant")).toBeFalsy(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/onboard-readiness.test.js` around lines 12 - 14, Several tests use the pattern expect(!isSandboxReady(...)).toBeTruthy(), which is less idiomatic; update each such assertion to assert the positive condition with toBeFalsy() instead (e.g., replace expect(!isSandboxReady("my-assistant NotReady init failed", "my-assistant")).toBeTruthy() with expect(isSandboxReady("my-assistant NotReady init failed", "my-assistant")).toBeFalsy()). Apply this change for all occurrences referencing isSandboxReady in this test file (the instances noted in the review: lines around 13, 17-18, 29, 37, 41-43, 54, 66-68, 83, 116-118, 146-156, 167) so assertions read expect(isSandboxReady(...)).toBeFalsy() instead of negating inside expect().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/onboard-readiness.test.js`:
- Around line 12-14: Several tests use the pattern
expect(!isSandboxReady(...)).toBeTruthy(), which is less idiomatic; update each
such assertion to assert the positive condition with toBeFalsy() instead (e.g.,
replace expect(!isSandboxReady("my-assistant NotReady init failed",
"my-assistant")).toBeTruthy() with expect(isSandboxReady("my-assistant
NotReady init failed", "my-assistant")).toBeFalsy()). Apply this change for
all occurrences referencing isSandboxReady in this test file (the instances
noted in the review: lines around 13, 17-18, 29, 37, 41-43, 54, 66-68, 83,
116-118, 146-156, 167) so assertions read
expect(isSandboxReady(...)).toBeFalsy() instead of negating inside expect().
In `@test/setup-sandbox-name.test.js`:
- Around line 18-20: Replace the boolean inclusion assertions with Vitest's
clearer string matcher: in the test case it("accepts sandbox name as $1 with
default", ...) change the assertion from
expect(content.includes('SANDBOX_NAME="${1:-nemoclaw}"')).toBeTruthy() to using
expect(content).toContain('SANDBOX_NAME="${1:-nemoclaw}"'); do the same refactor
for the other similar assertions referenced around the test (the lines near the
second occurrence at lines ~47-48) so all string inclusion checks use
toContain() instead of includes(...).toBeTruthy().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7b3e343b-cbc7-48fd-9997-d4b1ccb328fc
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (24)
.github/workflows/pr.yamlpackage.jsontest/cli.test.jstest/credentials.test.jstest/inference-config.test.jstest/install-preflight.test.jstest/local-inference.test.jstest/nim.test.jstest/onboard-readiness.test.jstest/onboard-selection.test.jstest/onboard.test.jstest/platform.test.jstest/policies.test.jstest/preflight.test.jstest/registry.test.jstest/runner.test.jstest/runtime-shell.test.jstest/security-c2-dockerfile-injection.test.jstest/security-c4-manifest-traversal.test.jstest/service-env.test.jstest/setup-sandbox-name.test.jstest/smoke-macos-install.test.jstest/uninstall.test.jsvitest.config.ts
cv
force-pushed
the
chore/migrate-tests-to-vitest
branch
2 times, most recently
from
362ee37 to
0e95f24
Compare
Unify the test runner across the project — the nemoclaw/ TypeScript plugin already uses Vitest, and this brings the root test/ suite in line. Changes: - Replace node:test describe/it/beforeEach/afterEach with vitest globals - Replace node:assert/strict with vitest expect() matchers: assert.equal → expect().toBe() assert.deepEqual → expect().toEqual() assert.ok → expect().toBeTruthy() assert.match → expect().toMatch() assert.throws → expect().toThrow() assert.fail → expect.unreachable() - Add vitest devDependency and root vitest.config.ts - Update npm test script from `node --test` to `vitest run` All 21 test files migrated via jscodeshift AST codemod (no regex). Zero changes to production code in bin/ or scripts/. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The root test suite was migrated from node:test to vitest in the previous commit. Update the CI workflow to match. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cv
force-pushed
the
chore/migrate-tests-to-vitest
branch
from
0e95f24 to
a95e32b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/onboard-selection.test.js (1)
78-84: ConsidertoSatisfy()for predicate-based array checks.The current pattern works but Vitest's
toSatisfy()matcher can make the intent clearer:♻️ Suggested improvement
- expect( - payload.lines.some((line) => line.includes("Detected local inference option")) - ).toBeTruthy(); + expect(payload.lines).toSatisfy((lines) => + lines.some((line) => line.includes("Detected local inference option")) + );Alternatively, keep as-is since
.some().toBeTruthy()is a common pattern and the current form is clear enough.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/onboard-selection.test.js` around lines 78 - 84, Replace the three assertions that use payload.lines.some(...).toBeTruthy() with Vitest's predicate matcher to make intent clearer: use expect(payload.lines).toSatisfy(lines => lines.some(line => line.includes("Detected local inference option"))) and similarly for "Press Enter to keep the cloud default" and "Cloud models:" so each check references payload.lines and the predicate string; update the three assertions in test/onboard-selection.test.js accordingly.test/nim.test.js (1)
14-17: Consider using Vitest'stoBeTypeOf()for cleaner type assertions.The current pattern
expect(typeof x === "number").toBeTruthy()works but is verbose. Vitest provides a more idiomatic matcher:♻️ Suggested improvement
for (const m of nim.listModels()) { expect(m.name).toBeTruthy(); expect(m.image).toBeTruthy(); - expect(typeof m.minGpuMemoryMB === "number").toBeTruthy(); + expect(m.minGpuMemoryMB).toBeTypeOf("number"); expect(m.minGpuMemoryMB > 0).toBeTruthy(); }The same pattern appears at lines 43-45 for
gpu.count,gpu.totalMemoryMB, andgpu.nimCapable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/nim.test.js` around lines 14 - 17, Replace verbose typeof checks with Vitest's toBeTypeOf matcher: for the model property m.minGpuMemoryMB, use expect(m.minGpuMemoryMB).toBeTypeOf("number") and for the GPU properties use expect(gpu.count).toBeTypeOf("number"), expect(gpu.totalMemoryMB).toBeTypeOf("number"), and expect(gpu.nimCapable).toBeTypeOf("boolean"); locate these assertions around the existing checks for m.minGpuMemoryMB and gpu.count/totalMemoryMB/nimCapable and swap the typeof-expression + toBeTruthy() patterns for the cleaner toBeTypeOf calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/nim.test.js`:
- Around line 14-17: Replace verbose typeof checks with Vitest's toBeTypeOf
matcher: for the model property m.minGpuMemoryMB, use
expect(m.minGpuMemoryMB).toBeTypeOf("number") and for the GPU properties use
expect(gpu.count).toBeTypeOf("number"),
expect(gpu.totalMemoryMB).toBeTypeOf("number"), and
expect(gpu.nimCapable).toBeTypeOf("boolean"); locate these assertions around the
existing checks for m.minGpuMemoryMB and gpu.count/totalMemoryMB/nimCapable and
swap the typeof-expression + toBeTruthy() patterns for the cleaner toBeTypeOf
calls.
In `@test/onboard-selection.test.js`:
- Around line 78-84: Replace the three assertions that use
payload.lines.some(...).toBeTruthy() with Vitest's predicate matcher to make
intent clearer: use expect(payload.lines).toSatisfy(lines => lines.some(line =>
line.includes("Detected local inference option"))) and similarly for "Press
Enter to keep the cloud default" and "Cloud models:" so each check references
payload.lines and the predicate string; update the three assertions in
test/onboard-selection.test.js accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 849070db-b767-42f6-a15a-984ef4432cc8
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (24)
.github/workflows/pr.yamlpackage.jsontest/cli.test.jstest/credentials.test.jstest/inference-config.test.jstest/install-preflight.test.jstest/local-inference.test.jstest/nim.test.jstest/onboard-readiness.test.jstest/onboard-selection.test.jstest/onboard.test.jstest/platform.test.jstest/policies.test.jstest/preflight.test.jstest/registry.test.jstest/runner.test.jstest/runtime-shell.test.jstest/security-c2-dockerfile-injection.test.jstest/security-c4-manifest-traversal.test.jstest/service-env.test.jstest/setup-sandbox-name.test.jstest/smoke-macos-install.test.jstest/uninstall.test.jsvitest.config.ts
✅ Files skipped from review due to trivial changes (12)
- package.json
- .github/workflows/pr.yaml
- vitest.config.ts
- test/service-env.test.js
- test/local-inference.test.js
- test/cli.test.js
- test/onboard.test.js
- test/preflight.test.js
- test/policies.test.js
- test/inference-config.test.js
- test/runner.test.js
- test/uninstall.test.js
🚧 Files skipped from review as they are similar to previous changes (9)
- test/credentials.test.js
- test/registry.test.js
- test/platform.test.js
- test/runtime-shell.test.js
- test/setup-sandbox-name.test.js
- test/security-c2-dockerfile-injection.test.js
- test/onboard-readiness.test.js
- test/security-c4-manifest-traversal.test.js
- test/smoke-macos-install.test.js
npm install with a cached npm store can skip platform-specific optional dependencies (like @rollup/rollup-linux-x64-gnu) that vitest needs. npm ci does a clean install from the lockfile, ensuring all platform binaries are resolved. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The macOS-generated lockfile is missing platform-specific optional dependencies (@rollup/rollup-linux-x64-gnu) needed by vitest on Linux. npm 10 on macOS only records lockfile entries for the current platform. Fix by reverting to the upstream lockfile and using `npm install` (not `npm ci`) in CI, so npm resolves vitest and its platform-specific deps fresh on each run. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ericksoa
approved these changes
Mar 22, 2026
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
node:test+node:assert/strictto Vitest with globalexpect()matchersvitestdevDependency and rootvitest.config.ts(withglobals: true)npm testscript fromnode --testtovitest runpr.yaml) to usenpx vitest runfor root testsWhat changed in test files
All assertion calls converted via jscodeshift AST codemod (no regex):
node:assert/strict)assert.equal(a, b)expect(a).toBe(b)assert.deepEqual(a, b)expect(a).toEqual(b)assert.ok(expr)expect(expr).toBeTruthy()assert.match(s, re)expect(s).toMatch(re)assert.throws(fn, re)expect(fn).toThrow(re)assert.fail(msg)expect.unreachable(msg)What didn't change
bin/,scripts/,nemoclaw/)nemoclaw/plugin keeps its ownvitest.config.ts(no overlap)Motivation
The
nemoclaw/TypeScript plugin already uses Vitest. This unifies the test runner across the project, giving us oneexpect()API, one reporter, and one config format.Test plan
npx vitest run— 20/21 files pass, 213/217 tests passinstall-preflight.test.jsare pre-existing (nvm PATH contamination) — fixed separately in fix(install): prevent nvm/login shell from resetting PATH in subshells #651🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
Tests