-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix(coverage): v8 to ignore Vite's generated cjs import helpers #8718
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
Conversation
✅ Deploy Preview for vitest-dev ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
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.
Looks good, thanks! I've also verified that the minimal repro works with these changes.
Ideally we would also add test case into test/coverage-test, but I wasn't able to trigger the condition without adding react as dependency. Tried using @vitest/cjs-lib but it just would not work.
|
Vite is injecting code through https://github.com/vitejs/vite/blob/84079a84ad94de4c1ef4f1bdb2ab448ff2c01196/packages/vite/src/node/plugins/importAnalysis.ts#L988. There might be other cases like |
|
So, probably this case: import * as React from 'react';
export default function() {
return React;
}becomes import __vite__cjsImport0_react from "/node_modules/.vite/vitest/da39a3ee5e6b4b0d3255bfef95601890afd80709/deps/react.js?v=fb913633";
const React = ( (m$2) => m$2?.__esModule ? m$2 : {
...typeof m$2 === "object" && !Array.isArray(m$2) || typeof m$2 === "function" ? m$2 : {},
default: m$2
})(__vite__cjsImport0_react);
export default function () {
return React;
}with the coverage: Direct named import seems fine: import { version } from 'react';becomes import __vite__cjsImport0_react from "/node_modules/.vite/vitest/da39a3ee5e6b4b0d3255bfef95601890afd80709/deps/react.js?v=fb913633";
const version = __vite__cjsImport0_react["version"]; |
9442110 to
9c0b789
Compare
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.
Ok the test case actually works when using file: protocol instead of link:. I guess Vite deps optimizer works only with that.
@hi-ogawa it would be great if Vite's transforms supported something like Babel's auxiliaryCommentBefore so that we could tell it to add ignore hint comments before all custom transforms when coverage is enabled. This is how Jest does with Babel.
Sounds like a good idea. Can you create an issue on Vite? |
|
Adding if (type === 'branch'
&& node.type === 'ConditionalExpression'
&& node.test.type === 'ChainExpression'
&& node.test.expression.type === 'MemberExpression'
&& node.test.expression.property.type === 'Identifier'
&& node.test.expression.property.name === '__esModule'
) {
return 'ignore-this-and-nested-nodes'
}will handle the |
@hi-ogawa what's an easy way to obtain this transformed form? I'm finding a lot of bugs in the V8 coverage which will be easiest to diagnose when I can see the transformed source. |
I was directly checking network devtool to see the response of source code, like 1. run vitest with headed playwright, 2. open devtools, 3. rerun some test to see response. |
Description
Adds conditions to ignore CJS imports in v8 branch coverage. These are generated of the form e.g.
and obviously only one branch will be covered.
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
pnpm-lock.yamlunless you introduce a new test example.Tests
pnpm test:ci.Documentation
pnpm run docscommand.Changesets
feat:,fix:,perf:,docs:, orchore:.