Repository navigation
ci: fail when a checked-in test file has no runner - #33080
Merged
Merged
Conversation
Two node:test files under .github/actions lost their runner when #31792 trimmed cloud pretest, and the two monaco .bench.ts budgets were never run by any CI job. Wire both back in and add a static check that every test-like file is collected by a declared runner or imported by a collected entry (the src .spec.* suite-module convention).
…dits Type-only imports are now dropped with the TypeScript parser, so a type alias before a runtime re-export no longer hides it. UNIT_EXCLUDE files must be named by a pr.yml or unit-tests.yml step. A cloud-only diff that adds a test or edits a runner config now runs the static job. The monaco benchmarks stay manual (pnpm test:perf:benchmarks) until a CI runner proves their wall-clock budgets.
The unit dependency graph keeps every plausible candidate so it can invalidate conservatively, which cannot prove a suite runs: an import of ./case.spec counted both case.spec.ts and case.spec.tsx. Resolve with Vite's extension order instead.
Imports used only as types are erased without a type modifier, so parsing for type-only syntax still followed them. Derive edges from ts.transpileModule output, which erases what Vitest's transform erases, and only transpile files that can reach an uncollected candidate.
The agent-status benchmark config inherited the unit projects, whose includes override its own, so it ran the whole unit suite and never the benchmark. Clear projects as the performance config does. Route every cloud code file to the static job, since content-detected tests can have any name.
Contributor
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 4 minutes. View limit details
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. Comment |
1 task done
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
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.
ELI5
Some checked-in test files were never run by anything, so they could break without anyone noticing. This change wires those files back into a runner. It also adds a CI check that fails whenever a test file exists that no runner picks up.
What Changed
Before: four test files and one benchmark config had no working runner.
.github/actions/cloud-sql-rollout-lease/{action-contract,storage-lease}.test.mjs: 16node:testcases. Move Relay deployment configuration into private cloud operations #31792 dropped them from cloudpretest.src/renderer/src/components/editor/monaco-content-sync{,.undo-retention}.bench.ts: Vitest benchmark files. Nothing ranvitest bench, andvitest runrefuses thebenchfixture.config/vitest.agent-status-benchmark.config.ts: it inherited the unitprojects, whose includes override its own. So it ran all 13.5k unit files and never the benchmark.The
src/**/*.spec.ts(x)files, such as theAutomationsPage.*.spec.tsxcases, already run. They are suite modules that a*.test.*entry imports (for example,AutomationsPage.behavior.test.tsxruns all 35 of those cases). So this change does not add*.spec.*toUNIT_INCLUDE; doing so would run each suite twice and without its entry's setup.After:
test:runner. The glob is quoted so it also works undercmd.exe.pnpm test:perf:benchmarks(vitest bench --runwithbenchmark.includein the performance config) runs the monaco benchmarks. They stay manual, because their wall-clock budgets have never run on a CI runner.projects, so it now selects only its file.node config/scripts/check-test-file-runners.mjs(also inpnpm lint). It finds every tracked file that is test-like, meaning a test-style name or a test-framework import plus a test registration. It fails unless a runner in its table collects the file, or a collected file imports it at runtime. Details:ts.transpileModuleoutput. That erases type-only and unused imports the same way Vitest's transform does.UNIT_EXCLUDE. Each excluded file must be named inpr.ymlorunit-tests.yml.package.jsonnow run the static job (isCloudTestRunnerInput). Without this, an orphaned cloud test could land green.Why
A test that no runner collects passes vacuously, and nothing reports it. The check uses a declared runner table plus a runtime-import walk. The alternatives were weaker or too slow:
*.spec.*to the include would double-run the suite modules.vitest listfor every config is too slow, and needs mobile and cloud installs, which the static job doesn't have.ci-unit-dependency-graphwas tried first. It deliberately keeps every plausible resolution candidate and type-only edge, so it cannot prove that a suite runs.Linked Issue
N/A. This is an internal CI gap found during review, with no issue filed.
Visual Proof
N/A: CI-only change, no UI.
Testing
I manually tested these changes locally
Automated tests added/updated, or explained why not below
node config/scripts/check-test-file-runners.mjspasses on this branch, in about 15–20s under load. With the new runner rows removed, it lists exactly the four orphaned files above.config/scripts/check-test-file-runners.test.mjscovers:UNIT_EXCLUDEwithwithin;type,typeof import()and type-position-only imports don't count; competing.ts/.tsxtwins);node --teston the lease tests passes 16/16.cloud pnpm test:runnerpasses 32.pnpm test:perf:benchmarkspasses. Two adjustments were needed:arrayBuffersdelta under 1 MiB instead of exactly 0. I observed −45 bytes, against 104,857,529 bytes for the undoable arm.vitest list --filesOnly --config config/vitest.agent-status-benchmark.config.tsnow lists only the benchmark.The full static job passes locally (lint, format, anti-slop, code-quality, tc, ratchets, generators). So do pr-code-change-scope, pr-preflight-gates and the other routing tests.
Platforms: macOS locally. The glob quoting was checked for sh and
cmd.exe.Review
Agent skill upstream boundary
Notes
.github/actions/cloud-sql-rollout-leaseis no longer used by any workflow here. It may be worth removing separately.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)