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
Changes from all commits
71c9abe
e5c0cfa
9ac74c5
9202548
cb0f4d1
a338b20
9fcfe2f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
{ | ||
"jest": { | ||
"rootDir": "./", | ||
"testEnvironment": "node", | ||
"collectCoverageFrom": [ | ||
"package/**/*.js" | ||
] | ||
} | ||
} |
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)); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1267,7 +1267,6 @@ export default class Runtime { | |
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) && | ||
shouldInstrument(res.url, this._coverageOptions, this._config), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 blamed the file and I see that 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
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 So it made sense that If I didn't miss some detail. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ... or it's ok to have There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let’s wait for Simen’s feedback. He was commenting that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My thought was that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @SimenB you're right, my change didn't make sense. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, this seems more reasonable to me 👍 |
||
) | ||
.map(result => { | ||
|
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.
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
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.
I'm good with your proposal. Thanks!