Incorrect empty file coverage #7388

mengdage · 2018-11-20T02:59:29Z

Summary

When a file is not covered by any unit tests, jest considers it as an untested file and generate empty coverage for it. However, the empty coverage is generated without using babel-plugin-istanbul and the coverage data is incorrect.

Here is the issue I created to describe the bug. Here is the repo to reproduce the bug.

Fixes #7302

Test plan

The existing unit test for generateEmptyCoverage is updated.

…ed-file-coverage

codecov-io · 2018-11-20T03:47:00Z

Codecov Report

Merging #7388 into master will increase coverage by 0.81%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #7388      +/-   ##
=========================================
+ Coverage   66.79%   67.6%   +0.81%     
=========================================
  Files         242     240       -2     
  Lines        9371    9258     -113     
  Branches        5       6       +1     
=========================================
  Hits         6259    6259              
+ Misses       3110    2997     -113     
  Partials        2       2

Impacted Files	Coverage Δ
packages/jest-cli/src/generateEmptyCoverage.js	`85.71% <100%> (ø)`	⬆️
packages/jest-jasmine2/src/jasmine/Env.js	`0% <0%> (ø)`	⬆️
packages/jest-jasmine2/src/errorOnPrivate.js	`0% <0%> (ø)`	⬆️
packages/jest-validate/src/index.js	`0% <0%> (ø)`	⬆️
...ackages/jest-jasmine2/src/assertionErrorMessage.js	`0% <0%> (ø)`	⬆️
...ackages/jest-jasmine2/src/jasmine/JsApiReporter.js	`0% <0%> (ø)`	⬆️
...ackages/jest-watcher/src/lib/patternModeHelpers.js	`0% <0%> (ø)`	⬆️
packages/jest-jasmine2/src/each.js	`0% <0%> (ø)`	⬆️
packages/jest-runner/src/test_worker.js	`0% <0%> (ø)`	⬆️
packages/jest-watcher/src/BaseWatchPlugin.js	`0% <0%> (ø)`	⬆️
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 993bc5c...d550165. Read the comment docs.

mengdage · 2018-11-20T03:43:22Z

packages/jest-cli/src/__tests__/generateEmptyCoverage.test.js

+  const coverageMap = istanbulCoverage.createCoverageMap({});
+  const sourceMapStore = libSourceMaps.createSourceMapStore();
+  const rootDir = '/tmp';
+  const filepath = path.join(rootDir, './sum.js');


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

mengdage · 2018-11-20T03:45:10Z

packages/jest-cli/src/__tests__/generateEmptyCoverage.test.js

