Conversation
Coverage Report
File CoverageNo changed files found. |
|
Regarding the choice of coverage provider between istanbul and v8: I believe we should go for v8. Vitest lays out a good comparison of the two here. Pulling the most important points:
Another possible option is Bun coverage testing, but this does not seem to be suitably mature for our purposes yet. It does not seem to support workspace-level codecoverage reporting, like Vitest does, and it doesn't seem to support HTML code coverage reporting, which is a useful tool for seeing the current state of code coverage. |
There was a problem hiding this comment.
Pull Request Overview
This PR enables Vitest coverage reporting across most packages, sets up Vitest in TS/JS packages, and updates GitHub workflows to run and publish coverage.
- Adds
vitest.setup.tsandvitest.config.tsto integrate JSDOM, jest-dom matchers, and DOM cleanup. - Updates
package.jsonfiles to include Vitest, testing-library, and coverage dependencies. - Introduces GitHub Actions workflows for coverage runs and GitHub Pages deployment.
Reviewed Changes
Copilot reviewed 19 out of 28 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/storybook-addon-baseline-grid/vitest.setup.ts | Add DOM cleanup after each test |
| packages/storybook-addon-baseline-grid/vitest.config.ts | Configure Vitest with JSDOM and setup files |
| packages/storybook-addon-baseline-grid/package.json | Add Vitest and testing-library dependencies |
| packages/react/ssr/vitest.setup.ts | Add DOM cleanup after each test |
| packages/react/ssr/vitest.config.ts | Configure Vitest for SSR package |
| packages/react/ssr/package.json | Add Vitest and testing-library dependencies |
| packages/generator-ds/vitest.config.ts | Add Vitest configuration |
| packages/generator-ds/package.json | Add Vitest dependency |
| package.json | Add coverage script and Vitest dependencies |
| configs/storybook/vitest.config.ts | Add shared Vitest config for Storybook |
| configs/storybook/package.json | Add Vitest dependency |
| apps/react/demo/vitest.setup.ts | Add DOM cleanup after each test |
| apps/react/demo/vitest.config.ts | Configure Vitest with JSDOM and setup files |
| apps/react/demo/package.json | Add Vitest and testing-library dependencies |
| apps/react/boilerplate-vite/vitest.setup.ts | Add DOM cleanup and ResizeObserver mock |
| apps/react/boilerplate-vite/vitest.config.ts | Configure Vitest with JSDOM and setup files |
| apps/react/boilerplate-vite/package.json | Add Vitest and coverage-v8 dependency |
| .github/workflows/push.yml | Update CI to run coverage action |
| .github/workflows/coverage.yml | Add new coverage workflow |
Comments suppressed due to low confidence (2)
apps/react/boilerplate-vite/vitest.setup.ts:11
- Vitest's mocking API uses
vi.fn(), notvitest.fn(); this line will error. Replacevitest.fn()withvi.fn().
global.ResizeObserver = vitest.fn().mockImplementation(() => ({
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
advl
left a comment
There was a problem hiding this comment.
I approve the approach. Minor comments and questions related to some of your choices.
Also would something like this help ?
https://nx.dev/technologies/build-tools/vite/api/generators/vitest
| @@ -0,0 +1,52 @@ | |||
| name: "Coverage" | |||
| on: | |||
| pull_request: | |||
There was a problem hiding this comment.
I sense that these should be two workflows - maybe with a reusable bit ? (A reusable workflow)
This would allow for the conditional check further down which seems unidiomatic
There was a problem hiding this comment.
What do you propose the two workflows to be - the running of the coverage test and the creation of the coverage dashboard? Can you speak a bit to your thinking re: splitting those actions up into separate workflows? To my mind, coverage seems to be nicely covered by one workflow, which always runs the coverage test, and in some cases (merges to main) also uploads a coverage report - not sure if there's a case for making an additional reusable action unless there are foreseeable scenarios where we'd want to re-use it outside of this scope.
There was a problem hiding this comment.
My rationale is that this workflow runs on both
- pr
- push on main
and the second job (upload coverage pages) applies what is to me an undiomatic if
It would likely be better that we have two workflows
- pr (runs only test coverage)
- push (tests coverage and uploads artifact)
Maybe benefiting from a shared action
| pages: write | ||
| steps: | ||
| - name: Download coverage report | ||
| uses: actions/download-artifact@v4 |
There was a problem hiding this comment.
I wonder if when splitting the workflow in two we could avoid the step of uploading and downloading the artifact to simplify
| "@canonical/biome-config": "^0.9.0-experimental.12", | ||
| "@canonical/typescript-config-react": "^0.9.0-experimental.12", | ||
| "jsdom": "^26.0.0", | ||
| "vitest": "^3.0.9", |
There was a problem hiding this comment.
Please review S9 best practices - I believe vitest is already a default. I am also unsure of the side effects of pinning a version here
| "@biomejs/biome": "^1.9.4", | ||
| "@canonical/biome-config": "^0.9.0-experimental.12", | ||
| "@canonical/typescript-config-react": "^0.9.0-experimental.12", | ||
| "jsdom": "^26.0.0", |
There was a problem hiding this comment.
Doesn't necessarily need to be tackled here but : is there a way we could reuse or modularize vitest configs ?
There was a problem hiding this comment.
Similarly might be worth adding to the msw addon
| test: { | ||
| // include vite globals for terser test code | ||
| globals: true, | ||
| include: ["src/**/*.tests.ts", "src/**/*.tests.tsx"], |
There was a problem hiding this comment.
Minus tsx as this doesn't contain react code
| import { defineConfig } from 'vitest/config' | ||
|
|
||
| export default defineConfig({ | ||
| test: { |
There was a problem hiding this comment.
I wonder how this top level guest config would play out with Adam's Pr ?
| environment: "jsdom", | ||
| coverage: { | ||
| provider: "v8", | ||
| reporter: ["text", "json-summary", "json", "html", "lcov"], |
There was a problem hiding this comment.
Could you comment briefly on why these are sensible defaults ?
Done
Fixes WD-22631
TODO:
istanbulandv8. PR is currently using v8. The choice should be documented and added to testing/arch docs.QA
PR readiness check
Feature 🎁,Breaking Change 💣,Bug 🐛,Documentation 📝,Maintenance 🔨.package.json:check,check:fix, andtest.build.