feat: add test coverage run with vitest + v8#364
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis pull request sets up code coverage reporting infrastructure for the project. It adds Vitest coverage configuration with V8 as the provider, introduces a test coverage script, updates the build system to emit linked sourcemaps, adds the coverage dependency, and removes the outdated coverage report file from version control. ChangesCode Coverage Setup
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
Will pick this up and address must-fix items on branch. Must fix:
Worth deciding: Notes:
|
…itignore COVERAGE.md
Address adversarial-review findings on the coverage harness:
- vitest.config.ts comment claimed cli-happy-dom but the script runs
--project browser. Reword to match reality (Playwright Chromium = V8
runtime) and document the Firefox/WebKit caveat (JSC/SpiderMonkey
don't expose V8's coverage protocol).
- Anchor the index-barrel exclude to packages/ only. The previous
**/index.ts pattern silently masked builds/{knockout,reference}/src/
index.ts — the ONLY src file in each build, containing real
configuration (Builder wiring, options, jsx.render). builds/* now
appears in the report again via source-map remap from
builds/*/src/**/*.ts.
- Drop hand-committed COVERAGE.md and gitignore it. The snapshot
rotted on every run that wasn't followed by a commit. Reintroduce
later when test:coverage deterministically rewrites it.
- Pin @vitest/coverage-v8 to exact 4.1.4 (drop the caret).
@vitest/coverage-v8 declares peerDependencies: { vitest: 4.1.4 }
exact; caret on both sides will warn on the next patch bump.
@vitest/browser and @vitest/browser-playwright keep their carets
because their own peer-deps allow it.
Considered but rejected: adding builds/*/dist/**/*.js to include.
That sweeps in browser.js / browser.min.js / common.js / index.js
(8 files across 2 builds) where only browser.min.js is actually
executed by tests. The minified bundle is also strictly worse to
instrument than the unminified packages/*/dist/**/*.js paths that
already cover the same code. Source-map remap from builds/*/src is
sufficient.
Adversarial review: two parallel subagents on this branch + a
verification pass on the proposed edits. Initial draft included
builds/*/dist/**/*.js; verification subagent flagged false-positive
0% rows for unexecuted bundles; rolled back.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
vitest.config.ts (1)
57-58:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winComment wording is now misleading for the coverage entrypoint.
Line 57 says “Additive CLI coverage,” but
test:coveragetargets only--project browser. Please reword to avoid implying this project contributes to that coverage run.Proposed wording tweak
- // Additive CLI coverage — happy-dom. - // Intent: prove the binding engine works in a JS DOM (SSR, headless, TUI adapters). + // Additional CLI project (happy-dom) for JS-DOM execution scenarios. + // Note: `bun run test:coverage` targets `--project browser`.🤖 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 `@vitest.config.ts` around lines 57 - 58, Update the misleading comment "Additive CLI coverage — happy-dom." near the browser project coverage entry so it doesn’t imply inclusion in the global test:coverage run; change it to clearly state that this entry only targets the browser project (e.g., mention "--project browser" or "browser-only coverage") and that it does not add to the overall CLI coverage aggregate for other projects or the test:coverage script.
🤖 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.
Outside diff comments:
In `@vitest.config.ts`:
- Around line 57-58: Update the misleading comment "Additive CLI coverage —
happy-dom." near the browser project coverage entry so it doesn’t imply
inclusion in the global test:coverage run; change it to clearly state that this
entry only targets the browser project (e.g., mention "--project browser" or
"browser-only coverage") and that it does not add to the overall CLI coverage
aggregate for other projects or the test:coverage script.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4fea9e2b-d6fe-4915-b2bf-c267f6c77701
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.gitignoreCOVERAGE.mdpackage.jsontools/build.tsvitest.config.ts
💤 Files with no reviewable changes (1)
- COVERAGE.md
CI lint failed: Biome wanted the include array inline. format --write fixes it. Behavior unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…in report
Local run with the prior config showed builds/{knockout,reference}
missing from coverage entirely — including builds/*/src/**/*.ts is
necessary but not sufficient because v8 only instruments executed
code, and the test runner loads the IIFE bundle, not src.
builds/*/dist/browser.min.js is what builds/knockout/helpers/
vitest-setup.js imports. v8 instruments the minified output;
source-map remap (enabled by tools/build.ts switch to
--sourcemap=linked earlier in this PR) surfaces the original
builds/*/src/index.ts lines in the report.
Other dist outputs (browser.js, index.cjs, index.mjs, common.js)
are alternate formats not loaded by the test runner; including
them would add 0%-coverage rows. Pin to browser.min.js explicitly.
Verified locally: builds/knockout/src/index.ts at 100% / builds/
reference/src/index.ts at 60% (jsx.render uncovered — pre-existing
gap exposed). Overall lines coverage 92.03% → 94.35%.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Runs vitest+v8 coverage on every PR and every push to main. Outputs: - Workflow run summary table (lines / statements / branches / functions with covered/total counts) for at-a-glance visibility. - coverage-data artifact (coverage-summary.json + lcov.info, ~220 KB, 30-day retention) — small enough that future closed-loop tooling can download a base commit's data to compute deltas. - coverage-html artifact (lcov-report HTML, ~5 MB, 7-day retention, main-only) for human download via the workflow run page. Open-loop only — no PR comment, no delta gate. Closed-loop dark-factory feedback (post deltas to the PR; gate regressions) is a follow-up; the first attempt at the gate logic shipped four blockers in adversarial review (gh run download flag, missing reporter file, PATCH body encoding, heredoc terminator indentation), so split out and ship the simple version first. Adversarial review: subagent on this branch verified the simple flow works in the Playwright container, container pin matches build-and-test.yml, permissions defaults are sufficient, and the artifact-namespace + concurrency-group combination doesn't collide. The closed-loop design + its known blockers are tracked as a follow-up issue. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Coverage workflow's summary step uses jq to format the run-summary table. The Playwright noble image ships without jq → exit 127. Install via apt-get before the step. Pre-merge verification gap: I claimed the workflow was verified locally. It wasn't run in a container that mirrors CI; the local run on macOS used the system jq. Fix here, but adversarial process should have spawned a container parity check. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
brianmhunt
left a comment
brianmhunt
left a comment
There was a problem hiding this comment.
👍 great idea, thanks @phillipc
2 participants
Summary
Recovering a lost feature: Adds a
bun run test:coveragescript that produces a v8 coverage report against the authoritative chromium browser project.Changes
package.json— newtest:coveragescript:bun run build && bunx vitest run --coverage --project browser. Adds@vitest/coverage-v8as a devDependency (pinned exact4.1.4— its declaredpeerDependencies: { vitest: 4.1.4 }is exact, so a caret on both sides would warn on the next patch bump).vitest.config.ts— adds a top-levelcoverageblock (v8 provider; reporters:text,text-summary,html,lcov,json-summary; outputs tocoverage/). Includespackages/*/dist/**/*.js(so v8 picks up actually-executed code) and bothpackages/*/src/**/*.tsandbuilds/*/src/**/*.ts(so source-map remapping surfaces TS line numbers in the report). Index-barrel exclude is anchored topackages/only sobuilds/{knockout,reference}/src/index.ts(the only real src file in each build) is not silently dropped.tools/build.ts— switches esbuild from--sourcemap=externalto--sourcemap=linked. Publisheddist/*.jsgain one trailing//# sourceMappingURL=...comment line so v8 can remap coverage back to the original TS sources..mapfiles were already shipped in npm tarballs (files: ["dist/"]), so no consumer-facing 404 risk..gitignore— addsCOVERAGE.md. The snapshot rots on every run that isn't followed by a commit; reintroduce later whentest:coveragedeterministically rewrites it.Verification
bun run test:coverageexits 0; report written tocoverage/.bun run test) is unchanged — coverage is opt-in via the new script.Notes
dist/*.jsartifacts gain one trailing//# sourceMappingURL=...comment line. No other build-output changes.Summary by CodeRabbit
New Features
Chores