fix(test): bump cli.test.js runWithEnv default timeout from 10s to 30s by TSavo · Pull Request #1627 · NVIDIA/NemoClaw

TSavo · 2026-04-08T20:51:30Z

Summary

One-line change: bump runWithEnv's default timeout in test/cli.test.js from 10s to 30s. The CLI cold-starts a Node child for each test case, and 10s isn't enough on a self-hosted runner under modest CPU contention — several tests would intermittently abort with ETIMEDOUT from execSync itself rather than from a real test failure.

Related Issue

Closes #1621 (sub-item 1 of three: runWithEnv timeout too low for slow CI hosts).

Changes

test/cli.test.js: function runWithEnv(args, env = {}, timeout = 10000) → ... timeout = 30000. Per-call sites that explicitly pass a third argument continue to override the default unchanged.

Diff: +1 / −1 in 1 file.

Type of Change

Code change for a new feature, bug fix, or refactor.
Code change with doc updates.
Doc only. Prose changes without code sample modifications.
Doc only. Includes code sample changes.

Testing

npx vitest run --project cli test/cli.test.js — entire suite passes locally
npx prettier --check test/cli.test.js clean
npx prek run --all-files / npm test — not run on the local machine because the full CLI test suite hits two separate baseline failures on a WSL2 host: the shouldPatchCoredns issue covered by fix(platform): allow shouldPatchCoredns isWsl override for deterministic WSL2 tests #1626 and the install-preflight sysbin leakage covered by fix(test): isolate sysbin in install-preflight tests to prevent host PATH leakage #1628. Upstream CI runs on Linux runners so it'll exercise the full suite normally.
make docs builds without warnings. (for doc-only changes — N/A)

Checklist

General

I have read and followed the contributing guide.
I have read and followed the style guide. (for doc-only changes — N/A)

Code Changes

Formatters applied — npx prettier --check clean.
Tests added or updated for new or changed behavior — N/A, this is a test infrastructure tweak; the test bodies are unchanged. The fast path is unchanged: tests still finish in milliseconds when nothing's wrong, and 30s is well under vitest's default hook timeout so a genuinely-stuck test still surfaces as a timeout.
No secrets, API keys, or credentials committed.
Doc pages updated for any user-facing behavior changes — N/A, test-only change.

Doc Changes

N/A (no doc changes)

Signed-off-by: T Savo evilgenius@nefariousplan.com

Summary by CodeRabbit

Tests
- Increased default test execution and hook timeouts to 30 seconds for the CLI test suite.
- Raised the default timeout used by the test helper so tests that don’t specify a timeout run longer by default, reducing flaky failures in slower or resource-constrained environments.

coderabbitai · 2026-04-08T20:51:44Z

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: 16aa631f-200d-435e-9976-c87ede001da7

📥 Commits

Reviewing files that changed from the base of the PR and between ff14ce3 and 2634408.

📒 Files selected for processing (2)

test/cli.test.js
vitest.config.ts

📝 Walkthrough

Walkthrough

The CLI test timeout was increased: runWithEnv in test/cli.test.js now defaults to 30000 ms (was 10000 ms). The cli Vitest project config (vitest.config.ts) now sets testTimeout and hookTimeout to 30_000 ms.

Changes

Cohort / File(s)	Summary
CLI Test Helper `test/cli.test.js`	Default `timeout` parameter of `runWithEnv` changed from `10000` to `30000`. Tests invoking this helper without an explicit timeout will use the longer execSync timeout.
Vitest CLI Project Config `vitest.config.ts`	Added `testTimeout: 30_000` and `hookTimeout: 30_000` to the `cli` project config, enforcing a 30s timeout for tests and hooks in that project.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 I stretched the seconds, threefold and neat,
So slow hosts and tests may calmly meet.
No frantic timeouts, just steady hops,
I nibble logs and dodge the flops. 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately reflects the main change: increasing the default timeout in runWithEnv from 10s to 30s in test/cli.test.js, which is the primary objective.
Linked Issues check	✅ Passed	The pull request addresses item 1 (runWithEnv timeout) from issue `#1621` by raising the default timeout from 10000ms to 30000ms and aligning Vitest timeouts. Items 2 and 3 remain unaddressed but are noted as independent fixes.
Out of Scope Changes check	✅ Passed	All changes are in-scope: runWithEnv timeout bump and vitest.config.ts timeout additions directly address the ETIMEDOUT flake issue from `#1621` item 1 without introducing unrelated modifications.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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

Copilot

Pull request overview

This PR reduces CLI test flakiness on slower/contended CI hosts by increasing the default execSync timeout used by the runWithEnv helper in test/cli.test.js.

Changes:

