Skip to content

Conversation

@AriPerkkio
Copy link
Member

@AriPerkkio AriPerkkio commented Mar 25, 2025

Description

Testing:

  • vuejs/core: 100% match with @vitest/coverage-istanbul, except branch coverage is 87.69% 12059/13751 vs 87.46% 12047/13773. There are couple of uncovered function default arguments that are not reported by V8 at all. These are marked as covered.
  • vitejs/vite: Close to 100% match with @vitest/coverage-istanbul. Differences coming from V8 limitations, e.g. uncovered default parameters.
  • Self-test: 100% match test: self test with vitest-dev/vitest#7736 AriPerkkio/ast-v8-to-istanbul#31

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. If the feature is substantial or introduces breaking changes without a discussion, PR might be closed.
  • Ideally, include a test that fails without this PR but passes with it.
  • Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.

Tests

  • Run the tests with pnpm test:ci.

Documentation

  • If you introduce new functionality, document it. You can run documentation with pnpm run docs command.

Changesets

  • Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.

@netlify
Copy link

netlify bot commented Mar 25, 2025

Deploy Preview for vitest-dev ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 4070914
🔍 Latest deploy log https://app.netlify.com/sites/vitest-dev/deploys/68177ab962d1870008ca7181
😎 Deploy Preview https://deploy-preview-7736--vitest-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@AriPerkkio AriPerkkio force-pushed the feat/v8-ast-aware-coverage branch 4 times, most recently from 44d9a4b to b5f7d06 Compare March 28, 2025 09:44
@AriPerkkio AriPerkkio added this to the 3.2.0 milestone Mar 28, 2025
@AriPerkkio AriPerkkio force-pushed the feat/v8-ast-aware-coverage branch from b1e8c9f to edf0b10 Compare March 29, 2025 15:13
@AriPerkkio AriPerkkio force-pushed the feat/v8-ast-aware-coverage branch 4 times, most recently from addd2df to 47a3e21 Compare April 1, 2025 07:57
@pkg-pr-new
Copy link

pkg-pr-new bot commented Apr 1, 2025

@vitest/browser

npm i https://pkg.pr.new/@vitest/browser@7736

@vitest/coverage-istanbul

npm i https://pkg.pr.new/@vitest/coverage-istanbul@7736

@vitest/expect

npm i https://pkg.pr.new/@vitest/expect@7736

@vitest/coverage-v8

npm i https://pkg.pr.new/@vitest/coverage-v8@7736

@vitest/mocker

npm i https://pkg.pr.new/@vitest/mocker@7736

@vitest/pretty-format

npm i https://pkg.pr.new/@vitest/pretty-format@7736

@vitest/runner

npm i https://pkg.pr.new/@vitest/runner@7736

@vitest/snapshot

npm i https://pkg.pr.new/@vitest/snapshot@7736

@vitest/spy

npm i https://pkg.pr.new/@vitest/spy@7736

@vitest/ui

npm i https://pkg.pr.new/@vitest/ui@7736

@vitest/utils

npm i https://pkg.pr.new/@vitest/utils@7736

vite-node

npm i https://pkg.pr.new/vite-node@7736

vitest

npm i https://pkg.pr.new/vitest@7736

@vitest/web-worker

npm i https://pkg.pr.new/@vitest/web-worker@7736

@vitest/ws-client

npm i https://pkg.pr.new/@vitest/ws-client@7736

commit: 4070914

Comment on lines +232 to +286
ignoreNode: (node, type) => {
// SSR transformed imports
if (
type === 'statement'
&& node.type === 'AwaitExpression'
&& node.argument.type === 'CallExpression'
&& node.argument.callee.type === 'Identifier'
&& node.argument.callee.name === '__vite_ssr_import__'
) {
return true
}

// SSR transformed exports
if (
type === 'statement'
&& node.type === 'ExpressionStatement'
&& node.expression.type === 'AssignmentExpression'
&& node.expression.left.type === 'MemberExpression'
&& node.expression.left.object.type === 'Identifier'
&& node.expression.left.object.name === '__vite_ssr_exports__'
) {
return true
}
},
Copy link
Member Author

@AriPerkkio AriPerkkio Apr 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hi-ogawa this approach should provide us an easier way to control how Vite's generated code is excluded from coverage reports. We no longer need to modify source maps - instead we ignore the nodes while remapping coverage.

https://github.com/AriPerkkio/ast-v8-to-istanbul?tab=readme-ov-file#ignoring-generated-code

