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.

Sign up for GitHub

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

Jump to bottom

Incorrect empty file coverage #7388

Merged
merged 7 commits into from
Dec 24, 2018
+37 −13
Merged

Incorrect empty file coverage #7388

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d854088
Test transformed coverage from generateEmptyCoverage
mengdage Nov 20, 2018
1608330
Generate correct empty coverage
mengdage Nov 20, 2018
91fe2ee
Merge branch 'master' of https://github.com/facebook/jest into untest…
mengdage Nov 20, 2018
8dc0887
Update CHANGELOG.md
mengdage Nov 20, 2018
7bef9f4
Make rootDir a constant value.
mengdage Nov 20, 2018
d550165
Return sourceMapPath based on
mengdage Nov 22, 2018
22093dd
Merge branch 'master' into untested-file-coverage
mengdage Dec 3, 2018
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@

### Fixes

- `[jest-cli]` Fix empty coverage data for untested files ([#7388](https://github.com/facebook/jest/pull/7388))
- `[expect]` Standardize file naming in `expect` ([#7306](https://github.com/facebook/jest/pull/7306))
- `[jest-each]` Add empty array validation check ([#7249](https://github.com/facebook/jest/pull/7249))
- `[jest-cli]` Interrupt tests if interactive watch plugin key is pressed ([#7222](https://github.com/facebook/jest/pull/7222))
Expand Down
2 changes: 1 addition & 1 deletion packages/jest-cli/src/__tests__/__snapshots__/generateEmptyCoverage.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ Object {
"name": "(anonymous_0)",
},
},
"path": "/sum.js",
"path": "/tmp/sum.js",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the only change? Not sure how that shows the bug being fixed?

Copy link
Contributor Author

@mengdage mengdage Nov 22, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original unit test does not transpile the code. It does not pass any transformer.

src code --> jest.Runtime.ScriptTransformer without any transformer --> code without any change --> instrumenter --> coverageData

That's why the coverage data in the snapshot is CORRECT because it just instruments the src code. So the original snapchat is CORRECT! But in real life, we always use some transformer to process code like jsx, TS.

So what we want is:
src code --> jest.Runtime.ScriptTransformer with some transformer --> code transpiled --> instrumenter --> coverageData

However, the coverageData is for transpiled code, so we have to source map it to the original code.

Since we use babel, we should use babel-plugin-istanbul to instrument the code in order to instrument the ES6 code correctly. And when we use babel-plugin-istanbul, the coverageData is already for source map.

src code --> jest.Runtime.ScriptTransformer with some transformer and babel-plugin-istanbul --> code transpiled & coverageData.

Copy link
Contributor Author

@mengdage mengdage Nov 22, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason why I change the path which is the file path, is that babel requires the file path be inside the rootDir. I set the rootDir to /tmp and thus the path should be /tmp/sum.js

"s": Object {
"0": 0,
"1": 0,
Expand Down
29 changes: 26 additions & 3 deletions packages/jest-cli/src/__tests__/generateEmptyCoverage.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,11 @@
*/
'use strict';

import istanbulCoverage from 'istanbul-lib-coverage';
import libSourceMaps from 'istanbul-lib-source-maps';
import generateEmptyCoverage from '../generateEmptyCoverage';

const path = require('path');
const os = require('os');
const {makeGlobalConfig, makeProjectConfig} = require('../../../../TestUtils');

Expand All @@ -35,14 +38,34 @@ module.exports = {
};`;

it('generates an empty coverage object for a file without running it', () => {
const coverageMap = istanbulCoverage.createCoverageMap({});
const sourceMapStore = libSourceMaps.createSourceMapStore();
const rootDir = '/tmp';
const filepath = path.join(rootDir, './sum.js');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filepath must be under rootDir. Otherwise it will not be instrumented.


const emptyCoverage = generateEmptyCoverage(
src,
'/sum.js',
filepath,
makeGlobalConfig(),
makeProjectConfig({
cacheDirectory: os.tmpdir(),
rootDir: os.tmpdir(),
rootDir,
transform: [
['^.+\\.js$', path.join(__dirname, '../../../babel-jest/src/index.js')],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have to provide a transform. Otherwise, it will not be transformed.

mengdage marked this conversation as resolved.
Show resolved Hide resolved
],
}),
);
expect(emptyCoverage && emptyCoverage.coverage).toMatchSnapshot();

expect(typeof emptyCoverage).toBe('object');

let coverage = emptyCoverage && emptyCoverage.coverage;

if (emptyCoverage && emptyCoverage.sourceMapPath) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The result from generateEmptyCoverage is a coverage data and an optional sourceMapPath. If an sourceMapPath is returned, jest will use it to transform the coverage data.

Copy link
Contributor Author

@mengdage mengdage Nov 20, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The actually coverage for this piece of code after source map transformation is in this commit and looks like this

      ...
      "4": Object {
        "end": Object {
          "column": -1,
          "line": null,
        },
        "start": Object {
          "column": 4,
          "line": 8,
        },
      },
      "5": Object {
        "end": Object {
          "column": -1,
          "line": null,
        },
        "start": Object {
          "column": 0,
          "line": 12,
        },
      ...

coverageMap.addFileCoverage(emptyCoverage.coverage);
sourceMapStore.registerURL(filepath, emptyCoverage.sourceMapPath);

coverage = sourceMapStore.transformCoverage(coverageMap).map;
}

expect(coverage).toMatchSnapshot();
});
18 changes: 10 additions & 8 deletions packages/jest-cli/src/generateEmptyCoverage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,17 @@

import type {GlobalConfig, ProjectConfig, Path} from 'types/Config';

import {createInstrumenter} from 'istanbul-lib-instrument';
import {readInitialCoverage} from 'istanbul-lib-instrument';
import {classes} from 'istanbul-lib-coverage';
import Runtime from 'jest-runtime';

export type CoverageWorkerResult = {|
coverage: any,
sourceMapPath: ?string,
|};

const FileCoverage = classes.FileCoverage;
mengdage marked this conversation as resolved.
Show resolved Hide resolved

export default function(
source: string,
filename: Path,
Expand All @@ -29,16 +32,15 @@ export default function(
collectCoverageOnlyFrom: globalConfig.collectCoverageOnlyFrom,
};
if (Runtime.shouldInstrument(filename, coverageOptions, config)) {
// Transform file without instrumentation first, to make sure produced
// source code is ES6 (no flowtypes etc.) and can be instrumented
// Transform file with instrumentation to make sure initial coverage data is well mapped to original code.
const transformResult = new Runtime.ScriptTransformer(
config,
).transformSource(filename, source, false);
const instrumenter = createInstrumenter();
instrumenter.instrumentSync(transformResult.code, filename);
).transformSource(filename, source, true);
const extracted = readInitialCoverage(transformResult.code);

return {
coverage: instrumenter.fileCoverage,
sourceMapPath: transformResult.sourceMapPath,
coverage: new FileCoverage(extracted.coverageData),
sourceMapPath: null,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sourceMapPath should still be relevant here. Runtime.ScriptTransformer may call out to custom transformers (instead of babel-jest) that do a two-step pass (compile, then instrument) similar to what was happening here originally. Without a source map, we'd just have coverage data for compiled code, not source code. It's the way things currently work for non-empty coverage.

};
} else {
return null;
Expand Down