Skip to content

test(inference): share the stopped foreign-container cases - #8439

Merged
prekshivyas merged 2 commits into
mainfrom
test/managed-cluster-stopped-foreign-fixtures
Aug 6, 2026
Merged

test(inference): share the stopped foreign-container cases#8439
prekshivyas merged 2 commits into
mainfrom
test/managed-cluster-stopped-foreign-fixtures

Conversation

@laitingsheng

@laitingsheng laitingsheng commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Summary

Three managed-cluster suites each carried a byte-identical copy of the stopped foreign-container case table. The table and its type now live in the existing managed-cluster test-support module, so one canonical set of signals drives all three parameterized suites.

Related Issue

Resolves #8348

Changes

  • Exported StoppedForeignContainerFixture and STOPPED_FOREIGN_CONTAINER_FIXTURES from src/lib/inference/serving/managed-cluster-fixture.test-support.ts, moved verbatim from the suites.
  • Removed the duplicate type and table from managed-cluster-discovery.test.ts, managed-cluster-executor.test.ts, and managed-cluster-lifecycle.test.ts, which now import the shared pair.
  • Dropped the MANAGED_CLUSTER_MANAGED_LABEL import from the executor and lifecycle suites, where the removed table was its only consumer. The discovery suite keeps it for its other cases.
  • Each suite retains its own setup and expectations: discovery asserts runtime-conflict, the executor asserts the worker-first boundary stays closed, and lifecycle asserts conflict with no staging, start, or removal.
  • Applied a pending Biome organizeImports fix to the test-support module. That module already failed biome check on origin/main, and the pre-commit hook rewrites it as soon as the file is touched.
  • Net 48 fewer lines. No production source changed and no production export added.

Type of Change

  • 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

  • Tests added or updated for changed behavior
  • Existing tests cover changed behavior — justification: this is a test-only consolidation; all three suites keep every case, title, and assertion, and the 62 tests pass unchanged.
  • Tests not applicable — justification:
  • Docs updated for user-facing behavior changes
  • Docs not applicable — justification: no public API, CLI, configuration, default, or error message changes.
  • Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
  • Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: the changed files sit under inference but are test modules; no production behavior is touched. Left for a maintainer to record.
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Documentation Writer Review

  • Documentation writer subagent reviewed the completed changes
  • Result: no-docs-needed
  • Evidence: four files changed, all test-side under src/lib/inference/serving/. No production module, CLI command, blueprint YAML, catalog, default, or error string is touched, and docs/ has no reference to the affected symbols. The documented contract these cases exercise, at docs/inference/set-up-vllm-on-two-dgx-sparks.mdx:227, is unchanged because the table moved verbatim.
  • Agent: Claude Code

Verification

  • PR description includes a Signed-off-by: line and every commit appears as Verified in GitHub
  • Normal pre-commit, commit-msg, and pre-push hooks passed, or npm run validate:pr passed after refreshing origin/main when hooks were skipped or unavailable
  • Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: npx vitest run --project cli src/lib/inference/serving/managed-cluster-discovery.test.ts src/lib/inference/serving/managed-cluster-executor.test.ts src/lib/inference/serving/managed-cluster-lifecycle.test.ts — 3 files, 62 tests passed; npm run typecheck:cli clean; npm run test:projects:check reports exact membership.
  • Applicable broad gate passed — npm test for broad runtime/test-harness changes; npm run check for repo-wide validation/coverage changes — command/result:
  • Quality Gates section completed with required justifications or waivers
  • No secrets, API keys, or credentials committed
  • npm run docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: Tinson Lai tinsonl@nvidia.com

Summary by CodeRabbit

  • Tests
    • Consolidated stopped-container test scenarios into shared fixtures.
    • Expanded coverage for containers identified by name, image, or managed labels.
    • Improved consistency across managed-cluster discovery, execution, and lifecycle test scenarios.

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 882f76c5-e1ff-4fed-98b5-e8ef2c4e0458

📥 Commits

Reviewing files that changed from the base of the PR and between ce9d174 and c4bc297.

📒 Files selected for processing (4)
  • src/lib/inference/serving/managed-cluster-discovery.test.ts
  • src/lib/inference/serving/managed-cluster-executor.test.ts
  • src/lib/inference/serving/managed-cluster-fixture.test-support.ts
  • src/lib/inference/serving/managed-cluster-lifecycle.test.ts