-      rootDir: os.tmpdir(),
+      rootDir,
+      transform: [
+        ['^.+\\.js$', path.join(__dirname, '../../../babel-jest/src/index.js')],


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

mengdage · 2018-11-20T03:48:37Z

packages/jest-cli/src/__tests__/generateEmptyCoverage.test.js

+
+  let coverage = emptyCoverage && emptyCoverage.coverage;
+
+  if (emptyCoverage && emptyCoverage.sourceMapPath) {


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.

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

SimenB

Thanks for PR! Could you explain a bit more how the current approach is wrong, and what it now looks like?

packages/jest-cli/src/__tests__/generateEmptyCoverage.test.js

packages/jest-cli/src/generateEmptyCoverage.js

SimenB · 2018-11-20T07:27:25Z

packages/jest-cli/src/__tests__/__snapshots__/generateEmptyCoverage.test.js.snap

@@ -75,7 +75,7 @@ Object {
      "name": "(anonymous_0)",
    },
  },
-  "path": "/sum.js",
+  "path": "/tmp/sum.js",


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

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.

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

jwbay · 2018-11-20T11:12:15Z

packages/jest-cli/src/generateEmptyCoverage.js

-      coverage: instrumenter.fileCoverage,
-      sourceMapPath: transformResult.sourceMapPath,
+      coverage: new FileCoverage(extracted.coverageData),
+      sourceMapPath: null,


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.

mengdage · 2018-11-22T21:02:53Z

packages/jest-cli/src/generateEmptyCoverage.js

-      coverage: instrumenter.fileCoverage,
-      sourceMapPath: transformResult.sourceMapPath,
+      coverage: new FileCoverage(extracted.coverageData),
+      sourceMapPath: mapCoverage ? sourceMapPath : null,


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

mengdage · 2018-12-03T01:52:55Z

@SimenB @jwbay @aaronabramov Any thoughts on this PR?

mengdage · 2018-12-06T18:37:32Z

Hi @SimenB, when do you think you can merge this PR?

SimenB · 2018-12-24T13:37:48Z

This is awesome, it fixed #7202! Just because it was empty, but still

/cc @thymikee

github-actions · 2021-05-12T05:13:50Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

mengdage added 2 commits November 19, 2018 21:38

Test transformed coverage from generateEmptyCoverage

d854088

Generate correct empty coverage

1608330

facebook-github-bot added the cla signed label Nov 20, 2018

mengdage added 3 commits November 19, 2018 22:07

Merge branch 'master' of https://github.com/facebook/jest into untest…

91fe2ee

…ed-file-coverage

Update CHANGELOG.md

8dc0887

Make rootDir a constant value.

7bef9f4

mengdage commented Nov 20, 2018

View reviewed changes

SimenB reviewed Nov 20, 2018

View reviewed changes

SimenB requested a review from aaronabramov November 20, 2018 07:30

jwbay reviewed Nov 20, 2018

View reviewed changes

Return sourceMapPath based on

d550165

mengdage commented Nov 22, 2018

View reviewed changes

SimenB requested a review from rickhanlonii December 3, 2018 07:23

SimenB approved these changes Dec 3, 2018

View reviewed changes

Merge branch 'master' into untested-file-coverage

22093dd

SimenB merged commit f7f65bf into jestjs:master Dec 24, 2018

alcferreira mentioned this pull request Jan 30, 2019

Error Cannot read property converageData of null #7758

Closed

captain-yossarian pushed a commit to captain-yossarian/jest that referenced this pull request Jul 18, 2019

Incorrect empty file coverage (jestjs#7388)

b7d8939

github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Incorrect empty file coverage #7388

Incorrect empty file coverage #7388

mengdage commented Nov 20, 2018 •

edited by thymikee

codecov-io commented Nov 20, 2018 •

edited

mengdage Nov 20, 2018

mengdage Nov 20, 2018

mengdage Nov 20, 2018

mengdage Nov 20, 2018 •

edited

SimenB left a comment

SimenB Nov 20, 2018

mengdage Nov 22, 2018 •

edited

mengdage Nov 22, 2018 •

edited

jwbay Nov 20, 2018

mengdage Nov 22, 2018

mengdage commented Dec 3, 2018

mengdage commented Dec 6, 2018 •

edited

SimenB commented Dec 24, 2018

github-actions bot commented May 12, 2021


		let coverage = emptyCoverage && emptyCoverage.coverage;

		if (emptyCoverage && emptyCoverage.sourceMapPath) {

Incorrect empty file coverage #7388

Incorrect empty file coverage #7388

Conversation

mengdage commented Nov 20, 2018 • edited by thymikee

Summary

Test plan

codecov-io commented Nov 20, 2018 • edited

Codecov Report

mengdage Nov 20, 2018

Choose a reason for hiding this comment

mengdage Nov 20, 2018

Choose a reason for hiding this comment

mengdage Nov 20, 2018

Choose a reason for hiding this comment

mengdage Nov 20, 2018 • edited

Choose a reason for hiding this comment

SimenB left a comment

Choose a reason for hiding this comment

SimenB Nov 20, 2018

Choose a reason for hiding this comment

mengdage Nov 22, 2018 • edited

Choose a reason for hiding this comment

mengdage Nov 22, 2018 • edited

Choose a reason for hiding this comment

jwbay Nov 20, 2018

Choose a reason for hiding this comment

mengdage Nov 22, 2018

Choose a reason for hiding this comment

mengdage commented Dec 3, 2018

mengdage commented Dec 6, 2018 • edited

SimenB commented Dec 24, 2018

github-actions bot commented May 12, 2021

mengdage commented Nov 20, 2018 •

edited by thymikee

codecov-io commented Nov 20, 2018 •

edited

mengdage Nov 20, 2018 •

edited

mengdage Nov 22, 2018 •

edited

mengdage Nov 22, 2018 •

edited

mengdage commented Dec 6, 2018 •

edited