-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
Add One Double Zero as coverage provider #15356
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for jestjs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
a294926
to
6f9e982
Compare
aef6c8b
to
436bf7f
Compare
1fe4bce
to
8973217
Compare
Another interesting test is using jest to check the coverage of the jest project itself, with every
$ yarn jest --coverage --coverageProvider=babel
------------------------------------------|---------|----------|---------|---------|---------------------------------------------------------------------------------------------------------------------------------------
File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
------------------------------------------|---------|----------|---------|---------|---------------------------------------------------------------------------------------------------------------------------------------
All files | 68.51 | 65.55 | 65.33 | 68.54 |
Test Suites: 1 failed, 470 passed, 471 total
Tests: 1 failed, 51 skipped, 5132 passed, 5184 total
Snapshots: 1766 passed, 1766 total
Time: 96.649 s
Ran all test suites in 15 projects.
$ yarn jest --coverage --coverageProvider=v8
------------------------------------------|---------|----------|---------|---------|---------------------------------------------------------------------------------------------------------------------------------------
File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
------------------------------------------|---------|----------|---------|---------|---------------------------------------------------------------------------------------------------------------------------------------
All files | 68.33 | 86.69 | 75.97 | 68.33 |
Test Suites: 1 failed, 470 passed, 471 total
Tests: 1 failed, 51 skipped, 5132 passed, 5184 total
Snapshots: 1766 passed, 1766 total
Time: 81.74 s, estimated 90 s
Ran all test suites in 15 projects.
$ yarn jest --coverage --coverageProvider=odz
------------------------------------------|---------|----------|---------|---------|---------------------------------------------------------------------------------------------------------------------------------------
File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
------------------------------------------|---------|----------|---------|---------|---------------------------------------------------------------------------------------------------------------------------------------
All files | 65.68 | 61.61 | 64.67 | 65.76 |
Test Suites: 1 failed, 470 passed, 471 total
Tests: 1 failed, 51 skipped, 5132 passed, 5184 total
Snapshots: 1766 passed, 1766 total
Time: 76.922 s, estimated 79 s
Ran all test suites in 15 projects. The most striking difference is the function and branch coverage reported by
Using The differences between About performance differences: without a proper benchmark under some controlled environment and context, and multiple executions, they don't mean much. But it is likely that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for this, looks really exciting!
Left a comment inline, but I think we should avoid the dependency and make it pluggable, but the code itself looks good!
packages/jest-reporters/package.json
Outdated
@@ -34,6 +34,7 @@ | |||
"jest-message-util": "workspace:*", | |||
"jest-util": "workspace:*", | |||
"jest-worker": "workspace:*", | |||
"one-double-zero": "1.0.0-beta.14", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems v1 stable is out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Let me update the version. It should also make the coverage even closer to nyc.
package.json
Outdated
@@ -202,7 +202,8 @@ | |||
"jest": "workspace:*", | |||
"jest-environment-node": "workspace:*", | |||
"psl": "patch:psl@npm:^1.9.0#./.yarn/patches/psl-npm-1.9.0-a546edad1a.patch", | |||
"ts-node@^10.5.0": "patch:ts-node@npm:^10.5.0#./.yarn/patches/ts-node-npm-10.9.1-6c268be7f4.patch" | |||
"ts-node@^10.5.0": "patch:ts-node@npm:^10.5.0#./.yarn/patches/ts-node-npm-10.9.1-6c268be7f4.patch", | |||
"typescript": "~5.5.4" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this shouldn't be here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. It is likely an oversight, there is no dependency on these two packages. I will fix that.
yarn.lock
Outdated
istanbul-lib-report: ^3.0.1 | ||
istanbul-reports: ^3.1.7 | ||
toml: ^3.0.0 | ||
typescript: ^5.5.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should add a dependency that brings in typescript 🤔 I guess we could make the dep pluggable (i.e. optional peer dependency).
That would also handle the fact that it is minimum node v18 and Jest v30 supports Node 16
92a2ac1
to
c575d8d
Compare
Thanks a lot for the review.
I replaced I rebased my PR on the head of Let me know what you think. |
one-double-zero-core embarks its own lightweight parser, instead of depending on the gigantic TypeScript package.
|
||
:::info | ||
|
||
The `babel` and `v8` coverage providers use `/* istanbul ignore next */` and `/* c8 ignore next */` comments to exclude lines from coverage reports, respectively. For more information, you can view the [`istanbuljs` documentation](https://github.com/istanbuljs/nyc#parsing-hints-ignoring-lines) and the [`c8` documentation](https://github.com/bcoe/c8#ignoring-uncovered-lines-functions-and-blocks). | ||
The `babel` and `v8` coverage providers use `/* istanbul ignore next */` and `/* c8 ignore next */` comments to exclude lines from coverage reports, respectively. For more information, you can view the [`istanbuljs` documentation](https://github.com/istanbuljs/nyc#parsing-hints-ignoring-lines) and the [`c8` documentation](https://github.com/bcoe/c8#ignoring-uncovered-lines-functions-and-blocks). The `odz` coverage provider doesn't support exclusion comment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been checking this PR from time to time and thinking whether odz
would fit into Vitest as well. We ended up taking v8-to-istanbul
's API and adding AST-awareness to that.
It doesn't have the tradeoffs that odz
has, and outputs ~100% identical coverage maps as Istanbul does. Also package size is much smaller (1.9MB vs 29kB).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. I was about to investigate having odz fueled by v8-to-istanbul.
What parser did you end up using? The vast majority of odz size comes from the typescript parser, so if you were able to keep the size under control, i'm'very interested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's intentionally not tied to any parser. Testing tools like Jest and Vitest have their own transform pipelines, and they already have a parser that they can utilize. Similar to source maps, this tool accepts AST as argument. See https://www.npmjs.com/package/ast-v8-to-istanbul for the API.
I'm not sure how odz
works, but you probably should not be using Typescript parser there. Instead it should be parsing the executed, possibly transpiled, Javascript code. Otherwise you'll need to bring in compilers for all transpiled languages, like Vue, Svelte, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea, a lot. That would also solve the current PR issue: odz core would not include a parser at all.
I assume that the parse function excepted by v8-to-istanbul needs to return an estree-compatible ast, does it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's tested with 4 different parsers; 3 that output their own kind of estree, and Babel parser for Babel AST:
Passes all 195 tests* of
istanbul-lib-instrument
. ✅Test cases run against:
vite/parseAst
✅acorn
✅oxc-parser
✅@babel/parser
✅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀
Summary
As explained in the issue,
v8
coverage provider comes with some important tradeoffs compared to the babel/istanbul one. One Double Zero is a code coverage tool and API that consumes V8 coverage data and targets the accuracy and correctness of istanbul. It does this by operating at the AST level.This PR adds One Double Zero as a coverage provider.
It also updates the documentation, and explains the tradeoffs of the
v8
coverage provider.Test plan
The plan is to execute
odz
on each e2e test executed by thev8
coverage provider test suite, and use the output result as the snapshot fore2e/__tests__/coverageProviderODZ.test.ts
. The test script of eache2e
has to be changed to be executable wth either node or ts-node, which does not impact the coverage result.A few changes had to be made, like passing the list of files to cover to the
_getCoverageResult
method, but globally the changes required to addodz
were minimal.