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

CHANGELOG.md

@@ -7,6 +7,7 @@
 ### Fixes
 
 - `[jest-runtime]` Support importing CJS from ESM using `import` statements ([#9850](https://github.com/facebook/jest/pull/9850))
+- `[jest-transform]` Improve source map handling when instrumenting transformed code ([#9811](https://github.com/facebook/jest/pull/9811))
 
 ### Chore & Maintenance
 

e2e/__tests__/stackTraceSourceMapsWithCoverage.test.ts

@@ -0,0 +1,23 @@
+/**
+ * Copyright (c) Facebook, Inc. and its affiliates. 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.
+ */
+import * as path from 'path';
+import {run} from '../Utils';
+import runJest from '../runJest';
+
+it('processes stack traces and code frames with source maps with coverage', () => {
+  const dir = path.resolve(
+    __dirname,
+    '../stack-trace-source-maps-with-coverage',
+  );
+  run('yarn', dir);
+  const {stderr} = runJest(dir, ['--no-cache', '--coverage']);
+
+  // Should report an error at source line 13 in lib.ts at line 10 of the test
+  expect(stderr).toMatch("13 |   throw new Error('This did not work!');\n");
+  expect(stderr).toMatch(`at Object.error (lib.ts:13:9)
+      at Object.<anonymous> (__tests__/fails.ts:10:3)`);
+});

e2e/stack-trace-source-maps-with-coverage/__tests__/fails.ts

@@ -0,0 +1,11 @@
+/**
+ * Copyright (c) Facebook, Inc. and its affiliates. 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.
+ */
+import {error} from '../lib';
+
+it('fails', () => {
+  error();
+});

e2e/stack-trace-source-maps-with-coverage/lib.ts

@@ -0,0 +1,14 @@
+/**
+ * Copyright (c) Facebook, Inc. and its affiliates. 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.
+ */
+interface NotUsedButTakesUpLines {
+  a: number;
+  b: string;
+}
+
+export function error() {
+  throw new Error('This did not work!');
+}

e2e/stack-trace-source-maps-with-coverage/package.json

@@ -0,0 +1,13 @@
+{
+  "jest": {
+    "rootDir": "./",
+    "transform": {
+      "^.+\\.(ts)$": "<rootDir>/preprocessor.js"
+    },
+    "testEnvironment": "node",
+    "testRegex": "fails"
+  },
+  "dependencies": {
+    "typescript": "^3.7.4"
+  }
+}

e2e/stack-trace-source-maps-with-coverage/preprocessor.js

@@ -0,0 +1,21 @@
+/**
+ * Copyright (c) Facebook, Inc. and its affiliates. 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.
+ */
+
+const tsc = require('typescript');
+
+module.exports = {
+  process(src, path) {
+    return tsc.transpileModule(src, {
+      compilerOptions: {
+        inlineSourceMap: true,
+        module: tsc.ModuleKind.CommonJS,
+        target: 'es5',
+      },
+      fileName: path,
+    }).outputText;
+  },
+};

e2e/stack-trace-source-maps-with-coverage/yarn.lock

@@ -0,0 +1,8 @@
+# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
+# yarn lockfile v1
+
+
+typescript@^3.7.4:
+  version "3.8.3"
+  resolved "https://registry.yarnpkg.com/typescript/-/typescript-3.8.3.tgz#409eb8544ea0335711205869ec458ab109ee1061"
+  integrity sha512-MYlEfn5VrLNsgudQTVJeNaQFUAI7DkhnOjdpAp4T+ku1TfQClewlbSuTVHiA+8skNBgaf02TL/kLOvig4y3G8w==

packages/jest-reporters/src/coverage_reporter.ts

@@ -71,28 +71,6 @@ export default class CoverageReporter extends BaseReporter {
     if (testResult.coverage) {
       this._coverageMap.merge(testResult.coverage);
     }
-
-    const sourceMaps = testResult.sourceMaps;
-    if (sourceMaps) {
-      Object.keys(sourceMaps).forEach(sourcePath => {
-        let inputSourceMap: RawSourceMap | undefined;
-        try {
-          const coverage: istanbulCoverage.FileCoverage = this._coverageMap.fileCoverageFor(
-            sourcePath,
-          );
-          inputSourceMap = (coverage.toJSON() as any).inputSourceMap;
-        } finally {
-          if (inputSourceMap) {
-            this._sourceMapStore.registerMap(sourcePath, inputSourceMap);
-          } else {
-            this._sourceMapStore.registerURL(
-              sourcePath,
-              sourceMaps[sourcePath],
-            );
-          }
-        }
-      });
-    }
   }
 
   async onRunComplete(
@@ -215,13 +193,6 @@ export default class CoverageReporter extends BaseReporter {
               ]);
             } else {
               this._coverageMap.addFileCoverage(result.coverage);
-
-              if (result.sourceMapPath) {
-                this._sourceMapStore.registerURL(
-                  filename,
-                  result.sourceMapPath,
-                );
-              }
             }
           }
         } catch (error) {

packages/jest-reporters/src/generateEmptyCoverage.ts

@@ -18,7 +18,6 @@ export type CoverageWorkerResult =
   | {
       kind: 'BabelCoverage';
       coverage: FileCoverage;
-      sourceMapPath?: string | null;
     }
   | {
       kind: 'V8Coverage';
@@ -66,17 +65,18 @@ export default function (
     }
 
     // Transform file with instrumentation to make sure initial coverage data is well mapped to original code.
-    const {code, mapCoverage, sourceMapPath} = new ScriptTransformer(
-      config,
-    ).transformSource(filename, source, true);
+    const {code} = new ScriptTransformer(config).transformSource(
+      filename,
+      source,
+      true,
+    );
     // TODO: consider passing AST
     const extracted = readInitialCoverage(code);
     // Check extracted initial coverage is not null, this can happen when using /* istanbul ignore file */
     if (extracted) {
       coverageWorkerResult = {
         coverage: createFileCoverage(extracted.coverageData),
         kind: 'BabelCoverage',
-        sourceMapPath: mapCoverage ? sourceMapPath : null,
       };
     }
   }

packages/jest-runner/src/runTest.ts

@@ -275,7 +275,6 @@ async function runTestInternal(
       const coverageKeys = Object.keys(coverage);
       if (coverageKeys.length) {
         result.coverage = coverage;
-        result.sourceMaps = runtime.getSourceMapInfo(new Set(coverageKeys));
       }
     }
 

packages/jest-runtime/src/index.ts

@@ -134,7 +134,6 @@ class Runtime {
   private _isolatedModuleRegistry: ModuleRegistry | null;
   private _moduleRegistry: ModuleRegistry;
   private _esmoduleRegistry: Map<string, VMModule>;
-  private _needsCoverageMapped: Set<string>;
   private _resolver: Resolver;
   private _shouldAutoMock: boolean;
   private _shouldMockModuleCache: BooleanObject;
@@ -178,7 +177,6 @@ class Runtime {
     this._isolatedMockRegistry = null;
     this._moduleRegistry = new Map();
     this._esmoduleRegistry = new Map();
-    this._needsCoverageMapped = new Set();
     this._resolver = resolver;
     this._scriptTransformer = new ScriptTransformer(config);
     this._shouldAutoMock = config.automock;
@@ -765,20 +763,9 @@ class Runtime {
       });
   }
 
-  getSourceMapInfo(coveredFiles: Set<string>): Record<string, string> {
-    return Object.keys(this._sourceMapRegistry).reduce<Record<string, string>>(
-      (result, sourcePath) => {
-        if (
-          coveredFiles.has(sourcePath) &&
-          this._needsCoverageMapped.has(sourcePath) &&
-          fs.existsSync(this._sourceMapRegistry[sourcePath])
-        ) {
-          result[sourcePath] = this._sourceMapRegistry[sourcePath];
-        }
-        return result;
-      },
-      {},
-    );
+  // TODO - remove in Jest 26
+  getSourceMapInfo(_coveredFiles: Set<string>): Record<string, string> {
+    return {};
   }
 
   getSourceMaps(): SourceMapRegistry {
@@ -1026,9 +1013,6 @@ class Runtime {
 
     if (transformedFile.sourceMapPath) {
       this._sourceMapRegistry[filename] = transformedFile.sourceMapPath;
-      if (transformedFile.mapCoverage) {
-        this._needsCoverageMapped.add(filename);
-      }
     }
     return transformedFile;
   }

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

@@ -104,6 +104,7 @@ export type TestResult = {
     unmatched: number;
     updated: number;
   };
+  // TODO - Remove in Jest 26
   sourceMaps?: {
     [sourcePath: string]: string;
   };

packages/jest-transform/src/ScriptTransformer.ts

@@ -205,11 +205,15 @@ export default class ScriptTransformer {
 
   private _instrumentFile(
     filename: Config.Path,
-    content: string,
+    input: TransformedSource,
     supportsDynamicImport: boolean,
     supportsStaticESM: boolean,
-  ): string {
-    const result = babelTransform(content, {
+    canMapToInput: boolean,
+  ): TransformedSource {
+    const inputCode = typeof input === 'string' ? input : input.code;
+    const inputMap = typeof input === 'string' ? null : input.map;
+
+    const result = babelTransform(inputCode, {
       auxiliaryCommentBefore: ' istanbul ignore next ',
       babelrc: false,
       caller: {
@@ -228,21 +232,19 @@ export default class ScriptTransformer {
             cwd: this._config.rootDir,
             exclude: [],
             extension: false,
+            inputSourceMap: inputMap,
             useInlineSourceMaps: false,
           },
         ],
       ],
+      sourceMaps: canMapToInput ? 'both' : false,
     });
 
-    if (result) {
-      const {code} = result;
-
-      if (code) {
-        return code;
-      }
+    if (result && result.code) {
+      return result as TransformResult;
     }
 
-    return content;
+    return input;
   }
 
   private _getRealPath(filepath: Config.Path): Config.Path {
@@ -287,18 +289,13 @@ export default class ScriptTransformer {
     const transformWillInstrument =
       shouldCallTransform && transform && transform.canInstrument;
 
-    // If we handle the coverage instrumentation, we should try to map code
-    // coverage against original source with any provided source map
-    const mapCoverage = instrument && !transformWillInstrument;
-
     if (code) {
       // This is broken: we return the code, and a path for the source map
       // directly from the cache. But, nothing ensures the source map actually
       // matches that source code. They could have gotten out-of-sync in case
       // two separate processes write concurrently to the same cache files.
       return {
         code,
-        mapCoverage,
         originalCode: content,
         sourceMapPath,
       };
@@ -333,9 +330,8 @@ export default class ScriptTransformer {
         //Could be a potential freeze here.
         //See: https://github.com/facebook/jest/pull/5177#discussion_r158883570
         const inlineSourceMap = sourcemapFromSource(transformed.code);
-
         if (inlineSourceMap) {
-          transformed.map = inlineSourceMap.toJSON();
+          transformed.map = inlineSourceMap.toObject();
         }
       } catch (e) {
         const transformPath = this._getTransformPath(filename);
@@ -347,22 +343,38 @@ export default class ScriptTransformer {
       }
     }
 
+    // Apply instrumentation to the code if necessary, keeping the instrumented code and new map
+    let map = transformed.map;
     if (!transformWillInstrument && instrument) {
-      code = this._instrumentFile(
+      /**
+       * We can map the original source code to the instrumented code ONLY if
+       * - the process of transforming the code produced a source map e.g. ts-jest
+       * - we did not transform the source code
+       *
+       * Otherwise we cannot make any statements about how the instrumented code corresponds to the original code,
+       * and we should NOT emit any source maps
+       *
+       */
+      const shouldEmitSourceMaps = (!!transform && !!map) || !transform;
+
+      const instrumented = this._instrumentFile(
         filename,
-        transformed.code,
+        transformed,
         supportsDynamicImport,
         supportsStaticESM,
+        shouldEmitSourceMaps,
       );
+
+      code =
+        typeof instrumented === 'string' ? instrumented : instrumented.code;
+      map = typeof instrumented === 'string' ? null : instrumented.map;
     } else {
       code = transformed.code;
     }
 
-    if (transformed.map) {
+    if (map) {
       const sourceMapContent =
-        typeof transformed.map === 'string'
-          ? transformed.map
-          : JSON.stringify(transformed.map);
+        typeof map === 'string' ? map : JSON.stringify(map);
       writeCacheFile(sourceMapPath, sourceMapContent);
     } else {
       sourceMapPath = null;
@@ -372,7 +384,6 @@ export default class ScriptTransformer {
 
     return {
       code,
-      mapCoverage,
       originalCode: content,
       sourceMapPath,
     };
@@ -396,7 +407,6 @@ export default class ScriptTransformer {
 
     let code = content;
     let sourceMapPath: string | null = null;
-    let mapCoverage = false;
 
     const willTransform =
       !isInternalModule &&
@@ -415,12 +425,10 @@ export default class ScriptTransformer {
 
         code = transformedSource.code;
         sourceMapPath = transformedSource.sourceMapPath;
-        mapCoverage = transformedSource.mapCoverage;
       }
 
       return {
         code,
-        mapCoverage,
         originalCode: content,
         sourceMapPath,
       };