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 master, add back retainLines to ./.babelrc #5449

Merged
merged 1 commit into from
Feb 4, 2018

Conversation

rickhanlonii
Copy link
Member

Summary

This PR fixes master by rolling back the retainLines change

It looks like removing the retainLines gave the integration tests the wrong line number. For example in this test (and the rest from what I could tell), the snapshot is correct:

@rickhanlonii rickhanlonii changed the title Fix master, add back retainLines Fix master, add back retainLines to ./.babelrc Feb 2, 2018
@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@d743fec). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #5449   +/-   ##
=========================================
  Coverage          ?   62.18%           
=========================================
  Files             ?      205           
  Lines             ?     6928           
  Branches          ?        3           
=========================================
  Hits              ?     4308           
  Misses            ?     2619           
  Partials          ?        1

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 d743fec...81dda5f. Read the comment docs.

@cpojer
Copy link
Member

cpojer commented Feb 2, 2018

Oh, the right fix would be to fix up all the tests instead. cc @jessecarfb

@rickhanlonii
Copy link
Member Author

I looked into that and I think the tests are actually correct as they are written. Updating the tests would assert that the line numbers don't match the actual file lines

Also this is just the retainLines that I changed, I kept @jessecarfb's change

@SimenB
Copy link
Member

SimenB commented Feb 3, 2018

This change shouldn't be needed - I wonder if the stack trace is wrong?

@SimenB
Copy link
Member

SimenB commented Feb 3, 2018

The issue is that the sourcemap is not applied - #5177 might fix this. This diff fixes most of the test suite for me locally, except for tests related to mapCoverage and a couple other (I haven't dug into it). All stack traces are correct at least.

diff --git c/packages/babel-jest/src/index.js w/packages/babel-jest/src/index.js
index 647b56a1..593041c5 100644
--- c/packages/babel-jest/src/index.js
+++ w/packages/babel-jest/src/index.js
@@ -69,7 +69,7 @@ const createTransformer = (options: any) => {
   options = Object.assign({}, options, {
     plugins: (options && options.plugins) || [],
     presets: ((options && options.presets) || []).concat([jestPreset]),
-    sourceMaps: 'inline',
+    sourceMaps: 'both',
   });
   delete options.cacheDirectory;
   delete options.filename;
@@ -128,7 +128,7 @@ const createTransformer = (options: any) => {
 
       // babel v7 might return null in the case when the file has been ignored.
       const transformResult = babelTransform(src, theseOptions);
-      return transformResult ? transformResult.code : src;
+      return transformResult || src;
     },
   };
 };
diff --git c/packages/jest-runtime/src/script_transformer.js w/packages/jest-runtime/src/script_transformer.js
index 9cd85d27..bfcc4792 100644
--- c/packages/jest-runtime/src/script_transformer.js
+++ w/packages/jest-runtime/src/script_transformer.js
@@ -242,12 +242,10 @@ export default class ScriptTransformer {
       }
     }
 
-    if (mapCoverage) {
-      if (!transformed.map) {
-        const inlineSourceMap = convertSourceMap.fromSource(transformed.code);
-        if (inlineSourceMap) {
-          transformed.map = inlineSourceMap.toJSON();
-        }
+    if (!transformed.map) {
+      const inlineSourceMap = convertSourceMap.fromSource(transformed.code);
+      if (inlineSourceMap) {
+        transformed.map = inlineSourceMap.toJSON();
       }
     }
 
@@ -261,7 +259,7 @@ export default class ScriptTransformer {
       code = transformed.code;
     }
 
-    if (instrument && mapCoverage && transformed.map) {
+    if (transformed.map) {
       const sourceMapContent =
         typeof transformed.map === 'string'
           ? transformed.map

The return transformResult || src; part is a breaking change in babel-jest (not for Jest as it supports both forms, but for programmatic usage of it), but maybe we can put it behind an option?

@SimenB
Copy link
Member

SimenB commented Feb 3, 2018

I vote for merging this to fix master, and looking into dropping it later

@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.

5 participants