Fix coverage test for Babel 7 #7202

SimenB · 2018-10-18T08:15:36Z

Summary

Opening this up separately to keep it focused. I think this is a bug?

Essentially https://github.com/facebook/jest/blob/ed671678796d4ceb7a6744e36490316ceedff5c2/e2e/coverage-report/notRequiredInTestSuite.js#L8-L11 is not reported as not covered. This is how the generated html report looks:

So the throw is no longer counted as a statement, which is wrong. (right?)

@bcoe any idea what's going on here? Help would be greatly appreciated 🙂 Background is that we're moving to Babel 7 (dropping 6). I'm not sure if it's a difference in the last versions of the babel istanbul plugin or if we're doing something odd.
Full diff against master can be seen in #7016.

Relevant istanbul dependnecy changes:

Test plan

I'd like for this PR to have 0 diff in the coverage snapshots, while still passing those tests (others will fail). Then I'll merge it into #7016.

The relevant test is e2e/__tests__/coverage_report.test.js

e2e/__tests__/__snapshots__/coverage_report.test.js.snap

SimenB · 2018-10-18T08:20:14Z

e2e/__tests__/coverage_report.test.js

@@ -17,7 +17,7 @@ const DIR = path.resolve(__dirname, '../coverage-report');

 test('outputs coverage report', () => {
  const {stdout, status} = runJest(DIR, ['--no-cache', '--coverage']);
-  const coverageDir = path.resolve(__dirname, '../coverage-report/coverage');
+  const coverageDir = path.join(DIR, 'coverage');


ignore changes in this file, this is just an unrelated quick cleanup

SimenB · 2018-10-20T20:54:45Z

@coreyfarrell @JaKXz thoughts on this one?

coreyfarrell · 2018-10-20T21:59:06Z

I just copied notRequiredInTestSuite.js to a local folder, ran:
npx nyc --all --reporter html --reporter text ls.

This produced the report I expected. It reported 0/5 statements and 0/5 lines covered. The same 2 branches and 1 function not covered as your report. I'm also seeing the expected transformation when I run npx nyc instrument notRequiredInTestSuite.js.

From grep it looks like jest instruments the source here:
https://github.com/facebook/jest/blob/7be2a8ef3f4d17b3ca139ba1a3dab72134306b50/packages/jest-cli/src/generateEmptyCoverage.js#L37-L38

If so can you post the value of transformResult.code for notRequriedInTestSuite.js? If not please point me to how jest instruments notRequiredInTestSuite.js in this failing case.

coreyfarrell · 2018-10-20T22:09:59Z

Just want to mention I downloaded PR 7016 when I was looking at the jest sources locally. I notice now that the link I gave to is not that PR's revision but those two lines don't appear to be different.

Also when testing your file with nyc I tried removing all options from our call to createInstrumenter and still got the expected result.

SimenB · 2018-10-20T22:26:13Z

Instrumentation of files not required during the process of the test happens in the code you linked to.

Jest instruments other stuff by using the babel plugin: https://github.com/facebook/jest/blob/7be2a8ef3f4d17b3ca139ba1a3dab72134306b50/packages/jest-runtime/src/script_transformer.js#L171-L189 and https://github.com/facebook/jest/blob/7be2a8ef3f4d17b3ca139ba1a3dab72134306b50/packages/babel-jest/src/index.js#L129-L142

SimenB · 2018-10-20T22:39:11Z

transformResult.code being passed to instrumentSync is:

"use strict";

/**
 * Copyright (c) 2014-present, Facebook, Inc. All rights reserved.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 */
throw new Error(`this error should not be a problem because` + `this file is never required or executed`); // Flow annotations to make sure istanbul can instrument non ES6 source

/* eslint-disable no-unreachable */

module.exports = function (j, d) {
  if (j) {
    return d;
  } else {
    return j + d;
  }
};
//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbIm5vdFJlcXVpcmVkSW5UZXN0U3VpdGUuanMiXSwibmFtZXMiOlsiRXJyb3IiLCJtb2R1bGUiLCJleHBvcnRzIiwiaiIsImQiXSwibWFwcGluZ3MiOiI7O0FBQUE7Ozs7OztBQU9BLE1BQU0sSUFBSUEsS0FBSixDQUNILDRDQUFELEdBQ0cseUNBRkMsQ0FBTixDLENBS0E7O0FBQ0E7O0FBQ0FDLE1BQU0sQ0FBQ0MsT0FBUCxHQUFpQixVQUFTQyxDQUFULEVBQW9CQyxDQUFwQixFQUF1QztBQUN0RCxNQUFJRCxDQUFKLEVBQU87QUFDTCxXQUFPQyxDQUFQO0FBQ0QsR0FGRCxNQUVPO0FBQ0wsV0FBT0QsQ0FBQyxHQUFHQyxDQUFYO0FBQ0Q7QUFDRixDQU5EIiwic291cmNlc0NvbnRlbnQiOlsiLyoqXG4gKiBDb3B5cmlnaHQgKGMpIDIwMTQtcHJlc2VudCwgRmFjZWJvb2ssIEluYy4gQWxsIHJpZ2h0cyByZXNlcnZlZC5cbiAqXG4gKiBUaGlzIHNvdXJjZSBjb2RlIGlzIGxpY2Vuc2VkIHVuZGVyIHRoZSBNSVQgbGljZW5zZSBmb3VuZCBpbiB0aGVcbiAqIExJQ0VOU0UgZmlsZSBpbiB0aGUgcm9vdCBkaXJlY3Rvcnkgb2YgdGhpcyBzb3VyY2UgdHJlZS5cbiAqL1xuXG50aHJvdyBuZXcgRXJyb3IoXG4gIGB0aGlzIGVycm9yIHNob3VsZCBub3QgYmUgYSBwcm9ibGVtIGJlY2F1c2VgICtcbiAgICBgdGhpcyBmaWxlIGlzIG5ldmVyIHJlcXVpcmVkIG9yIGV4ZWN1dGVkYFxuKTtcblxuLy8gRmxvdyBhbm5vdGF0aW9ucyB0byBtYWtlIHN1cmUgaXN0YW5idWwgY2FuIGluc3RydW1lbnQgbm9uIEVTNiBzb3VyY2Vcbi8qIGVzbGludC1kaXNhYmxlIG5vLXVucmVhY2hhYmxlICovXG5tb2R1bGUuZXhwb3J0cyA9IGZ1bmN0aW9uKGo6IHN0cmluZywgZDogc3RyaW5nKTogc3RyaW5nIHtcbiAgaWYgKGopIHtcbiAgICByZXR1cm4gZDtcbiAgfSBlbHNlIHtcbiAgICByZXR1cm4gaiArIGQ7XG4gIH1cbn07XG4iXX0=

coreyfarrell · 2018-10-20T23:18:03Z

Sorry to ask such a simple question but how can I run e2e/__tests__/coverage_report.test.js only?

One thing I noticed in the code you just linked is babelrc: false. This does not block use of options from babel.config.js, it's possible you need configFile: false instead. I point this out since it looks like #7016 renames .babelrc files to babel.config.js.

SimenB · 2018-10-20T23:26:11Z

Interestingly, there are 5 statements being returned in fileCoverage from that instrument call:

We feed that into a CoverageMap: https://github.com/facebook/jest/blob/7be2a8ef3f4d17b3ca139ba1a3dab72134306b50/packages/jest-cli/src/reporters/coverage_reporter.js#L177

But for some reason only 4 show up in the reports?

Sorry to ask such a simple question but how can I run e2e/__tests__/coverage_report.test.js only?

Do yarn jest e2e/__tests__/coverage_report.test.js from the root of the repository.

However, that will spawn jest in a subprocess. To debug the actual test, I do this.

$ cd e2e/coverage-report
$ ndb ../../packages/jest/bin/jest.js -i --coverage --watchAll

You can also do just node ../../packages/jest/bin/jest.js --coverage and you can inspect the reports generated.

ndb: https://github.com/GoogleChromeLabs/ndb

One thing I noticed in the code you just linked is babelrc: false. This does not block use of options from babel.config.js, it's possible you need configFile: false instead. I point this out since it looks like #7016 renames .babelrc files to babel.config.js.

Yeah, sorry, I'm linking to code in master. This is from the babel 7 branch:
https://github.com/facebook/jest/blob/e3a4df488fd4b161b4d04d545c8425fe15308835/packages/jest-runtime/src/script_transformer.js#L171-L193
https://github.com/facebook/jest/blob/e3a4df488fd4b161b4d04d545c8425fe15308835/packages/babel-jest/src/index.js#L87-L100

It has configFile: false.

coreyfarrell · 2018-10-21T00:50:15Z

It appears the loss occurs in istanbul-lib-source-maps.
https://github.com/facebook/jest/blob/9d5ead81bd5ad067544da8fc655f672a232445ff/packages/jest-cli/src/reporters/coverage_reporter.js#L86-L88
After this map.data contains 4 statements for notRequiredInTestSuite.js, this._coverageMap correctly contains the 5 statements. This is as far as I can troubleshoot right now.

SimenB · 2018-10-21T09:47:00Z

To isolate things, I've set up a repository with 3 commits:

using babel 6 and the old istanbul module
upgrading istanbul to latest
upgrading to babel 7

Interestingly, the report is correct after step 2, it's only after changing to Babel 7 for the transpilation that we get the error. Could this be a babel bug instead?

See https://github.com/SimenB/weird-coverage-babel7

coreyfarrell · 2018-10-21T10:44:46Z

I'll have to take a look at this after I wake up some more (it's 6:30AM here and I'm just starting my coffee). Before reporting to babel I'd like to take a look at the instrumented source and source-map from commits 2 and 3. This way we can see what differences in source & source-map, determine if babel 7 is actually doing something wrong, or simply doing something different that istanbul-lib-source-maps doesn't properly handle. Would probably help to use createInstrumenter({compact:false}) so that the instrument step doesn't produce compact code and it'll be easier to diff the results.

Interesting thing about your 3 step test, istanbul-lib-instrument@3.0.0 uses babel 7 to translate the source.

coreyfarrell · 2018-10-21T20:07:52Z

I've collected some additional data though my knowledge of source maps is limited.
https://github.com/coreyfarrell/weird-coverage-babel7/tree/working patched index.js to save sources/source-maps after babel and istanbul. Unfortunately I really don't what I'm looking at with the source maps. They're different but I think that's expected since the babel output is different.

SimenB · 2018-10-21T20:21:59Z

I know nothing about how source-maps work...

@loganfsmyth any thoughts or ideas here?

The interesting part is probably if you check out https://github.com/SimenB/weird-coverage-babel7, or @coreyfarrell's fork linked above.

node index there reports 0/4 statements covered, but if you revert the last commit (which changes from using babel 7 to babel 6 for transpilation) it reports 0/5 statements. 5 is correct

loganfsmyth · 2018-10-21T21:23:03Z

Hmm, this is the tough part of sourcemaps. I'd consider this an issue with how istanbul takes maps into account.

On the first line of JS code, with Babel 7, istanbul is bailing out at https://github.com/gotwarlost/istanbul-lib-source-maps/blob/8c43f14e48baeae6bb461639b4c91fe73c693ce0/lib/transformer.js#L27 because the end didn't map back to a location.

In this case, Babel maps the single space between ; and // Flow comment to no original location, because there isn't one really. The problem is that Istanbul tries to map line start and end location, and since it doesn't get an end location, it skips mapping the whole line.

SimenB · 2018-10-21T22:39:54Z

Oh that's interesting! Thanks for digging into it.
Removing that comment indeed fixes it! Or, "fixes" it I guess. Is this something that's possible to fix in babel? From the outside, it seems like a bug to not properly map back to where a statement ends, no? Or do you think Istanbul shouldn't care that it doesn't know where the statement ends?

Btw, what tool do you use to look at the source maps? Seems useful 🙂

loganfsmyth · 2018-10-21T22:56:10Z

That's a screenshot from https://sokra.github.io/source-map-visualization/

From the outside, it seems like a bug to not properly map back to where a statement ends, no?

The issue in this case is that Babel does map the line with an exactly end location, but the way that istanbul is trying to process those ranges isn't a good way to do it. The reason it's failing is because it is essentially asking where the end of the statements maps back to, and rightly, there is no location, because the mapping ended an exactly the location it is asking about. Think of it like this:

abc;
// foo

compiled to

abc; // foo

Babel essentials says that line 1 column 0 maps to line 0 column 0, and then Babel says line 1 column 4 maps nowhere, and then it says line 1 column 5 maps to line 2 column 0.

Instanbul is getting the end location of the statement (line 1 column 4), and saying "where does this map in the original code", and the answer is no-where, because the space between ; and // doesn't have an original location.

Essentially Istanbul is asking the wrong question, in my opinion, because it is trying to ask "where does abc; start and end in the original file, but what it is actually asking is where a starts in the original file, and where (empty space between ; and //) starts in the original file.

Answering the question of where a range maps to is not an easy question to answer with sourcemaps, but I think the way istanbul is doing it here is asking to run into issues like this.

Edit: Deleted like 10 duplicate copies of this comment 😬

bcoe · 2018-10-22T06:32:00Z

@SimenB @loganfsmyth if there's a reasonable simple fix to the logic to make Istanbul more forgiving, I'd happily fix it on our end -- haven't had a chance to dig yet.

loganfsmyth · 2018-10-23T02:30:05Z

Wow, sorry my comment got posted so many times. With the Github issue yesterday it kept saying it hadn't gone through.

@bcoe Yeah I'd have to dig into it more to see how it might approach it. I'm not realistically sure I'll have time any time soon though, so I'm hesitant to make any promises. I'd probably be field questions from someone else if they wanted to try to tackle it though.

SimenB · 2018-11-20T12:04:51Z

Rebased this, FWIW. @bcoe @loganfsmyth you wouldn't by any chance have the bandwidth to handle this now? 🙏

@jwbay maybe you know? (see the last few comments, identifying how the current approach in Istanbul is a bit brittle)

bcoe · 2018-11-20T17:43:48Z

@SimenB with the long weekend coming up, I can try to carve out some time for this 👍 (have been pretty heads down with my on going scheme to help move coverage to v8 and Node.js core).

From your digging @loganfsmyth, it sounds like the problem is in the instrumenter potentially not being permissive enough when remapping lines of code from instrumented code back to the original?

@SimenB if we can figure out a reproduction here, it would make it easier or me to turn around a fix quickly.

SimenB · 2018-11-20T18:15:07Z

@bcoe Awesome, thank you!

Regarding a reprodcuction, there is https://github.com/SimenB/weird-coverage-babel7, which is relatively minimal, just using @babel/core and 4 istanbul packages.

bcoe · 2018-12-02T22:48:11Z

@loganfsmyth thank you for the reference implementation; @SimenB I'll see if I can't port it to an actual pull tonight.

thymikee · 2018-12-10T12:43:00Z

@bcoe did you find some time to take a look? Wonder if there are any blockers

SimenB · 2018-12-11T22:30:11Z

Opened up istanbuljs/istanbuljs#254

bcoe · 2018-12-12T05:53:53Z

@loganfsmyth @SimenB please see istanbuljs/istanbuljs#255

SimenB · 2018-12-18T08:35:29Z

We'll land Babel 7 with the slightly buggy coverage. Hopefully the alpha doesn't break too many people. But I think we should wait for the fix to land in istanbul (especially as it seems it'll be released as a major) before making Jest 24 stable.

I updated this branch by manually updating node_modules with the updated version from the PR above and pushed that.

bcoe · 2018-12-19T04:13:37Z

@SimenB @thymikee please give:

Successfully published:
 - istanbul-api@2.0.7
 - istanbul-lib-source-maps@3.0.0
 - istanbul-reports@2.0.2

a shot, the more we test the better since it's a relatively major change to what coverage JSON output looks like.

SimenB · 2018-12-19T08:03:43Z

Awesome, thanks @bcoe! I updated, and will merge when CI is happy (the coverage tests passed locally, so no reason for it not to be happy).

We'll be releasing a new alpha today, hopefully it'll be without issue for folks 🙂

thymikee · 2018-12-19T08:17:49Z

Node 8:

ReferenceError: location is not defined
    at getMapping (/home/circleci/jest/node_modules/istanbul-lib-source-maps/lib/transformer.js:121:19)

I'll restart to see if it's something flaky

SimenB · 2018-12-19T08:19:03Z

Hmm, we got a failure in our coverage run:

ReferenceError: location is not defined
    at getMapping (/home/circleci/jest/node_modules/istanbul-lib-source-maps/lib/transformer.js:121:19)
    at /home/circleci/jest/node_modules/istanbul-lib-source-maps/lib/transformer.js:155:23
    at Array.forEach (<anonymous>)
    at SourceMapTransformer.processFile (/home/circleci/jest/node_modules/istanbul-lib-source-maps/lib/transformer.js:152:34)
    at /home/circleci/jest/node_modules/istanbul-lib-source-maps/lib/transformer.js:233:24
    at Array.forEach (<anonymous>)
    at SourceMapTransformer.transform (/home/circleci/jest/node_modules/istanbul-lib-source-maps/lib/transformer.js:223:25)
    at MapStore.transformCoverage (/home/circleci/jest/node_modules/istanbul-lib-source-maps/lib/map-store.js:143:8)
    at /home/circleci/jest/packages/jest-cli/build/reporters/coverage_reporter.js:172:59
    at Generator.next (<anonymous>)

You can reproduce by doing yarn jest init --coverage on this branch.

SimenB · 2018-12-19T08:20:26Z

Just a typo, I'll send a PR!

EDIT: istanbuljs/istanbuljs#257

bcoe · 2018-12-25T00:41:01Z

@SimenB @thymikee please give this a shot:

Successfully published:
 - istanbul-api@2.0.8
 - istanbul-lib-coverage@2.0.2
 - istanbul-lib-hook@2.0.2
 - istanbul-lib-instrument@3.0.1
 - istanbul-lib-report@2.0.3
 - istanbul-lib-source-maps@3.0.1
 - istanbul-reports@2.0.3
 - test-exclude@5.0.1

happy holidays, 🎄 🌴 🕎

SimenB · 2018-12-25T10:52:50Z

Perfect, thanks @bcoe!

SimenB · 2018-12-25T10:58:45Z

Oh, and thank you @loganfsmyth and @coreyfarrell!

github-actions · 2021-05-12T04:09:20Z

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.

facebook-github-bot added the cla signed label Oct 18, 2018

SimenB commented Oct 18, 2018

View reviewed changes

e2e/__tests__/__snapshots__/coverage_report.test.js.snap Outdated Show resolved Hide resolved

SimenB commented Oct 18, 2018

View reviewed changes

SimenB force-pushed the babel-7 branch from 35638aa to 0f0c1bd Compare October 18, 2018 12:43

SimenB force-pushed the babel-7-coverage branch from f15e978 to 9d5ead8 Compare October 20, 2018 22:30

coreyfarrell mentioned this pull request Oct 21, 2018

Issue with istanbul-lib-source-maps istanbuljs/istanbuljs#231

Closed

thymikee added this to the Jest 24 milestone Oct 29, 2018

SimenB force-pushed the babel-7-coverage branch from 9d5ead8 to e43b024 Compare November 20, 2018 10:47

SimenB force-pushed the babel-7 branch from ba9ac39 to 2bca3b6 Compare November 20, 2018 12:43

SimenB force-pushed the babel-7-coverage branch from e43b024 to fa69114 Compare November 20, 2018 12:43

SimenB force-pushed the babel-7-coverage branch from 2b37b64 to 521cd3a Compare November 27, 2018 08:16

SimenB mentioned this pull request Dec 3, 2018

Coverage incomplete using projects #5457

Closed

SimenB mentioned this pull request Dec 11, 2018

fix: correctly calculate end position of sourcemap statement istanbuljs/istanbuljs#254

Closed

thymikee force-pushed the babel-7 branch from e1632c6 to 63e06be Compare December 15, 2018 14:11

SimenB force-pushed the babel-7-coverage branch from 521cd3a to a9da3a3 Compare December 18, 2018 08:27

SimenB mentioned this pull request Dec 18, 2018

Babel 7 #7016

Merged

6 tasks

update coverage snapshots

192cb64

SimenB force-pushed the babel-7-coverage branch from a9da3a3 to 192cb64 Compare December 18, 2018 09:29

SimenB changed the base branch from babel-7 to master December 18, 2018 09:30

upgrade istanbul deps

6daaa07

SimenB mentioned this pull request Dec 24, 2018

Incorrect empty file coverage #7388

Merged

--wip-- [skip ci]

f800a1e

SimenB merged commit df56fca into jestjs:master Dec 25, 2018

SimenB deleted the babel-7-coverage branch December 25, 2018 10:53

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

Fix coverage test for Babel 7 (jestjs#7202)

7603d61

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

Fix coverage test for Babel 7 #7202

Fix coverage test for Babel 7 #7202

Conversation

SimenB commented Oct 18, 2018 • edited Loading

Summary

Test plan

SimenB Oct 18, 2018

Choose a reason for hiding this comment

SimenB commented Oct 20, 2018

coreyfarrell commented Oct 20, 2018

coreyfarrell commented Oct 20, 2018

SimenB commented Oct 20, 2018 • edited Loading

SimenB commented Oct 20, 2018 • edited Loading

coreyfarrell commented Oct 20, 2018

SimenB commented Oct 20, 2018

coreyfarrell commented Oct 21, 2018

SimenB commented Oct 21, 2018

coreyfarrell commented Oct 21, 2018

coreyfarrell commented Oct 21, 2018

SimenB commented Oct 21, 2018

loganfsmyth commented Oct 21, 2018

SimenB commented Oct 21, 2018

loganfsmyth commented Oct 21, 2018 • edited Loading

bcoe commented Oct 22, 2018

loganfsmyth commented Oct 23, 2018

SimenB commented Nov 20, 2018

bcoe commented Nov 20, 2018

SimenB commented Nov 20, 2018

bcoe commented Dec 2, 2018

thymikee commented Dec 10, 2018

SimenB commented Dec 11, 2018

bcoe commented Dec 12, 2018

SimenB commented Dec 18, 2018 • edited Loading

bcoe commented Dec 19, 2018

SimenB commented Dec 19, 2018

thymikee commented Dec 19, 2018

SimenB commented Dec 19, 2018

SimenB commented Dec 19, 2018 • edited Loading

bcoe commented Dec 25, 2018 • edited Loading

SimenB commented Dec 25, 2018

SimenB commented Dec 25, 2018

github-actions bot commented May 12, 2021

SimenB commented Oct 18, 2018 •

edited

Loading

SimenB commented Oct 20, 2018 •

edited

Loading

SimenB commented Oct 20, 2018 •

edited

Loading

loganfsmyth commented Oct 21, 2018 •

edited

Loading

SimenB commented Dec 18, 2018 •

edited

Loading

SimenB commented Dec 19, 2018 •

edited

Loading

bcoe commented Dec 25, 2018 •

edited

Loading