Increase runWithEnv default timeout from 10s to 30s for CLI invocations in the test suite.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@test/cli.test.js`:
- Line 16: The tests set execSync's timeout to 30000ms inside runWithEnv but
Vitest's own default timeout (10s) will still abort tests; update the test suite
to increase Vitest's timeout to at least match execSync by calling
vi.setTimeout(30000) (or a larger value) near the top of the test file before
tests run, or alternatively bump the cli project's timeout in Vitest config;
ensure you reference runWithEnv and its use of execSync so the framework-level
timeout aligns with the execSync timeout to prevent ETIMEDOUT flakes.

🪄 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: Pro

Run ID: 2c559d89-e739-4a82-a50f-943d06b8a964

📥 Commits

Reviewing files that changed from the base of the PR and between b8a5245 and a4c09c6.

📒 Files selected for processing (1)

test/cli.test.js

TSavo · 2026-04-08T21:07:31Z

Force-pushed to add a Signed-off-by trailer (DCO compliance) — the diff itself is unchanged. CodeRabbit's prior review still applies to the same content.

Ready for a maintainer to approve workflow runs whenever you have a moment — first-time-contributor gate.

… timeout to 30s Two coordinated changes so the timeout bump actually takes effect: 1. `test/cli.test.js`: `runWithEnv` default `timeout` 10000 → 30000. The CLI cold-starts a Node child for each test case via `execSync`. Under modest CPU contention from a sibling job on a self-hosted runner, 10s isn't enough — several tests would intermittently abort with ETIMEDOUT from `execSync` itself rather than from a real test failure, masking actual output and making the suite flaky on slower hardware. 2. `vitest.config.ts`: add `testTimeout: 30_000` and `hookTimeout: 30_000` to the `cli` project. Why both: Vitest's default `testTimeout` is 5000ms. Bumping `execSync`'s timeout to 30s WITHOUT also bumping vitest's per-test ceiling is a no-op — vitest aborts the whole test at 5s long before execSync's 30s timer can fire. CodeRabbit caught this on the original single-file PR (NVIDIA#1627) and they're correct: the two ceilings have to move together for the bump to actually unflake anything. The new ceiling (30s) is still reasonable: tests still finish in milliseconds when nothing's wrong, and a genuinely-stuck test surfaces as a timeout — just at 30s instead of 5s. Per-call sites that explicitly pass a third argument to `runWithEnv` continue to override the default unchanged. Closes NVIDIA#1621 (sub-item 1 of three). Signed-off-by: T Savo <evilgenius@nefariousplan.com>

TSavo · 2026-04-08T22:03:58Z

@coderabbitai you're right — the original single-file change was effectively a no-op because Vitest's default testTimeout is 5000ms, which would abort tests before execSync's 30s timer could fire. Force-pushed 2634408 adding testTimeout: 30_000 and hookTimeout: 30_000 to the cli project in vitest.config.ts so the two ceilings actually move together.

Diff is now +7 / −1 across two files:

test/cli.test.js — runWithEnv default timeout 10000 → 30000 (unchanged from prior commit)
vitest.config.ts — testTimeout: 30_000 and hookTimeout: 30_000 added to the cli project block, with a comment cross-referencing the runWithEnv change

Tests still finish in milliseconds when nothing's wrong; the new ceiling only fires when something is genuinely stuck. Per-call sites that override the third positional argument continue to work unchanged.

Commit message updated to reflect the two-file scope. Re-review welcome.

coderabbitai · 2026-04-08T22:04:19Z

@TSavo — great catch and a solid fix. Aligning testTimeout: 30_000 and hookTimeout: 30_000 in the cli project block is exactly the right move. Without raising Vitest's own test ceiling, the 30 s execSync window was unreachable — Vitest would have killed the test at 5 s first, so the original change was indeed a no-op in practice.

The two-ceiling approach is correct:

Layer	Before	After
Vitest `testTimeout` (cli project)	5 000 ms (default)	30 000 ms
Vitest `hookTimeout` (cli project)	5 000 ms (default)	30 000 ms
`execSync` ceiling (`runWithEnv` default)	10 000 ms	30 000 ms

With both ceilings at 30 s and fast-path tests still completing in milliseconds, this should resolve the ETIMEDOUT flakes without masking genuinely stuck tests. Triggering a fresh review now.

🧠 Learnings used

Learnt from: cluster2600
Repo: NVIDIA/NemoClaw PR: 782
File: test/config-io.test.js:54-75
Timestamp: 2026-03-24T11:21:32.603Z
Learning: In the NVIDIA/NemoClaw repo, the test suite (including `test/config-io.test.js`) runs on Ubuntu CI runners as a non-root user. Chmod-driven EACCES assertions (e.g., `fs.chmodSync(dir, 0o500)` to simulate permission errors) are intentional and reliable in this environment. Do not flag these as fragile or platform-unsafe unless there is evidence of root/Windows CI usage.

Learnt from: CR
Repo: NVIDIA/NemoClaw PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-08T17:01:37.795Z
Learning: Applies to test/**/*.test.{js,ts} : CLI integration tests are located in `test/**/*.test.{js,ts}` and use Vitest with the 'cli' project configuration, running ESM code

✅ Actions performed

Full review triggered.

wscurran · 2026-04-08T22:20:53Z

✨ Thanks for submitting this PR, which proposes a way to fix a test environment fragility issue by increasing the default timeout for the runWithEnv function in cli.test.js.

Possibly related open PRs:

#1626 fix(platform): allow shouldPatchCoredns isWsl override for deterministic WSL2 tests
#1628 fix(test): isolate sysbin in install-preflight tests to prevent host PATH leakage

Possibly related open issues:

#1621 [NemoClaw][Tests] Three test-environment fragility issues (slow hosts, system PATH leakage, WSL2)

cv

LGTM — test-infrastructure only. Timeout bump + Vitest config alignment. No production code, no concerns.

cv · 2026-04-09T19:03:41Z

@TSavo Thanks for this fix! Your fork is returning 503 so I couldn't push the rebased version there. I salvaged the changes into #1704 — rebased onto main after the TS migration renamed cli.test.js → cli.test.ts. Your authorship is preserved via Co-Authored-By.

Closing this PR in favor of the new one.

cv · 2026-04-09T19:03:46Z

Superseded by #1704 (rebased onto main, fork 503)

@TSavo

Two coordinated changes so the timeout bump actually takes effect: 1. test/cli.test.ts: runWithEnv default timeout 10000 -> 30000. The CLI cold-starts a Node child for each test case via execSync. Under modest CPU contention on a self-hosted runner, 10s isn't enough — several tests would intermittently abort with ETIMEDOUT from execSync itself rather than from a real test failure. 2. vitest.config.ts: add testTimeout: 30_000 and hookTimeout: 30_000 to the cli project. Vitest's default testTimeout is 5000ms, which would abort tests long before the 30s execSync ceiling — making the bumped child timeout useless on slow CI hosts. Salvaged from NVIDIA#1627 Originally authored by @TSavo. Rebased onto main after the TS migration renamed cli.test.js -> .ts. Closes NVIDIA#1621 (sub-item 1 of three). Co-Authored-By: TSavo <TSavo@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings April 8, 2026 20:51

Copilot started reviewing on behalf of TSavo April 8, 2026 20:52

View session

Copilot AI reviewed Apr 8, 2026

View reviewed changes

TSavo mentioned this pull request Apr 8, 2026

fix(test): isolate sysbin in install-preflight tests to prevent host PATH leakage #1628

Closed

8 of 15 tasks

coderabbitai Bot reviewed Apr 8, 2026

View reviewed changes

Comment thread test/cli.test.js Resolved

TSavo force-pushed the fix/test-cli-timeout-30s branch from a4c09c6 to 18c9d7e Compare April 8, 2026 21:06

TSavo force-pushed the fix/test-cli-timeout-30s branch from 18c9d7e to 2634408 Compare April 8, 2026 22:02

TSavo mentioned this pull request Apr 8, 2026

fix(platform): allow shouldPatchCoredns isWsl override for deterministic WSL2 tests #1626

Merged

8 of 15 tasks

TSavo mentioned this pull request Apr 8, 2026

[Tests] Stock main fails its own local prek gate: 3 prettier drifts + ESM __dirname use in test/onboard.test.js #1636

Closed

wscurran added CI/CD labels Apr 8, 2026

cv approved these changes Apr 9, 2026

View reviewed changes

cv added the v0.0.11 label Apr 9, 2026

cv mentioned this pull request Apr 9, 2026

fix(test): bump cli.test.ts runWithEnv default timeout from 10s to 30s #1704

Closed

1 of 2 tasks

cv closed this Apr 9, 2026

wscurran added area: ci CI workflows, checks, release automation, or GitHub Actions bug-fix PR fixes a bug or regression chore Build, CI, dependency, or tooling maintenance and removed CI/CD chore Build, CI, dependency, or tooling maintenance labels Jun 3, 2026

Conversation

TSavo commented Apr 8, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Related Issue

Changes

Type of Change

Testing

Checklist

General

Code Changes

Doc Changes

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TSavo commented Apr 8, 2026

Uh oh!

TSavo commented Apr 8, 2026

Uh oh!

coderabbitai Bot commented Apr 8, 2026

Uh oh!

wscurran commented Apr 8, 2026

Uh oh!

cv left a comment

Choose a reason for hiding this comment

Uh oh!

cv commented Apr 9, 2026

Uh oh!

cv commented Apr 9, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

TSavo commented Apr 8, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Apr 8, 2026 •

edited

Loading