Repository navigation
test(cli): start splitting cli test suite - #4895
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
Ready to act? Review this PR in Change Stack to turn feedback into patch suggestions you can inspect and refine. Warning Review limit reached
More reviews will be available in 3 minutes and 33 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a centralized CLI test helpers module, refactors existing CLI tests to use it (including hermetic temporary HOME handling), and introduces focused Vitest suites for Docker-outage classification and live inference/upgrade/share CLI behaviors; also tweaks a preflight docker stub. ChangesCLI Test Infrastructure and Coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 |
E2E Advisor RecommendationRequired E2E: None Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
|
E2E Scenario Advisor RecommendationRequired scenario E2E: None Full scenario advisor summaryE2E Scenario AdvisorBase: Required scenario E2E
Optional scenario E2E
Relevant changed files
|
PR Review AdvisorFindings: 0 needs attention, 0 worth checking, 0 nice ideas Consider writing more tests for
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/cli/helpers.ts (1)
166-170: WidenwriteSandboxRegistry()override types to match what it actually writes tosandboxes.json
test/cli/helpers.tsdefinesSandboxEntrywith only a small set of fields, butwriteSandboxRegistry()spreads whatever keys it’s given into the JSON. Tests therefore resort toas unknown as Partial<SandboxEntry>to pass fields not inSandboxEntry(e.g.dashboardPort,openshellVersion,hostGpuDetected,sandboxGpuEnabled,sandboxGpuMode,sandboxGpuDevice, etc.).Suggested fix
+export type SandboxOverrides = Partial<SandboxEntry> & Record<string, unknown>; + export function writeSandboxRegistry( home: string, - sandboxNameOrOverrides: string | Partial<SandboxEntry> = "alpha", - sandboxOverridesArg: Partial<SandboxEntry> = {}, + sandboxNameOrOverrides: string | SandboxOverrides = "alpha", + sandboxOverridesArg: SandboxOverrides = {}, ): void {🤖 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/cli/helpers.ts` around lines 166 - 170, The tests pass arbitrary extra fields into writeSandboxRegistry but SandboxEntry is too narrow, causing callers to cast to unknown; widen the function's parameter types so it accurately models what is written to sandboxes.json by changing the signatures for sandboxNameOrOverrides and sandboxOverridesArg in writeSandboxRegistry to accept either the existing SandboxEntry or a more permissive type (e.g. Partial<Record<string, unknown>> or an extended SandboxEntry type) and/or update the SandboxEntry type itself to include the commonly used fields (dashboardPort, openshellVersion, hostGpuDetected, sandboxGpuEnabled, sandboxGpuMode, sandboxGpuDevice, etc.) so no unsafe casts are needed when spreading arbitrary keys into the JSON.
🤖 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/cli/helpers.ts`:
- Around line 108-120: Replace the hard-coded "node" invocation in runWithEnv()
with process.execPath so the spawned subprocess uses the exact Node binary
running the tests (change the execSync call to use process.execPath and keep the
same arg quoting/CLI variable usage); additionally, widen the SandboxEntry type
used by writeSandboxRegistry() (and any test overrides) to match the actual JSON
shape written by sandboxes.json—make the override accept Partial<Record<string,
unknown>> or extend SandboxEntry with optional fields like dashboardPort,
openshellVersion and GPU metadata so tests can pass unknown fields without
unsafe casting.
In `@test/cli/list-share-live-inference.test.ts`:
- Around line 372-423: The share-command tests are not hermetic because they
only set HOME; update the test block to create a temporary localBin, write no-op
stub executables for the external commands used by the share flow (e.g., "sshfs"
and the sandbox helper invoked by the CLI), make them executable, and then call
runWithEnv with both HOME and PATH pointing to this localBin (prepend localBin
to PATH) so the tests use the stubs; use the existing helpers in the test
(writeSandboxRegistry, runWithEnv) and the same stub pattern used in other CLI
tests to locate where to modify the tests that call runWithEnv("alpha share",
...), runWithEnv("alpha share mount", ...), and similar.
---
Nitpick comments:
In `@test/cli/helpers.ts`:
- Around line 166-170: The tests pass arbitrary extra fields into
writeSandboxRegistry but SandboxEntry is too narrow, causing callers to cast to
unknown; widen the function's parameter types so it accurately models what is
written to sandboxes.json by changing the signatures for sandboxNameOrOverrides
and sandboxOverridesArg in writeSandboxRegistry to accept either the existing
SandboxEntry or a more permissive type (e.g. Partial<Record<string, unknown>> or
an extended SandboxEntry type) and/or update the SandboxEntry type itself to
include the commonly used fields (dashboardPort, openshellVersion,
hostGpuDetected, sandboxGpuEnabled, sandboxGpuMode, sandboxGpuDevice, etc.) so
no unsafe casts are needed when spreading arbitrary keys into the JSON.
🪄 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: c44de56d-4e77-417e-a1d7-ea87ea85aab6
📒 Files selected for processing (5)
test/cli.test.tstest/cli/docker-outage.test.tstest/cli/helpers.tstest/cli/list-share-live-inference.test.tstest/install-preflight.test.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
3 participants
Summary
Start the mechanical split of the large CLI test file by moving shared CLI test setup into
test/cli/helpers.tsand extracting two already-isolated describe blocks into dedicatedtest/cli/files. This establishes the reviewable pattern for follow-up splits without changing CLI behavior.Related Issue
Refs #4892
Changes
test/cli.test.tsintotest/cli/helpers.ts.test/cli/docker-outage.test.ts.test/cli/list-share-live-inference.test.ts.HOMEsetup tomkdtempSyncso split files can run in parallel without timestamp collisions.CDISpecDirs, keeping the test isolated from host CDI state.Type of Change
Verification
Ran the full checks with
umask 022because this local shell defaults to077, which makes existing permission-mode tests create0o600fixtures before the code under test runs.npx prek run --all-filespassesnpm testpassesnpm run docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit