test(inference): share vLLM install setup across source tests - #8392
test(inference): share vLLM install setup across source tests#8392AzeelSajjad wants to merge 25 commits into
Conversation
) Rewrite the mockSuccessfulVllmInstall JSDoc to describe the actual alternation between an unconditional empty row and the queued ownershipResponses entries, instead of the misleading "in order" claim. Strengthen the freshness guard test with a third dockerCapture(["container"]) call on the first instance so a module-scoped ownershipCallIndex regression is caught instead of passing silently.
…ity suite (NVIDIA#8351) Adopt vllm-install.test-support.ts in the third suite carrying the same duplicated install-mock mechanics: probe defaults, spy lifecycle, and mockDockerSpawnSuccess. Also aligns the crash-loop watchdog describe block's env reset with the shared helper, which now also clears HUGGING_FACE_HUB_TOKEN there for consistency with the preflight describe block.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds shared vLLM installation test utilities and migrates three inference test suites to use them. It centralizes mocks, spies, process stubs, storage probes, and environment cleanup while preserving suite-specific scenarios and assertions. ChangesvLLM installation test consolidation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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.
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 `@src/lib/inference/vllm-install.test-support.test.ts`:
- Around line 62-71: Update the test around applyVllmInstallProbeDefaults to
reuse the original first mock registry: after mutating
first.probeHostStorage().capacity.availableBytes, invoke
applyVllmInstallProbeDefaults(first) again, then assert that
first.probeHostStorage() reports the default availableBytes value. Remove the
separate second registry so the test verifies repeated setup replaces mutated
results.
🪄 Autofix
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: 0c307abe-2592-4e46-8cdf-79679e0a8bb6
📒 Files selected for processing (6)
ci/source-architecture-budget.jsonsrc/lib/inference/vllm-compute-capability.test.tssrc/lib/inference/vllm-install-storage.test.tssrc/lib/inference/vllm-install.test-support.test.tssrc/lib/inference/vllm-install.test-support.tssrc/lib/inference/vllm.test.ts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
2 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
3 additional E2E selections from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Security review result: PASS for the current revision ( No security findings. The PR changes only vLLM test scaffolding and the source-architecture budget; production runtime behavior is unchanged.
Files reviewed: Validation: 102 targeted tests passed; formatting, test-title policy, source-shape/project membership, CLI typechecking, repository hooks, secret scanning, and normal pre-push checks passed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/inference/vllm.test.ts (1)
82-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftUse the shared setup path for dual-Station installation tests.
src/lib/inference/vllm-dual-station.test.tsstill duplicates probe defaults, spies, and environment setup. Move this common setup intovllm-install.test-support.ts, or document and link the bounded exclusion.🤖 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 `@src/lib/inference/vllm.test.ts` around lines 82 - 96, Consolidate the duplicated probe defaults, spies, and environment setup used by dual-Station installation tests into vllm-install.test-support.ts, reusing helpers such as applyVllmInstallProbeDefaults, createVllmInstallSpies, and resetVllmInstallEnv from vllm.test.ts. Update vllm-dual-station.test.ts to use this shared setup path and remove its local duplicates; only retain the duplication if it is explicitly documented with a link to the bounded exclusion.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/inference/vllm.test.ts`:
- Around line 113-124: Reset the closure-backed dockerCapture mock after the
ownership-queue assertions by calling mocks.dockerCapture.mockReset(). Place the
reset before the test exits so subsequent tests do not inherit the exhausted
queue; do not alter the existing queue expectations.
---
Nitpick comments:
In `@src/lib/inference/vllm.test.ts`:
- Around line 82-96: Consolidate the duplicated probe defaults, spies, and
environment setup used by dual-Station installation tests into
vllm-install.test-support.ts, reusing helpers such as
applyVllmInstallProbeDefaults, createVllmInstallSpies, and resetVllmInstallEnv
from vllm.test.ts. Update vllm-dual-station.test.ts to use this shared setup
path and remove its local duplicates; only retain the duplication if it is
explicitly documented with a link to the bounded exclusion.
🪄 Autofix
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: ba510dcb-2214-4b0e-b74d-14997a854e28
📒 Files selected for processing (1)
src/lib/inference/vllm.test.ts
|
Maintainer update for the current revision (
Remaining gates are the advisor retry, ordinary fork CI workflows awaiting repository approval/execution, E2E, and independent maintainer approval. The active merge freeze is honored; no merge was attempted. |
|
The bounded advisor retry completed successfully: both model lanes now report high confidence with zero blockers, warnings, or suggestions. The prior failure was limited to advisor service configuration and is resolved. Ordinary CI is still running, and protected E2E plus independent approval remain required. No merge will be attempted during the release freeze. |
|
Protected E2E is complete for branch revision |
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Verdict
PASS for exact head 095f138365c63f39d02b7f9aa3cd8e7a2f653641 on base 21955a57815e9da1ac356025c5b75e90eb8c5e00. The patch consolidates source-suite test scaffolding, resets closure-backed mocks, returns a fresh process stub per model-download invocation, and isolates legacy install unit tests from real managed-cluster discovery. It changes no production behavior or runtime boundary.
Findings
No security findings.
Detailed analysis
- Secrets and Credentials — PASS. Test token values remain synthetic, shared cleanup removes both supported token environment variables, and no credentials or secret material are added.
- Input Validation and Data Sanitization — PASS. No production input path changes. The mock ownership queue continues to fail loudly on unexpected inspections, and test-only command dispatch remains keyed by fixed command names.
- Authentication and Authorization — PASS. Not applicable; no production identity, authentication, authorization, or permission behavior changes.
- Dependencies and Third-Party Libraries — PASS. No dependency or lockfile changes. The source-architecture budget increases by one solely for the new test-support module.
- Error Handling and Logging — PASS. Test spies continue to suppress and restore console and stream output, and all mock state is reset explicitly without exposing sensitive values.
- Cryptography and Data Protection — PASS. Not applicable; no cryptographic or protected-data operations change.
- Configuration and Security Headers — PASS. No runtime configuration, image, port, network policy, privilege, capability, or HTTP-header changes.
- Security Testing — PASS. The three owning suites retain their behavior assertions; exact-head validation passes 102 tests. Fresh process emitters and isolated managed-cluster selection remove timing and host-environment leakage from security-adjacent inference tests.
- Holistic Security Posture — PASS. The patch does not affect SSRF controls, sandbox isolation, Docker production calls, credentials, or blueprint policy. It strengthens deterministic fail-closed test behavior and introduces no alternate runtime path.
Files reviewed
ci/source-architecture-budget.jsonsrc/lib/inference/vllm-compute-capability.test.tssrc/lib/inference/vllm-install-storage.test.tssrc/lib/inference/vllm-install.test-support.tssrc/lib/inference/vllm.test.ts
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Verdict
PASS for exact head 459990c3ca647642b84944bd19023cd94d991d89 on base db31c286129e878c3356eed49f76ab259561e47e. The patch consolidates source-suite test scaffolding, resets closure-backed mocks, returns a fresh process stub per model-download invocation, and isolates legacy install unit tests from real managed-cluster discovery. It changes no production behavior or runtime boundary.
Findings
No security findings.
Detailed analysis
- Secrets and Credentials — PASS. Test token values remain synthetic, shared cleanup removes both supported token environment variables, and no credentials or secret material are added.
- Input Validation and Data Sanitization — PASS. No production input path changes. The mock ownership queue continues to fail loudly on unexpected inspections, and test-only command dispatch remains keyed by fixed command names.
- Authentication and Authorization — PASS. Not applicable; no production identity, authentication, authorization, or permission behavior changes.
- Dependencies and Third-Party Libraries — PASS. No dependency or lockfile changes. The source-architecture budget increases by one solely for the new test-support module.
- Error Handling and Logging — PASS. Test spies continue to suppress and restore console and stream output, and all mock state is reset explicitly without exposing sensitive values.
- Cryptography and Data Protection — PASS. Not applicable; no cryptographic or protected-data operations change.
- Configuration and Security Headers — PASS. No runtime configuration, image, port, network policy, privilege, capability, or HTTP-header changes.
- Security Testing — PASS. The three owning suites retain their behavior assertions; exact-head validation passes 102 tests. Fresh process emitters and isolated managed-cluster selection remove timing and host-environment leakage from security-adjacent inference tests.
- Holistic Security Posture — PASS. The patch does not affect SSRF controls, sandbox isolation, Docker production calls, credentials, or blueprint policy. It strengthens deterministic fail-closed test behavior and introduces no alternate runtime path.
Files reviewed
ci/source-architecture-budget.jsonsrc/lib/inference/vllm-compute-capability.test.tssrc/lib/inference/vllm-install-storage.test.tssrc/lib/inference/vllm-install.test-support.tssrc/lib/inference/vllm.test.ts
|
Protected-gate disposition for the current branch revision: Ordinary CI passed again with no failures. The protected gate is terminal because two maintainer approval attempts overlapped; canceling the duplicate controller also canceled the sole child run before the selected inference-routing and network-policy jobs started. A subsequent clean CI rerun confirmed the code remains green, but the controller correctly refuses to reuse the completed gate state for the same branch/base pair. This is an orchestration failure, not a product or test failure. I will not create an artificial commit or dispatch additional duplicate controllers. The already-required post-tag branch reconciliation will create a fresh validation state, after which the two selected protected checks will run once. The merge freeze remains active. |
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Verdict
PASS for exact head 2dd1cf1060bccf796ea8634d50e7e8562fcb2a6e on base db31c286129e878c3356eed49f76ab259561e47e. The effective source diff is identical to the previously reviewed revision; the only new commit is a signed, empty CI-retry commit required after a terminal E2E-controller timeout. The patch changes test scaffolding only and introduces no production behavior or runtime boundary.
Findings
No security findings.
Detailed analysis
- Secrets and Credentials — PASS. Test token values remain synthetic, shared cleanup removes both supported token environment variables, and no credentials or secret material are added.
- Input Validation and Data Sanitization — PASS. No production input path changes. The mock ownership queue continues to fail loudly on unexpected inspections, and test-only command dispatch remains keyed by fixed command names.
- Authentication and Authorization — PASS. Not applicable; no production identity, authentication, authorization, or permission behavior changes.
- Dependencies and Third-Party Libraries — PASS. No dependency or lockfile changes. The source-architecture budget increases by one solely for the new test-support module.
- Error Handling and Logging — PASS. Test spies continue to suppress and restore console and stream output, and all mock state is reset explicitly without exposing sensitive values.
- Cryptography and Data Protection — PASS. Not applicable; no cryptographic or protected-data operations change.
- Configuration and Security Headers — PASS. No runtime configuration, image, port, network policy, privilege, capability, or HTTP-header changes.
- Security Testing — PASS. The three owning suites retain their behavior assertions; exact-diff validation passed 102 tests before the empty retry commit. Fresh process emitters and isolated managed-cluster selection remove timing and host-environment leakage from security-adjacent inference tests.
- Holistic Security Posture — PASS. The patch does not affect SSRF controls, sandbox isolation, Docker production calls, credentials, or blueprint policy. It strengthens deterministic fail-closed test behavior and introduces no alternate runtime path.
Files reviewed
ci/source-architecture-budget.jsonsrc/lib/inference/vllm-compute-capability.test.tssrc/lib/inference/vllm-install-storage.test.tssrc/lib/inference/vllm-install.test-support.tssrc/lib/inference/vllm.test.ts
|
Validation disposition for branch revision
The protected gate is therefore blocked specifically on external ClawHub availability. Independent approval and the active merge freeze also remain. |
|
Release status update: v0.0.103 has been cut and the merge freeze is lifted. This does not change the remaining gates: the protected network-policy result is still externally blocked by the documented ClawHub HTTP 503, and independent approval is still required. No merge will be attempted without a fresh passing required gate. |
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Reverts commit 1130650. Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
cv
changed the title
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
4 participants
Summary
Moves repeated
installVllmsetup from three source tests intosrc/lib/inference/vllm-install.test-support.ts.Production code and supported behavior do not change.
The helpers replace configured probe responses and Docker ownership responses whenever they are invoked.
Related Issue
Fixes #8351
Changes
vllm.test.ts,vllm-install-storage.test.ts, andvllm-compute-capability.test.ts.vllm.test.ts.src/lib/inferencesource-file budget from 62 to 63 because the repository counts.test-support.tsas a source file.Type of Change
Quality Gates
3e1dc3959dc4d4aa42bafd117cff8dc6b1e003a2against base SHAbf429e90e5b217632356e2d918bfc333e5b8e959.Documentation Writer Review
no-docs-neededci/source-architecture-budget.json,src/lib/inference/vllm-compute-capability.test.ts,src/lib/inference/vllm-install-storage.test.ts,src/lib/inference/vllm-install.test-support.ts, andsrc/lib/inference/vllm.test.tsfor commit3e1dc3959dc4d4aa42bafd117cff8dc6b1e003a2against base SHAbf429e90e5b217632356e2d918bfc333e5b8e959. The change consolidates setup for three source tests and updates one CI architecture budget. Production code, public interfaces, configuration, workflows, defaults, errors, and supported behavior do not change. The changed comments and test title followWRITING.mdand the controlled word list.DGX Station Hardware Evidence
scripts/prepare-dgx-station-host.shdoes not change.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHub — all 23 commits appear asVerified.pre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailable — normal hooks passed for the maintainer corrections and current merge commit.3e1dc3959dc4d4aa42bafd117cff8dc6b1e003a2. No standalone local test suite ran after the merge.npm run docsbuilds without warnings (doc changes only) — not applicable; no documentation changes.Sensitive-Path Security Review
Independent review found no security finding for PR commit
3e1dc3959dc4d4aa42bafd117cff8dc6b1e003a2against base SHAbf429e90e5b217632356e2d918bfc333e5b8e959.Required checks and independent maintainer approval remain separate merge requirements.
GitHub Actions
GitHub Actions is authoritative for PR commit
3e1dc3959dc4d4aa42bafd117cff8dc6b1e003a2against base SHAbf429e90e5b217632356e2d918bfc333e5b8e959.Fork workflows require explicit maintainer approval before required checks can run.
No workflow or check waiver is recorded.
Signed-off-by: Azeel Sajjad aasajjad05@gmail.com