Removing the mapCoverage condition on reading inlineSourceMaps. #5177

Aftabnack · 2017-12-25T16:55:12Z

Summary

Fixes support for sourceMaps in transforms. Fix support for sourcemaps in transforms #5121

Test plan

Removing the skip from the testcase skipped in fix(jest-runtime): only write map files to disk when mapCoverage: true #5117
Testcase passing in my local!! 😄

codecov-io · 2017-12-25T17:04:34Z

Codecov Report

Merging #5177 into master will decrease coverage by 1.09%.
The diff coverage is 92%.

@@            Coverage Diff            @@
##           master    #5177     +/-   ##
=========================================
- Coverage   61.72%   60.63%   -1.1%     
=========================================
  Files         213      213             
  Lines        7170     7311    +141     
  Branches        4        3      -1     
=========================================
+ Hits         4426     4433      +7     
- Misses       2743     2877    +134     
  Partials        1        1

Impacted Files	Coverage Δ
packages/jest-config/src/defaults.js	`100% <ø> (ø)`	⬆️
packages/jest-jasmine2/src/index.js	`5.47% <ø> (ø)`	⬆️
packages/jest-config/src/valid_config.js	`100% <ø> (ø)`	⬆️
packages/jest-cli/src/generate_empty_coverage.js	`85.71% <ø> (ø)`	⬆️
packages/jest-config/src/index.js	`25.92% <ø> (ø)`	⬆️
packages/jest-runner/src/run_test.js	`2.22% <0%> (ø)`	⬆️
packages/jest-config/src/deprecated.js	`57.14% <100%> (-9.53%)`	⬇️
packages/jest-runtime/src/index.js	`73.55% <100%> (-0.45%)`	⬇️
packages/jest-runtime/src/script_transformer.js	`85.38% <100%> (+0.85%)`	⬆️
...n-tests/coverage-transform-instrumented/covered.js	`100% <100%> (ø)`
... and 34 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 d065e87...520a141. Read the comment docs.

SimenB · 2017-12-25T17:52:48Z

Interesting, nice find!

I think we also want to revert the changes in the snapshot in #5117 as well.

Adding an integration test showing that the coverage is mapped correctly in a stack would be great. So not just testing the .map file, but the mapping as well

Aftabnack · 2017-12-25T17:56:25Z

I think, I might need some pointers adding the integration tests. Is there an example I can look at?

PS: I will do this in the morning. It's night time here! :P

Aftabnack · 2017-12-25T19:57:38Z

The snapshot test case that was modified as part of fix(jest-runtime): only write map files to disk when mapCoverage: true #5117 uses ts preprocessor, which emits sourcemap and code separately. So no change there.
This only affects the places where there are inlineSourceMaps
PS: Found some integration tests..:P

SimenB · 2017-12-26T10:10:16Z

Hmm, I wonder if the correct fix is to pass mapCoverage from the skipped test. Looking at its docs it's a way to disable source maps in order to increase performance.

Do you agree?

I think I'll have to dig a bit deeper into how sourcemaps work and how they're merged

Aftabnack · 2017-12-26T11:27:37Z

But what doesn't make sense is the mapCoverage option seems to affect only inline source maps. The Boolean is used only at the place where inline sourcemaps are attempted to be parsed.

If we have to stick to it's intended usage mentioned in docs, we should enforce mapCoverage on all sourcemaps, which would mean nullifying map emitted from preprocessors.

I'm strongly against that, since the coverage remapping will be messed up, the coverage annotations will be all wrong.

jwbay · 2017-12-28T00:21:00Z

Hi 👋 I implemented the initial source map support for Jest, so I think I can provide some context.

It was added in Map code coverage from transformers #2290 to support code coverage remapping
Some checks were removed in Sourcemaps #3458 to support stack trace remapping
That last one was reverted in fix(jest-runtime): only write map files to disk when mapCoverage: true #5117

And I think that brings us to this PR.

I think the integration test snapshot should definitely stay reverted. It shouldn't have changed in #3458. I haven't checked, but the line/column numbers in that snapshot map directly to the visual alignment of covered code in the HTML report, so if they changed things probably broke. I can't think of a good way to keep that from happening again in the future.

The mapCoverage option used to affect more than inline source maps. It used to null out the map property on the transform result when it was false, whether it was populated via a separate map or an inline source map. You can find that in the initial source map PR: https://github.com/facebook/jest/pull/2290/files#diff-7de44d08766eddcf4bca597034b5b4c2R314

To be clear, mapCoverage was added and defaulted to false to prevent silent performance regressions for the case where people with large projects had a transformer set up that happened to return inline source maps.

jwbay · 2017-12-28T00:23:38Z

integration_tests/coverage-remapping-inline-sourcemap/package.json

+  "jest": {
+    "rootDir": "./",
+    "transform": {
+      "^.+\\.(js)$": "babel-jest"


I don't think this test is valuable currently because babel-jest doesn't deal with source maps. It instruments and then transforms, so there's no coverage remapping necessary.

Doesn't it emit inline sourcemaps? Here Which are then read?

I'd also like to better understand this 🙂

FWIW, inline sourcemaps is pretty recent: #3738

Since mapCoverage is not passed in this integration test (which is correct*), no remapping should be performed here. I'm not positive, but I would expect this integration test to pass with the same output on master. Since the main purpose of this PR, as I understand it, is accurate line/col numbers for error stacktraces and codeframes, maybe the integration test should try to cover one of those use cases.

* If a transformer instruments before doing downlevel compilation (as babel-jest does), any sort of source map processing for coverage is probably going to break things. Source map processing for coverage only makes sense when going the other direction (downlevel compilation, then instrumentation)

Edit: I see mapCoverage is being passed after all. I'm curious what the HTML report looks like

jwbay · 2017-12-28T00:38:41Z

packages/jest-runtime/src/script_transformer.js

-          transformed.map = inlineSourceMap.toJSON();
-        }
+    if (!transformed.map) {
+      const inlineSourceMap = convertSourceMap.fromSource(transformed.code);


This is where performance issues may surface with this unguarded. convertSourceMap.fromSource looks for inline source maps in code files with a regex (src). For sufficiently huge source files, this can freeze node (ex: babel/babel#5081). It depends on the regex as well as file size, I think.

I don't have a good answer here. Some tests are probably in order; it may not be a problem. Or we may need a new sourceMaps option or something. Or we could kill inline source map support and required a separate code/map pair always.

I hadn't looked into the source code of convert-source-map. I'm okay with using mapCoverage to guard against this. But like it was in the original PR, it should be nullifying the explicit sourcemaps emitted from preprocessors.

Also as a user of Jest, I would want it to come with sensible defaults, IMO defaulting map coverage to true, adding a caveat that this should be set to false in case you are facing some freeze issue is better than defaulting to false. Cuz more often than not, it will work and is incredibly important.

Is mapCoverage used anymore with this changeset. Also, if we combine this with #5257 at least it wouldn't hit the default babel-jest configuration

With this change, inline sourceMaps emitted from the default babel-jest will be read. #5257 may not be necessary.

The point is that inline sourcempas are way slower than just using the returned sourcemap.

Agreed. I'm actually leaning towards removing inline-source-map support. But I don't know the ramifications of it.

Inline source map support helps when debugging tests. VS Code at least tends to behave better when everything it needs is in script it's executing.

jwbay · 2017-12-28T00:40:24Z

packages/jest-runtime/src/script_transformer.js

@@ -261,7 +259,7 @@ export default class ScriptTransformer {
      code = transformed.code;
    }

-    if (instrument && mapCoverage && transformed.map) {
+    if (instrument && transformed.map) {


For proper stack trace support, the instrument check should probably be killed as well. Test files are (almost) never instrumented, so line numbers in stacktraces for errors won't be fully correct otherwise.

Agreed. If map is there save it, otherwise don't.

Can you remove instrument?

Aftabnack · 2017-12-28T03:09:21Z

Also what do you think about stripping out the inline sourcemaps before caching the actual code file.? I think it's important we do that, we can cut down the amount of cache space we are taking by almost half for inline sourcemaps. 😉

lightsofapollo · 2018-01-14T02:22:29Z

Looking forward to this landing!

SimenB · 2018-01-16T09:22:59Z

@jwbay thanks for chiming in!

Are we able to test this against a real project and check that it works correctly?

@Aftabnack Please update the changelog as well 🙂

Aftabnack · 2018-01-16T10:37:50Z

@SimenB Updated the changelog.

Also added this comment:
https://github.com/Aftabnack/jest/blob/24d822848f576554e8f2ce99ef0bcac76ad0b2fe/packages/jest-runtime/src/script_transformer.js#L246-L247

I don't know of any big projects where this could be tried.

SimenB · 2018-01-16T10:45:26Z

I don't know of any big projects where this could be tried.

We can at least test against some of the repros in #5109

Aftabnack · 2018-01-16T10:52:12Z

The repo in that issue, worked out of the box without me having to link my local jest-cli

SimenB · 2018-01-16T10:52:57Z

It does now as we have reverted the change which broke them. Does it still work if you link in this? If any of the reproductions have lockfiles, that'd help

Aftabnack · 2018-01-16T11:00:05Z

It works after linking my local jest-cli too.
Also the jest test-ci-partial also works in my local. On CircleCI it says timed out. Do I have to change the timeout somewhere?

SimenB · 2018-01-16T11:45:19Z

If the tests time out with this change, I consider than an issue. I don't think we should increase the timeout, we don't want to make Jest slower

Aftabnack · 2018-01-16T11:49:24Z

But why am I not able to see the same thing in my local.?

This is what I got when running yarn test-ci-partial

SimenB · 2018-01-16T11:50:58Z

My guess is because your machine is more powerful.

Try running with and without your change 3 times or something, is there a difference in execution time?

Aftabnack · 2018-01-16T11:52:34Z

I'll try running on master and my branch separately 3 times.

My config is 8GB ram with 256GB SSD running on i5 processor. Wouldn't call it powerful.. 😉

Aftabnack · 2018-01-16T12:13:18Z

This was my mistake. I didn't rebuild the packages. Testcases are actually failing. It is because we have removed the instrument check. I will re-add the instrument check and try to figure out why that is important and what it is needed for.

Aftabnack · 2018-01-16T18:01:30Z

@SimenB Why do we save sourcemaps of test files themselves?

SimenB · 2018-01-16T18:17:34Z

Debugging the tests themselves, including codeframe on assertion errors

Aftabnack · 2018-01-16T18:37:08Z

After hours of debugging, I found this:

https://github.com/facebook/jest/blob/7743c0970e974bca166bff67099d28462ddb3187/packages/jest-runtime/src/index.js#L436-L447

The testcases that were failing, were because the Files that were returned in the coverage collected (getAllCoverageInfoCopy) were lesser than what was there in the sourceMaps map returned from getSourceMapInfo.

Because this was the case, when it tries to assert which file the sourceMaps belonged to, it throws since coverageCollected from the test run won't have the file in which the tets case was written; here:
https://github.com/facebook/jest/blob/7743c0970e974bca166bff67099d28462ddb3187/packages/jest-cli/src/reporters/coverage_reporter.js#L63

Aftabnack · 2018-01-16T18:39:19Z

I have modified the getCoverageInfo() to accept array of filenames for which the coverage was generated, and I am returning sourceMaps for files which are present in that array.

Thoughts @SimenB ?

Aftabnack · 2018-01-17T06:50:09Z

Upon debugging the timeout issue, I found out that the testcase is timing out when the automock is set to true in that particular testcase. I've pushed removing that to verify that was the case.

@SimenB can you help me understand how automock is related to the changes that I have done?

mapCoverage has been deprecated since Jest@22.4.0 https://github.com/facebook/jest/blob/master/CHANGELOG.md#2240 jestjs/jest#5177

github-actions · 2021-05-12T10:07:11Z

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 Dec 25, 2017

Aftabnack force-pushed the fix-coverage branch from a821f90 to 364d956 Compare December 25, 2017 17:01

jwbay reviewed Dec 28, 2017

View reviewed changes

SimenB mentioned this pull request Jan 16, 2018

Rely on jest sourcemaps kulshekhar/ts-jest#340

Closed

Aftabnack force-pushed the fix-coverage branch from 24d8228 to daa9fe7 Compare January 16, 2018 12:18

Aftabnack deleted the fix-coverage branch February 15, 2018 18:08

manu-st mentioned this pull request Feb 16, 2018

Jest drop of mapCoverage kulshekhar/ts-jest#444

Closed

This was referenced Feb 17, 2018

Fix support for sourcemaps in transforms #5121

Closed

TypeError: sourceMap.map.originalPositionFor is not a function #5589

Closed

Jest giving wrong line number for error #5446

Closed

[WIP] Remove usage of retainLines #5594

Merged

This was referenced Feb 20, 2018

Update jest in / from 22.3.0 to 22.4.0 opencomponents/storage-adapters#48

Closed

Update 14 dependencies from npm storybookjs/storybook#3041

Merged

This was referenced Feb 21, 2018

mapCoverage removed from Jest vuejs/vue-jest#61

Merged

mapCoverage true by default #4509

Closed

otroboe mentioned this pull request Feb 22, 2018

Testing and code coverage with JEST nestjs/nest#446

Closed

termosa mentioned this pull request Feb 25, 2018

Remove mapCoverage as it is deprecated vuejs-templates/webpack#1314

Open

maxpou mentioned this pull request Mar 3, 2018

Source map files disappear from coverage report vuejs/vue-jest#32

Closed

This was referenced Mar 5, 2018

Update jest-environment-jsdom in / from 22.2.0 to 22.4.1 opencomponents/base-templates#271

Closed

Update jest in / from 22.2.1 to 22.4.2 opencomponents/base-templates#272

Merged

SimenB mentioned this pull request Apr 15, 2018

always return object from babel-jest transform #5991

Merged

eranshmil mentioned this pull request Jul 27, 2018

fix(test): remove mapCoverage ngrx/platform#1204

Closed

jodarove mentioned this pull request Oct 4, 2018

feat(jest-transformer): generate jest tests sourcemaps salesforce/lwc#706

Merged

2 tasks

chiubaka mentioned this pull request Jul 29, 2019

line number incorrect even in a simple example (Node 7 and no React) kulshekhar/ts-jest#412

Closed

SimenB mentioned this pull request Mar 30, 2020

collectCoverage breaks TypeScript source stepping #5739

Closed

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

Removing the mapCoverage condition on reading inlineSourceMaps. #5177

Removing the mapCoverage condition on reading inlineSourceMaps. #5177

Conversation

Aftabnack commented Dec 25, 2017

codecov-io commented Dec 25, 2017 • edited Loading

Codecov Report

SimenB commented Dec 25, 2017

Aftabnack commented Dec 25, 2017

Aftabnack commented Dec 25, 2017

SimenB commented Dec 26, 2017

Aftabnack commented Dec 26, 2017

jwbay commented Dec 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB Jan 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwbay Jan 20, 2018 • edited Loading

Choose a reason for hiding this comment

jwbay Dec 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB Jan 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aftabnack commented Dec 28, 2017

lightsofapollo commented Jan 14, 2018

SimenB commented Jan 16, 2018 • edited Loading

Aftabnack commented Jan 16, 2018

SimenB commented Jan 16, 2018

Aftabnack commented Jan 16, 2018

SimenB commented Jan 16, 2018 • edited Loading

Aftabnack commented Jan 16, 2018

SimenB commented Jan 16, 2018

Aftabnack commented Jan 16, 2018

SimenB commented Jan 16, 2018

Aftabnack commented Jan 16, 2018

Aftabnack commented Jan 16, 2018

Aftabnack commented Jan 16, 2018

SimenB commented Jan 16, 2018

Aftabnack commented Jan 16, 2018

Aftabnack commented Jan 16, 2018 • edited Loading

Aftabnack commented Jan 17, 2018

github-actions bot commented May 12, 2021

codecov-io commented Dec 25, 2017 •

edited

Loading

SimenB Jan 16, 2018 •

edited

Loading

jwbay Jan 20, 2018 •

edited

Loading

jwbay Dec 28, 2017 •

edited

Loading

SimenB Jan 16, 2018 •

edited

Loading

SimenB commented Jan 16, 2018 •

edited

Loading

SimenB commented Jan 16, 2018 •

edited

Loading

Aftabnack commented Jan 16, 2018 •

edited

Loading