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

[Bug]: V8 coverage does not work with native ESM #11891

Closed
mrazauskas opened this issue Sep 22, 2021 · 8 comments · Fixed by #11893
Closed

[Bug]: V8 coverage does not work with native ESM #11891

mrazauskas opened this issue Sep 22, 2021 · 8 comments · Fixed by #11893

Comments

@mrazauskas
Copy link
Contributor

Version

27.2.0

Steps to reproduce

As it was mentioned in #9430 (comment), I was trying out V8 coverage with native ESM. Babel coverage works well, but for some reason there are issues with V8 coverage.

I was testing:

  • ESM without transforming, hence without source maps included
  • TypeScript modules transformed into ESM with source maps included

@SimenB thanks for quick response and hints. I started from creating failing tests inside a clone of Jest repo. The same result – coverage works well with Babel, but not with V8. So I will dig into the code and will try to find the reason.

Expected behavior

I also tried out coverage using c8 module directly on an ESM file (without involving Jest). All is good. Quick, nice and precise.

Similar result is expected.

Actual behavior

V8 coverage reports seems to be buggy.

Additional context

No response

Environment

System:
    OS: macOS 10.15.7
    CPU: (8) x64 Intel(R) Core(TM) i7-3615QM CPU @ 2.30GHz
  Binaries:
    Node: 16.9.1 - /usr/local/bin/node
    Yarn: 2.4.3 - /usr/local/bin/yarn
    npm: 7.21.1 - /usr/local/bin/npm
@bompus
Copy link

bompus commented Sep 22, 2021

I'm also experiencing this on 27.2.1 with --coverage=true --coverageProvider=v8. I didn't check covered lines closely, but uncovered lines are off by ~3 lines it seems.

Running through c8 and using --coverage=false on jest, the uncovered line numbers are correct.

@mrazauskas
Copy link
Contributor Author

mrazauskas commented Sep 23, 2021

@SimenB The problem is clear. All works well with native ESM files, if the value of wrapperLength is 0 here. If I get it right, the wrapper here is the CJS module wrapper. In this case ~3 lines offset also makes sense.

Possible fix: check if the module wrapper will be used before assigning the value inside transformFile and transformFileAsync (here and here). Only question – what should be checked?

@SimenB
Copy link
Member

SimenB commented Sep 23, 2021

transformFile should only every be called with CJS and transformFileAsync should only ever be called with ESM.

Could you push your failing test case somewhere I can take a look?

@mrazauskas
Copy link
Contributor Author

Sure. I will send a PR tomorrow.

@mrazauskas
Copy link
Contributor Author

Found interesting case while cleaning up. I was trying to include empty-sourcemap test in the new coverage-provider-c8 test suite. A type is imported from types.ts to module.ts.

(1.) Babel transformer and Babel based coverage:
Screenshot 2021-09-24 at 10 01 42

(2.) Any transformer and V8 based coverage (only types.ts line is in question, the rest is correct):
Screenshot 2021-09-24 at 10 01 54

It is not ESM related, but still it made me curious. V8 checks if code was executed without inserting counters. types.ts is not executable and one should not expect coverage. Just exclude it manually from the coverage report and all is sorted out. But isn’t it too much manual work in the context of automated testing?

Just thinking out loud. There are two kinds of files – the ones which can be consumed directly and the others which have to be transpiled. If source file is transpiled, can V8 provide coverage without having a source map? It is not needed for directly consumed files, but what about the transpiled ones? Possible to try, but lines will not match in many cases.

What if V8 coverage would simply grey out transpiled files which do not have coverage (as in (1.))? ”Sorry, it is not possible to provide coverage for this file.”

In the other hand, if an executable file has no tests... It is much better idea to keep it red in the report instead of greying out. And might be worth to include a note on Babel / V8 coverage differences in documentation together with #11188.

@SimenB Could you confirm if this is expected behaviour? Looks so. I add it to tests like here, right?

@SimenB
Copy link
Member

SimenB commented Sep 25, 2021

If types.ts is not imported, we mark the entire file as uncovered. We should probably transform it and mark the transformed result as uncovered instead: https://github.com/facebook/jest/blob/da7c4a42840230ddaa7c101dbc86edb752787c64/packages/jest-reporters/src/generateEmptyCoverage.ts#L45-L67

@mrazauskas
Copy link
Contributor Author

Thanks for a hint. Now I see where '(empty-report)' was coming from. That one was a puzzle. Will play with the idea.

@github-actions
Copy link

This issue 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 a pull request may close this issue.

3 participants