Improve source map handling when instrumenting transformed code (#5739) #9811

mbpreble · 2020-04-14T20:44:23Z

Summary

This PR is motivated by issue #5739. There are a few issues around Jest's implementation of code coverage using babel-plugin-istanbul which are corrected here

Uncaught errors in code under test are reported using instrumented line numbers instead of source
It's not easy to debug through code under test, due to no mapping between instrumented code and source.

The change here ensures that source maps are explicitly passed along to the instrumenting process when dealing with transformed code, that the instrumented code includes an inline source map back to the original source, and that this source map is stored in Jest's cache.

Here we bypass a mechanism intended to map reported coverage back to the original source - this should be unnecessary as coverage will already refer to the original source in all cases where it's possible to do so.

Fixes #5739.

Test plan

I have a test repository containing both TS and JS test files and code. When I run the master version of Jest against this repository, I can confirm uncaught errors are reported using the wrong line numbers. I'm unable to hit a breakpoint in the TS code under test when debugging. The reported coverage appears correct:

 FAIL  javascript/app.test.js
  ● hello world › should say hello world

    hello world



      at helloWorld (javascript/app.js:149:9)
      at Object.<anonymous> (javascript/app.test.js:5:16)

 FAIL  typescript/app.test.ts
  ● hello world › should say hello world

    hello world



      at Object.<anonymous>.exports.helloWorld (typescript/app.ts:157:9)
      at Object.<anonymous> (typescript/app.test.ts:5:16)

---------------|---------|----------|---------|---------|-------------------
File           | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
---------------|---------|----------|---------|---------|-------------------
All files      |   53.85 |      100 |   33.33 |   53.85 |                   
 javascript    |   57.14 |      100 |   33.33 |   57.14 |                   
  app.js       |      80 |      100 |      50 |      80 | 6                 
  uncovered.js |       0 |      100 |       0 |       0 | 1-2               
 typescript    |      50 |      100 |   33.33 |      50 |                   
  app.ts       |      75 |      100 |      50 |      75 | 12                
  uncovered.ts |       0 |      100 |       0 |       0 | 1-2               
---------------|---------|----------|---------|---------|-------------------
Test Suites: 2 failed, 2 total
Tests:       2 failed, 2 total
Snapshots:   0 total
Time:        1.439s
Ran all test suites.

Process finished with exit code 1

Running Jest with this change, line numbers for errors are corrected and it is possible to debug TS code under test. Errors are even shown in context. Note that no obvious harm has been done to coverage reporting for either the TS or JS files.

 FAIL  javascript/app.test.js
  ● hello world › should say hello world

    hello world

      1 | const helloWorld =  () => {
    > 2 |     throw new Error('hello world')
        |           ^
      3 | };
      4 | 
      5 | const uncoveredCode = () => {

      at helloWorld (javascript/app.js:2:11)
      at Object.<anonymous> (javascript/app.test.js:5:16)

 FAIL  typescript/app.test.ts
  ● hello world › should say hello world

    hello world

       6 | 
       7 | export const helloWorld =  () => {
    >  8 |     throw new Error('hello world')
         |           ^
       9 | };
      10 | 
      11 | export const uncoveredCode = () => {

      at Object.<anonymous>.exports.helloWorld (typescript/app.ts:8:11)
      at Object.<anonymous> (typescript/app.test.ts:5:16)

---------------|---------|----------|---------|---------|-------------------
File           | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
---------------|---------|----------|---------|---------|-------------------
All files      |   53.85 |      100 |   33.33 |   53.85 |                   
 javascript    |   57.14 |      100 |   33.33 |   57.14 |                   
  app.js       |      80 |      100 |      50 |      80 | 6                 
  uncovered.js |       0 |      100 |       0 |       0 | 1-2               
 typescript    |      50 |      100 |   33.33 |      50 |                   
  app.ts       |      75 |      100 |      50 |      75 | 12                
  uncovered.ts |       0 |      100 |       0 |       0 | 1-2               
---------------|---------|----------|---------|---------|-------------------
Test Suites: 2 failed, 2 total
Tests:       2 failed, 2 total
Snapshots:   0 total
Time:        1.413s
Ran all test suites.

Process finished with exit code 1

SimenB · 2020-04-15T08:19:48Z

Thanks @mbpreble! Is mapCoverage ever set to true anymore?

mbpreble · 2020-04-15T14:33:20Z

@SimenB not as far as I can tell.

It still appears to be checked in a couple of places to drive mapping behavior if true
Line 811 of jest-runtime
Line 79 of jest-reporters/generateEmptyCoverage.

Might be a candidate for breaking changes and removal down the line. This seems to leave behind some code paths which aren't exercised internally by Jest.

SimenB · 2020-04-16T18:02:46Z

mapCoverage is already internal, so I think we can remove it if it's not used... And it makes sense to me that we don't use it when the instrumentation can take an existing source map as input.

@jwbay would love your thoughts on this one 🙂

mbpreble · 2020-04-18T17:31:06Z

@SimenB - new commit here to remove dead code that was gated by mapCoverage if you want to see what that looks like.

codecov-io · 2020-04-18T17:48:19Z

Codecov Report

Merging #9811 into master will increase coverage by 0.07%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##           master    #9811      +/-   ##
==========================================
+ Coverage   64.43%   64.50%   +0.07%     
==========================================
  Files         290      291       +1     
  Lines       12376    12361      -15     
  Branches     3058     3057       -1     
==========================================
  Hits         7974     7974              
+ Misses       3762     3745      -17     
- Partials      640      642       +2

Impacted Files	Coverage Δ
packages/jest-reporters/src/coverage_reporter.ts	`57.69% <ø> (+3.80%)`	⬆️
packages/jest-runner/src/runTest.ts	`2.15% <ø> (+0.02%)`	⬆️
packages/jest-runtime/src/index.ts	`59.51% <0.00%> (+0.57%)`	⬆️
packages/jest-transform/src/ScriptTransformer.ts	`67.41% <56.25%> (-1.37%)`	⬇️
e2e/stack-trace-source-maps-with-coverage/lib.ts	`100.00% <100.00%> (ø)`
...ckages/jest-reporters/src/generateEmptyCoverage.ts	`72.72% <100.00%> (+6.06%)`	⬆️

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 1c2011d...28ccbf4. Read the comment docs.

SimenB · 2020-04-19T08:06:51Z

Oh yeah, this is lovely! This does make sense to me - mapCoverage is used to know when we need to map from instrumented to transpiled code, but if we feed the original sourcemap to babel when instrumenting this is no longer needed. Lovely stuff.

There's a small conflict, mind merging or rebasing on master?

Uncaught errors in code under test are reported using instrumented line numbers instead of source

Can you add an integration test for this? Then snapshot the error which hopefully points to the correct thing.

SimenB · 2020-04-19T08:07:51Z

packages/jest-runtime/src/index.ts

@@ -808,9 +808,6 @@ class Runtime {

    if (transformedFile.sourceMapPath) {
      this._sourceMapRegistry[filename] = transformedFile.sourceMapPath;
-      if (transformedFile.mapCoverage) {
-        this._needsCoverageMapped.add(filename);


can we delete the _needsCoverageMapped property?

I pulled on this thread a bit more - we can remove the property and there's an entire getSourceMapInfo function which collapses down into returning an empty object.

There was a call site in runTest which used this to attach a sourceMaps property to TestResult, leading to some more apparently defunct code in coverage_reporter.

Deleted a few more things here and added a couple more TODOs for deletion in Jest 26.

packages/jest-types/src/Transform.ts

…js#5739)

…rage - inputSourceMap should always be available.

…instrumented.

mbpreble · 2020-04-21T02:47:20Z

Rebased on master and tracked down some more things to delete related to coverage mapping.
e2e test stackTraceSourceMapsWithCoverage.test.ts confirms a correct stack trace in instrumented TS code under test - this test fails if cherry-picked over to master, reporting line numbers in the instrumented code.

Existing breaking change to TransformResult was walked back as a Jest 26 TODO. Found a couple candidates for future cleanup.

jwbay · 2020-04-21T02:47:58Z

@jwbay would love your thoughts on this one 🙂

This is a lot to catch up on, but I can say with confidence that when it comes to mapCoverage, it was only added to guard against a potential performance regression -- basically a rollout feature flag that stuck around. I think it's since been defaulted to true since with no issues, so it makes sense to me to remove it as much as possible :)

packages/jest-test-result/src/types.ts

packages/jest-transform/src/ScriptTransformer.ts

e2e/__tests__/stackTraceSourceMapsWithCoverage.test.ts

SimenB · 2020-04-23T20:05:26Z

e2e/__tests__/__snapshots__/stackTraceSourceMapsWithCoverage.test.ts.snap

+
+      at Object.error (lib.ts:13:9)
+      at Object.<anonymous> (__tests__/fails.ts:10:3)
+`;


For posterity (if we ever come back to this and wonder why we pass sourcemap into Babel), this is the error if running this test without the other changes in this PR

SimenB · 2020-04-23T20:36:19Z

Thanks again @mbpreble, this is awesome stuff!

mbpreble · 2020-04-23T20:36:59Z

Thanks again @mbpreble, this is awesome stuff!

Thanks for the help!

…pshots * upstream/master: (39 commits) Prints the Symbol name into the error message with a custom asymmetric matcher (jestjs#9888) Show coverage of sources related to tests in changed files (jestjs#9769) fix: don't /// <reference types="jest" /> (jestjs#9875) noCodeFrame respects noStackTrace (jestjs#9866) chore: update example to [email protected] (jestjs#9746) Improve source map handling when instrumenting transformed code (jestjs#9811) Update .vscode/launch.json settings (jestjs#9868) chore: verify all packages have the same engine requirement (jestjs#9871) fix: pass custom cached realpath function to `resolve` (jestjs#9873) chore: mock stealthy-require in tests (jestjs#9855) chore: update resolve (jestjs#9872) chore: run CircleCI on node 14 (jestjs#9870) Add an option to vscode settings to always use workspace TS (jestjs#9869) fix(esm): handle parallel imports (jestjs#9858) chore: run CI on Node 14 (jestjs#9861) feat: add `@jest/globals` package for importing globals explici… (jestjs#9849) chore: bump resolve package (jestjs#9859) chore(runtime): simplify `createJestObjectFor` (jestjs#9857) chore: fix symlink creation failures on windows in tests (jestjs#9852) chore: skip mercurial tests when no hg installed (jestjs#9840) ...

github-actions · 2021-05-11T08:06:54Z

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 Apr 14, 2020

mbpreble force-pushed the coverage-maps-patch branch from 35c82d4 to c65d3c2 Compare April 14, 2020 20:45

SimenB requested review from jeysal and thymikee April 19, 2020 08:06

SimenB reviewed Apr 19, 2020

View reviewed changes

Matthew Preble added 7 commits April 20, 2020 21:27

Improve source map handling when instrumenting transformed code (jest…

9174c60

…js#5739)

Remove the concept of using Jest's stored sourceMaps to map BabelCove…

118c951

…rage - inputSourceMap should always be available.

Remove use of mapCoverage in jest-runtime.

40fcece

Restore 'mapCoverage' as a nullable field on TransformResult.

6adade9

Removing more code downstream of 'mapCoverage' in jest-runtime.

5a02bb1

Add test arounds stack traces for code which is both transformed and …

fcc7383

…instrumented.

Update changelog.

472b882

mbpreble force-pushed the coverage-maps-patch branch from fc78a83 to 472b882 Compare April 21, 2020 02:32

SimenB reviewed Apr 21, 2020

View reviewed changes

packages/jest-test-result/src/types.ts Show resolved Hide resolved

SimenB reviewed Apr 21, 2020

View reviewed changes

packages/jest-transform/src/ScriptTransformer.ts Outdated Show resolved Hide resolved

SimenB reviewed Apr 21, 2020

View reviewed changes

packages/jest-transform/src/ScriptTransformer.ts Show resolved Hide resolved

Matthew Preble and others added 3 commits April 21, 2020 15:13

Feedback - add Jest 26 TODO for restructuring TransformedSource.

a24fbe6

Feedback - rephrase determination of 'shouldEmitSourceMap'

4cc5905

Merge branch 'master' into coverage-maps-patch

28ccbf4