perf(test): reduce provider-selection process isolation - #6336
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
📝 WalkthroughWalkthroughRefactors onboarding setupNim into a dependency-injected ChangessetupNim extraction and onboarding test refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most covered files.
Updated |
PR Review Advisor (Nemotron Ultra) — BlockedMerge posture: Do not merge until addressed Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
E2E Target RecommendationRequired E2E targets: Dispatch required E2E targets:
Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/onboard-selection.test.ts (2)
189-225: 🎯 Functional Correctness | 🔵 TrivialGood DRY consolidation of provider-menu defaults.
Centralizing shared menu defaults into
buildProviderMenuand havingbuildWindowsProviderMenudelegate to it removes duplicated inline construction. Please confirmwindowsHostStartLabel's function-typed default (Line 204) matches the realbuildInferenceProviderMenuparameter contract used by production code, since a signature drift here would let tests pass while diverging fromsrc/lib/onboard.ts's actual usage.🤖 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/onboard-selection.test.ts` around lines 189 - 225, Verify that the function-typed default passed as windowsHostStartLabel in buildProviderMenu matches the real buildInferenceProviderMenu contract used by production code, and update the test helper if the expected signature differs. Check buildProviderMenu, buildWindowsProviderMenu, and buildInferenceProviderMenu together so the default callback shape stays aligned with src/lib/onboard.ts and does not mask a signature drift in tests.
653-702: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMock-call assertions layered on top of observable-state checks.
expect(probeOpenAiLikeEndpoint).toHaveBeenCalledWith(...)(Lines 697-702, 849-853) andexpect(validateNvidiaEndpointModelFn).toHaveBeenCalledWith(...)(Lines 757-760) assert on mock invocation arguments in addition to the observablestate/validatedoutcomes. Since the state assertions (state.preferredInferenceApi,state.model,validated.result) already confirm the behavior, the call-argument assertions add implementation lock-in without additional confidence about the public boundary.As per path instructions for
**/*.test.{ts,js,mts,mjs,cts,cjs}: "Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions."Also applies to: 709-761, 802-853
🤖 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/onboard-selection.test.ts` around lines 653 - 702, The test cases are asserting on mock invocation arguments in addition to already verified observable outcomes, which adds unnecessary implementation coupling. Remove the `toHaveBeenCalledWith(...)` expectations from `probeOpenAiLikeEndpoint` and `validateNvidiaEndpointModelFn` in the affected tests, and keep the assertions on public-facing results like `state`, `validated.result`, and emitted output so the checks stay focused on observable behavior through the test boundary.Source: Path instructions
🤖 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 `@src/lib/onboard/setup-nim-flow.ts`:
- Around line 248-535: The single-option provider flow bypasses the selection
dispatcher, so setup can ավարտ without calling the provider-specific handler.
Update setup-nim-flow.ts so the same dispatch path used inside selectionLoop
also runs when options.length <= 1, ensuring functions like
handleRemoteProviderSelection, handleNimLocalSelection,
handleRunningOllamaSelection, and handleVllmSelection still execute and populate
state. Add a regression test with only one available provider to verify the
selected provider is dispatched and the handler runs.
In `@test/onboard-selection.test.ts`:
- Line 633: The new test titles in the onboarding selection spec are missing the
required local issue-ref suffix. Update each affected test case in
test/onboard-selection.test.ts (the NVIDIA Endpoints, Kimi K2.6, manual model
entry, reprompt, and Gemini model tests) so their behavior-oriented titles end
with the appropriate final "(`#1234`)"-style issue reference, matching the test
naming convention used throughout the suite.
---
Nitpick comments:
In `@test/onboard-selection.test.ts`:
- Around line 189-225: Verify that the function-typed default passed as
windowsHostStartLabel in buildProviderMenu matches the real
buildInferenceProviderMenu contract used by production code, and update the test
helper if the expected signature differs. Check buildProviderMenu,
buildWindowsProviderMenu, and buildInferenceProviderMenu together so the default
callback shape stays aligned with src/lib/onboard.ts and does not mask a
signature drift in tests.
- Around line 653-702: The test cases are asserting on mock invocation arguments
in addition to already verified observable outcomes, which adds unnecessary
implementation coupling. Remove the `toHaveBeenCalledWith(...)` expectations
from `probeOpenAiLikeEndpoint` and `validateNvidiaEndpointModelFn` in the
affected tests, and keep the assertions on public-facing results like `state`,
`validated.result`, and emitted output so the checks stay focused on observable
behavior through the test boundary.
🪄 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: 3c770fba-3b08-4376-a3f7-da83a84f50e4
📒 Files selected for processing (5)
ci/test-file-size-budget.jsonsrc/lib/onboard.tssrc/lib/onboard/setup-nim-flow.test.tssrc/lib/onboard/setup-nim-flow.tstest/onboard-selection.test.ts
PR Review Advisor — No blocking findingsMerge posture: No blocking advisor findings Action checklist
Findings index
Review findings by urgency: 0 required fixes, 0 items to resolve/justify, 1 in-scope improvement
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Automated review follow-up (
|
Final-head Nemotron triageI reviewed the
No production change or additional push is warranted for these items. The original independent security review also found no behavior/security delta, and CodeRabbit’s final-head review has no open threads. |
<!-- markdownlint-disable MD041 --> ## Summary Run the integration project as a bounded four-worker phase during the canonical local `npm test`, while keeping CI, coverage, focused integration, and direct Vitest runs serialized. Isolate two onboarding fixtures from host-global dashboard ports so the parallel suite remains deterministic. This is the final cumulative #6245 step after the named onboarding conversions, representative process-contract work, and sequenced loader cleanup already merged; the final clean-build Node 22 suite passes in 3:52.03. ## Related Issue Closes #6245. ## Changes - Replace the dashboard-exhaustion fixture's real host listeners with a fake `lsof` while retaining the real CLI, preflight, diagnostic, and non-zero exit contract. - Give the restore-intent fixture an explicit existing dashboard forward so unrelated host port occupancy cannot divert the behavior under test. - Resolve integration scheduling from npm lifecycle, CI, coverage, and worker-cap inputs: local `npm test` uses at most four workers in group 1, while every safety-sensitive route stays serial. - Add a behavior matrix covering local, CI, coverage, focused, direct, and explicit worker-throttle modes. - Complete the cumulative #6245 acceptance path after #6276/#6336/#6383 converted the named onboarding hotspots, #6285/#6417 retained representative process contracts, and #6286/#6299/#6388/#6415 sequenced loader cleanup after process removal. - Record the final host-specific timings, hotspot disposition, and retained process-contract inventory in `test/README.md` as an advisory acceptance snapshot rather than a permanent CI budget. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates <!-- Check exactly one tests line and one docs line. Check other lines when applicable. Add every requested justification or approval reference. --> - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: Test fixtures and local test-runner scheduling changed; NemoClaw commands, configuration, runtime behavior, and CI/coverage workflows are unchanged. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Independent final-diff review confirmed that the fake `lsof` preserves the real CLI/preflight/exit contract, the restore-intent assertions remain intact, and resolved CI/coverage configurations remain serialized. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect the tested behavior. Do not rerun hook-covered checks. --> - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: Real CLI exhaustion contract passed; restore-intent passed with all 11 dashboard ports deliberately occupied; scheduling matrix passed 14/14 through the lifecycle-triggered config; `npm run test:projects:check` reported 1,327 files disjoint across 8 projects. - [x] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: Clean-build Node 22 `npm test -- --reporter=blob` under the normal `umask 022` passed 1,251 files and 13,879 tests with 39 skipped, 1 todo, and zero failures in 3:52.03, down 73% from the issue's 14:19.65 baseline despite a larger suite. The matching diff-scoped routine pre-commit stage passed in 13.95s. #6270 separately removed full coverage from routine pre-commit while preserving manual and authoritative CI gates. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Integration test runs now use adaptive scheduling to speed up local execution while keeping CI/focused runs serialized. * **Bug Fixes** * Improved reliability of onboarding regression coverage by simulating dashboard port exhaustion in a hermetic way. * Updated onboarding-related fixtures to better match the intended readiness/exit behavior. * **Tests** * Added coverage for integration scheduling behavior (local caps, invalid inputs, and CI/coverage scenarios). * **Documentation** * Added test-suite documentation with a local performance snapshot and key test hotspots. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
<!-- markdownlint-disable MD041 --> ## Summary Extracts onboarding provider-selection orchestration into a lightweight typed module so unit-shaped selection tests can run directly instead of spawning Node processes. Controlled base/head runs reduced the selection target's median wall time from 20.57s to 18.74s (8.9%) while retaining process-isolated coverage for boundary-sensitive paths. ## Related Issue Part of NVIDIA#6245. ## Changes - Extract the provider-selection coordinator into `setup-nim-flow.ts` while preserving provider branches, recovery precedence, fail-closed behavior, and agent/inference API coercion. - Add five direct coordinator tests and convert six unit-shaped selection fixtures from child-process execution. - Reduce child-process launches in `test/onboard-selection.test.ts` from 46 to 40; controlled median Vitest duration fell from 20.29s to 18.39s and median test-body time from 19.37s to 17.45s. - Retain 40 process-boundary cases for credentials, Ollama, NIM, vLLM, Windows, and fail-closed behavior. - Ratchet the selection test file-size budget from 6,146 to 5,835 lines. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates <!-- Check exactly one tests line and one docs line. Check other lines when applicable. Add every requested justification or approval reference. --> - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: Internal test-performance refactor only; no user-facing command, configuration, prompt, output, or documentation contract changed. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Independent local security review found no issues; fail-closed exits, credential isolation, recovery precedence, and agent/inference API coercion remain intact. CodeRabbit has no open threads, and the automated advisor findings are resolved or evidence-backed in the PR follow-up comments. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect the tested behavior. Do not rerun hook-covered checks. --> - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: `setup-nim-flow` passed 5/5 on the final head; the unchanged `onboard-selection` and Anthropic-compatible OpenAI-agent compatibility suites passed 68/68 and 7/7. `npm run typecheck:cli`, `npm run checks`, and `npm run test-size:check` also passed. - [x] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: Final-head CI passed 40 checks, including all five CLI shards and the merged CLI coverage ratchet: statements 72.34%, branches 65.47%, functions 74.73%, lines 73.05%. One unrelated order-sensitive shard failure passed 7/7 in local isolation and on the single-job CI rerun; the dependent aggregate coverage and checks jobs are green. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Carlos Villela <cvillela@nvidia.com> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
<!-- markdownlint-disable MD041 --> ## Summary Run the integration project as a bounded four-worker phase during the canonical local `npm test`, while keeping CI, coverage, focused integration, and direct Vitest runs serialized. Isolate two onboarding fixtures from host-global dashboard ports so the parallel suite remains deterministic. This is the final cumulative NVIDIA#6245 step after the named onboarding conversions, representative process-contract work, and sequenced loader cleanup already merged; the final clean-build Node 22 suite passes in 3:52.03. ## Related Issue Closes NVIDIA#6245. ## Changes - Replace the dashboard-exhaustion fixture's real host listeners with a fake `lsof` while retaining the real CLI, preflight, diagnostic, and non-zero exit contract. - Give the restore-intent fixture an explicit existing dashboard forward so unrelated host port occupancy cannot divert the behavior under test. - Resolve integration scheduling from npm lifecycle, CI, coverage, and worker-cap inputs: local `npm test` uses at most four workers in group 1, while every safety-sensitive route stays serial. - Add a behavior matrix covering local, CI, coverage, focused, direct, and explicit worker-throttle modes. - Complete the cumulative NVIDIA#6245 acceptance path after NVIDIA#6276/NVIDIA#6336/NVIDIA#6383 converted the named onboarding hotspots, NVIDIA#6285/NVIDIA#6417 retained representative process contracts, and NVIDIA#6286/NVIDIA#6299/NVIDIA#6388/NVIDIA#6415 sequenced loader cleanup after process removal. - Record the final host-specific timings, hotspot disposition, and retained process-contract inventory in `test/README.md` as an advisory acceptance snapshot rather than a permanent CI budget. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates <!-- Check exactly one tests line and one docs line. Check other lines when applicable. Add every requested justification or approval reference. --> - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: Test fixtures and local test-runner scheduling changed; NemoClaw commands, configuration, runtime behavior, and CI/coverage workflows are unchanged. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Independent final-diff review confirmed that the fake `lsof` preserves the real CLI/preflight/exit contract, the restore-intent assertions remain intact, and resolved CI/coverage configurations remain serialized. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect the tested behavior. Do not rerun hook-covered checks. --> - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: Real CLI exhaustion contract passed; restore-intent passed with all 11 dashboard ports deliberately occupied; scheduling matrix passed 14/14 through the lifecycle-triggered config; `npm run test:projects:check` reported 1,327 files disjoint across 8 projects. - [x] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: Clean-build Node 22 `npm test -- --reporter=blob` under the normal `umask 022` passed 1,251 files and 13,879 tests with 39 skipped, 1 todo, and zero failures in 3:52.03, down 73% from the issue's 14:19.65 baseline despite a larger suite. The matching diff-scoped routine pre-commit stage passed in 13.95s. NVIDIA#6270 separately removed full coverage from routine pre-commit while preserving manual and authoritative CI gates. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Integration test runs now use adaptive scheduling to speed up local execution while keeping CI/focused runs serialized. * **Bug Fixes** * Improved reliability of onboarding regression coverage by simulating dashboard port exhaustion in a hermetic way. * Updated onboarding-related fixtures to better match the intended readiness/exit behavior. * **Tests** * Added coverage for integration scheduling behavior (local caps, invalid inputs, and CI/coverage scenarios). * **Documentation** * Added test-suite documentation with a local performance snapshot and key test hotspots. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary
Extracts onboarding provider-selection orchestration into a lightweight typed module so unit-shaped selection tests can run directly instead of spawning Node processes. Controlled base/head runs reduced the selection target's median wall time from 20.57s to 18.74s (8.9%) while retaining process-isolated coverage for boundary-sensitive paths.
Related Issue
Part of #6245.
Changes
setup-nim-flow.tswhile preserving provider branches, recovery precedence, fail-closed behavior, and agent/inference API coercion.test/onboard-selection.test.tsfrom 46 to 40; controlled median Vitest duration fell from 20.29s to 18.39s and median test-body time from 19.37s to 17.45s.Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablesetup-nim-flowpassed 5/5 on the final head; the unchangedonboard-selectionand Anthropic-compatible OpenAI-agent compatibility suites passed 68/68 and 7/7.npm run typecheck:cli,npm run checks, andnpm run test-size:checkalso passed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Final-head CI passed 40 checks, including all five CLI shards and the merged CLI coverage ratchet: statements 72.34%, branches 65.47%, functions 74.73%, lines 73.05%. One unrelated order-sensitive shard failure passed 7/7 in local isolation and on the single-job CI rerun; the dependent aggregate coverage and checks jobs are green.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com