Skip to content

[Refactor] Add test-utils guardrails and convert remaining stragglers - #2279

Merged
edelauna merged 2 commits into
mainfrom
feature/test-utils-guardrails-26vxix3b5tqmz
Oct 31, 2026
Merged

edelauna merged 2 commits into
mainfrom
feature/test-utils-guardrails-26vxix3b5tqmz

Conversation

@zoomote2

@zoomote2 zoomote2 Bot commented Oct 31, 2026 •

Copy link
Copy Markdown
Contributor

​Created by Roomote. Follow up by mentioning @roomote, in the web UI, or in Discord.

Related GitHub Issue

N/A — internal test-infrastructure initiative (test-utils rollout, Phase 4 guardrails). No user-facing issue.

Description

Adds guardrails so the test-utils migration cannot regress, and converts the remaining stragglers so the guardrails land green. Rebased onto latest main and refreshed: conflicts resolved and new stragglers introduced since the original branch were converted.

Guardrails

  • src/eslint.config.mjs: a no-restricted-syntax rule blocks direct vi.clearAllMocks() / vitest.clearAllMocks() in the converted lanes (api/providers/__tests__, core/config/__tests__, services/code-index/**/__tests__, integrations/terminal/**/__tests__). The error message points to the shared clearAllMocks() helper in src/test-utils/reset. The rule shares one testResetHelperSelector const and repeats main's test path-traversal and API-provider-hygiene selectors in the lane blocks, because later no-restricted-syntax blocks replace earlier ones for the same files.
  • webview-ui/eslint.config.mjs: a no-restricted-imports rule blocks QueryClientProvider imports in src/components/**/__tests__/**/*.tsx, pointing to renderWithExtensionState. Hook specs (including .tsx ones, which pass a specific QueryClient to renderHook) and Playwright *.visual.fixture.tsx files are explicitly exempt.
  • .github/pull_request_template.md: adds a "Test Helpers" checklist item — shared helpers are only for mechanical duplication; test intent and scenario-specific mocks stay visible.

Straggler conversions (assertions and scenario mocks untouched)

  • CustomModesManager.spec.ts: 9 call sites moved to the shared clearAllMocks().
  • New on main since the original branch: github-copilot (3 sites), vscode-lm, io-intelligence, nanogpt, code-index-manager-registry, code-index-scope, code-index-status-manager, code-index-workspace-scope, embedder-validation-manager, processor-factories, vector-store-factory.
  • Webview hook specs useSelectedModel, useZooGatewayRouterModelsSync, and useOpenRouterModelProviders now use createTestQueryClient().
  • Webview component specs Poe, Moonshot, and LiteLLM render through renderWithExtensionState (with createTestQueryClient() where a test spies on cache invalidation); their ExtensionStateContext mocks spread the real module so the provider stays available. ProviderRouting uses the shared render from @/utils/test-utils (QueryClient + Tooltip only) because the extension-state provider's mount posts an extra message that would break an exact postMessage call-count assertion.

Test Procedure

  • Probe-tested both rules with temporary offending files: the extension rule fires in the api/providers and services/code-index lanes, and the webview rule fires for a hand-wrapped QueryClientProvider — each with the intended error message. Exempted hook specs and visual fixtures lint clean. Probes removed.
  • Focused Vitest runs for every touched spec: 702/702 extension tests, all touched webview suites green.
  • Full pnpm test green (extension 11,980 passed / 552 files; webview and all other packages green). One earlier full run flaked in __tests__/extension.spec.ts (untouched by this PR); it passes in isolation and on the full rerun.
  • pnpm lint and pnpm check-types green across all packages. src/eslint-suppressions.json unchanged.

Pre-Submission Checklist

  • Issue Linked: N/A — internal test-infrastructure initiative, no user-facing issue.
  • Scope: My changes are focused on the linked issue (one major feature/fix per PR).
  • Self-Review: I reviewed my own code with care.
  • Writing Style: The PR title and description use Simplified Technical English (ASD-STE100): short sentences, plain words, active voice, one idea per sentence, no contractions.
  • Testing: New and/or updated tests have been added to cover my changes (if applicable).
  • Test Helpers (test changes only): Shared test helpers (src/test-utils/, @/utils/test-utils) are used only for mechanical duplication; test intent and scenario-specific mocks remain visible.
  • Visual Snapshot (UI changes only): No user-visible UI change; no snapshot needed.
  • Documentation Impact: I checked if my changes need documentation updates (see "Documentation Updates" section below).
  • Contribution Guidelines: I read and agree to the Contributor Guidelines.

