-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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: collect coverage from vm script #13962
Closed
Closed
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
71c9abe
fix: collect coverage from vm script
GP4cK e5c0cfa
style: format vmscript-coverage/package.json with prettier
GP4cK 9ac74c5
chore: update comment Facebook -> Meta
GP4cK 9202548
fix: fix coverage
GP4cK cb0f4d1
chore: update changelog
GP4cK a338b20
refactor: only rely on shouldInstrument
GP4cK 9fcfe2f
chore: update changelog
GP4cK File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
/** | ||
* Copyright (c) Meta Platforms, Inc. and affiliates. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
const fs = require('fs'); | ||
const path = require('path'); | ||
const vm = require('vm'); | ||
const filePath = path.resolve(__dirname, '../package/vmscript.js'); | ||
|
||
test('extract coverage', () => { | ||
const content = fs.readFileSync(filePath, {encoding: 'utf8'}); | ||
|
||
const case1 = vm.runInNewContext( | ||
content, | ||
{ | ||
inputObject: { | ||
number: 0, | ||
}, | ||
}, | ||
{ | ||
filename: filePath, | ||
}, | ||
); | ||
|
||
const case2 = vm.runInNewContext( | ||
content, | ||
{ | ||
inputObject: { | ||
number: 7, | ||
}, | ||
}, | ||
{ | ||
filename: filePath, | ||
}, | ||
); | ||
|
||
expect(case1).toBe(false); | ||
expect(case2).toBe(true); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
{ | ||
"jest": { | ||
"rootDir": "./", | ||
"testEnvironment": "node", | ||
"collectCoverageFrom": [ | ||
"package/**/*.js" | ||
] | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
/** | ||
* Copyright (c) Meta Platforms, Inc. and affiliates. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
function addOne(inputNumber) { | ||
return ++inputNumber; | ||
} | ||
|
||
function isEven(inputNumber) { | ||
if (inputNumber % 2 === 0) { | ||
return true; | ||
} else { | ||
return false; | ||
} | ||
} | ||
|
||
function notCovered() { | ||
return 'not covered'; | ||
} | ||
|
||
/* global inputObject */ | ||
if (inputObject.number / 1 === inputObject.number) { | ||
isEven(addOne(inputObject.number)); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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 🙈)
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.
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 callingjest.resetModules()
.However, I don't see why we would need this in the filter. The 2 other checks should be enough:
This is why this PR was removing it: #10657
So if you agree, I will
cssTransform.js
in the other test withcoveragePathIgnorePatterns
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
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.
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 haveall
,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.
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.
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
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.
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.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.
@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
?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.
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 andcollectCoverageFrom
option could be used to override that. This is what your current implementation does. If I got it right.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.
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 reportThere 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.
@SimenB you're right, my change didn't make sense.
I've removed the line
this._v8CoverageSources!.has(res.url)
so we only rely onshouldInstrument
and addedcssTransform.js
to thecoveragePathIgnorePatterns
in the other tests. I hope that's ok.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.
Right, this seems more reasonable to me 👍