📝 Walkthrough

Walkthrough

The change centralizes stopped foreign-container fixtures in the shared test-support module. The discovery, executor, and lifecycle test suites now import the shared type and fixture collection instead of defining duplicate local cases.

Changes

Managed-cluster fixture sharing

Layer / File(s) Summary
Define shared stopped-container fixtures
src/lib/inference/serving/managed-cluster-fixture.test-support.ts
Adds the exported StoppedForeignContainerFixture type and immutable three-case fixture collection for foreign name, image, and managed-label signals.
Migrate managed-cluster test suites
src/lib/inference/serving/managed-cluster-discovery.test.ts, src/lib/inference/serving/managed-cluster-executor.test.ts, src/lib/inference/serving/managed-cluster-lifecycle.test.ts
Updates all three suites to use the shared fixture collection and removes duplicated local fixture definitions and unused imports.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested labels: area: inference, chore

Suggested reviewers: apurvvkumaria, cv, prekshivyas

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes sharing the stopped foreign-container test cases, which is the primary change.
Linked Issues check ✅ Passed The PR exports one immutable fixture table, migrates all three suites, preserves local assertions, avoids production exports, and reduces LOC for issue #8348.
Out of Scope Changes check ✅ Passed All changes are limited to shared test fixtures and imports required by issue #8348; no unrelated or production changes are present.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch test/managed-cluster-stopped-foreign-fixtures

Comment @coderabbitai help to get the list of available commands.

@laitingsheng laitingsheng added the refactor PR restructures code without intended behavior change label Aug 6, 2026
@github-code-quality

github-code-quality Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in commit 91e3d9e in the test/managed-cluster... branch remains at 96%, unchanged from commit 0fc30f0 in the main branch.

TypeScript / code-coverage/cli

The overall coverage in commit 91e3d9e in the test/managed-cluster... branch remains at 82%, unchanged from commit 0fc30f0 in the main branch.

Show a code coverage summary of the most impacted files.
File main 0fc30f0 test/managed-cluster... 91e3d9e +/-
src/lib/onboard...shboard-port.ts 96% 90% -6%
src/lib/domain/...dbox/destroy.ts 89% 84% -5%
src/lib/shields/index.ts 81% 81% 0%
src/lib/core/immutable.ts 97% 97% 0%
src/lib/messagi...flow-planner.ts 93% 93% 0%
src/lib/sandbox...rce-identity.ts 87% 87% 0%
src/lib/inferen...test-support.ts 93% 94% +1%

Updated August 06, 2026 08:29 UTC

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — No blocking findings reported

Advisor assessment: No blocking advisor findings reported
Next action: No advisor follow-up needed.
Findings: 0 blockers · 0 warnings · 0 suggestions

Model lanes

  • GPT-5.6 Terra (primary): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
  • Nemotron 3 Ultra (second opinion): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
  • Model comparison: normalized findings match; normalized terminology decisions match; normalized E2E selections differ; severity counts match.
3 additional E2E selections from the second opinion

Advisory only. The primary lane did not select these E2E jobs or targets.

  • security-posture: The completed second-opinion lane identified E2E coverage that the primary lane omitted.
  • sandbox-survival: The completed second-opinion lane identified E2E coverage that the primary lane omitted.
  • vllm-docker-storage: The completed second-opinion lane identified E2E coverage that the primary lane omitted.

Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests.

E2E guidance

Advisory only. A maintainer can dispatch the default E2E suite against this exact revision.

Recommended E2E: inference-routing, network-policy

Workflow run details

This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge.

@laitingsheng laitingsheng added the area: inference Inference routing, serving, model selection, or outputs label Aug 6, 2026
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
@prekshivyas
prekshivyas merged commit 70b3561 into main Aug 6, 2026
46 checks passed
@prekshivyas
prekshivyas deleted the test/managed-cluster-stopped-foreign-fixtures branch August 6, 2026 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: inference Inference routing, serving, model selection, or outputs refactor PR restructures code without intended behavior change release-target

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Share stopped foreign-container cases across managed-cluster tests

3 participants