fix(test): remove the duplicate WSL spy from the connect harness - #9019
fix(test): remove the duplicate WSL spy from the connect harness#9019Dreamstick9 wants to merge 1 commit into
Conversation
The connect harness installs two spies on platform.isWsl. The explicit options.isWsl spy from NVIDIA#8951 runs first, then the platform pin from NVIDIA#8984 replaces it and delegates to the captured binding, so a case that passes isWsl: true resolves to the environment instead of the option. "repairs a WSL Ollama route without requiring an auth proxy token" then takes the non-WSL branch and exits 1. Remove the pin. The explicit option supersedes it: it states the WSL decision per case instead of inferring one from the host, and it already keeps the case host-independent, which is what the pin was for. All seven suites that use the harness pass. Signed-off-by: Kushagar Garg <dreamstick909@gmail.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 with no reviewable changes (1)
📝 WalkthroughWalkthroughThe test harness removes the Changes
Estimated code review effort: 1 (Trivial) | ~2 minutes Mergeability Score: ⚪ Minimal · up to This localized test-harness change preserves the explicit per-case WSL behavior, and no actionable merge-blocking risk remains beyond normal checks and review. Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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 This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
|
PR #9013 supersedes this change. It keeps the platform pin and puts it behind the explicit |
Summary
test/support/connect-flow-test-harness.tsinstalls two spies onplatform.isWsl. The explicitoptions.isWslspy added in #8951 runs first, then the platform pin added in #8984 replaces it and delegates to the binding captured just before, so a case that passesisWsl: trueresolves against the environment instead of the option. Onmaintoday,repairs a WSL Ollama route without requiring an auth proxy tokentakes the non-WSL branch, calls the auth-proxy probe, and exits 1. This change removes the pin and keeps the explicit option.Changes
test/support/connect-flow-test-harness.ts: remove theplatform.isWslpin and its now-unusedWslDetectionOptionsimport. The explicitoptions.isWslsupersedes it — it states the WSL decision per case instead of inferring one from the host, and it already delivers the host independence the pin was added for.Type of Change
Quality Gates
repairs a WSL Ollama route without requiring an auth proxy token, is the regression test; it fails onmainand passes here.Documentation Writer Review
no-docs-neededVerification
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 unavailablenpx vitest run --project cliover all seven suites that use the harness → 92 passed; the same command on unmodifiedmain→ 1 failed, 91 passed.npm run typecheck:cliclean.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: not applicable; the change removes one spy from one test-support file, validated by every suite that consumes it.npm run docsbuilds without warnings (doc changes only)Note on an unrelated red check:
static-checksfails on this branch throughsource-architecture, reportingsrc/lib/onboard: root files fell from 309 to 308. Lower the limit.That reproduces on unmodifiedmainwith no changes applied, so it is a stale ratchet inci/source-architecture-budget.jsonrather than anything in this diff. I left it alone to keep this change to one concern; happy to send the one-line ratchet update separately if that helps.Signed-off-by: Kushagar Garg dreamstick909@gmail.com
Summary by CodeRabbit