test: add unit tests for onboard, logs, and connect modules by kagura-agent · Pull Request #279 · NVIDIA/NemoClaw

kagura-agent · 2026-03-18T03:09:20Z

Summary

Adds 35 new unit tests across 4 test files, covering critical modules that previously had zero test coverage. Complements the existing status.test.ts (22 tests) to bring the total to 57 tests.

No source files were modified — this PR only adds test files.

New test files

File	Tests	What it covers
`src/onboard/validate.test.ts`	14	`validateApiKey`: successful validation, HTTP 401/403/500, timeout (AbortError), network failure, non-Error rejection, trailing-slash stripping, body truncation. `maskApiKey`: short keys, `nvapi-` prefix, regular keys, boundary lengths.
`src/onboard/config.test.ts`	5	`saveOnboardConfig`/`loadOnboardConfig` round-trip, overwrite behavior, missing file returns `null`, `clearOnboardConfig` removes file, no-throw on missing file.
`src/commands/logs.test.ts`	9	Sandbox not running → message, sandbox running → correct `openshell` spawn args, `--follow` adds `-f`, `--lines` sets `-n`, `--run-id` shows run info, state fallback, sandbox name from state, spawn error handling.
`src/commands/connect.test.ts`	7	Correct spawn args, `ENOENT` → "openshell not found" error, generic spawn error, non-zero exit code logging, success/null exit code handling.

Why these modules