AriPerkkio added a commit to AriPerkkio/ast-v8-to-istanbul that referenced this pull request Apr 1, 2025
@AriPerkkio AriPerkkio force-pushed the feat/v8-ast-aware-coverage branch 2 times, most recently from 3bef604 to ec9dfe8 Compare April 1, 2025 08:52
@AriPerkkio AriPerkkio force-pushed the feat/v8-ast-aware-coverage branch 3 times, most recently from 79d1852 to add0a2d Compare April 3, 2025 08:05
@AriPerkkio AriPerkkio marked this pull request as ready for review April 3, 2025 08:05
AriPerkkio added a commit to vitest-tests/coverage-large that referenced this pull request Apr 3, 2025
AriPerkkio added a commit to vitest-tests/coverage-large that referenced this pull request Apr 3, 2025
AriPerkkio added a commit to vitest-tests/coverage-large that referenced this pull request Apr 3, 2025
AriPerkkio added a commit to vitest-tests/coverage-large that referenced this pull request Apr 3, 2025
@AriPerkkio AriPerkkio force-pushed the feat/v8-ast-aware-coverage branch from add0a2d to 4c28b62 Compare April 5, 2025 17:25
AriPerkkio added a commit to AriPerkkio/ast-v8-to-istanbul that referenced this pull request Apr 6, 2025
hi-ogawa
hi-ogawa previously approved these changes Apr 8, 2025
Copy link
Contributor

@hi-ogawa hi-ogawa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked ast-v8-to-istanbul too and it looks all good to me. Awesome stuff!

Report generation is slightly slower than with previous @vitest/coverage-v8, but faster than overall istanbul provider run.

I'm curious how you understand the performance characteristic leading to this result.

I think ast-v8-to-istanbul being slower than v8-to-istanbul is probably obvious since it now parses and traverse AST and remap node by node while v8-to-istanbul just goes through v8 coverage once.

What about @vitest/coverage-istanbul? I guess istanbul's instrumentation transform can be slower than ast-v8-to-istanbul's ast manipulation? Also do you take into account instrumented code being running slower? Or maybe other aspect of coverage processing logic?

@AriPerkkio
Copy link
Member Author

AriPerkkio commented Apr 8, 2025

I think ast-v8-to-istanbul being slower than v8-to-istanbul is probably obvious since it now parses and traverse AST and remap node by node while v8-to-istanbul just goes through v8 coverage once.

Yes, exactly. It is expected that AST based remapping is slower than the original v8-to-istanbul. The original one is very naive and produces many false positives, but it's fast. It's not expected that ast-v8-to-istanbul would reach the speed of v8-to-istanbul, but I'm hoping we could get very close to that. Both of these tools process a lot of text (JSON, code, source maps) so there's plenty of optimization possibilities. Looking at the API of ast-v8-to-istanbul, I think in future all of this could even run on Rust based tooling like oxc.

What about @vitest/coverage-istanbul? I guess istanbul's instrumentation transform can be slower than ast-v8-to-istanbul's ast manipulation? Also do you take into account instrumented code being running slower? Or maybe other aspect of coverage processing logic?

This speed difference reproduces on vuejs/core repository really well. When using Istanbul's pre-instrumented coverage, it's actually visible for end-users to see that files remain in [queued] filename.test.ts state for a while when transform is running. Also executing this pre-instrumented code is a lot slower - I think there would be good optimization opportunities in istanbul-lib-instrumentation's code transforms. Pre-instrumenting increases transpiled code and combined source maps a lot.

When comparing AST parsing, visiting and analysing of istanbul-lib-instrument and ast-v8-to-istanbul, we should see similar speed results.

I'm curious how you understand the performance characteristic leading to this result.

I've been comparing @vitest/coverage-istanbul, @vitest/coverage-v8 and @vitest/coverage-v8 + ast-v8-to-istanbul in:

In these tests I've checked overall speed with time vitest --run --coverage and compared coverage summaries between each run. I'm also planning to add debug timer for coverage generation via DEBUG=vitest:coverage in separate PR at some point.

sheremet-va
sheremet-va previously approved these changes Apr 9, 2025
Copy link
Member

@sheremet-va sheremet-va left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks incredible! Thank you for working on it 🙏🏻

@AriPerkkio AriPerkkio dismissed stale reviews from sheremet-va and hi-ogawa via 4070914 May 4, 2025 14:33
@AriPerkkio AriPerkkio force-pushed the feat/v8-ast-aware-coverage branch from 4c28b62 to 4070914 Compare May 4, 2025 14:33
@sheremet-va sheremet-va merged commit 78a3d27 into vitest-dev:main May 5, 2025
20 of 23 checks passed
@AriPerkkio AriPerkkio deleted the feat/v8-ast-aware-coverage branch May 5, 2025 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

3 participants