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 (jest-runtime): correct wrapperLength value for ESM modules #11893

Merged
merged 7 commits into from
Sep 25, 2021
Merged

fix (jest-runtime): correct wrapperLength value for ESM modules #11893

merged 7 commits into from
Sep 25, 2021

Conversation

mrazauskas
Copy link
Contributor

@mrazauskas mrazauskas commented Sep 25, 2021

Summary

Resolves #11891.

v8 coverage reports were incorrect in ESM mode. Seems like wrapperLength value was the problem. If I got it right, transformFileAsync is called only in ESM mode. And if I get it right, the wrapper is CJS module wrapper. ESM is not using it, so perhaps the best solution is to keep the value of wrapperLength undefined. Simply: the wrapper does not exist.

Test plan

I started form failing tests. The failing ones were: esm-native-without-sourcemap and esm-with-custom-transformer.

It seemed useful to rename the v8 test directory by prefixing it with coverage- (coverage-provider-v8 instead of v8-coverage). This way all coverage related tests live next to each other. Easier to find them.

I created tests for both native cjs and native ems. The wrapperLength value is the imporntant difference, which has to be covered by tests. The wrapper is only important for v8 coverage.

cjs-native and esm-native tests are also proving that coverage report looks good if source map is not included. Here the source code is not transformed at all, hence we do not have source map. These two tests are fully replacing current no-sourcemap test, which I removed as unnecessary.

Copy link
Member

@SimenB SimenB 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 pretty good! I'd like to keep the existent test as is, but beyond that this LGTM 🙂

packages/jest-reporters/src/CoverageReporter.ts Outdated Show resolved Hide resolved
packages/jest-runtime/src/index.ts Outdated Show resolved Hide resolved
packages/jest-test-result/src/types.ts Outdated Show resolved Hide resolved
e2e/v8-coverage/no-sourcemap/cssTransform.js Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@mrazauskas
Copy link
Contributor Author

mrazauskas commented Sep 25, 2021

@SimenB All (should be) fixed as suggested. Thanks for the review.

@mrazauskas mrazauskas changed the title fix (jest-runtime): do not add wrapperLength to fileTransforms for ESM modules fix (jest-runtime): correct wrapperLength value for ESM modules Sep 25, 2021
@codecov-commenter
Copy link

Codecov Report

Merging #11893 (6727a01) into main (da7c4a4) will decrease coverage by 0.13%.
The diff coverage is 54.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #11893      +/-   ##
==========================================
- Coverage   68.87%   68.73%   -0.14%     
==========================================
  Files         312      322      +10     
  Lines       16432    16580     +148     
  Branches     4768     4784      +16     
==========================================
+ Hits        11318    11397      +79     
- Misses       5087     5150      +63     
- Partials       27       33       +6     
Impacted Files Coverage Δ
...vider-v8/cjs-native-without-sourcemap/uncovered.js 0.00% <0.00%> (ø)
...ge-provider-v8/cjs-with-babel-transformer/types.ts 0.00% <0.00%> (ø)
...rovider-v8/cjs-with-babel-transformer/uncovered.ts 0.00% <0.00%> (ø)
e2e/coverage-provider-v8/empty-sourcemap/types.ts 100.00% <ø> (ø)
...vider-v8/esm-native-without-sourcemap/uncovered.js 0.00% <0.00%> (ø)
...e-provider-v8/esm-with-custom-transformer/types.ts 0.00% <0.00%> (ø)
...ovider-v8/esm-with-custom-transformer/uncovered.ts 0.00% <0.00%> (ø)
e2e/coverage-provider-v8/no-sourcemap/Thing.js 100.00% <ø> (ø)
e2e/coverage-provider-v8/no-sourcemap/x.css 100.00% <ø> (ø)
packages/jest-runtime/src/index.ts 56.44% <ø> (ø)
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da7c4a4...6727a01. Read the comment docs.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

thanks!

@SimenB SimenB merged commit d4fff6a into jestjs:main Sep 25, 2021
@mrazauskas mrazauskas deleted the fix-v8-coverage branch September 25, 2021 13:22
@SimenB
Copy link
Member

SimenB commented Sep 25, 2021

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: V8 coverage does not work with native ESM
4 participants