Visual Snapshots

N/A — ESLint configuration and test-only changes; no rendered surface changes.

Videos (interaction / animation only)

N/A.

Documentation Updates

  • No documentation updates are required.

Additional Notes

Supersedes #1206 (closed; GitHub cannot reopen it because the branch was force-pushed while closed). Same branch, rebased onto latest main, with the original guardrail intent preserved and the rule implementation adapted to compose with main's newer no-restricted-syntax blocks.

@coderabbitai

coderabbitai Bot commented Oct 31, 2026 •

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are limited based on label configuration.

🏷️ Required labels (at least one): 1
  • coderabbit-review-active

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

@zoomote2

zoomote2 Bot commented Oct 31, 2026 •

Copy link
Copy Markdown
Contributor Author

No code issues found. See session

Reviewed 22f1cf4

@github-actions

github-actions Bot commented Oct 31, 2026 •

Copy link
Copy Markdown
Contributor

Review status

This PR was opened by an automated account. A human maintainer must verify the change intent, provenance, and validation before merging.

Current step: The required review sequence passed. Remaining merge requirements apply.

Review-state labels are managed by this workflow; do not edit them manually. community-approved is managed the same way — do not add or remove it manually. It signals a fresh community code approval for the current head as an advisory priority only; maintainer review is still required.

@zoomote2

zoomote2 Bot commented Oct 31, 2026

Copy link
Copy Markdown
Contributor Author

Self-review summary (rebased refresh, supersedes #1206)

Reviewed the full branch diff against main (22 files, +217/−148). No behavior, scope, or assertion changes beyond the intended guardrails and helper conversions.

  • Conflict resolutions: src/eslint.config.mjs now shares one testResetHelperSelector const and repeats main's test path-traversal and API-provider-hygiene selectors inside the two guardrail lane blocks — later no-restricted-syntax blocks replace earlier ones for the same files, so this keeps both rule sets active (verified: no new lint errors repo-wide). PR template keeps main's reworded checklist with the Test Helpers item inserted. webview-ui/eslint.config.mjs and all four original spec conversions merged cleanly.
  • New stragglers converted: 11 extension specs and 5 webview specs that gained direct vi.clearAllMocks() or hand-wrapped QueryClientProvider on main since the original branch. Assertions and scenario mocks untouched throughout.
  • Webview rule scoping: .tsx hook specs and Playwright *.visual.fixture.tsx files are explicitly exempt — hooks pass a specific QueryClient to renderHook, and visual fixtures mount real providers inside stories. ProviderRouting deliberately uses the shared render (QueryClient + Tooltip only) rather than renderWithExtensionState, because the extension-state provider's mount posts an extra message that broke an exact postMessage call-count assertion.
  • Probes: both rules fired with the intended messages on temporary offending files; exempted files lint clean; probes removed.
  • Validation: 702/702 focused extension tests, all touched webview suites green, full pnpm test green (extension 11,980 / 552 files), pnpm lint and pnpm check-types green, src/eslint-suppressions.json unchanged. One earlier full run flaked in the untouched __tests__/extension.spec.ts; green in isolation and on rerun.

No changesets, no runtime or user-facing behavior change. Visual proof not applicable (ESLint config + test-only).

@codecov

codecov Bot commented Oct 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@edelauna
edelauna marked this pull request as ready for review October 31, 2026 14:50
@edelauna
edelauna enabled auto-merge October 31, 2026 14:50
@edelauna
edelauna added this pull request to the merge queue Oct 31, 2026
Merged via the queue into main with commit 102e3b1 Oct 31, 2026
51 of 54 checks passed
@edelauna
edelauna deleted the feature/test-utils-guardrails-26vxix3b5tqmz branch October 31, 2026 15:16
edelauna pushed a commit that referenced this pull request Nov 2, 2026
…#2279)

* refactor: add test-utils guardrails and convert remaining stragglers

Co-authored-by: Roomote <roomote@roomote.dev>

* refactor: convert new test-utils stragglers to shared helpers

---------

Co-authored-by: Roomote <roomote@roomote.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants