test: remove duplicate behavior coverage - #9501
Conversation
Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review. 📝 WalkthroughWalkthroughThe PR removes duplicate tests and unused test-local helpers from broad suites. Focused suite coverage remains. Selected vLLM profile assertions are more exact. The line budget for ChangesTest coverage consolidation
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change removes duplicate test coverage while retaining focused behavior assertions and does not alter product behavior; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
jyaunches
left a comment
There was a problem hiding this comment.
LOC Reduction / Codebase Simplicity Review
Why this blocks
The consolidation is substantially net-negative and the focused owners retain the other audited contracts, but one unique Spark assertion was deleted rather than moved.
The removed case at src/lib/inference/vllm.test.ts:296-308 was the only test that tied detectVllmProfile({ platform: "spark" }) to defaultModel.envValue === "qwen3.6-35b-a3b-nvfp4". The retained focused case at test/detect-vllm-profile.test.ts:13-23 now covers the Spark name, model ID, image, and image sizes, but not that environment value. Other occurrences of the slug exercise model catalog or resume inputs, not the Spark profile association.
That contradicts #9497's requirement to preserve every unique assertion. Negative LOC is not a simplification when it silently drops the sole executable contract for a selected runtime value.
Refactor direction
Move the one defaultModel.envValue assertion into the focused Spark profile case before deleting the broad duplicate. No helper or additional scenario is needed.
Expected result
Each behavior still has one focused owner, the PR remains roughly 1,619 net lines smaller, and the Spark model ID plus its runtime environment value cannot drift independently.
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite for the commit under review. Recommended E2E: None Manual-only E2E: 3 optional E2E recommendations
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com>
Resolved by ad44f19: the focused Spark profile owner now preserves the unique envValue assertion without restoring duplicate coverage.
jyaunches
left a comment
There was a problem hiding this comment.
LOC Reduction / Codebase Simplicity Review
The prior blocking finding is resolved at ad44f19a099da2509b813df66880e89d22d63adb.
The retained focused Spark profile case now preserves defaultModel.envValue === "qwen3.6-35b-a3b-nvfp4" beside the model ID, image, and sizing assertions. The fix adds one line without restoring the broad duplicate suite, so each audited behavior keeps one focused owner and the PR remains 1,619 net lines smaller.
This follow-up review is limited to the prior finding and the new-head delta. I found no replacement LOC-reduction or codebase-simplicity blocker.
prekshivyas
left a comment
There was a problem hiding this comment.
Approved at exact head ad44f19. Reviewed the duplicate-test removal and retained unique assertions; no unresolved review threads remain and all required checks pass.
Summary
Remove 60 duplicated test declarations across 18 audited clusters, plus one orphaned assertion that exercised only a test-local BuildKit helper. Focused owner files now hold each behavior contract, and the unique vLLM size and model assertions move to the focused profile test.
Related Issue
Closes #9497
Changes
test/detect-vllm-profile.test.ts.test/nemoclaw-start.test.tssize ceiling to its new line count.Type of Change
Quality Gates
DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpm testwas attempted and reached its 600-second local ceiling with unrelated macOS, service, network, and cross-suite failures. GitHub CI will provide the clean aggregate result.npm run docsbuilds without warnings (doc changes only)Documentation Review
no-docs-neededad44f19a0changes only tests andci/test-file-size-budget.json. The follow-up commit restores the Spark profile environment-value assertion and changes no user-facing product behavior or documentation contract.Signed-off-by: Rebecca Sliter 571084+rsliter@users.noreply.github.com
Summary by CodeRabbit
Tests
Chores