Skip to content
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

fix(coverage): vite-node to pass correct execution wrapper offset #7417

Merged
merged 3 commits into from
Feb 13, 2025

Conversation

AriPerkkio
Copy link
Member

@AriPerkkio AriPerkkio commented Feb 4, 2025

Description

Makes vite-node to hold info about the execution wrapper it uses when executing modules. Coverage providers are passed this info on the test runner thread so that they can attach it into the coverage results. There's no longer need to hard-code the wrapper length on main thread codebase. The example web-worker.test.ts has a case where in a single test runner thread some code is executed with 185 long wrapper and some with ~430 long wrapper.

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:.

Copy link

netlify bot commented Feb 4, 2025

Deploy Preview for vitest-dev ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 89102c3
🔍 Latest deploy log https://app.netlify.com/sites/vitest-dev/deploys/67ac6c5dc5f70e000834fac7
😎 Deploy Preview https://deploy-preview-7417--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 fix/coverage-web-worker-v8 branch from 954d2a1 to 0f3f4f5 Compare February 4, 2025 14:35
@AriPerkkio AriPerkkio changed the title fix(coverage): @vitest/web-worker with v8 coverage fix(coverage): vite-node to pass correct execution wrapper offset Feb 6, 2025
@AriPerkkio AriPerkkio force-pushed the fix/coverage-web-worker-v8 branch 2 times, most recently from e7a39cc to 4e458b0 Compare February 6, 2025 15:21
@AriPerkkio AriPerkkio force-pushed the fix/coverage-web-worker-v8 branch from 4e458b0 to 008b10d Compare February 6, 2025 15:36
@AriPerkkio AriPerkkio marked this pull request as ready for review February 6, 2025 15:36
@AriPerkkio AriPerkkio requested review from hi-ogawa and sheremet-va and removed request for hi-ogawa February 6, 2025 15:40
hi-ogawa
hi-ogawa previously approved these changes Feb 7, 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.

Makes sense to me 👍
I'm wondering what happens if the same file is imported from test and from @vitest/web-worker. Does startOffset conflict (last loaded win?) and so coverage is likely broken?

packages/coverage-v8/src/index.ts Outdated Show resolved Hide resolved
@AriPerkkio
Copy link
Member Author

Makes sense to me 👍 I'm wondering what happens if the same file is imported from test and from @vitest/web-worker. Does startOffset conflict (last loaded win?) and so coverage is likely broken?

Yep, coverage could break then. Also V8 gets confused as the same file ID had different contents when executed. The script coverage is a mess then.

@sheremet-va
Copy link
Member

Do we need to update vitest's package.json's peerDependencies? Is it technically a breaking change?

packages/vitest/src/runtime/execute.ts Show resolved Hide resolved
packages/vitest/src/node/types/coverage.ts Outdated Show resolved Hide resolved
packages/vite-node/src/types.ts Show resolved Hide resolved
@AriPerkkio
Copy link
Member Author

Do we need to update vitest's package.json's peerDependencies? Is it technically a breaking change?

Which package here?

"peerDependencies": {
"@edge-runtime/vm": "*",
"@types/debug": "^4.1.12",
"@types/node": "^18.0.0 || ^20.0.0 || >=22.0.0",
"@vitest/browser": "workspace:*",
"@vitest/ui": "workspace:*",
"happy-dom": "*",
"jsdom": "*"
},

Or you mean about using mixed versions? A year ago (#5208) we made the decision that Vitest monorepo's packages do not work with mixed versions. All packages must have the same version. And we've been introducing breaking changes between the packages ever since. There are also runtime warnings in @vitest/browser, @vitest/ui and coverage packages about mixed versions.

@AriPerkkio AriPerkkio force-pushed the fix/coverage-web-worker-v8 branch 2 times, most recently from e5fa58a to 89102c3 Compare February 12, 2025 09:39
@AriPerkkio AriPerkkio force-pushed the fix/coverage-web-worker-v8 branch from 8c9f3f8 to 20a348b Compare February 13, 2025 10:53
@AriPerkkio AriPerkkio merged commit 1f2e555 into vitest-dev:main Feb 13, 2025
13 checks passed
@AriPerkkio AriPerkkio deleted the fix/coverage-web-worker-v8 branch February 13, 2025 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect coverage in web workers with @vitest/web-worker and @vitest/coverage-v8
3 participants