test(cli): split oversized CLI suites - #4898
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughSplits monolithic CLI tests into focused Vitest suites and adds end-to-end tests for host-aliases, sandbox mutations, snapshot/shields, onboard/debug, logs streaming and lifecycle, status/health, connect recovery, list/inference, destroy/gateway cleanup, and maintenance; also extends logs test helper and reorders a few imports. ChangesCLI Test Suite Reorganization and Expansion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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: None Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
|
Contributor
E2E Scenario Advisor RecommendationRequired scenario E2E: None Full scenario advisor summaryE2E Scenario AdvisorBase: Required scenario E2E
Optional scenario E2E
Relevant changed files
|
Contributor
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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/cli/sandbox-mutations.test.ts (1)
173-181: ⚡ Quick winUse consistent exit code assertions for parser validation failures.
Lines 176 and 181 both test parser-owned validation failures, but use different assertions:
- Line 176:
expect(missingPolicyFile.code).not.toBe(0)- Line 181:
expect(missingChannel.code).toBe(PARSER_EXIT_CODE)For consistency and precision, consider using
toBe(PARSER_EXIT_CODE)for both, sincePARSER_EXIT_CODEis specifically defined for parser validation failures.♻️ Suggested change for consistency
const missingPolicyFile = runWithEnv("alpha policy-add --from-file 2>&1", { HOME: home, }); - expect(missingPolicyFile.code).not.toBe(0); + expect(missingPolicyFile.code).toBe(PARSER_EXIT_CODE); expect(missingPolicyFile.out).toContain("--from-file");🤖 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/sandbox-mutations.test.ts` around lines 173 - 181, Change the loose assertion on the parser validation for the policy file to use the explicit PARSER_EXIT_CODE constant: replace the `expect(missingPolicyFile.code).not.toBe(0)` check with an equality check against PARSER_EXIT_CODE so it matches the subsequent loop assertions; locate the test that calls runWithEnv (variable names missingPolicyFile and missingChannel) in sandbox-mutations.test.ts and update the assertion to `expect(missingPolicyFile.code).toBe(PARSER_EXIT_CODE)`.
🤖 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.
Nitpick comments:
In `@test/cli/sandbox-mutations.test.ts`:
- Around line 173-181: Change the loose assertion on the parser validation for
the policy file to use the explicit PARSER_EXIT_CODE constant: replace the
`expect(missingPolicyFile.code).not.toBe(0)` check with an equality check
against PARSER_EXIT_CODE so it matches the subsequent loop assertions; locate
the test that calls runWithEnv (variable names missingPolicyFile and
missingChannel) in sandbox-mutations.test.ts and update the assertion to
`expect(missingPolicyFile.code).toBe(PARSER_EXIT_CODE)`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b2e8ace7-a66b-4e09-ba21-b242b3a18f35
📒 Files selected for processing (4)
test/cli.test.tstest/cli/sandbox-host-aliases.test.tstest/cli/sandbox-mutations.test.tstest/cli/snapshot-shields.test.ts
💤 Files with no reviewable changes (1)
- test/cli.test.ts
cv
changed the title
2 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/logs.test.ts`:
- Around line 216-239: The test "starts OpenClaw logs before enabling audit logs
for logs --follow" currently doesn't assert that the OpenClaw (sandbox exec -n
alpha -- tail -n 200 -f /tmp/gateway.log) start occurs before the audit enable
(settings set alpha --key ocsf_json_enabled --value true); update the test (and
the createLogsTestSetup stub) so the OpenClaw stub emits a deterministic
"gateway-started" marker (e.g., write a known marker string to the simulated
output or a marker file when the sandbox exec stub runs) and then assert that
this marker appears in the recorded calls or outputs before the audit enable
call completes by checking that the index/position of the OpenClaw
"gateway-started" marker is less than the index of the settings set alpha --key
ocsf_json_enabled --value true call, while keeping the existing assertions for
openshell logs ordering.
🪄 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: b2817218-80c4-4ea9-9c0a-57b3f7bb0930
📒 Files selected for processing (2)
test/cli.test.tstest/cli/logs.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/cli.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (5)
test/cli/list-inference.test.ts (1)
179-253: 💤 Low valueConsider adding try/finally cleanup for consistency.
This test creates a temporary directory but doesn't clean it up. Adding a try/finally block (similar to the pattern in test/cli/dispatch-basics.test.ts lines 215-273) would prevent test artifact accumulation.
♻️ Suggested cleanup pattern
Wrap the test body starting after line 180 in a try block and add:
} finally { fs.rmSync(home, { recursive: true, force: true }); }🤖 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/list-inference.test.ts` around lines 179 - 253, The test "list --json emits structured sandbox details" creates a temporary home dir (variable home) but doesn't remove it; wrap the test body after creating home in a try { ... } finally { fs.rmSync(home, { recursive: true, force: true }); } block so the temp directory is always cleaned up (place the existing assertions and runWithEnv(...) call inside the try); reference the test function name and the home variable to locate where to add the try/finally.test/cli/dispatch-basics.test.ts (3)
107-125: 💤 Low valueConsider adding try/finally cleanup for consistency.
This test creates a temporary directory but doesn't clean it up. While the OS typically purges tmp directories periodically, other tests in this file (e.g., lines 166-177, 215-273) use try/finally blocks for immediate cleanup. Adding cleanup here would improve consistency and prevent test artifact accumulation during local development.
♻️ Suggested cleanup pattern
it("bare unknown name surfaces sandbox-not-found (`#2164`)", testTimeoutOptions(35_000), () => { const home = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-cli-unknown-sandbox-")); - const localBin = path.join(home, "bin"); - fs.mkdirSync(localBin, { recursive: true }); - fs.writeFileSync(path.join(localBin, "openshell"), "#!/usr/bin/env bash\nexit 1\n", { - mode: 0o755, - }); + try { + const localBin = path.join(home, "bin"); + fs.mkdirSync(localBin, { recursive: true }); + fs.writeFileSync(path.join(localBin, "openshell"), "#!/usr/bin/env bash\nexit 1\n", { + mode: 0o755, + }); - const r = runWithEnv( - "boguscmd", - { - HOME: home, - PATH: `${localBin}:${process.env.PATH || ""}`, - }, - execTimeout(30_000), - ); - expect(r.code).toBe(1); - expect(r.out.includes("Sandbox 'boguscmd' does not exist")).toBeTruthy(); + const r = runWithEnv( + "boguscmd", + { + HOME: home, + PATH: `${localBin}:${process.env.PATH || ""}`, + }, + execTimeout(30_000), + ); + expect(r.code).toBe(1); + expect(r.out.includes("Sandbox 'boguscmd' does not exist")).toBeTruthy(); + } finally { + fs.rmSync(home, { recursive: true, force: true }); + } });🤖 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/dispatch-basics.test.ts` around lines 107 - 125, The test "bare unknown name surfaces sandbox-not-found" creates a temp dir with fs.mkdtempSync assigned to home and writes localBin/openshell but doesn't remove it; wrap the test body that uses home/localBin and runWithEnv in a try/finally and in the finally call fs.rmSync(home, { recursive: true, force: true }) (or the project's existing cleanup helper) so the temporary directory and created files are removed after the assertion; update the test function surrounding the uses of home, localBin, and runWithEnv to ensure cleanup always runs.
276-296: 💤 Low valueConsider adding try/finally cleanup for consistency.
Same cleanup suggestion - adding a try/finally block would prevent test artifact accumulation and match the pattern used in other tests in this file.
🤖 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/dispatch-basics.test.ts` around lines 276 - 296, Wrap the test body in a try/finally: keep the existing setup (creating home, localBin, writeSandboxRegistry, writing openshell and calling runWithEnv with "hermes connect alpha") inside the try block, and in the finally block remove the temporary directory created (home) to clean up artifacts (e.g., call fs.rmSync(home, { recursive: true, force: true }) or equivalent). This ensures the test "explains sandbox connect command order when the sandbox name is last" cleans up created files after runWithEnv and matches the pattern used by other tests.
180-212: 💤 Low valueConsider adding try/finally cleanup for consistency.
Same cleanup suggestion as the previous test - adding a try/finally block would match the cleanup pattern used elsewhere in this file.
🤖 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/dispatch-basics.test.ts` around lines 180 - 212, Wrap the test body that creates the temporary HOME and localBin in a try/finally so the temp directory is always removed; specifically, after creating home and localBin and writing the openshell stub, run the existing assertions inside a try block and in finally call fs.rmSync(home, { recursive: true, force: true }) (or the same cleanup used elsewhere) to mirror the pattern used in other tests and ensure cleanup even on assertion failures; keep runWithEnv("liost", {...}), the openshell file creation, and the expect(...) assertions inside the try so they execute before cleanup.test/cli/destroy-gateway-cleanup.test.ts (1)
12-547: ⚡ Quick winConsider extracting common test setup to reduce duplication.
All 8 tests repeat nearly identical setup patterns (~400+ lines total):
- Creating temp directories and paths
- Writing sandboxes.json registry
- Creating fake
openshellanddockerexecutables- Running destroy command and reading logs
The variation is limited to:
- Registry contents (number/names of sandboxes)
- Stub command responses (list output, delete behavior)
- Optional stubs (pgrep/lsof in tests 2-3)
Extracting a parameterized helper would improve maintainability and reduce the test file size significantly.
♻️ Example refactor pattern
function setupDestroyTest(opts: { tmpPrefix: string; sandboxes: Record<string, any>; openshellBehavior?: { listResponse?: string; deleteError?: string; }; includePgrepLsof?: boolean; }) { const home = fs.mkdtempSync(path.join(os.tmpdir(), opts.tmpPrefix)); const localBin = path.join(home, "bin"); const registryDir = path.join(home, ".nemoclaw"); const openshellLog = path.join(home, "openshell.log"); const bashLog = path.join(home, "docker.log"); fs.mkdirSync(localBin, { recursive: true }); fs.mkdirSync(registryDir, { recursive: true }); fs.writeFileSync( path.join(registryDir, "sandboxes.json"), JSON.stringify({ sandboxes: opts.sandboxes, defaultSandbox: Object.keys(opts.sandboxes)[0] }), { mode: 0o600 } ); // ... create stubs based on opts.openshellBehavior, opts.includePgrepLsof return { home, localBin, openshellLog, bashLog }; }Then each test becomes:
it("preserves the gateway runtime by default...", () => { const { home, localBin, openshellLog, bashLog } = setupDestroyTest({ tmpPrefix: "nemoclaw-cli-destroy-last-", sandboxes: { alpha: { name: "alpha", model: "test-model", ... } } }); const r = runWithEnv("alpha destroy -y", { HOME: home, PATH: `${localBin}:${process.env.PATH || ""}`, }); expect(r.code).toBe(0); // ... assertions });🤖 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/destroy-gateway-cleanup.test.ts` around lines 12 - 547, Extract the repeated test setup into a single helper (e.g. setupDestroyTest) and use it from each it(...) block: factor out creation of temp dirs, localBin, registryDir, openshellLog, bashLog, writing sandboxes.json, and generating stub executables (openshell, docker, optional pgrep/lsof), returning {home, localBin, openshellLog, bashLog}; update tests to call runWithEnv with the returned home and PATH and only supply per-test variations (sandboxes, openshell behavior, includePgrepLsof) so tests use runWithEnv, expects, and file reads unchanged while removing the duplicated setup code.
🤖 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.
Nitpick comments:
In `@test/cli/destroy-gateway-cleanup.test.ts`:
- Around line 12-547: Extract the repeated test setup into a single helper (e.g.
setupDestroyTest) and use it from each it(...) block: factor out creation of
temp dirs, localBin, registryDir, openshellLog, bashLog, writing sandboxes.json,
and generating stub executables (openshell, docker, optional pgrep/lsof),
returning {home, localBin, openshellLog, bashLog}; update tests to call
runWithEnv with the returned home and PATH and only supply per-test variations
(sandboxes, openshell behavior, includePgrepLsof) so tests use runWithEnv,
expects, and file reads unchanged while removing the duplicated setup code.
In `@test/cli/dispatch-basics.test.ts`:
- Around line 107-125: The test "bare unknown name surfaces sandbox-not-found"
creates a temp dir with fs.mkdtempSync assigned to home and writes
localBin/openshell but doesn't remove it; wrap the test body that uses
home/localBin and runWithEnv in a try/finally and in the finally call
fs.rmSync(home, { recursive: true, force: true }) (or the project's existing
cleanup helper) so the temporary directory and created files are removed after
the assertion; update the test function surrounding the uses of home, localBin,
and runWithEnv to ensure cleanup always runs.
- Around line 276-296: Wrap the test body in a try/finally: keep the existing
setup (creating home, localBin, writeSandboxRegistry, writing openshell and
calling runWithEnv with "hermes connect alpha") inside the try block, and in the
finally block remove the temporary directory created (home) to clean up
artifacts (e.g., call fs.rmSync(home, { recursive: true, force: true }) or
equivalent). This ensures the test "explains sandbox connect command order when
the sandbox name is last" cleans up created files after runWithEnv and matches
the pattern used by other tests.
- Around line 180-212: Wrap the test body that creates the temporary HOME and
localBin in a try/finally so the temp directory is always removed; specifically,
after creating home and localBin and writing the openshell stub, run the
existing assertions inside a try block and in finally call fs.rmSync(home, {
recursive: true, force: true }) (or the same cleanup used elsewhere) to mirror
the pattern used in other tests and ensure cleanup even on assertion failures;
keep runWithEnv("liost", {...}), the openshell file creation, and the
expect(...) assertions inside the try so they execute before cleanup.
In `@test/cli/list-inference.test.ts`:
- Around line 179-253: The test "list --json emits structured sandbox details"
creates a temporary home dir (variable home) but doesn't remove it; wrap the
test body after creating home in a try { ... } finally { fs.rmSync(home, {
recursive: true, force: true }); } block so the temp directory is always cleaned
up (place the existing assertions and runWithEnv(...) call inside the try);
reference the test function name and the home variable to locate where to add
the try/finally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2584f777-9818-4330-ad77-0c431c03d915
📒 Files selected for processing (10)
test/cli.test.tstest/cli/connect-recovery.test.tstest/cli/destroy-gateway-cleanup.test.tstest/cli/dispatch-basics.test.tstest/cli/doctor-gateway-token.test.tstest/cli/helpers.tstest/cli/list-inference.test.tstest/cli/logs.test.tstest/cli/status-gateway-lifecycle.test.tstest/cli/status-health.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- test/cli/logs.test.ts
- test/cli/status-health.test.ts
12 tasks
This was referenced Jun 7, 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
Splits the oversized
test/cli.test.tscatch-all into focusedtest/cli/suites following the #4892 parallelization plan. The migration keeps the existing assertions mechanical and verifiable, then removestest/cli.test.tsso future CLI tests have to land in a specific destination.Related Issue
Refs #4892
Changes
test/cli/sandbox-mutations.test.ts.test/cli/sandbox-host-aliases.test.ts.test/cli/snapshot-shields.test.ts.test/cli/onboard-debug.test.ts.test/cli/logs.test.ts.test/cli/tunnel-credentials-maintenance-skills.test.ts.test/cli/status-health.test.ts.test/cli/connect-recovery.test.ts.test/cli/status-gateway-lifecycle.test.ts.test/cli/list-inference.test.ts.test/cli/dispatch-basics.test.ts.test/cli/doctor-gateway-token.test.ts.test/cli/destroy-gateway-cleanup.test.ts.test/cli.test.tsafter the final extraction.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesnpx vitest run --project cli test/cli.test.ts test/cli/tunnel-credentials-maintenance-skills.test.tspassesnpx vitest run --project cli test/cli.test.ts test/cli/status-health.test.tspassesnpx vitest run --project cli test/cli.test.ts test/cli/connect-recovery.test.tspassesnpx vitest run --project cli test/cli.test.ts test/cli/status-gateway-lifecycle.test.tspassesnpx vitest run --project cli test/cli.test.ts test/cli/list-inference.test.tspassesnpx vitest run --project cli test/cli.test.ts test/cli/dispatch-basics.test.tspassesnpx vitest run --project cli test/cli.test.ts test/cli/doctor-gateway-token.test.tspassesnpx vitest run --project cli test/cli/*.test.tspassesnpm run typecheck:clipassesnpm run docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit