Map code coverage from transformers #2290

jwbay · 2016-12-11T23:24:45Z

Summary
With this PR transformers can supply source maps to Jest and see code coverage mapped against the original sources in all reports without any further processing.

Example transformer - https://github.com/jwbay/jestcoveragetest/blob/master/preprocessor.js#L30-L33

The support for threading source maps through like this was only recently added to the istanbul-lib-x packages.

Test plan
jest

There is a test repo here that I've been using to verify behavior. Screenshots are from that. https://github.com/jwbay/jestcoveragetest

Before:

After:

It's not completely perfect but I think it's the closest we can get for now -- I think the trailing braces are an issue with fidelity of the supplied source maps. It seems inline with the results of remap-istanbul, which is basically the gold standard.

TODO:

Get Accept source map input for the visitor istanbuljs/babel-plugin-istanbul#75 landed and released
Accept non-inline source maps from transformers
Add integration test to assert on remapped coverage
Explore external sources
Explore on-the-fly remapping
Add option to explicitly disable remapping

cc @kulshekhar, @kwonoj, and @Igmat who have been working with coverage remapping in ts-jest. If you all have any test cases it would be good to see if this handles them.

kwonoj · 2016-12-11T23:48:27Z

One of the suffering point of external code coverage mapping is jest does own caching mechanism and does not invoke preprocessor if a file has not changed. Is this change resolves those issue? Just looking at glimpse it relies on existing preprocessor pipeline.

jwbay · 2016-12-11T23:54:34Z

Short answer, yes it resolves that. ~~This approach does rely on the source map having the original sources inlined into the sourcesText property.~~ (fixed) The source map supplied to the instrumenter is attached to the coverage object by istanbul and written out alongside the covered code, which is what Jest caches.

aaronabramov

that is actually pretty awesome.
It seems like it affects the coverage as well (not only line mapping)

is that expected?

aaronabramov · 2016-12-13T03:52:11Z

packages/jest-runtime/src/transform.js

+
+  if (shouldTransform(filename, config)) {
+    if (transform.canInstrument && typeof transform.getEmptyCoverage === 'function') {
+      return transform.getEmptyCoverage(content, filename, config, {


does that delegate getting the empty coverage object to a transformer if it implements it?

Yes, exactly. Not sure how important visual accuracy is for uncovered code, but it can look pretty busted without this, so you'd probably get a couple issues now and then. Also prevents the branch/etc count from jumping up/down depending on whether the file executed or not.

aaronabramov · 2016-12-13T03:52:26Z

scripts/build.js

@@ -69,7 +69,7 @@ function buildFile(file, silent) {
  const relativeToSrcPath = path.relative(packageSrcPath, file);
  const destPath = path.resolve(packageBuildPath, relativeToSrcPath);

-  spawnSync('mkdir', ['-p', path.dirname(destPath)]);
+  mkdir.sync(path.dirname(destPath));


did it fail on win or something? :)

Yes 😅 Since mkdirp was already in jest's dependencies I figured it wouldn't hurt.

aaronabramov · 2016-12-13T03:53:20Z

packages/jest-cli/src/reporters/CoverageReporter.js

@@ -59,6 +62,7 @@ class CoverageReporter extends BaseReporter {
    runnerContext: RunnerContext,
  ) {
    this._addUntestedFiles(config, runnerContext);
+    const { map, sourceFinder } = this._sourceMapStore.transformCoverage(this._coverageMap);


which part is actually reading the source maps? and what happens if source maps are not available?

nit: code style, this should be {map, sourceFinder}.

@DmitriiAbramov istanbul-lib-source-maps has code that plucks the source map from the coverage object as part of this transformCoverage call. If no source map is tacked onto the object, it's not mapped.

@cpojer Will fix shortly!

cpojer · 2016-12-13T21:33:49Z

packages/jest-cli/src/reporters/CoverageReporter.js

@@ -75,7 +79,7 @@ class CoverageReporter extends BaseReporter {
      }

      reporter.addAll(coverageReporters);
-      reporter.write(this._coverageMap);
+      reporter.write(map, { sourceFinder });


code style too.

cpojer · 2016-12-13T21:34:07Z

packages/jest-runtime/src/transform.js

@@ -342,3 +367,4 @@ module.exports = (

 module.exports.EVAL_RESULT_VARIABLE = EVAL_RESULT_VARIABLE;
 module.exports.transformSource = transformSource;
+module.exports.generateEmptyCoverage = generateEmptyCoverage;


please add an empty line at the end of this file.

cpojer · 2016-12-13T21:37:12Z

oh wow, this is indeed cool! That will make ts-jest a lot nicer I'm sure.

jwbay · 2016-12-13T22:25:52Z

@DmitriiAbramov I can't seem to reply directly to your question with the images, but yes, it's expected. The branch/statement and sometimes function count can be different in the original source vs. the transformed/covered code if a compiler injects new branches/statements/etc. When this is reconciled during the remapping, the percentages are changed as a result. Whether that's correct behavior seems to be an open question with lots of opinions :)

aaronabramov · 2016-12-13T22:52:01Z

and what about the performance? is it much slower to do the mapping?

jwbay · 2016-12-14T00:59:33Z

I'll do some performance testing. Hopefully it's not a huge hit, but I'm sure it's not free. It's going to be doing this extra work per file with coverage - https://github.com/istanbuljs/istanbul-lib-source-maps/blob/master/lib/transformer.js#L84.

Also, I found a way to make this work with babel that should make the transformers do far less work. They won't need to take over instrumentation to see remapped coverage and we won't need to add getEmptyCoverage to the transformer API. It needs this PR merged and a different kind of transformer API change. Inline source maps will just work and we could limit this to transformers that support them, but I'd like to make transformer.process return either a content string as it does today, or an object like { code: string, sourceMap: Object }.

cpojer · 2016-12-14T01:01:27Z

This will only affect performance when doing jest --no-cache --coverage, right? Can you take a few samples before and after and give us some numbers? This may make sense on the Jest repo itself.

jwbay · 2016-12-16T02:01:15Z

@cpojer It will affect performance on every run that does --coverage if source maps are present. Cache doesn't come into play unless we're talking about cost of generating source maps, which depends on the transformer.

Here are some perf numbers. I opted for synthetic tests to have more control and test project sizes. Each file has 60 statements, 50 branches, 20 functions, and 50 lines. Each row is an average of 5 runs. 'Coverage time' is time spent in the onRunComplete method of CoverageReporter.

Without mapped coverage (master branch)

Test file count	Total time	Coverage time
10	3.00s	0.23s
100	7.95s	0.47s
1000	55.42s	2.90s

With mapped coverage and source maps

Test file count	Total time	Coverage time
10	3.08s	0.28s
100	8.67s	0.76s
1000	57.18s	4.43s

With mapped coverage, but no source maps supplied

Test file count	Total time	Coverage time
1000	55.61s	2.86s

jwbay · 2016-12-16T02:09:14Z

Another concern is memory. If the source map has its source content inlined, you could end up with the source code text of your entire project held in memory by the main Jest process. This could be bad 😅 I'll look at whether it's possible to remap on the fly and clean up after.

aaronabramov · 2016-12-17T00:00:11Z

@jwbay i don't think it's possible to clean up on the fly, because you don't know if there's going to be more coverage coming for a specific file until the end of the test suite. But we definitely need to make sure we only collect source map for files we intend to cover and that we provide an option to disable mapping (at fb souce maps will be gigabytes of data 😄 )

jwbay · 2016-12-17T00:51:33Z

I meant more cleaning up the source map than the coverage. When a single test result comes in with a source map attached, I think we might able to remap there and delete the source map, so we're only ever merging mapped coverage and the source map doesn't live past its usefulness. This would hurt runtime if a file is executed multiple times across tests, though. You could be mapping against the same source map multiple times instead of once at the end of the run.

Completely turning off source maps is going to be difficult with this PR in its current form. If source maps are inlined with the content, they'll get picked up by Babel and attached to coverage by the plugin. I'll add some kind of flag to prevent that behavior since it's pretty memory intensive to not have an off switch.

codecov-io · 2016-12-17T23:45:37Z

Codecov Report

Merging #2290 into master will increase coverage by 0.06%.
The diff coverage is 80.43%.

@@            Coverage Diff             @@
##           master    #2290      +/-   ##
==========================================
+ Coverage   67.12%   67.18%   +0.06%     
==========================================
  Files         142      143       +1     
  Lines        5126     5163      +37     
  Branches        0        3       +3     
==========================================
+ Hits         3441     3469      +28     
- Misses       1685     1693       +8     
- Partials        0        1       +1

Impacted Files	Coverage Δ
packages/jest-config/src/validConfig.js	`100% <ø> (ø)`	✅
packages/jest-config/src/normalize.js	`85.51% <ø> (ø)`	✅
packages/jest-config/src/defaults.js	`100% <ø> (ø)`	✅
packages/jest-config/src/setFromArgv.js	`0% <ø> (ø)`	✅
packages/jest-runtime/src/transform.js	`89.33% <100%> (+1.45%)`	✅
...ackages/jest-cli/src/reporters/CoverageReporter.js	`61.11% <57.14%> (-3.41%)`	❌
integration_tests/coverage-remapping/covered.ts	`85.71% <85.71%> (ø)`

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 8c17c0f...ad11e77. Read the comment docs.

aaronabramov · 2016-12-19T18:15:52Z

i'm super excited about this RP, i'll try to test in on the internal FB repo and see how much memory it eats

jwbay · 2016-12-29T19:14:35Z

This is ready for review. The babel plugin update is released as an RC at the moment. Once that's promoted all the boxes will be ticked.

jwbay · 2017-02-13T05:10:20Z

@DmitriiAbramov You might get a kick out of this -- turns out the test lockups I assumed were caused by memory pressure were actually caused by the same issue you found. In my case it was a vendored library that ended up being a 3MB string after instrumentation. It was a coincidence that a couple of the largest dependency graphs in the app happened to hit that file.

I switched the implementation to store source maps on disk anyway, though. It's much safer performance wise. The cost is now concentrated as CPU time at the end of the run instead of spread out as memory during the run (and costing CPU time at the end anyway). A project with 3K files takes about 1500ms to remap, and a project with a couple hundred files takes 300ms.

aaronabramov

@jwbay have you tried mapping coverage for babel transforms?

i know it's not very useful, since babel-jest can handle transformation with the need to remap anything, but i was wondering if we should at least add a test case for it.

@cpojer what do you think?

aaronabramov · 2017-02-13T18:42:36Z

packages/jest-runtime/src/transform.js

+    const sourceMapFilePath = cacheFilePath + '.map';
+    writeCacheFile(sourceMapFilePath, sourceMapContent);
+    result +=
+      `\n;global.__coverage__['${escapePathForJavaScript(filename)}']` + 


isn't __coverage__ name configurable from istanbul? we can potentially break coverage collection.
i also think that this wrapping logic deserves to be a separate function and inputSourceMapPath property name should be extracted into a separate constant, since it's something we introduce to istanbul coverage object

@DmitriiAbramov well in practice, it can pretty much be assumed that __covearge__ is the variable (I don't know that I've seen many deviations), this variable can be configured:

https://github.com/istanbuljs/istanbul-lib-instrument/blob/b08e4f55d60a43d542add9b09b3bf52a1d88ce6f/src/instrumenter.js#L13

Yeah, the more I look at this the less I like it. To do this correctly, we could try and update the istanbul-lib-x packages to either overload the inputSourceMap property to accept a path to a source map as well as a source map proper, or add a new inputSourceMapPath property. Either one would be fairly straightforward, it would just need to touch two or three packages. @bcoe what do you think?

aaronabramov · 2017-02-13T18:43:48Z

types/TestResult.js

+  statementMap: { [statementId: number]: any },
+  branchMap: { [branchId: number]: any },
+  inputSourceMap?: Object,
+  inputSourceMapPath?: string,


is this istanbul shape, or are we adding sourcemap properties to it?

inputSourceMap is official. inputSourceMapPath is not (yet).

aaronabramov · 2017-02-13T18:44:58Z

types/Transform.js

@@ -11,6 +11,11 @@

 import type {Config, Path} from 'types/Config';

+export type TransformedSource = {|
+  content: string,
+  sourceMap: ?Object | string,


is that the way typescript compiler returns it by default?
if not, should we rename it to babel's defaults?
i think it was {code, map} shape

No, typescript returns something different. I was trying not to play favorites with compilers too much. I'll change to babel's shape.

cpojer · 2017-02-14T17:07:07Z

I'd prefer to hold off until after Jest 19 with this one.

jwbay · 2017-02-15T00:47:00Z

@DmitriiAbramov I haven't really tried to remap babel output for the reason you mentioned. I'll see if I can add a test.

# Conflicts: # packages/jest-cli/package.json # packages/jest-config/src/setFromArgv.js # packages/jest-runtime/package.json # types/TestResult.js

# Conflicts: # packages/jest-cli/package.json

jwbay · 2017-02-24T04:10:58Z

@DmitriiAbramov ecbde20 switches the approach from source map info hitching a ride on the global coverage object to being more of a first class citizen in jest-runtime. It required some function signature changes, but disconnecting source maps from coverage a bit has some benefits. It sets Jest up for remapping stacktraces for errors, and jest-editor-support might be able to do some cool stuff with source maps exposed. Also, we can now map coverage for untested files, which doesn't seem too important but will probably save Jest a few github issues now and then.

jwbay · 2017-02-24T04:18:54Z

As far as compiler support, it doesn’t look good for babel. I tested four compilers: babel, typescript, buble, and coffeescript. The testbed is updated with the transformers I used.

It seems Babel is doing something with its source maps that the source-map package doesn’t like. It ends up returning a lot of wrong/invalid positions. Sometimes it gets things right, but not often. I tried retainLines to help it along but it didn’t do much.
Typescript is pretty solid. It’s not perfect, but I’ve not seen things go horribly wrong.
Buble is also solid, which is impressive since it apparently does string transformations instead of an AST transform.
Coffeescript is basically all over the place.

I debugged a bit and it looks like istanbul-lib-source-maps isn’t doing anything weird or wrong. It seems to come down to the source-map package returning odd source positions for source maps from some compilers. Given all this, I’ve added a disclaimer of sorts to the docs for the flag.

bookman25 · 2017-03-02T21:24:44Z

Just ran into a good use case for this. By having jest provide the source map information, it would allow jest-editor-support to leverage that info for generating code coverage overlays.

aaronabramov · 2017-03-03T00:26:01Z

i tested it locally and it works really nice!
no luck with www, but it's because of our monstrous transform pipeline (i don't think it's a blocker since it'll not affect www and we won't be able to use it right now anyway)

aaronabramov · 2017-03-03T00:30:28Z

@jwbay thank you so much for working on it! and sorry it took so long :)
This is going to be a game changer for TS projects!

* source map support for coverage * be less dangerous only enforce files in src/ having @flow since everything else may not be run through babel * lazy require for istanbul-lib-source-maps * add documentation for mapCoverage config option * store source maps on disk instead of in worker process memory * add mapCoverage to CLI, switch off by default * move source map info from coverage object to jest-runtime * tweak docs, fix a test

github-actions · 2021-05-13T20:07:12Z

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 11, 2016

cpojer assigned aaronabramov Dec 12, 2016

aaronabramov reviewed Dec 13, 2016

View reviewed changes

cpojer reviewed Dec 13, 2016

View reviewed changes

jwbay mentioned this pull request Dec 14, 2016

Accept source map input for the visitor istanbuljs/babel-plugin-istanbul#75

Merged

jwbay force-pushed the map-coverage branch from a990881 to 08e134f Compare December 15, 2016 07:49

jwbay force-pushed the map-coverage branch from 08e134f to ff11557 Compare December 17, 2016 23:36

jwbay force-pushed the map-coverage branch 3 times, most recently from 7f68b71 to 8320519 Compare December 17, 2016 23:53

aaronabramov mentioned this pull request Dec 19, 2016

[coverage] ES6 import and Flow pragma have issues with coverage #2339

Closed

jwbay force-pushed the map-coverage branch from bd19866 to e090153 Compare December 21, 2016 04:20

jwbay force-pushed the map-coverage branch 2 times, most recently from eee261e to 59d3f14 Compare December 29, 2016 18:13

jwbay changed the title ~~[WIP] Map code coverage from transformers~~ Map code coverage from transformers Dec 29, 2016

jwbay force-pushed the map-coverage branch 2 times, most recently from d7964c2 to 78c0a62 Compare February 13, 2017 04:40

jwbay force-pushed the map-coverage branch from 78c0a62 to 03086ac Compare February 13, 2017 05:26

add mapCoverage to CLI, switch off by default

ad11e77

jwbay force-pushed the map-coverage branch from 03086ac to ad11e77 Compare February 13, 2017 05:56

aaronabramov reviewed Feb 13, 2017

View reviewed changes

jwbay added 4 commits February 16, 2017 00:12

move source map info from coverage object to jest-runtime

ecbde20

Merge remote-tracking branch 'origin/master' into map-coverage-runtime

998c640

# Conflicts: # packages/jest-cli/package.json # packages/jest-config/src/setFromArgv.js # packages/jest-runtime/package.json # types/TestResult.js

Merge remote-tracking branch 'origin/master' into map-coverage

6a3ac5d

# Conflicts: # packages/jest-cli/package.json

tweak docs, fix a test

254c619

jwbay force-pushed the map-coverage branch from 670da2b to 254c619 Compare February 24, 2017 01:35

jwbay changed the title ~~[WIP] Map code coverage from transformers~~ Map code coverage from transformers Feb 24, 2017

aaronabramov merged commit c6d2b08 into jestjs:master Mar 3, 2017

jwbay mentioned this pull request Mar 7, 2017

Proposal: cache auto-generated mocks across test runs #3084

Closed

jwbay deleted the map-coverage branch March 9, 2017 23:29

Armour mentioned this pull request Mar 13, 2017

Use jest build-in coverage mapping Armour/express-webpack-react-redux-typescript-boilerplate#7

Closed

Vinnl mentioned this pull request Apr 21, 2017

How to display remapped coverage report in the terminal? kulshekhar/ts-jest#101

Closed

jwbay mentioned this pull request Dec 28, 2017

Removing the mapCoverage condition on reading inlineSourceMaps. #5177

Merged

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

Map code coverage from transformers #2290

Map code coverage from transformers #2290

Conversation

jwbay commented Dec 11, 2016 • edited Loading

Before:

After:

kwonoj commented Dec 11, 2016

jwbay commented Dec 11, 2016 • edited Loading

aaronabramov left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpojer commented Dec 13, 2016

jwbay commented Dec 13, 2016

aaronabramov commented Dec 13, 2016

jwbay commented Dec 14, 2016

cpojer commented Dec 14, 2016

jwbay commented Dec 16, 2016

jwbay commented Dec 16, 2016

aaronabramov commented Dec 17, 2016

jwbay commented Dec 17, 2016

codecov-io commented Dec 17, 2016 • edited Loading

Codecov Report

aaronabramov commented Dec 19, 2016

jwbay commented Dec 29, 2016

jwbay commented Feb 13, 2017

aaronabramov left a 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

cpojer commented Feb 14, 2017

jwbay commented Feb 15, 2017

jwbay commented Feb 24, 2017

jwbay commented Feb 24, 2017

bookman25 commented Mar 2, 2017

aaronabramov commented Mar 3, 2017

aaronabramov commented Mar 3, 2017

github-actions bot commented May 13, 2021

jwbay commented Dec 11, 2016 •

edited

Loading

jwbay commented Dec 11, 2016 •

edited

Loading

codecov-io commented Dec 17, 2016 •

edited

Loading