test(onboard): speed up dashboard curl mocks#3374
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a CommonJS test helper to normalize and mock ChangesMock Helper and Test Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
E2E Advisor RecommendationRequired E2E: None Full advisor summaryPi Semantic E2E AdvisorFailed: pi exited with status 1; see /home/runner/work/NemoClaw/NemoClaw/artifacts/e2e-advisor/e2e-advisor-pi-raw-output.txt |
jyaunches
approved these changes
May 12, 2026
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (1)
test/cli.test.ts (1)
2197-2198:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTimeout math now collapses polling windows to ~1s after the default reduction.
Line 2197, Line 2261, and Line 2278 still derive windows from
testTimeout()(default), so with a 5s default the loop budget gets clamped to 1s even though these tests are explicitly set to 10s. This is likely to reintroduce CI flakes.Suggested fix
- const pollTimeoutMs = Math.min(testTimeout(10_000), Math.max(1_000, testTimeout() - 5_000)); + const pollTimeoutMs = testTimeout(10_000); ... - const pollTimeoutMs = Math.min(testTimeout(10_000), Math.max(1_000, testTimeout() - 5_000)); + const pollTimeoutMs = testTimeout(10_000); ... - const termDeadline = Date.now() + Math.min(testTimeout(5_000), Math.max(1_000, testTimeout() - 5_000)); + const termDeadline = Date.now() + testTimeout(5_000);Also applies to: 2261-2262, 2278-2278
🤖 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.test.ts` around lines 2197 - 2198, The polling timeout math uses testTimeout() (the default) when computing poll windows, which collapses the loop budget to ~1s; update the expressions that compute pollTimeoutMs and deadline to use the explicit test timeout (testTimeout(10_000)) rather than testTimeout() so the 10s test setting is honored (i.e., replace testTimeout() with testTimeout(10_000) in the pollTimeoutMs/deadline calculation and apply the same fix to the other occurrences that compute polling windows).
🤖 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.
Outside diff comments:
In `@test/cli.test.ts`:
- Around line 2197-2198: The polling timeout math uses testTimeout() (the
default) when computing poll windows, which collapses the loop budget to ~1s;
update the expressions that compute pollTimeoutMs and deadline to use the
explicit test timeout (testTimeout(10_000)) rather than testTimeout() so the 10s
test setting is honored (i.e., replace testTimeout() with testTimeout(10_000) in
the pollTimeoutMs/deadline calculation and apply the same fix to the other
occurrences that compute polling windows).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a2847c0f-fb0d-4557-a3ee-ea882feeea64
📒 Files selected for processing (2)
src/lib/onboard/temp-files.test.tstest/cli.test.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
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.test.ts`:
- Around line 2197-2199: The current polling budget uses the full testBudgetMs
(10_000) across both pre- and post-SIGTERM phases which can exhaust the test
timeout; change the poll timeout calculation to reserve a safety buffer (e.g.,
subtract 2000ms or a configurable SLACK_MS) so both phases never collectively
consume the entire budget. Update the computation that sets pollTimeoutMs and
deadline (variables testBudgetMs, pollTimeoutMs, deadline) to compute
pollTimeoutMs = Math.min(testBudgetMs - SLACK_MS, Math.max(1_000, testBudgetMs -
5_000)) (ensuring non-negative) or otherwise subtract SLACK_MS before using
testBudgetMs for a second-phase deadline; apply the same change at the other
occurrences noted (the blocks around the other three locations).
🪄 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: cb6222eb-636f-45f9-a395-573cde68b623
📒 Files selected for processing (1)
test/cli.test.ts
Comment on lines
+2197
to
2199
| const testBudgetMs = testTimeout(10_000); | ||
| const pollTimeoutMs = Math.min(testBudgetMs, Math.max(1_000, testBudgetMs - 5_000)); | ||
| const deadline = Date.now() + pollTimeoutMs; |
Contributor
There was a problem hiding this comment.
Avoid consuming the full 10s budget across both polling phases.
In the SIGTERM test, you budget ~5s before SIGTERM and another ~5s after SIGTERM. That can hit the full 10_000ms test timeout with almost no setup/assertion slack and cause CI flakes.
Suggested adjustment
+ const testStartMs = Date.now();
let calls: string[] = [];
const testBudgetMs = testTimeout(10_000);
const pollTimeoutMs = Math.min(testBudgetMs, Math.max(1_000, testBudgetMs - 5_000));
- const deadline = Date.now() + pollTimeoutMs;
+ const deadline = Date.now() + pollTimeoutMs;
while (Date.now() < deadline) {
calls = readCalls();
if (
calls.includes("logs alpha -n 200 --source all --tail") &&
calls.includes("sandbox exec -n alpha -- tail -n 200 -f /tmp/gateway.log")
) {
break;
}
await new Promise((resolve) => setTimeout(resolve, 100));
}
expect(calls).toContain("logs alpha -n 200 --source all --tail");
expect(calls).toContain("sandbox exec -n alpha -- tail -n 200 -f /tmp/gateway.log");
child.kill("SIGTERM");
let callsAfterTerm: string[] = [];
- const termTimeoutMs = Math.min(testBudgetMs, Math.max(1_000, testBudgetMs - 5_000));
+ const elapsedMs = Date.now() - testStartMs;
+ const remainingBudgetMs = Math.max(1_000, testBudgetMs - elapsedMs - 250);
+ const termTimeoutMs = remainingBudgetMs;
const termDeadline = Date.now() + termTimeoutMs;
while (Date.now() < termDeadline) {
callsAfterTerm = readCalls();
if (callsAfterTerm.some((call) => call.endsWith("term-start")) || hasExited) {
break;
}
await new Promise((resolve) => setTimeout(resolve, 50));
}Also applies to: 2262-2265, 2280-2282
🤖 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.test.ts` around lines 2197 - 2199, The current polling budget uses
the full testBudgetMs (10_000) across both pre- and post-SIGTERM phases which
can exhaust the test timeout; change the poll timeout calculation to reserve a
safety buffer (e.g., subtract 2000ms or a configurable SLACK_MS) so both phases
never collectively consume the entire budget. Update the computation that sets
pollTimeoutMs and deadline (variables testBudgetMs, pollTimeoutMs, deadline) to
compute pollTimeoutMs = Math.min(testBudgetMs - SLACK_MS, Math.max(1_000,
testBudgetMs - 5_000)) (ensuring non-negative) or otherwise subtract SLACK_MS
before using testBudgetMs for a second-phase deadline; apply the same change at
the other occurrences noted (the blocks around the other three locations).
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
3 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
Extracts shared onboard script curl mock handling and uses it across generated onboard child-process test scripts. The helper returns HTTP 200 for dashboard health probes so mocked createSandbox flows no longer burn the full dashboard readiness timeout. It also lowers the default CLI test timeout to 5s so future accidental sleeps fail fast, with explicit 10s allowances for the three known Ollama installer-selection outliers.
Changes
test/helpers/onboard-script-mocks.cjswith reusable command normalization and sandbox curl mock handling.sandbox exec/curlmock branches intest/onboard.test.tswith the shared helper.defaultCurlOutputwhile ensuring dashboard/healthprobes return an HTTP status code.test/helpers/timeouts.ts.test/onboard-selection.test.tsOllama install-selection cases with explicit 10s timeouts.Type of Change
Verification
npm testwas attempted but currently fails ininstaller-integrationattest/install-preflight.test.ts(skips onboarding when shared host preflight detects Docker is missing), unrelated to this test-only change; leaving that box unchecked.npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit