test: improve CLI test determinism and remove redundant test logic#1123
test: improve CLI test determinism and remove redundant test logic#1123ksapru wants to merge 5 commits intoNVIDIA:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughType annotations are added to TypeScript filesystem mocks across three test files to explicitly cast Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
nemoclaw/src/blueprint/runner.test.ts (1)
577-641:⚠️ Potential issue | 🟠 MajorPlease restore regression coverage for
apply --planrejection.
mainno longer tests the unsupported--planpath, but runtime still rejects it inactionApply. This leaves CLI parse/dispatch behavior unguarded.Proposed test addition
describe("main (CLI)", () => { @@ it("parses apply with --profile and --endpoint-url", async () => { await main(["apply", "--profile", "default", "--endpoint-url", "https://override.test/v1"]); expect(mockedValidateEndpoint).toHaveBeenCalledWith("https://override.test/v1"); expect(stdoutText()).toContain("PROGRESS:100:Apply complete"); }); + + it("rejects apply when --plan is provided (not yet implemented)", async () => { + await expect( + main(["apply", "--profile", "default", "--plan", "/tmp/plan.json"]), + ).rejects.toThrow(/--plan is not yet implemented/); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/blueprint/runner.test.ts` around lines 577 - 641, Add a test in the existing "main (CLI)" suite that exercises the unsupported apply --plan path: call main with arguments like ["apply","--plan","some-plan.json"] (after the existing beforeEach setup) and assert it rejects with an error containing "--plan" (or the exact rejection text emitted by actionApply); this restores regression coverage for the main -> actionApply dispatch path and ensures CLI parsing still rejects the --plan option at runtime.test/cli.test.js (1)
16-27:⚠️ Potential issue | 🔴 CriticalCritical bug:
spawnSyncis misconfigured — tests will fail withTypeError: r.out.includes is not a function.The current implementation has multiple issues:
spawnSyncdoes not throw exceptions — unlikeexecSync, it always returns a result object with anerrorproperty. Thetry-catchblock will never catch non-zero exits; the returnedoutis always the full result object{error, status, stdout, stderr, ...}, which has no.includes()method.Tests call
.includes()on an object — every test assertion liker.out.includes("Getting Started")will fail at runtime withTypeError: r.out.includes is not a function.Missing
shell: true— without it,spawnSynctreats the string as a literal executable name (looking for a file namednode "${CLI}" ${args}), resulting in ENOENT instead of executing the shell command.To fix, use:
Corrected implementation
function runWithEnv(args, env = {}, timeout = 10000) { - try { - const out = spawnSync(`node "${CLI}" ${args}`, { - encoding: "utf-8", - timeout, - env: { ...process.env, HOME: "/tmp/nemoclaw-cli-test-" + Date.now(), ...env }, - }); - return { code: 0, out }; - } catch (err) { - return { code: err.status, out: (err.stdout || "") + (err.stderr || "") }; - } + const result = spawnSync(`node "${CLI}" ${args}`, { + shell: true, + encoding: "utf-8", + timeout, + env: { ...process.env, HOME: "/tmp/nemoclaw-cli-test-" + Date.now(), ...env }, + }); + const out = (result.stdout || "") + (result.stderr || ""); + return { code: result.status ?? 1, out }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cli.test.js` around lines 16 - 27, The runWithEnv function misuses spawnSync: it never throws, returns a result object (so tests calling r.out.includes fail), and the command string needs shell: true; fix runWithEnv by calling spawnSync with shell: true (or pass command and args as an array), then read the returned result.stdout/stderr (convert to string) and result.status/result.error to determine exit code; return { code: <numeric status or error.status>, out: <stdout + stderr as string> } so callers can safely call r.out.includes; update references in runWithEnv to use the result object fields instead of assuming spawnSync throws.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@nemoclaw/src/blueprint/runner.test.ts`:
- Around line 577-641: Add a test in the existing "main (CLI)" suite that
exercises the unsupported apply --plan path: call main with arguments like
["apply","--plan","some-plan.json"] (after the existing beforeEach setup) and
assert it rejects with an error containing "--plan" (or the exact rejection text
emitted by actionApply); this restores regression coverage for the main ->
actionApply dispatch path and ensures CLI parsing still rejects the --plan
option at runtime.
In `@test/cli.test.js`:
- Around line 16-27: The runWithEnv function misuses spawnSync: it never throws,
returns a result object (so tests calling r.out.includes fail), and the command
string needs shell: true; fix runWithEnv by calling spawnSync with shell: true
(or pass command and args as an array), then read the returned
result.stdout/stderr (convert to string) and result.status/result.error to
determine exit code; return { code: <numeric status or error.status>, out:
<stdout + stderr as string> } so callers can safely call r.out.includes; update
references in runWithEnv to use the result object fields instead of assuming
spawnSync throws.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 10c32d50-fb8f-495a-8091-6c998082c50e
📒 Files selected for processing (5)
nemoclaw/src/blueprint/runner.test.tsnemoclaw/src/blueprint/snapshot.test.tsnemoclaw/src/blueprint/state.test.tstest/cli.test.jstest/uninstall.test.js
ksapru
force-pushed
the
fix/dead-code-cleanup-v2
branch
from
e965901 to
93ab0bb
Compare
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Thanks — the determinism goal here makes sense, but I think this needs a bit more work before merge.
Two blockers from the current diff:
-
test/cli.test.js: theexecSync->spawnSyncswap is not equivalent as written.spawnSync(node "${CLI}" ${args}, ...)will try to execute a binary with that full string as the executable name unlessshell: trueis set, so this should hitENOENT. Also,spawnSyncreturns{ status, stdout, stderr, error }and does not throw on non-zero exit, so the helper now returns{ code: 0, out: resultObj }on success instead of a string, and the existing error path no longer matchesexecSyncsemantics. I think this is the likely cause of the failingtest-unitjob. If we wantspawnSynchere, I would switch tospawnSync("node", [CLI, ...args], ...)and rebuild the helper aroundstatus/stdout/stderr/error. -
nemoclaw/src/blueprint/runner.test.ts: I don’t think the--plantest is redundant yet.runner.tson currentmainstill explicitly throws--plan is not yet implemented...inactionApply(), so removing this test drops coverage for behavior that still exists in production code.
Optional follow-up: the bash -lc -> bash -c direction in test/uninstall.test.js seems reasonable, but the file is not Prettier-clean right now, which may explain the red lint job. Also, for the HOME cases, setting HOME via env is safer than embedding HOME="..." source ... inside the command string.
Happy to re-review once those are addressed.
ksapru
force-pushed
the
fix/dead-code-cleanup-v2
branch
from
07e6b03 to
8324977
Compare
|
I’ve reverted the execSync → spawnSync change in test/cli.test.js. The previous swap wasn’t equivalent (as you pointed out: ENOENT risk + different return/error semantics), and keeping execSync preserves the current behavior and test expectations. Also re-added the --plan test to retain coverage for the existing behavior in runner.ts. For test/uninstall.test.js, I switched to bash -c and moved HOME into env for more deterministic behavior. I’ll make sure the file is Prettier-clean as well. Happy to revisit a proper spawnSync refactor separately if that’s something we want to pursue. |
2 participants
Summary
Improves test determinism, consistency, and reliability across CLI, uninstall, and blueprint test suites by standardizing shell invocation, tightening execution patterns, and removing redundant or outdated test code.
Related Issue
Fixes #977 (part 1)
Changes
bash -lcwithbash -cin uninstall tests to avoid shell initialization side effectsrunWithEnv(..., timeout)runner.test.tsVerification
npm testpasses locallynpx prek run --all-filespasses in CIRationale
Some tests relied on shell initialization behavior (
bash -lc) and inconsistent execution patterns, leading to flakiness and non-deterministic outcomes.These updates:
Additionally, minor cleanup removes redundant or outdated test code to improve maintainability.
Risk Assessment
Low risk
Rollback
Type of Change
Testing
npm testpassesnpx prek run --all-filespasses (CI)Checklist
General
Code Changes
Summary by CodeRabbit (updated)
Summary by CodeRabbit