fix: collect coverage from vm script #13962

GP4cK · 2023-02-28T02:48:56Z

Summary

I'm basically continuing the work of: #10657.
If you think it's ok, I'll update the CHANGELOG.md.

Test plan

I extend v8Coverage.test and compare if the code coverage report goes 100% in each columns as expection.

GP4cK · 2023-02-28T03:09:27Z

The CI is failing because it's adding coverage on cssTransform.js. Is it ok to update the snapshot? Or should it not be part of the coverage?

SimenB · 2023-02-28T08:29:21Z

cssTransform.js should not be included as it's not part of the tests - it's part of the Jest config. This is what the condition you've removed is supposed to guard against (only check coverage of files part of the test run, not other files). So I think we need to figure out something more clever, not remove it altogether.

GP4cK · 2023-02-28T09:24:37Z

I see, thanks for the explanation.
Then how about leaving the line this._v8CoverageSources!.has(res.url), but if there is a collectCoverageFrom set, then it supersedes it?
This wouldn't fix the issue entirely but at least it should give a workaround for users who need it.

SimenB · 2023-02-28T11:56:27Z

Yeah, that sounds reasonable! Might give false positives if file is also pulled in by jest itself, but probably edge casey enough

SimenB · 2023-02-28T12:00:35Z

Seems ci is not totally happy

GP4cK · 2023-03-01T05:15:11Z

I made the change that we agreed on. The CI should be good now. Please let me know if there's anything else.

SimenB · 2023-03-01T07:04:55Z

