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: collect coverage from vm script #13962

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -17,6 +17,7 @@
- `[jest-circus]` Update message printed on test timeout ([#13830](https://github.com/facebook/jest/pull/13830))
- `[jest-circus]` Avoid creating the word "testfalse" when `takesDoneCallback` is `false` in the message printed on test timeout AND updated timeouts test ([#13954](https://github.com/facebook/jest/pull/13954))
- `[@jest/test-result]` Allow `TestResultsProcessor` type to return a Promise ([#13950](https://github.com/facebook/jest/pull/13950))
- `[jest-runtime]` Support coverage with v8 for scripts loaded with `vm.runInContext`. Note: `collectCoverageFrom` must be set for it to work ([#](https://github.com/facebook/jest/pull/13962))

### Chore & Maintenance

Expand Down
9 changes: 9 additions & 0 deletions e2e/__tests__/__snapshots__/coverageProviderV8.test.ts.snap
Expand Up @@ -105,3 +105,12 @@ All files | 59.37 | 33.33 | 33.33 | 59.37 |
uncovered.js | 0 | 0 | 0 | 0 | 1-8
--------------|---------|----------|---------|---------|-------------------"
`;

exports[`vm script coverage generator 1`] = `
"-------------|---------|----------|---------|---------|-------------------
File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
-------------|---------|----------|---------|---------|-------------------
All files | 88.88 | 100 | 66.66 | 88.88 |
vmscript.js | 88.88 | 100 | 66.66 | 88.88 | 20-22
-------------|---------|----------|---------|---------|-------------------"
`;
12 changes: 12 additions & 0 deletions e2e/__tests__/coverageProviderV8.test.ts
Expand Up @@ -106,3 +106,15 @@ test('prints correct coverage report, if a TS module is transpiled by custom tra
expect(exitCode).toBe(0);
expect(stdout).toMatchSnapshot();
});

test('vm script coverage generator', () => {
const dir = path.resolve(__dirname, '../vmscript-coverage');
const {stdout, exitCode} = runJest(
dir,
['--coverage', '--coverage-provider', 'v8'],
{stripAnsi: true},
);

expect(exitCode).toBe(0);
expect(stdout).toMatchSnapshot();
});
42 changes: 42 additions & 0 deletions e2e/vmscript-coverage/__tests__/extract-coverage.test.js
@@ -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);
});
9 changes: 9 additions & 0 deletions e2e/vmscript-coverage/package.json
@@ -0,0 +1,9 @@
{
"jest": {
"rootDir": "./",
"testEnvironment": "node",
"collectCoverageFrom": [
"package/**/*.js"
]
}
}
27 changes: 27 additions & 0 deletions e2e/vmscript-coverage/package/vmscript.js
@@ -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));
}
21 changes: 19 additions & 2 deletions packages/jest-runtime/src/index.ts
Expand Up @@ -53,7 +53,12 @@ import type {MockMetadata, ModuleMocker} from 'jest-mock';
import {escapePathForRegex} from 'jest-regex-util';
import Resolver, {ResolveModuleConfig} from 'jest-resolve';
import {EXTENSION as SnapshotExtension} from 'jest-snapshot';
import {createDirectory, deepCyclicCopy} from 'jest-util';
import {
createDirectory,
deepCyclicCopy,
globsToMatcher,
replacePathSepForGlob,
} from 'jest-util';
import {
createOutsideJestVmPath,
decodePossibleOutsideJestVmPath,
Expand Down Expand Up @@ -1260,14 +1265,26 @@ export default class Runtime {
throw new Error('You need to call `stopCollectingV8Coverage` first.');
}

const coverageMatcher = this._coverageOptions.collectCoverageFrom.length
? globsToMatcher(this._coverageOptions.collectCoverageFrom)
: undefined;
const matchesCoveragePattern = (file: string) => {
if (coverageMatcher) {
return coverageMatcher(
replacePathSepForGlob(path.relative(this._config.rootDir, file)),
);
}
return this._v8CoverageSources!.has(file);
};

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),
Copy link
Member

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 🙈)

Copy link
Contributor Author

@GP4cK GP4cK Mar 1, 2023

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 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

  1. remove this line
  2. 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

Copy link
Contributor

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 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.

Copy link
Contributor Author

@GP4cK GP4cK Mar 1, 2023

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

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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 and collectCoverageFrom option could be used to override that. This is what your current implementation does. If I got it right.

Copy link
Member

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 report

Copy link
Contributor Author

@GP4cK GP4cK Mar 1, 2023

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 on shouldInstrument and added cssTransform.js to the coveragePathIgnorePatterns in the other tests. I hope that's ok.

Copy link
Member

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 👍

)
.map(result => {
Expand Down