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): browser mode + coverage.all #7597

Merged
merged 5 commits into from
Mar 17, 2025

Conversation

AriPerkkio
Copy link
Member

@AriPerkkio AriPerkkio commented Mar 2, 2025

Description

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

Sorry, something went wrong.

Copy link

pkg-pr-new bot commented Mar 2, 2025

@vitest/browser

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

@vitest/coverage-istanbul

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

@vitest/coverage-v8

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

@vitest/expect

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

@vitest/mocker

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

@vitest/runner

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

@vitest/pretty-format

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

@vitest/snapshot

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

@vitest/spy

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

@vitest/ui

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

@vitest/utils

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

vite-node

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

vitest

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

@vitest/web-worker

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

@vitest/ws-client

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

commit: e653bec

Copy link

netlify bot commented Mar 2, 2025

Deploy Preview for vitest-dev ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit e653bec
🔍 Latest deploy log https://app.netlify.com/sites/vitest-dev/deploys/67d04ffe5dc8590008bda006
😎 Deploy Preview https://deploy-preview-7597--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/browser-mode-coverage-all branch from 85550bc to 874d14d Compare March 4, 2025 08:25
@AriPerkkio AriPerkkio force-pushed the fix/browser-mode-coverage-all branch from a2a5043 to 272ae0d Compare March 4, 2025 14:36
@AriPerkkio AriPerkkio force-pushed the fix/browser-mode-coverage-all branch 13 times, most recently from 2208f7d to 12d473c Compare March 8, 2025 13:55
Copy link
Member Author

@AriPerkkio AriPerkkio left a comment

Choose a reason for hiding this comment

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

Alright the Windows issues are finally resolved. @mrginglymus could you do one final check with https://pkg.pr.new/vitest@12d473c and equivalent preview packages with your real-world project?

Comment on lines 551 to 552
// On Windows root doesn't start with "/" while filenames do
if (!filename.startsWith(root) && !filename.startsWith(`/${root}`)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

On Windows the root is here D:/a/vitest/vitest/test/coverage-test while filename is /D:/a/vitest/vitest/test/coverage-test/fixtures/src/untested-file.ts.

@AriPerkkio AriPerkkio marked this pull request as ready for review March 8, 2025 14:21
@AriPerkkio

This comment was marked as outdated.

@vitest-ecosystem-ci

This comment was marked as resolved.

@AriPerkkio
Copy link
Member Author

/ecosystem-ci run

@vitest-ecosystem-ci
Copy link

vitest-ecosystem-ci bot commented Mar 10, 2025

📝 Ran ecosystem CI: Open

suite result
aria-live-capture ✅ success
nuxt ❌ failure
nuxt-test-utils ✅ success
elk ✅ success
effect ❌ failure
lerna-lite ✅ success
zustand ✅ success
vue ✅ success
vite ✅ success
vitest-vscode ✅ success
vitest-sonar-reporter ✅ success
vitest-browser-examples ✅ success
vitest-coverage-large ✅ success
vitest-reporters-large ✅ success
vitest-benchmark-large ✅ success

@mrginglymus
Copy link
Contributor

Alright the Windows issues are finally resolved. @mrginglymus could you do one final check with https://pkg.pr.new/vitest@12d473c and equivalent preview packages with your real-world project?

@AriPerkkio yes this is working correctly on my real-world project. Thanks so much for the effort in fixing this!

@AriPerkkio AriPerkkio force-pushed the fix/browser-mode-coverage-all branch from 03cd06b to e653bec Compare March 11, 2025 15:00
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.

Looks good to me 👍
How did you eventually fix optimize deps error? Is it by switching vitenode.transformRequest with browser.transformRequest #7597 (comment)?

@AriPerkkio
Copy link
Member Author

How did you eventually fix optimize deps error? Is it by switching vitenode.transformRequest with browser.transformRequest #7597 (comment)?

Switching to vitenode.transformRequest is a way to avoid it, yes.

But when using browser, this change was enough to fix it. And when doing parallel requests, it was important not to reset noDiscovery back.

+ browser.config.optimizeDeps.noDiscovery = true
const result = await browser?.transformRequest(filename).catch(() => null)

Setting the server.preTransformRequests had no effect at all - Vite was still following dependencies of requested modules. 🤷

@hi-ogawa
Copy link
Contributor

But, if this relies on vite node's server and browser mode's server being different, would it potentially block the plan of merging the server? #6912

@sheremet-va
Copy link
Member

But, if this relies on vite node's server and browser mode's server being different, would it potentially block the plan of merging the server? #6912

How does it matter though? The code output is the same, isn't it what matters here?

@hi-ogawa
Copy link
Contributor

I thought the difference is vite-node's server explicitly configures optimizeDeps.noDiscovery: true + preTransformRequests: false, but browser's server has opposite (i.e. normal Vite app default).

@sheremet-va
Copy link
Member

I thought the difference is vite-node's server explicitly configures optimizeDeps.noDiscovery: true + preTransformRequests: false, but browser's server has opposite (i.e. normal Vite app default).

Yes, but does it matter for coverage what these options are?

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Mar 14, 2025

I thought the difference is vite-node's server explicitly configures optimizeDeps.noDiscovery: true + preTransformRequests: false, but browser's server has opposite (i.e. normal Vite app default).

Yes, but does it matter for coverage what these options are?

I would imagine transformRequest output is same, but as Ari said #7597 (comment), without noDiscovery, Vite server would eagerly crawl and potentially trigger optimize deps, so such side effect is not ideal for our usage of transformRequest for coverage.all.
(actually I don't know what's bad about this, but at least, optimize deps might show odd error/warning from esbuild?)

I think it's good to land with vitenode.transformRequest, but we might need to revisit this when #6912. I don't know whether we need something new from Vite or we can use extra inline environment for this transform, or something else.

@sheremet-va sheremet-va merged commit 422ba66 into vitest-dev:main Mar 17, 2025
14 checks passed
@AriPerkkio AriPerkkio deleted the fix/browser-mode-coverage-all branch March 17, 2025 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Import conditions not respected on main thread
4 participants