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

AriPerkkio · 2025-02-04T10:56:15Z

Description

Fixes Incorrect coverage in web workers with @vitest/web-worker and @vitest/coverage-v8 #7379
Proper fix for TypeError: Cannot read properties of undefined (reading 'endCol') #6297

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

netlify · 2025-02-04T10:58:38Z

✅ 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... Use your smartphone camera to open QR code link.

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

AriPerkkio · 2025-02-06T15:29:07Z

packages/coverage-v8/src/provider.ts

-// TODO: vite-node should export this
-const WRAPPER_LENGTH = 185


Finally getting rid of this 🥳

packages/vite-node/src/types.ts

hi-ogawa

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

test/coverage-test/test/web-worker.test.ts

AriPerkkio · 2025-02-07T06:25:34Z

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 · 2025-02-07T15:31:48Z

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

sheremet-va · 2025-02-07T15:35:21Z

packages/vitest/src/runtime/execute.ts

@@ -313,6 +320,8 @@ export class VitestExecutor extends ViteNodeRunner {
      columnOffset: -codeDefinition.length,
    }

+    this.options.moduleExecutionInfo?.set(options.filename, { startOffset: codeDefinition.length })


Not sure if filename is a good choice here. Wouldn't url be better? It should include queries if there are any

Now as it's using moduleCache, the key must match that one. I think the options.filename here contains query params too 🤔

moduleCache doesn't use filename tho, it uses normalized module id

Ok, I'll add test case that loads Worker with query params later.

This change should break Vue tests if query params are missing. 🤔

The used id/filename must match the one that is being passed as filename in vm.runInThisContext. That's what V8 sets in the runtime results.

moduleCache doesn't use filename tho, it uses normalized module id

When are these two different? Can't see any differences between these two in the tests.

packages/vitest/src/node/types/coverage.ts

packages/vite-node/src/types.ts

AriPerkkio · 2025-02-07T15:41:18Z

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

Which package here?

vitest/packages/vitest/package.json

Lines 124 to 132 in 12eaf3e

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

…fter

…Offset`

sheremet-va · 2025-02-12T16:59:37Z

test/core/test/web-worker-node.test.ts

@@ -203,6 +203,7 @@ it('self injected into worker and its deps should be equal', async () => {
  await sleep(0)
  expect(await testSelfWorker(new MySelfWorker())).toBeTruthy()

+  vi.resetModules()


The test should work without clearing modules, that's the whole point of it, no?

I thought this test actually contained "two tests", as in first the MySelfWorker part and then Worker part.

If this test should pass without module reset, we cannot move the module invalidation in InlineWorker to happen before executeFile. So cannot use ModuleCache for holding the startOffset.

This is a single test, the worker is the same, just initiated differently.

So cannot use ModuleCache for holding the startOffset.

Seems like we need to go to the drawing board then

Yup I'll need to revert b736b63 and not rely on ModuleCache.

AriPerkkio force-pushed the fix/coverage-web-worker-v8 branch from 954d2a1 to 0f3f4f5 Compare February 4, 2025 14:35

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 force-pushed the fix/coverage-web-worker-v8 branch 2 times, most recently from e7a39cc to 4e458b0 Compare February 6, 2025 15:21

AriPerkkio mentioned this pull request Feb 6, 2025

TypeError: Cannot read properties of undefined (reading 'endCol') #6297

Closed

6 tasks

AriPerkkio commented Feb 6, 2025

View reviewed changes

AriPerkkio force-pushed the fix/coverage-web-worker-v8 branch from 4e458b0 to 008b10d Compare February 6, 2025 15:36

AriPerkkio marked this pull request as ready for review February 6, 2025 15:36

AriPerkkio commented Feb 6, 2025

View reviewed changes

packages/vite-node/src/types.ts Outdated Show resolved Hide resolved

AriPerkkio requested review from hi-ogawa and sheremet-va and removed request for hi-ogawa February 6, 2025 15:40

hi-ogawa previously approved these changes Feb 7, 2025

View reviewed changes

packages/coverage-v8/src/index.ts Outdated Show resolved Hide resolved

hi-ogawa reviewed Feb 7, 2025

View reviewed changes

test/coverage-test/test/web-worker.test.ts Show resolved Hide resolved

AriPerkkio dismissed hi-ogawa’s stale review via 0e40759 February 7, 2025 07:39

sheremet-va requested changes Feb 7, 2025

View reviewed changes

AriPerkkio force-pushed the fix/coverage-web-worker-v8 branch from de7dc2a to e5fa58a Compare February 9, 2025 09:32

AriPerkkio added 7 commits February 12, 2025 11:29

test: failing test for vitest-dev#7379

4a4aa17

fix(vite-node): hold info about vm context wrapper offset

c21bc26

fix(coverage): v8 to use vm context offset from vite-node

13cfe90

fix: review

7e874ec

fix(web-worker): invalidate worker file before execution instead of a…

ab0be4b

…fter

fix(vite-node): replace moduleExecutionInfo with `moduleCache.start…

b736b63

…Offset`

test: load worker with query params

89102c3

AriPerkkio force-pushed the fix/coverage-web-worker-v8 branch from e5fa58a to 89102c3 Compare February 12, 2025 09:39

test: worker test module isolation

8c9f3f8

sheremet-va reviewed Feb 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

AriPerkkio commented Feb 4, 2025 •

edited

Loading

netlify bot commented Feb 4, 2025 •

edited

Loading

AriPerkkio Feb 6, 2025

hi-ogawa left a comment

AriPerkkio commented Feb 7, 2025

sheremet-va commented Feb 7, 2025

sheremet-va Feb 7, 2025

AriPerkkio Feb 9, 2025

sheremet-va Feb 9, 2025

AriPerkkio Feb 9, 2025

AriPerkkio Feb 12, 2025

AriPerkkio commented Feb 7, 2025

sheremet-va Feb 12, 2025 •

edited

Loading

AriPerkkio Feb 12, 2025

sheremet-va Feb 12, 2025

AriPerkkio Feb 12, 2025

		// TODO: vite-node should export this
		const WRAPPER_LENGTH = 185

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

Are you sure you want to change the base?

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

Conversation

AriPerkkio commented Feb 4, 2025 • edited Loading

Description

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

Tests

Documentation

Changesets

netlify bot commented Feb 4, 2025 • edited Loading

✅ Deploy Preview for vitest-dev ready!

Choose a reason for hiding this comment

hi-ogawa left a comment

Choose a reason for hiding this comment

AriPerkkio commented Feb 7, 2025

sheremet-va commented Feb 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AriPerkkio commented Feb 7, 2025

sheremet-va Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

AriPerkkio commented Feb 4, 2025 •

edited

Loading

netlify bot commented Feb 4, 2025 •

edited

Loading

sheremet-va Feb 12, 2025 •

edited

Loading