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
Merged

Incorrect empty file coverage #7388

Show file tree
Hide file tree
Changes from all 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
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 @@ -36,6 +36,7 @@

### Fixes

- `[jest-cli]` Fix empty coverage data for untested files ([#7388](https://github.com/facebook/jest/pull/7388))
- `[jest-cli]` [**BREAKING**] Do not use `text-summary` coverage reporter by default if other reporters are configured ([#7058](https://github.com/facebook/jest/pull/7058))
- `[jest-mock]` [**BREAKING**] Fix bugs with mock/spy result tracking of recursive functions ([#6381](https://github.com/facebook/jest/pull/6381))
- `[jest-haste-map]` [**BREAKING**] Recover files correctly after haste name collisions are fixed ([#7329](https://github.com/facebook/jest/pull/7329))
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
Loading

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
Loading

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
27 changes: 24 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,32 @@ 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$', require.resolve('babel-jest')]],
}),
);
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
Loading

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();
});
20 changes: 11 additions & 9 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;

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
const transformResult = new Runtime.ScriptTransformer(
// Transform file with instrumentation to make sure initial coverage data is well mapped to original code.
const {code, mapCoverage, sourceMapPath} = new Runtime.ScriptTransformer(
config,
).transformSource(filename, source, false);
const instrumenter = createInstrumenter();
instrumenter.instrumentSync(transformResult.code, filename);
).transformSource(filename, source, true);
const extracted = readInitialCoverage(code);

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

Choose a reason for hiding this comment

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

@jwbay I think mapCoverage indicates whether the coverageData should be mapped back to original code. If should, we return the sourceMapPath, and coverage will be transformed automatically. Otherwise, such as when we use babel with babel-plugin-istanbul, the coverageData is already for source map so we don't pass back the source map.

};
} else {
return null;
Expand Down