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

Incorrect empty file coverage #7388

Merged
merged 7 commits into from
Dec 24, 2018
Merged

Conversation

mengdage
Copy link
Contributor

@mengdage mengdage commented Nov 20, 2018

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.

@codecov-io
Copy link

codecov-io commented Nov 20, 2018

Codecov Report

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

Impacted file tree graph

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

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.

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.


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

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

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

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/generateEmptyCoverage.js Outdated Show resolved Hide resolved
@@ -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

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

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

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.

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.

@mengdage
Copy link
Contributor Author

mengdage commented Dec 3, 2018

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

@mengdage
Copy link
Contributor Author

mengdage commented Dec 6, 2018

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

@SimenB SimenB merged commit f7f65bf into jestjs:master Dec 24, 2018
@SimenB
Copy link
Member

SimenB commented Dec 24, 2018

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

/cc @thymikee

captain-yossarian pushed a commit to captain-yossarian/jest that referenced this pull request Jul 18, 2019
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Empty coverage is not generated correctly
5 participants