[WIP] Remove usage of `retainLines` #5594

SimenB · 2018-02-17T14:28:51Z

Summary

~~Pushing up what I've got for now. Tests are failing because callsites does not look up sourcemaps. I've added a TODO in the code where the cause of the failing tests are.~~

Only outstanding issue is with columns after looking up a sourcemap. See #5594 (comment). Help appreciated!

Fixes #5326.

Test plan

Old tests should pass, even with no retainLines

SimenB · 2018-02-17T14:31:00Z

packages/jest-jasmine2/src/index.js


 const JASMINE = require.resolve('./jasmine/jasmine_light.js');

+// Copied from https://github.com/rexxars/sourcemap-decorate-callsites/blob/5b9735a156964973a75dc62fd2c7f0c1975458e8/lib/index.js#L113-L158


@rexxars thoughts on exporting extendCallsite from sourcemap-decorate-callsites?

I wanted to use your module directly, but I don't want the searching for sourcemaps - I have it already. I also need this to be sync

Or just expose an API where I can pass ina callsite and a sourcemap, that would be even better (as long as it's sync) 🙂

I'd welcome the change, but I'm absolutely swamped with work - be happy if anyone would submit a PR, though.

SimenB · 2018-02-17T14:31:36Z

packages/jest-jasmine2/package.json

    "jest-matcher-utils": "^22.2.0",
    "jest-message-util": "^22.2.0",
    "jest-snapshot": "^22.2.0",
+    "source-map": "^0.6.0",


newest is 0.7.1, but they changed their API to be asynchronous, so we can't use it

SimenB · 2018-02-17T14:33:25Z

packages/jest-util/src/buffered_console.js

    message: LogMessage,
    level: ?number,
  ) {
+    // TODO: This callsite needs to read sourcemaps


The issue is here: https://github.com/facebook/jest/blob/430aebe49e9f144d29f144f5a29482315e269e4a/packages/jest-runner/src/run_test.js#L86-L108

I need to pass in runtime to the console, but environment requires testConsole and the runtime requires environment

SimenB · 2018-02-17T14:34:27Z

packages/jest-runtime/src/index.js

+    const sourceMap = this._sourceMapRegistry[filename];
+
+    if (sourceMap && fs.existsSync(sourceMap)) {
+      return fs.readFileSync(sourceMap, 'utf8');


I wonder if we should cache these instead of hitting the disk every time

can try-catch instead of 2 fs operations, at least

SimenB · 2018-02-17T14:52:21Z

integration-tests/__tests__/__snapshots__/failures.test.js.snap

      }
      
-      at ../../packages/expect/build/index.js:145:57
+      at ../../packages/expect/build/index.js:104:76


we should probably remove this line entirely

Feel free to remove the lilne:column mapping in a followup

# Conflicts: # CHANGELOG.md

codecov-io · 2018-02-17T22:05:16Z

Codecov Report

Merging #5594 into master will increase coverage by 1.34%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##           master    #5594      +/-   ##
==========================================
+ Coverage   60.94%   62.28%   +1.34%     
==========================================
  Files         215      216       +1     
  Lines        7335     7898     +563     
  Branches        3        4       +1     
==========================================
+ Hits         4470     4919     +449     
- Misses       2864     2978     +114     
  Partials        1        1

Impacted Files	Coverage Δ
packages/jest-util/src/index.js	`90.47% <ø> (+1%)`	⬆️
packages/jest-runner/src/run_test.js	`2.17% <0%> (-0.05%)`	⬇️
packages/jest-util/src/get_callsite.js	`75% <100%> (ø)`
packages/jest-runtime/src/index.js	`73.59% <100%> (+0.18%)`	⬆️
packages/jest-jasmine2/src/index.js	`17.2% <60%> (+11.72%)`	⬆️
packages/jest-util/src/buffered_console.js	`63.15% <66.66%> (+2.04%)`	⬆️
packages/jest-jasmine2/src/jasmine_async.js	`11.86% <0%> (-21.47%)`	⬇️
packages/jest-config/src/validate_pattern.js	`85.71% <0%> (-14.29%)`	⬇️
packages/jest-config/src/get_max_workers.js	`90% <0%> (-10%)`	⬇️
packages/jest-config/src/defaults.js	`92.85% <0%> (-7.15%)`	⬇️
... and 73 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 7bcdc8c...736eb2b. Read the comment docs.

SimenB · 2018-02-17T22:41:53Z

packages/jest-util/src/get_callsite.js

+
+// Copied from https://github.com/rexxars/sourcemap-decorate-callsites/blob/5b9735a156964973a75dc62fd2c7f0c1975458e8/lib/index.js#L113-L158
+const addSourceMapConsumer = (callsite, consumer) => {
+  try {


this try can be removed, it's within another try

SimenB · 2018-02-17T22:42:44Z

packages/jest-util/src/get_callsite.js

+
+    return stack;
+  } catch (e) {
+    return null;


it should return stack at the end either way

SimenB · 2018-02-17T22:43:31Z

packages/jest-util/src/get_callsite.js

+export default (level: number, sourceMaps: SourceMapRegistry) => {
+  const levelAfterThisCall = level + 1;
+  const stack = callsites()[levelAfterThisCall];
+  const sourceMapFileName = sourceMaps && sourceMaps[stack.getFileName()];


Either add a ? to the type declaration, or drop the sourceMaps && part

SimenB · 2018-02-17T22:44:17Z

packages/jest-runner/src/run_test.js

      !!globalConfig.verbose,
      // 4 = the console call is buried 4 stack frames deep
-      BufferedConsole.write([], type, message, 4),
+      BufferedConsole.write(


oh, I didn't know this was the way it's hooked up. practical!

We should probably pass it to the BufferedConsole as well

SimenB · 2018-02-17T22:45:22Z

packages/jest-jasmine2/src/index.js

      handleUncaughtExceptions: false,
      retrieveSourceMap: source => {
-        if (runtime._sourceMapRegistry[source]) {
+        const sourceMap = runtime.getSourceMapForFile(source);


should probably use runtime.getSourceMaps() here as well and remove getSourceMapForFile

SimenB · 2018-02-17T22:53:57Z

packages/jest-util/src/get_callsite.js

+  try {
+    if (sourceMapFileName) {
+      const sourceMap = fs.readFileSync(sourceMapFileName, 'utf8');
+      if (sourceMap) {


is this needed? readFileSync will just throw

SimenB · 2018-02-17T23:05:33Z

Merging #5594 into master will increase coverage by 1.02%.

woah, what's up?

EDIT: Seemingly bringing the coverage back from #5177 (which was minus 1.10%)

https://codecov.io/gh/facebook/jest/pull/5177

SimenB · 2018-02-18T07:55:10Z

packages/jest-runner/src/run_test.js

  } else if (globalConfig.verbose) {
    testConsole = new Console(consoleOut, process.stderr, consoleFormatter);
  } else {
+    // TODO: Should `BufferedConsole` receive `consoleFormatter` as well?


@rickhanlonii I think so - the internal _log call in BufferedConsole is without sourcemaps now

thymikee · 2018-02-18T07:55:19Z

integration-tests/__tests__/__snapshots__/failures.test.js.snap

      }
      
-      at ../../packages/expect/build/index.js:145:57
+      at ../../packages/expect/build/index.js:104:76


Feel free to remove the lilne:column mapping in a followup

thymikee · 2018-02-18T07:56:48Z

CHANGELOG.md

  ([#5560](https://github.com/facebook/jest/pull/5560))
 * `[jest-config]` Make it possible to merge `transform` option with preset
  ([#5505](https://github.com/facebook/jest/pull/5505))
+* `[babel-jest]` Remove `retainLines` argument to babel.


"...from babel", or just "Remove usage of retainLines"

My thinking was that people would go "what's retainLines, I've never passed that to jest before"

SimenB · 2018-02-19T22:12:10Z

packages/jest-runner/src/run_test.js

    testConsole = new Console(consoleOut, process.stderr, consoleFormatter);
  } else {
-    testConsole = new BufferedConsole();
+    testConsole = new BufferedConsole(() => runtime && runtime.getSourceMaps());


went with a getter as consoleFormatter seemed like it did too much

looks great

fwiw i don't think there's an integration test for this path

SimenB · 2018-02-19T22:12:51Z

packages/jest-util/src/get_callsite.js

@@ -0,0 +1,64 @@
+/**


@rickhanlonii would you mind adding a couple of unit tests to this?

Yeah, will do

SimenB · 2018-02-20T18:25:36Z

packages/jest-util/src/__tests__/get_callsite.test.js

+
+describe('getCallsite', () => {
+  beforeEach(() => {
+    fs.readFileSync = jest.fn();


When you do jest.mock without a factory, all exports are mocks by default. So this beforeEach shouldn't be necessary

You can do jest.clearMocks if needed

The test also fails CI?

Fixed, not sure what I was thinking

# Conflicts: # packages/jest-jasmine2/package.json # packages/jest-util/package.json

SimenB · 2018-02-21T06:48:36Z

@cpojer This is ready except for the comments here: https://github.com/facebook/jest/pull/5594/files#r168943134

Thoughts?

cpojer · 2018-02-21T10:01:07Z

This is really good work. I like how it actually simplifies things in Jest and makes it more correct. The one offset that's wrong is fine for now, let's just make sure this is properly reported in the right repo and eventually fixed upstream :)

github-actions · 2021-05-12T20:06:46Z

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.

Remove usage of retainLines

2b8e256

facebook-github-bot added the CLA Signed ✔️ label Feb 17, 2018

SimenB commented Feb 17, 2018

View reviewed changes

update changelog

0c1dd5c

This was referenced Feb 17, 2018

[babel-jest] Failures on decorators with async methods #5326

Closed

retainLines breaks no LineTerminator requirements in single-arg async arrows and decorated methods babel/babel#7275

Closed

rickhanlonii added 6 commits February 17, 2018 16:31

Move callsite to jest-util, add sourcemap support

8f9e54f

This is a type

ab81892

Merge remote-tracking branch 'upstream/master' into remove-retainlines

1189fe7

# Conflicts: # CHANGELOG.md

Remove unused function moved to jest-util

8e50d0f

Fix lint

6ad7202

Minor clean `up

6ec7b74

SimenB commented Feb 17, 2018

View reviewed changes

SimenB requested a review from thymikee February 17, 2018 23:12

rickhanlonii and others added 3 commits February 17, 2018 18:25

Feedback updates

11948b8

fix flow violations and minimize diff

765f78a

add todo

f0c6e08

SimenB commented Feb 18, 2018

View reviewed changes

thymikee reviewed Feb 18, 2018

View reviewed changes

use sourcemaps in buffered console as well

7ce154d

SimenB commented Feb 19, 2018

View reviewed changes

SimenB and others added 2 commits February 20, 2018 00:16

fix stupid typo

3f2ccb8

Add unit tests for get_callsite

e73423d

SimenB commented Feb 20, 2018

View reviewed changes

rickhanlonii and others added 4 commits February 20, 2018 14:00

Fix tests

0a29175

Merge remote-tracking branch 'upstream/master' into remove-retainlines

e7ccbc6

# Conflicts: # packages/jest-jasmine2/package.json # packages/jest-util/package.json

Merge branch 'master' into remove-retainlines

2e0e0a3

move changelog entry

736eb2b

SimenB force-pushed the remove-retainlines branch from 8347bb5 to 736eb2b Compare February 21, 2018 06:46

cpojer merged commit 23eec74 into jestjs:master Feb 21, 2018

SimenB deleted the remove-retainlines branch February 21, 2018 10:38

SimenB mentioned this pull request Feb 22, 2018

jest-circus test utilities #5636

Merged

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 Aug 14, 2019

Have Jests console respect sourcemaps. #4227

Closed

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


		const JASMINE = require.resolve('./jasmine/jasmine_light.js');

		// Copied from https://github.com/rexxars/sourcemap-decorate-callsites/blob/5b9735a156964973a75dc62fd2c7f0c1975458e8/lib/index.js#L113-L158

Uh oh!

[WIP] Remove usage of retainLines #5594

[WIP] Remove usage of retainLines #5594

Uh oh!

Conversation

SimenB commented Feb 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

SimenB Feb 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Feb 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SimenB commented Feb 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SimenB commented Feb 21, 2018

Uh oh!

cpojer commented Feb 21, 2018

[WIP] Remove usage of `retainLines` #5594

[WIP] Remove usage of `retainLines` #5594

SimenB commented Feb 17, 2018 •

edited

Loading

SimenB Feb 17, 2018 •

edited

Loading

codecov-io commented Feb 17, 2018 •

edited

Loading

SimenB commented Feb 17, 2018 •

edited

Loading