The onboard flow is where the majority of user-facing bugs originate (#22, #93, #152, #197, #201, #208, #210, #245). With CI now running on PRs (#261), these tests will catch regressions in the most sensitive code paths.

Test conventions

All tests follow the patterns established in status.test.ts:

vitest with vi.mock for node:fs and node:child_process
captureLogger() pattern for output assertions
resetAllMocks() in beforeEach
SPDX Apache-2.0 license headers

Verification

$ cd nemoclaw && npm test

 ✓ src/onboard/validate.test.ts (14 tests) 10ms
 ✓ src/onboard/config.test.ts (5 tests) 12ms
 ✓ src/commands/logs.test.ts (9 tests) 17ms
 ✓ src/commands/connect.test.ts (7 tests) 23ms
 ✓ src/commands/status.test.ts (22 tests) 23ms

 Test Files  5 passed (5)
       Tests  57 passed (57)
    Duration  298ms

Summary by CodeRabbit

Tests
- Added comprehensive tests for the connect command covering sandbox handling, spawn errors, and exit-code logging.
- Added extensive logs command tests for streaming, follow/lines flags, run info display, sandbox resolution, and spawn error handling.
- Added onboard config tests for load/save/clear round-trips and edge cases.
- Added API key validation and masking tests (success, HTTP errors, timeouts, masking).
- Added shared test helpers to capture logs and mock subprocess behavior.

coderabbitai · 2026-03-18T03:09:35Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d9d98f2b-ae66-405e-b470-e66e18dcc575

📥 Commits

Reviewing files that changed from the base of the PR and between 9039579 and 6e64ccc.

📒 Files selected for processing (5)

nemoclaw/src/commands/connect.test.ts
nemoclaw/src/commands/logs.test.ts
nemoclaw/src/onboard/config.test.ts
nemoclaw/src/onboard/validate.test.ts
nemoclaw/src/test-helpers.ts

✅ Files skipped from review due to trivial changes (2)

nemoclaw/src/test-helpers.ts
nemoclaw/src/onboard/config.test.ts

🚧 Files skipped from review as they are similar to previous changes (3)

nemoclaw/src/onboard/validate.test.ts
nemoclaw/src/commands/logs.test.ts
nemoclaw/src/commands/connect.test.ts

📝 Walkthrough

Walkthrough

Adds multiple Vitest unit-test suites and a test-helpers module covering CLI connect, logs, onboard config persistence, and API key validation/masking. All changes are test-only; no runtime code or public exported declarations were modified.

Changes

Cohort / File(s)	Summary
Commands Module Tests `nemoclaw/src/commands/connect.test.ts`, `nemoclaw/src/commands/logs.test.ts`	Adds tests for `cliConnect` and `cliLogs`. Mocks `node:child_process` (`spawn`/`exec`), validates spawn/exec arguments and behavior (sandbox selection, `-f`, `-n`), simulates spawn `error`/`close` events, and asserts logged outputs for success/failure and run/state resolution.
Onboard Module Tests `nemoclaw/src/onboard/config.test.ts`, `nemoclaw/src/onboard/validate.test.ts`	Adds tests for onboard config persistence with a mocked `node:fs` (load/save/clear round-trips and overwrite) and for API key validation/masking with mocked `fetch` (2xx, 401/403/500, truncation, timeout/AbortError, network/non-Error rejections, masking edge cases).
Test Helpers `nemoclaw/src/test-helpers.ts`	Introduces `captureLogger()` to capture `info`/`warn`/`error` log lines and `mockSpawnProc()` to simulate async child-process `on`/`emit` behavior for spawn mocks used across tests.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐇 I hopped through tests with nimble feet,
Mocked spawns and fetches—no defeat,
Saved configs, traced logs in streams,
Masked keys and tamed timeout dreams,
A cheerful rabbit celebrates the suite.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'test: add unit tests for onboard, logs, and connect modules' directly summarizes the main change: adding comprehensive unit tests across three modules. It is specific, concise, and clearly conveys the primary purpose of the changeset.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

📝 Coding Plan

Generate coding plan for human review comments

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

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

nemoclaw/src/commands/connect.test.ts (1)
23-62: Consider extracting shared logger/spawn test helpers.

captureLogger and mockSpawnProc are effectively duplicated in nemoclaw/src/commands/logs.test.ts. Moving them into a small test utility module will reduce drift and simplify future updates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@nemoclaw/src/commands/connect.test.ts` around lines 23 - 62, Extract the
duplicated test helpers captureLogger and mockSpawnProc into a shared test
utility module (e.g., export captureLogger and mockSpawnProc from a new
tests/utils or similar module), update both tests that currently define them
(references: captureLogger, mockSpawnProc) to import them from that module, and
remove the local definitions; ensure the mockSpawnProc implementation and its
use of vi.mocked(spawn).mockReturnValue and the returned proc shape are
preserved so existing tests (like in connect.test.ts and logs.test.ts) continue
to work without other changes.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@nemoclaw/src/onboard/validate.test.ts`:
- Around line 39-41: The apiKey fixture in validate.test.ts (the constant apiKey
next to endpoint) is a realistic key-shaped string triggering secret scanners;
replace it with a clearly non-secret placeholder (e.g., "test-api-key" or
"API_KEY_PLACEHOLDER") so it no longer matches key patterns, keep endpoint
untouched, and run the test suite to confirm validate.test.ts still passes
(search for the symbols endpoint and apiKey to update all references).

---

Nitpick comments:
In `@nemoclaw/src/commands/connect.test.ts`:
- Around line 23-62: Extract the duplicated test helpers captureLogger and
mockSpawnProc into a shared test utility module (e.g., export captureLogger and
mockSpawnProc from a new tests/utils or similar module), update both tests that
currently define them (references: captureLogger, mockSpawnProc) to import them
from that module, and remove the local definitions; ensure the mockSpawnProc
implementation and its use of vi.mocked(spawn).mockReturnValue and the returned
proc shape are preserved so existing tests (like in connect.test.ts and
logs.test.ts) continue to work without other changes.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 2abd63f0-8dea-41d5-ad3e-5f27c8c11e0a

📥 Commits

Reviewing files that changed from the base of the PR and between 1e23347 and 3d51e0b.

📒 Files selected for processing (4)

nemoclaw/src/commands/connect.test.ts
nemoclaw/src/commands/logs.test.ts
nemoclaw/src/onboard/config.test.ts
nemoclaw/src/onboard/validate.test.ts

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

nemoclaw/src/commands/connect.test.ts (1)
57-65: Make the spawn-error expectations explicit.

These cases only assert the specific error text. nemoclaw/src/commands/connect.ts also resolves that branch as exit code 1, so this path may emit the generic exited with code ... guidance afterward. Please assert either presence or absence of that follow-up message so the error path is actually pinned down.

Also applies to: 67-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@nemoclaw/src/commands/connect.test.ts` around lines 57 - 65, The tests in
connect.test.ts that mock spawn errors (using mockSpawnProc and
vi.mocked(spawn)) currently only assert the specific ENOENT error text; update
each test (the one at 57-65 and the similar one at 67-75) to also assert the
expected behavior regarding the follow-up "exited with code" guidance emitted by
cliConnect: explicitly check that the logger output either contains the
exit-code guidance when the branch in connect.ts resolves with exit code 1, or
assert that it does not appear if the ENOENT branch should suppress it—i.e.,
after calling cliConnect({ sandbox: "openclaw", logger }) capture
lines.join("\n") and add an expectation for presence or absence of the "exited
with code" message so the error path is fully pinned down.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@nemoclaw/src/commands/logs.test.ts`:
- Around line 55-72: The mockExec helper currently invokes the exec callback
with a single result object but real Node exec callbacks used by promisify have
signature (err, stdout, stderr); update mockExec to call callback(err, stdout,
stderr) so tests mirror production: in mockExec, when response is an Error call
callback(response, "", response.message) and when response is a string call
callback(null, response, ""), and keep the
vi.mocked(exec).mockImplementation((...) => { ... }) cast; alternatively, mock
the small async wrapper that your code actually imports instead of exec but
ensure the exported function matches the (err, stdout, stderr) contract used by
promisify.

---

Nitpick comments:
In `@nemoclaw/src/commands/connect.test.ts`:
- Around line 57-65: The tests in connect.test.ts that mock spawn errors (using
mockSpawnProc and vi.mocked(spawn)) currently only assert the specific ENOENT
error text; update each test (the one at 57-65 and the similar one at 67-75) to
also assert the expected behavior regarding the follow-up "exited with code"
guidance emitted by cliConnect: explicitly check that the logger output either
contains the exit-code guidance when the branch in connect.ts resolves with exit
code 1, or assert that it does not appear if the ENOENT branch should suppress
it—i.e., after calling cliConnect({ sandbox: "openclaw", logger }) capture
lines.join("\n") and add an expectation for presence or absence of the "exited
with code" message so the error path is fully pinned down.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: bc340548-a9bc-4440-9e5e-361db8f2940a

📥 Commits

Reviewing files that changed from the base of the PR and between 3d51e0b and 7051593.

📒 Files selected for processing (4)

nemoclaw/src/commands/connect.test.ts
nemoclaw/src/commands/logs.test.ts
nemoclaw/src/onboard/validate.test.ts
nemoclaw/src/test-helpers.ts

🚧 Files skipped from review as they are similar to previous changes (1)

nemoclaw/src/onboard/validate.test.ts

kagura-agent · 2026-03-18T07:05:26Z

Re: CodeRabbit review feedback on validate.test.ts:

API key fixture string: Already fixed — uses "test-api-key-placeholder" instead of a realistic nvapi- shaped literal. Won't trigger secret scanners.
logs.test.ts mock shape: Already confirmed correct — util.promisify(exec) returns { stdout, stderr } object per Node.js docs.

All review items addressed. ✅

Add 35 new tests across 4 test files covering critical paths that previously had zero test coverage: - onboard/validate: API key validation (success, HTTP errors, timeouts, network failures) and key masking - onboard/config: save/load round-trip, missing file handling, config clearing - commands/logs: sandbox state detection, spawn argument construction, --follow/--lines/--run-id flags, error handling - commands/connect: spawn arguments, openshell errors, exit codes All tests follow the existing status.test.ts conventions (vitest, vi.mock, captureLogger). Total suite: 57 tests (22 existing + 35 new). No source files modified.

Address code review feedback: - Extract captureLogger and mockSpawnProc into src/test-helpers.ts to eliminate duplication between connect.test.ts and logs.test.ts - Replace realistic API key fixture ('nvapi-...') in validate.test.ts with 'test-api-key-placeholder' to avoid secret scanner triggers

Address second CodeRabbit review: spawn errors resolve with exit code 1, which triggers the follow-up 'exited with code' guidance. Tests now assert both the error message and the exit guidance. Also explain why mockExec's callback shape is correct — Node's promisify(exec) resolves with { stdout, stderr }, matching the existing status.test.ts pattern.

- Fix mockExec callback signature in logs.test.ts to use the real Node exec signature (err, stdout, stderr) instead of (err, {stdout, stderr}) - Attach util.promisify.custom to the mocked exec so promisify(exec) correctly folds the three-arg callback into {stdout, stderr}, matching the real Node implementation - connect.test.ts already asserts 'exited with code' follow-up messages (addressed in prior commit)

kagura-agent · 2026-03-23T23:36:00Z

Closing to consolidate — these unit tests were submitted during my early learning phase and may not match current project conventions. Happy to resubmit more focused test coverage if maintainers are interested.

coderabbitai bot reviewed Mar 18, 2026

View reviewed changes

Comment thread nemoclaw/src/onboard/validate.test.ts

areporeporepo mentioned this pull request Mar 18, 2026

test: add unit tests for eject and slash command handlers #282

Closed

coderabbitai bot reviewed Mar 18, 2026

View reviewed changes

Comment thread nemoclaw/src/commands/logs.test.ts

This was referenced Mar 18, 2026

fix: [Bug] Ollama reasoning models return empty content — agent responses are blank #277

Closed

fix: # fix(gateway): replace dead nvidia-device-plugin Helm repo URL #278

Closed

wscurran added the enhancement: testing Use this label to identify requests to improve NemoClaw test coverage. label Mar 18, 2026

Kagura Chen added 4 commits March 19, 2026 13:04

kagura-agent force-pushed the test/unit-tests-onboard-logs-connect branch from 9039579 to 6e64ccc Compare March 19, 2026 05:10

kagura-agent closed this Mar 23, 2026

mafueee pushed a commit to mafueee/NemoClaw that referenced this pull request Mar 28, 2026

chore: remove remaining navigator and nemoclaw references (NVIDIA#279)

f6ae1da

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add unit tests for onboard, logs, and connect modules#279

test: add unit tests for onboard, logs, and connect modules#279
kagura-agent wants to merge 4 commits intoNVIDIA:mainfrom
kagura-agent:test/unit-tests-onboard-logs-connect

kagura-agent commented Mar 18, 2026 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Mar 18, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Poem

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

kagura-agent commented Mar 18, 2026

Uh oh!

kagura-agent commented Mar 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

kagura-agent commented Mar 18, 2026 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

New test files

Why these modules

Test conventions

Verification

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kagura-agent commented Mar 18, 2026

Uh oh!

kagura-agent commented Mar 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kagura-agent commented Mar 18, 2026 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Mar 18, 2026 •

edited

Loading