Conversation
…#2970) Vitest could not run a single test in this repository. Since #2969 moved every suite to bun:test, invoking it produced "Failed to load url bun:test" and zero tests. The six test:vitest scripts, ten config files, four Stryker configs and the coverage plumbing were all dead weight that still had to be kept working. This removes them. Nothing here loses a working capability, because none of it worked: - Stryker's four configs all set testRunner "vitest" and were wired into no workflow; dev-docs/stryker.md documented a config deleted earlier. - No test script anywhere passes --coverage, so packages/*/coverage was never produced and post_coverage_comment downloaded nothing. - The SecureStore keyring/fallback split moves to the Bun-native scripts that already existed in packages/storage, emitting the same junit file. The ESLint swap is a tightening, not a loosening. @vitest/eslint-plugin identifies test blocks by import source and does not recognise bun:test, so all 17 of its rules had been silently inert since #2969. eslint-plugin-jest with globalPackage bun:test restores real enforcement: seven rules now run at error, which took fixing 28 genuine violations across 16 files. Five rules stay off because jest implements them more strictly than vitest ever did -- on the pre-#2969 tree, where the vitest plugin was live and lint was green at zero, the jest plugin found 728 violations in packages/core alone. That burn-down is #3129. The new lint:no-vitest guard fails on an import, a dependency (including an npm: alias), a config file, a lockfile entry, or a binary invocation in a manifest, workflow, shell script, Makefile or TOML -- while ignoring the word in prose.
…uard Open Code Review and an adversarial audit surfaced three classes of problem. Collateral damage. Replacing the vitest ESLint block also deleted two unrelated sibling blocks that ban self-imports in packages/core and packages/cli. Restored verbatim. A stale fsevents entry in the install-script allowlist is removed for the opposite reason: it entered the tree only via vite/rollup under vitest, so with Vitest gone the guard was right to flag it. Guard gaps. The guard missed a bare vitest command in a Makefile or workflow YAML, and false-positived on a comment containing 'vitest run'. It also reported every manifest violation as line 1 despite promising file:line:match. The test helper built a bash -c string from an environment-derived path and swallowed spawn errors whenever the exit code was non-zero, so a broken harness was indistinguishable from a real failure; it now passes arguments directly with no shell and throws on spawn failure or signal. Diagnostics. Several assertions rewritten for jest/no-conditional-expect had collapsed into expect(<boolean>).toBe(true), which prints only 'expected false to be true'. They now use toMatchObject or narrow-then-assert, so a failure shows the actual value. Also drops the misleading jest version: '29' setting -- the runner is bun:test, which has no Jest semver for the plugin to gate on.
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (9)
📝 WalkthroughWalkthroughChangesBun test-runner migration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
the
maintainer:e2e:ok
Contributor
WalkthroughBefore this PR, the repository's ongoing migration from Vitest to Bun's native test runner left several workspaces with retained but unused Release NotesBug Fixes
Tests
Documentation
Refactor
Chore
Changes
Magnitude🎯 5 (XXL) Related
Walkthrough generated by LLxprt PR Review. Planner issue: #2256 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| "test:bun": "bun ../../scripts/run_bun_tests.ts --workspace storage", | ||
| "test:ci": "bun ../../scripts/run_bun_tests.ts --workspace storage --junit junit.xml", | ||
| "test:vitest": "vitest run", | ||
| "typecheck": "tsc --noEmit", |
Contributor
There was a problem hiding this comment.
[bug/high] Root override logic is inverted: the script defaults to the real repository, not the override. This makes the temporary root guard (
NO_VITEST_ROOT) unusable for the behavioral tests added in this PR, and it also means CI always scans the whole tree even when an override is supplied.Bug:
resolve(NO_VITEST_ROOT ? ... : fallback)uses the override truthy/falsy test the wrong way round. Correct:const REPO_ROOT = process.env.NO_VITEST_ROOT ? resolve(...override...) : resolve(...fallback...);.
Contributor
OpenCodeReview — automatic reviews suspendedAutomatic OCR reviews are suspended for this PR after 2 of 2 automatic reviews. To get more reviews you can:
|
…ere widening
Removing the vitest devDependency removed the looser declarations that were
masking two type-aware lint errors in the CLI test suite. Under bun-types the
callback parameters of describe.each and it.each are 'any', so:
- useKeypress.test.tsx: 'useKitty' in a conditional tripped
strict-boolean-expressions.
- KeypressContext.test.tsx: 'writeSequence' returned 'any', which made
act(() => writeSequence(...)) look like a floating promise even though the
table's implementations all return void.
Both are annotated with the types the table already supplies. No behavior
changes; the surrounding reformatting is prettier reflowing the wrapped calls.
Caught by the full-tree lint:ci run that this PR triggers (a workflow change
forces lint full rather than scoped). Local runs missed it because the working
node_modules still contained vitest.
Removing the vitest devDependency broke npm run typecheck with ~500 errors:
error TS2593: Cannot find name 'describe'.
error TS2304: Cannot find name 'beforeEach'.
Every tsconfig sets types: ['node', 'bun-types/test'], and bun-types/test.d.ts
declares only the bun:test MODULE. The ambient globals live in a separate file,
bun-types/test-globals.d.ts, which that entry never loads. Vitest's presence was
masking the gap, so removing it exposed a latent misconfiguration rather than
creating one. All 14 tsconfigs now load both.
Nine files under packages/cli/src/config still carried a literal
/// <reference types="vitest/globals" /> directive. Those are removed, and the
guard is extended to catch triple-slash references -- it previously matched only
import and require forms, which is why it passed over them. Three positive cases
and a prose negative control cover the new detection.
useShellHistory.test.ts needed a real fix rather than a type widening: bun's
expect() infers its matcher type from the received value, so
'let command: string | null = null' narrowed to the null literal and .toBe('cmd2')
had no matching overload. The declarations now use a definite-assignment
assertion, keeping the declared type and the assertions unchanged.
This was invisible locally because the working node_modules still contained
vitest; it only appears on a clean install from the lockfile.
Ten findings from the PR review, all on scripts/check-no-vitest.ts. The guard documented 'no false positives on prose' but applied the import regex to raw file content, so a commented-out example or a migration note in a string literal failed CI. Matches are now skipped when they fall inside a comment, string or template literal. The masking computes character spans rather than deleting text, so the reported file:line stays accurate, and template interpolation is still treated as real code. Triple-slash references are deliberately exempt from the masking: that directive is a comment by syntax and must keep failing. Detection gaps closed: the binary patterns caught 'npx vitest' but not 'npm run vitest', 'pnpm exec vitest', 'yarn run vitest', 'bun run vitest' or 'bunx --bun vitest', all of which are ordinary ways to invoke it from a workflow or Makefile. The TOML pattern also required a file extension, missing a bare preload of './vitest', and lacked a word boundary, so an unrelated 'avitest-shim.ts' was reported as a violation. parseManifest swallowed JSON parse errors and returned undefined, which made scanManifest skip the file: a malformed package.json declaring vitest passed silently. It now reports through the operational-error path and fails. Seventeen behavioral cases added, covering each new detection and each new no-false-positive case. One review finding is rejected: the NO_VITEST_ROOT override was reported as an inverted ternary. It is not inverted, and fixture trees demonstrably resolve to the override while the real repository resolves to the fallback.
Collaborator
Author
Review findings addressed in e30ed7eTen of the eleven findings are fixed; one is rejected with evidence. False positives on comments and string literals (4 threads). Correct, and it contradicted the guard's own stated design principle. The import scan now skips matches falling inside a line comment, block comment, string or template literal. The masking computes character spans instead of deleting text, so the reported Missing binary invocation forms (2 threads). Correct. TOML word boundary (2 threads). Correct — TOML bare reference. Correct — a parseManifest swallowing JSON errors. Correct, and the most serious of the set: a malformed Seventeen behavioral cases were added, one per new detection and one per new no-false-positive case. The guard test suite is now 62 cases. Rejected: the
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/package.json (1)
541-541: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftAdd a required mutation-testing gate
No replacement exists for
test:mutation. Add a Bun-compatible mutation script and invoke it from a required CI job before merge.🤖 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 `@packages/core/package.json` at line 541, Add a Bun-compatible test:mutation script in package.json, then update the CI workflow to run it in a required pre-merge job. Ensure the job invokes the package script and is configured as a mandatory status check before merging.
🧹 Nitpick comments (3)
scripts/tests/no-vitest-guard.test.ts (1)
896-905: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicated real-repository scan.
This test calls
runScriptRealRepo(0)with the same assertion as the test at Lines 56-60. It runs a second full-repository walk and adds up to 90 seconds without new coverage. The comment already states that the earlier test proves the contract. Either delete this test, or replace it with a fixture that reproduces the self-excluded filenames and asserts they are ignored.♻️ Proposed fixture-based replacement
describe('self-exclusion', () => { - it('does not flag its own source or test fixtures', () => { - const { code } = runScriptRealRepo(0); - expect(code).toBe(0); - }, 90_000); + it('ignores the guard source and its helper files', () => { + const { code } = withFixture(({ root, write }) => { + write( + 'scripts/check-no-vitest.ts', + "import { it } from 'vitest';\nexport const x = 1;\n", + ); + write( + 'scripts/tests/no-vitest-guard-helpers.ts', + "import { it } from 'vitest';\nexport const y = 1;\n", + ); + return runScript(root, 0); + }); + expect(code).toBe(0); + }); });🤖 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 `@scripts/tests/no-vitest-guard.test.ts` around lines 896 - 905, Remove the duplicate self-exclusion test that calls runScriptRealRepo(0), since the earlier real-repository test already covers this behavior. If retaining coverage in the describe('self-exclusion') block, replace the full-repository scan with a focused fixture containing self-excluded filenames and assert the guard ignores them.scripts/check-no-vitest.ts (1)
554-594: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueHoist the global-flagged regexes to module scope.
scanCodeFileruns for every code file in the tree. Each call compiles two newRegExpobjects from.source. Declare the global variants once at module scope and resetlastIndexbefore each scan. This removes per-file compilation and keeps the two pattern definitions adjacent to their non-global documentation.♻️ Proposed refactor
+const VITEST_IMPORT_PATTERN_G = new RegExp(VITEST_IMPORT_PATTERN.source, 'g'); +const VITEST_TRIPLE_SLASH_REFERENCE_G = new RegExp( + VITEST_TRIPLE_SLASH_REFERENCE.source, + 'gm', +); + function scanCodeFile(filePath: string, content: string): Violation[] { const violations: Violation[] = []; const maskedSpans = computeMaskedSpans(content); - const globalPattern = new RegExp(VITEST_IMPORT_PATTERN.source, 'g'); + const globalPattern = VITEST_IMPORT_PATTERN_G; + globalPattern.lastIndex = 0;🤖 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 `@scripts/check-no-vitest.ts` around lines 554 - 594, Move the global RegExp variants currently created inside scanCodeFile—globalPattern and refPattern—to module scope adjacent to their corresponding VITEST pattern definitions. Reuse these shared regexes in scanCodeFile and reset each regex’s lastIndex to 0 before its scan loop, preserving the existing matching and empty-match safeguards.package.json (1)
240-240: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin
eslint-plugin-jestto28.14.0. This version supports ESLint9.29.0and flat configuration.🤖 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 `@package.json` at line 240, Update the eslint-plugin-jest dependency declaration in package.json from the range to the exact version 28.14.0, preserving the existing dependency key and JSON formatting.
🤖 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
`@packages/agents/src/compression/__tests__/compressionPrefixStability.test.ts`:
- Around line 473-474: Update the assertion in the compression prefix stability
test to reject any preserved response when its matching call is absent: require
responseIndex to be invalid or require callIndex to be present and precede
responseIndex. Preserve acceptance for responses with a valid preceding call and
for entries with no response.
In `@packages/tools/src/tools/ast-edit/__tests__/ast-edit-force-flag.test.ts`:
- Line 61: Update the test assertion around readFileSync to capture the thrown
error, then assert its code with toMatchObject({ code: 'ENOENT' }); do not pass
expect.objectContaining to toThrow, since Bun does not support that matcher
argument.
---
Outside diff comments:
In `@packages/core/package.json`:
- Line 541: Add a Bun-compatible test:mutation script in package.json, then
update the CI workflow to run it in a required pre-merge job. Ensure the job
invokes the package script and is configured as a mandatory status check before
merging.
---
Nitpick comments:
In `@package.json`:
- Line 240: Update the eslint-plugin-jest dependency declaration in package.json
from the range to the exact version 28.14.0, preserving the existing dependency
key and JSON formatting.
In `@scripts/check-no-vitest.ts`:
- Around line 554-594: Move the global RegExp variants currently created inside
scanCodeFile—globalPattern and refPattern—to module scope adjacent to their
corresponding VITEST pattern definitions. Reuse these shared regexes in
scanCodeFile and reset each regex’s lastIndex to 0 before its scan loop,
preserving the existing matching and empty-match safeguards.
In `@scripts/tests/no-vitest-guard.test.ts`:
- Around line 896-905: Remove the duplicate self-exclusion test that calls
runScriptRealRepo(0), since the earlier real-repository test already covers this
behavior. If retaining coverage in the describe('self-exclusion') block, replace
the full-repository scan with a focused fixture containing self-excluded
filenames and assert the guard ignores them.
🪄 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: Pro Plus
Run ID: e17a340b-27c5-4f4d-a7c1-9ac3fbff69bd
⛔ Files ignored due to path filters (11)
bun.lockis excluded by!**/*.lock,!**/*.lockdev-docs/PLAN.mdis excluded by!dev-docs/**dev-docs/REGRESSION_TESTS.mdis excluded by!dev-docs/**dev-docs/RULES.mdis excluded by!dev-docs/**dev-docs/bun.mdis excluded by!dev-docs/**dev-docs/npm.mdis excluded by!dev-docs/**dev-docs/schema-guide.mdis excluded by!dev-docs/**dev-docs/stryker.mdis excluded by!dev-docs/**dev-docs/test-runner-inventory.mdis excluded by!dev-docs/**package-lock.jsonis excluded by!**/package-lock.json,!package-lock.jsonproject-plans/issue2970-remove-vitest-escape-hatches.mdis excluded by!project-plans/**
📒 Files selected for processing (99)
.github/actions/post-coverage-comment/action.yml.github/scripts/issue-planner.ts.github/workflows/ci.yml.github/workflows/nightly.ymlCONTRIBUTING.mddocs/hooks/writing-hooks.mdeslint.config.jsevals/README.mdpackage.jsonpackages/a2a-server/package.jsonpackages/a2a-server/src/agent/task.factory-migration.integration.test.tspackages/agents/src/compression/__tests__/compressionPrefixStability.test.tspackages/agents/src/core/client.editor-context.test.tspackages/auth/package.jsonpackages/auth/tsconfig.jsonpackages/auth/vitest.config.tspackages/cli/package.jsonpackages/cli/run-bun-tests.tspackages/cli/src/config/settings-validation.test.tspackages/cli/src/config/settings.part2.test.tspackages/cli/src/config/settings.part3.test.tspackages/cli/src/config/settings.part4.test.tspackages/cli/src/config/settings.part5.test.tspackages/cli/src/config/settings.part6.test.tspackages/cli/src/config/settings.part7.test.tspackages/cli/src/config/settings.test.tspackages/cli/src/config/settingsLoader.trust.test.tspackages/cli/src/ui/contexts/KeypressContext.test.tsxpackages/cli/src/ui/hooks/useKeypress.test.tsxpackages/cli/src/ui/hooks/useShellHistory.test.tspackages/cli/tsconfig.base.jsonpackages/cli/tsconfig.jsonpackages/core/package.jsonpackages/core/src/integration-tests/provider-settings-integration.test.tspackages/core/stryker.conf.jsonpackages/core/tsconfig.jsonpackages/core/vitest.config.tspackages/ide-integration/package.jsonpackages/ide-integration/tsconfig.jsonpackages/lsp/package.jsonpackages/lsp/vitest.config.tspackages/mcp/package.jsonpackages/mcp/stryker.conf.jsonpackages/mcp/tsconfig.jsonpackages/mcp/vitest.config.tspackages/policy/package.jsonpackages/policy/tsconfig.jsonpackages/providers/package.jsonpackages/providers/src/anthropic/AnthropicMessageNormalizer.anchorCache.test.tspackages/providers/src/auth/__tests__/oauth-manager.issue913.spec.tspackages/providers/src/auth/proxy/__tests__/oauth-exchange.spec.tspackages/providers/src/auth/proxy/__tests__/oauth-poll.spec.tspackages/providers/src/package-boundary.test.tspackages/providers/stryker.conf.jsonpackages/providers/stryker.seam.conf.jsonpackages/providers/tsconfig.jsonpackages/providers/vitest.config.tspackages/settings/package.jsonpackages/settings/tsconfig.jsonpackages/storage/package.jsonpackages/storage/test-setup-bun-session-reset.tspackages/storage/tsconfig.jsonpackages/storage/vitest.config.fallback-behavior.tspackages/storage/vitest.config.native-keyring.tspackages/storage/vitest.config.tspackages/telemetry/package.jsonpackages/telemetry/tsconfig.jsonpackages/test-utils/package.jsonpackages/test-utils/src/quota-guard-vitest-integration.test.tspackages/test-utils/src/test-rig.test.tspackages/tools/package.jsonpackages/tools/src/__tests__/apply-patch-ax.bun.test.tspackages/tools/src/__tests__/package-boundary.test.tspackages/tools/src/__tests__/shell-tool.test.tspackages/tools/src/__tests__/todo-contract.test-d.tspackages/tools/src/__tests__/tool-key-storage.test.tspackages/tools/src/__tests__/tool-registry-mcp-lazy.test.tspackages/tools/src/tools/ast-edit/__tests__/ast-edit-force-flag.test.tspackages/tools/src/tools/github-ops.test.tspackages/tools/src/tools/line-range-tools-issue3036.bun.test.tspackages/tools/tsconfig.jsonpackages/tools/vitest.config.tspackages/vscode-ide-companion/package.jsonpackages/vscode-ide-companion/tsconfig.jsonscripts/affected-lint-targets.tsscripts/affected-test-shards.tsscripts/check-no-vitest.tsscripts/check-settings-boundary.tsscripts/genai-enclave/config.tsscripts/tests/affected-test-shards.test.tsscripts/tests/bun-workspaces.test.tsscripts/tests/ci-secure-store-workflow.test.tsscripts/tests/genai-enclave-adversarial.test.tsscripts/tests/no-vitest-guard-helpers.tsscripts/tests/no-vitest-guard.test.tsscripts/tests/vitest-coverage.test.tstsconfig.jsontsconfig.scripts.jsonvitest.coverage.ts
💤 Files with no reviewable changes (28)
- packages/tools/package.json
- packages/cli/src/config/settings.part6.test.ts
- packages/lsp/vitest.config.ts
- packages/core/vitest.config.ts
- packages/providers/stryker.seam.conf.json
- packages/cli/src/config/settings.part5.test.ts
- packages/cli/src/config/settings.part2.test.ts
- packages/cli/src/config/settings.test.ts
- packages/mcp/stryker.conf.json
- packages/storage/vitest.config.fallback-behavior.ts
- packages/cli/src/config/settingsLoader.trust.test.ts
- packages/test-utils/src/quota-guard-vitest-integration.test.ts
- packages/storage/vitest.config.ts
- packages/providers/stryker.conf.json
- packages/core/stryker.conf.json
- packages/cli/src/config/settings-validation.test.ts
- packages/cli/src/config/settings.part4.test.ts
- .github/actions/post-coverage-comment/action.yml
- packages/storage/vitest.config.native-keyring.ts
- packages/providers/vitest.config.ts
- packages/auth/vitest.config.ts
- packages/cli/src/config/settings.part7.test.ts
- scripts/tests/vitest-coverage.test.ts
- packages/mcp/vitest.config.ts
- vitest.coverage.ts
- packages/tools/vitest.config.ts
- packages/cli/src/config/settings.part3.test.ts
- scripts/affected-lint-targets.ts
The pull_request workflows did not schedule for the previous two pushes, although the pull_request_target ones did.
# Conflicts: # packages/cli/run-bun-tests.ts
Review point: the file-not-created check matched /ENOENT/ against the error
message, but the message wording is not part of Node's contract -- the code
property is. Capturing the error and matching on { code: 'ENOENT' } asserts the
documented surface, and still fails loudly if the file was created, because the
captured error is then undefined.
profileApplication.lb.contextWindowTimeout.test.ts arrived from main after this PR enabled jest/no-conditional-expect at error, and it wraps its assertion in the type guard it already asserted one line above. Converted to an early return rather than folding the guard into the assertion: it keeps expect out of a branch, still narrows the union for the compiler, and preserves the failure message, which the boolean-collapse form loses.
Merged from main after this PR enabled jest/require-to-throw-message at error. Writing to a frozen object throws a TypeError by spec, so the class is the stable assertion; the message wording differs between engines.
6 tasks
# Conflicts: # bun.lock
scripts/tests/issue-2978-launcher-exec-bit.test.ts arrived from main (#3086) importing the vitest specifier, which the lint:no-vitest guard this PR adds rejects. This is exactly the regression the guard exists to prevent, and it caught it on the merge. Only the import changed. The file's one failing assertion (git ls-files returning empty) fails identically with main's version in the same environment: execFileSync does not reliably capture a child's stdout under bun test here, which is the same limitation that made the guard's own test helper write to a file instead of a pipe. Not introduced by this change.
11 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TLDR
Removes Vitest from the repository and adds a CI guard so it cannot come back.
The headline fact is that Vitest could not run a single test here. Since #2969 moved every suite to
bun:test, invoking it produces:So the six
test:vitestscripts, ten config files, four Stryker configs and the coverage plumbing were not a fallback — they were a second test runner the project had to keep working, that had already stopped working. This deletes them, along with 28 dependency entries across every manifest and both lockfiles.The one thing reviewers should look at: the ESLint change is a tightening, not the loosening it looks like. Five rules move to
off, but@vitest/eslint-pluginidentifies test blocks by import source and does not recognisebun:test, so all 17 of its rules had been silently inert since #2969. Effective enforcement goes from 0 rules to 7, which required fixing 28 genuine violations. Details and evidence below.Fixes #2970. Terminal sub-issue of #2578 — when this merges, Vitest is gone.
Dive Deeper
Nothing here loses a working capability, because none of it worked. The issue's hard requirement is zero Vitest, with any genuinely irreplaceable capability treated as a blocker to resolve rather than a reason to keep a parallel runner. Each candidate was checked rather than assumed:
testRunner: "vitest", three also pinconfigFile: "vitest.config.ts".git grep test:mutation .github/returns nothing, so it was wired into no workflow, anddev-docs/stryker.mddocumentedpackages/cli/vitest.config.mutation.ts, deleted by an earlier slice. Removed; re-establishing it on Bun is a separate concern.@vitest/coverage-v8was the only producer, and no script anywhere passes--coverage, sopackages/*/coveragewas never written. CI's own comments already said cli and core "no longer upload a Vitest coverage artifact", and both downloads werecontinue-on-errorreporting "N/A". The upload step andpost_coverage_commentwere moving nothing.test:secure-store:keyring/test:secure-store:fallbackscripts that already existed inpackages/storage, emitting the samejunit.secure-store.xmlthe reporter step consumes. The matrix axis changes fromtest-configtotest-script.The ESLint swap, with evidence.
@vitest/eslint-pluginis replaced byeslint-plugin-jestwithsettings.jest.globalPackage: 'bun:test'. I proved the old plugin was inert by holding the file body and rules constant and changing only the import:Seven rules now run at
error:expect-expect,no-conditional-expect,no-identical-title,valid-describe-callback,valid-title,require-to-throw-message, andmax-nested-describe. Getting there meant fixing 28 real violations across 16 files.vitest/no-import-node-testbecomes ano-restricted-importsentry banningnode:test. Two rules encode Vitest-runner-only concepts and are genuinely inapplicable.Five rules stay
offwith their options preserved so re-enabling is a one-word change. The reason is thateslint-plugin-jestimplements them more strictly than@vitest/eslint-plugindid — measured directly: on the tree immediately before #2969 (a805a219f), where every test still importedvitest, the vitest plugin was live aterrorand lint was green at zero, the jest plugin reported 728 violations inpackages/corealone. Burning down the ~4,183 repo-wide is tracked in #3129, which flagsvalid-expectas highest value since an un-awaited async assertion silently passes.The guard.
scripts/check-no-vitest.tsfails on an import (including a multi-line dynamic one), a dependency entry (including annpm:vitest@…alias, which has no forbidden key), a config file (vitest.config.*,vitest.workspace.*, or avite.config.*carrying atestblock), a lockfile entry, or a binary invocation in a manifest, workflow YAML, shell script, Makefile or TOML — while ignoring the word in prose or a comment.Most of that breadth exists because an adversarial audit broke the first version six ways in ten minutes, including the two most likely real-world paths: a
./node_modules/.bin/vitestpackage script, andnpx vitest runin a workflow YAML — which is exactly where this PR is deleting Vitest invocations from.Three things the issue did not list but that break without a fix:
packages/providers/src/package-boundary.test.tsassertedvitest.config.tsexists (inverted to assert absence, so it still fails loudly if Vitest returns);scripts/affected-lint-targets.tsandaffected-test-shards.tslistedvitest.coverage.tsas a shard trigger; andtsconfig.scripts.jsonis an explicit allowlist, so the new guard files had to be added there or they would not be typechecked at all.fseventsis removed from the install-script allowlist inscripts/tests/bun-workspaces.test.tsfor the opposite reason to everything else here: it entered the tree only as an optional dependency ofvite/rollup, pulled in byvitest. With Vitest gone nothing depends on it, and that guard correctly flagged the stale entry.Reviewer Test Plan
Worth exercising directly:
Try to smuggle Vitest back in. Each of these should fail
npm run lint:no-vitest, and I would genuinely like to know if you find a seventh way:Then confirm it does not fire on a comment containing "vitest run", or on a file named
vitest.config.md.Check the ESLint claim yourself. Take any test file, flip its import from
bun:testtovitest, and watch rules that were silent start firing under the old plugin. That is the whole basis for calling this a tightening.Confirm no test was weakened. The 28 lint fixes are the riskiest part, particularly the 19
no-conditional-expectrestructurings, where turning a conditional assertion unconditional can quietly change what is asserted. They are all either logically equivalent or strictly stronger — e.g. baretoThrow()becametoThrow('Cache-anchor seq must be a positive integer: got 0'), and four assertion-freeawaitcalls inprovider-settings-integration.test.tsbecame real assertions.Testing Matrix
Verified on macOS: format, lint, eslint-guard, no-vitest guard, typecheck, build,
test:scripts, the workspace suite, and the CLI smoke test. Linux and Windows are left to CI.Known flaky and unrelated: under heavy parallel load the
packages/agentsAPI specs and onetest-utilsfile time out — a different file each run, all passing in isolation (agentsworkspace alone exits 0;test-utils12/12). None are touched by this PR. This matches the load-dependent flakiness #3122 documented for the agents workspace.Linked issues / bugs
Fixes #2970
Terminal sub-issue of #2578. Depends on #2969, merged as
18108c62c.Follow-up: #3129 (enable the five deferred
eslint-plugin-jestrules).Summary by CodeRabbit
New Features
Documentation
Chores