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

Fix coverage test for Babel 7 #7202

Merged
merged 3 commits into from
Dec 25, 2018
Merged

Fix coverage test for Babel 7 #7202

merged 3 commits into from
Dec 25, 2018

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Oct 18, 2018

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:

image

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:
image

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

@@ -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');
Copy link
Member Author

Choose a reason for hiding this comment

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

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

@SimenB
Copy link
Member Author

SimenB commented Oct 20, 2018

@coreyfarrell @JaKXz thoughts on this one?

@coreyfarrell
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member Author

SimenB commented Oct 20, 2018

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
Copy link
Member Author

SimenB commented Oct 20, 2018

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
Copy link
Contributor

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
Copy link
Member Author

SimenB commented Oct 20, 2018

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

image

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
Copy link
Contributor

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
Copy link
Member Author

SimenB commented Oct 21, 2018

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

  1. using babel 6 and the old istanbul module
  2. upgrading istanbul to latest
  3. 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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member Author

SimenB commented Oct 21, 2018

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
Copy link

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.

screen shot 2018-10-21 at 2 20 57 pm

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
Copy link
Member Author

SimenB commented Oct 21, 2018

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
Copy link

loganfsmyth commented Oct 21, 2018

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
Copy link
Contributor

bcoe commented Oct 22, 2018

@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
Copy link

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.

@thymikee thymikee added this to the Jest 24 milestone Oct 29, 2018
@SimenB
Copy link
Member Author

SimenB commented Nov 20, 2018

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
Copy link
Contributor

bcoe commented Nov 20, 2018

@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
Copy link
Member Author

SimenB commented Nov 20, 2018

@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
Copy link
Contributor

bcoe commented Dec 2, 2018

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

@thymikee
Copy link
Collaborator

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

@SimenB
Copy link
Member Author

SimenB commented Dec 11, 2018

Opened up istanbuljs/istanbuljs#254

@bcoe
Copy link
Contributor

bcoe commented Dec 12, 2018

@loganfsmyth @SimenB please see istanbuljs/istanbuljs#255

@SimenB
Copy link
Member Author

SimenB commented Dec 18, 2018

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.

@SimenB SimenB mentioned this pull request Dec 18, 2018
6 tasks
@SimenB SimenB changed the base branch from babel-7 to master December 18, 2018 09:30
@bcoe
Copy link
Contributor

bcoe commented Dec 19, 2018

@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
Copy link
Member Author

SimenB commented Dec 19, 2018

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
Copy link
Collaborator

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
Copy link
Member Author

SimenB commented Dec 19, 2018

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
Copy link
Member Author

SimenB commented Dec 19, 2018

Just a typo, I'll send a PR!

EDIT: istanbuljs/istanbuljs#257

@bcoe
Copy link
Contributor

bcoe commented Dec 25, 2018

@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
Copy link
Member Author

SimenB commented Dec 25, 2018

Perfect, thanks @bcoe!

@SimenB SimenB merged commit df56fca into jestjs:master Dec 25, 2018
@SimenB SimenB deleted the babel-7-coverage branch December 25, 2018 10:53
@SimenB
Copy link
Member Author

SimenB commented Dec 25, 2018

Oh, and thank you @loganfsmyth and @coreyfarrell!

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.

6 participants