packages/jest-runtime/src/index.ts

    return this._v8CoverageResult
      .filter(res => res.url.startsWith('file://'))
      .map(res => ({...res, url: fileURLToPath(res.url)}))
      .filter(
        res =>
          // TODO: will this work on windows? It might be better if `shouldInstrument` deals with it anyways
          res.url.startsWith(this._config.rootDir) &&
-          this._v8CoverageSources!.has(res.url) &&
+          matchesCoveragePattern(res.url) &&
          shouldInstrument(res.url, this._coverageOptions, this._config),


hmmm, thinking about it - shouldn't this option be enough? and the fact the css transform is included is because we are missing an ignore pattern in our test?

(sorry about the back and forth 🙈)

No worries at all. I'm really not familiar with the code base or v8 so I don't fully understand what's going on.
I was simply following the instructions from someone who answered my question on StackOverflow.

I blamed the file and I see that _v8CoverageSources was introduced as a substitute to _fileTransforms in #12912 to avoid losing the list of files when calling jest.resetModules().

However, I don't see why we would need this in the filter. The 2 other checks should be enough:

res.url.startsWith(this._config.rootDir) && shouldInstrument(...);

This is why this PR was removing it: #10657

So if you agree, I will

remove this line

exclude cssTransform.js in the other test with coveragePathIgnorePatterns so that it doesn't appear in the coverage.

Could you confirm this is the correct way?

Edit: The problem is if I do that, then this line here may break and I don't know where I could get the transformedFile

For me it looks right if by default only files explicitly imported in tests are included in the coverage report. For example, that’s the default of c8. They have all, include, exclude option to include other files.

So it made sense that collectCoverageFrom option is collecting coverage from not only imported files. Current Jest documentation of the option agrees: "If a file matches the specified glob pattern, coverage information will be collected for it even if no tests exist for this file and it's never required in the test suite."

If I didn't miss some detail.

So maybe if I can't find the file in _v8CoverageSource, I can call transformFile() on it and add it to the map. Would that be ok?

... or it's ok to have codeTransformResult = undefined

Not sure what you have in mind. I was trying to say that not including cssTransform.js in coverage report by default seems to be expected behaviour.

@mrazauskas so do you mean we shouldn't touch that part of the code and find another way to add the files loaded with vm.runInContext to _v8CoverageSources?

Let’s wait for Simen’s feedback. He was commenting that cssTransform.js could get included or not because of ignore pattern. I try to suggest that by default only explicitly imported files should be included in the report and collectCoverageFrom option could be used to override that. This is what your current implementation does. If I got it right.

My thought was that shouldInstrument should return the right thing - if a file shouldn't get instrumented, it shouldn't be include in a coverage report

@SimenB you're right, my change didn't make sense.
I've removed the line this._v8CoverageSources!.has(res.url) so we only rely on shouldInstrument and added cssTransform.js to the coveragePathIgnorePatterns in the other tests. I hope that's ok.

Right, this seems more reasonable to me 👍

mrazauskas · 2023-03-01T16:35:26Z

e2e/coverage-provider-v8/no-sourcemap/package.json

+    "coveragePathIgnorePatterns": [
+      "cssTransform\\.js"
+    ]


This looks breaking for me, because it is only needed with V8 provider. Wouldn’t it be better to keep default behaviour similar between Babel and V8 providers? See alternative proposal in #13974

I'm good with your proposal. Thanks!

github-actions · 2023-04-04T00:07:47Z

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.

fix: collect coverage from vm script

71c9abe

facebook-github-bot added the cla signed label Feb 28, 2023

style: format vmscript-coverage/package.json with prettier

e5c0cfa

GP4cK added 2 commits March 1, 2023 10:11

chore: update comment Facebook -> Meta

9ac74c5

fix: fix coverage

9202548

chore: update changelog

cb0f4d1

SimenB reviewed Mar 1, 2023

View reviewed changes

GP4cK added 2 commits March 1, 2023 21:39

refactor: only rely on shouldInstrument

a338b20

chore: update changelog

9fcfe2f

mrazauskas mentioned this pull request Mar 1, 2023

fix(jest-runtime, @jest/transform): allow V8 coverage provider to collect coverage from files which were not loaded explicitly #13974

Merged

mrazauskas reviewed Mar 1, 2023

View reviewed changes

SimenB closed this in #13974 Mar 4, 2023

GP4cK deleted the vmscript-coverage branch March 4, 2023 23:52

github-actions bot locked as resolved and limited conversation to collaborators Apr 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: collect coverage from vm script #13962

fix: collect coverage from vm script #13962

GP4cK commented Feb 28, 2023 •

edited

Loading

GP4cK commented Feb 28, 2023

SimenB commented Feb 28, 2023

GP4cK commented Feb 28, 2023 •

edited

Loading

SimenB commented Feb 28, 2023

SimenB commented Feb 28, 2023

GP4cK commented Mar 1, 2023 •

edited

Loading

SimenB Mar 1, 2023

GP4cK Mar 1, 2023 •

edited

Loading

mrazauskas Mar 1, 2023

GP4cK Mar 1, 2023 •

edited

Loading

mrazauskas Mar 1, 2023

GP4cK Mar 1, 2023

mrazauskas Mar 1, 2023

SimenB Mar 1, 2023

GP4cK Mar 1, 2023 •

edited

Loading

SimenB Mar 1, 2023

mrazauskas Mar 1, 2023 •

edited

Loading

GP4cK Mar 2, 2023

github-actions bot commented Apr 4, 2023

fix: collect coverage from vm script #13962

fix: collect coverage from vm script #13962

Conversation

GP4cK commented Feb 28, 2023 • edited Loading

Summary

Test plan

GP4cK commented Feb 28, 2023

SimenB commented Feb 28, 2023

GP4cK commented Feb 28, 2023 • edited Loading

SimenB commented Feb 28, 2023

SimenB commented Feb 28, 2023

GP4cK commented Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

GP4cK Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GP4cK Mar 1, 2023 • 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

Choose a reason for hiding this comment

GP4cK Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrazauskas Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 4, 2023

GP4cK commented Feb 28, 2023 •

edited

Loading

GP4cK commented Feb 28, 2023 •

edited

Loading

GP4cK commented Mar 1, 2023 •

edited

Loading

GP4cK Mar 1, 2023 •

edited

Loading

GP4cK Mar 1, 2023 •

edited

Loading

GP4cK Mar 1, 2023 •

edited

Loading

mrazauskas Mar 1, 2023 •

edited

Loading