Hoist mock calls #239

GeeWee · 2017-05-31T10:00:20Z

Resolves #227

Apologies for the huge reformat commit in postprocessor - I think it's unavoidable we get a few indentation commits after switching to .editorconfig.

I've had to adjust a few column numbers in the tests, and I've given most of them proper descriptions. My earlier experiments led me to think the coverage numbers would change, but they ended up remaining the same, which is fine I.

kulshekhar · 2017-05-31T10:09:53Z

src/postprocess.ts

+        ...options,
+        plugins: (options && options.plugins) || [],
+        presets: ((options && options.presets) || []).concat([jestPreset]),
+        retainLines: true,


We don't really need retainLines, do we?

Unsure. It breaks a bunch of tests if it is not included. Perhaps it only needs to be set if inline sourcemaps are turned off?

I think we need to set inlineSourcemaps to true for sourcemaps to work. This is because we don't generate sourcemap files on disk during testing

You mean "sourceMaps": "inline" in babel or in tsconfig? I think we already set it for tsconfig.
I have tried setting it for babel but it gives me off by one error for every tests.

inlineSourcemaps: true for typescript and devtool: inline-source-map for babel

Isn't devtool: inline-source-map a webpack thing? I'm not seeing in in the babel docs anywhere?

You're right.

I'm gonna lay off of this thing for today. I don't think I'm helping

I see that babel does have an inputSourceMap though. What problem are we trying to solve exactly?

That's cool. I think this is worth exploring, but if we think the sourcemaps work as-is, I don't think there's a need to change the code we copied from babel-jest

kulshekhar · 2017-05-31T10:12:12Z

tests/__tests__/ts-errors.spec.ts

@@ -9,9 +9,20 @@ describe('hello_world', () => {
    const stderr = result.stderr.toString();

    expect(result.status).toBe(1);
-    expect(stderr).toContain('Hello.ts:13:11');
+    expect(stderr).toContain('Hello.ts:18:11');


This doesn't look right. If the source file hasn't changed, why should the line number change in the test?

Good catch - the file has changed though. I wanted to be completely sure I didn't break anything, so I added a typescript interface to simple/Hello.ts - I wanted to put something in there that'd fill up a few lines, but I knew would be stripped by tsc

This uses simple/Hello.ts as the source, right? That should show an error on line 13

Yes. Not since I edited the file ;)

I checked the wrong version of the file while making that comment. It looks alright!

kulshekhar · 2017-05-31T11:37:05Z

src/postprocess.ts

+        if (transformOptions && transformOptions.instrument) {
+            theseOptions.auxiliaryCommentBefore = ' istanbul ignore next ';
+            // Copied from jest-runtime transform.js
+            theseOptions.plugins = theseOptions.plugins.concat([


Do we need to use istanbul? With mapCoverage set to true, won't Jest take care of the instrumentation?

I don't actually know - I just assumed it was there for a reason in babel-jest. I'll try removing it and see what happens.

Removing istanbul leads to some markedly different coverage numbers - I think it has to stay.

Conflicts: yarn.lock

GeeWee · 2017-05-31T17:20:37Z

I think we should confirm sourcemaps with correct line numbers in files with hoist statements

kulshekhar · 2017-06-01T12:26:51Z

I'm planning to take a look at this and try and get this working this weekend

bcruddy · 2017-06-01T12:36:55Z

@kulshekhar I'll finally be able to get into it this weekend as well

GeeWee · 2017-06-01T13:24:00Z

I've added a test that fails, because the sourcemaps are not updated after the jest.mock() calls are hoisted. For some reason the babel sourcemap transforms fuck everything up. I've tried forcing typescript to generate an external sourcemap, and then use inputSourceMap to put it into babel, but that also did not work. It seems like this is the approach stuff like ts-loader uses.
An alternate approach i have not tried here

Currently the only test case where we're failing is if the hook lines are below the functionality which makes sense as they're hoisted, and if they're above the line numbers remain above, albeit in a different order.

What puzzles me is why retainLines is needed - in my opinion I'd think that we should simply be able to pick up the sourcemaps.

I can see that babel does transform the inline sourcemaps, if sourcemaps are set to inline in the transformer, which I assume is fine. I'm wondering if the sourcemaps are actually fine, and it's just the stack traces that are off. Do we need to do something in the install() hook? I'm not quite getting what's going on in there yet, it seems like we only supply a file handler and not a sourcemap handler - and why is that file transpiled - it seems like it should fetch the actual typescript file?

Good to have you onboard @bcruddy !

kulshekhar · 2017-06-03T11:59:19Z

@GeeWee take a look at the changes I've pushed in whenever you get a change :)

GeeWee · 2017-06-03T12:01:15Z

tests/__tests__/jest-hoist.spec.ts

+        // The actual error occurs at line 16. However, because the mock calls
+        // are hoisted, this changes - in this case, to 26
+        // The column numbers are accurate.
+        expect(stderr).toContain('Hello.test.ts:26:19');


Should we not seek to provide a sourcemap that correctly resolves this to 16.19?

I'm not sure how that would be done without generating our own sourcemaps!
I think this is the best we can do. We'll have to add a note about this behavior, of course

GeeWee · 2017-06-03T12:07:19Z

@kulshekhar good call on removing the mocks from the actual code under test. I'm still unsure as to why we don't resolve the line numbers to the correct one in the ts file. If the sourcemaps are right, it's a problem with our install hook?

kulshekhar · 2017-06-03T12:08:59Z

I'm slightly confused. It seems to be getting resolved correctly 😕

GeeWee · 2017-06-03T12:13:47Z

Sorry, let me try again:
in jest-hoist-spec we have:

// The actual error occurs at line 16. However, because the mock calls
+        // are hoisted, this changes - in this case, to 26
+        // The column numbers are accurate.
expect(stderr).toContain('Hello.test.ts:26:19');

Shouldn't we attempt to make the stdErr contain 'Hello.test.ts:16:19' so it aligns with the typescript file?

kulshekhar · 2017-06-03T12:24:01Z

Oh that.. I thought you meant in general.

When mocking, we're rearranging the source code and we lose information about the real line numbers. The only way I can think of to avoid this would be to generate our own sourcemaps and update the original sourcemap. Currently this is what happens:

TS compiles & generates the sourcemap
Babel compiles, uses the TS output and the TS generated sourcemap to generate new output and new sourcemap.

What would be needed will be something like:

TS compiles & generates the sourcemap
Transpiler X hoists mock calls & uses the TS generated sourcemap to generate output (with hoisted mock calls) and new sourcemap

We'd have to write our own transpiler, of sorts, that would generate the sourcemap for hoisting.

How do they do this when using just babel? They'd face the same issue, right?

GeeWee · 2017-06-03T12:44:42Z

I actually expected the babel plugin to modify the source map so that it takes the moved lines into account - do you think that's way off?

kulshekhar · 2017-06-03T12:47:26Z

I'm not sure that's happening. Babel has no idea about jest. It just generates the sourcemap for the source it has been given. In this case, babel-jest hoists the mock calls and then calls babel. That's the version babel is generating sourcemaps for.

GeeWee · 2017-06-03T12:50:25Z

Hm, the hoist is a babel plugin though. Babel must have some sort of way to generate sourcemaps based on plugin transforms, otherwise it'd rarely work at all?

kulshekhar · 2017-06-03T12:55:02Z

I suspect users of babel-jest would be facing this issue as well. The plugin has no code related to sourcemaps.

In any case, I don't see a proper solution for this in ts-jest.

kulshekhar · 2017-06-03T12:57:04Z

Would it be reasonable to ask users to place calls to mock at the top of their code?

GeeWee · 2017-06-03T13:26:29Z

I don't think that's unreasonable.

I have however had an AHA moment.

This is a source-map visualisation of our output. Notice how the jest.mock marked is correctly inferred to have moved? To me this suggests that our problem is not in our sourcemaps, but in our install hook.

If you want to explore sourcemaps yourself:
Try going to this url.

it looks mangled as fuck - then, go inspect the table row with the inline sourcemap in and delete it, this will unfuck the formatting.

GeeWee · 2017-06-03T13:31:28Z

However, we might be solidly in the #notworth territory - I don't feel that forcing people to place their mock calls before their code is a terrible sacrifice.

kulshekhar · 2017-06-03T15:21:39Z

To me this suggests that our problem is not in our sourcemaps, but in our install hook.

If that's the case then we need to find a way to get this working right

kulshekhar · 2017-06-03T15:44:42Z

I'm not sure that source map visualization accurately reflects what's going on. What inputs did you give it?

GeeWee · 2017-06-03T15:47:06Z

Do we though? If the sourcemaps work in all other cases, I don't see it as a pressing issue.
Also, I don't really understand what's going on in the install hook.

I feel like we can test whether it works in all other cases with a test case that uses async/await transpiled to es3 - that mangles the source code completely.

response to latest comment:
I wrote out the file returned by the preprocessor. I took the jest-hoist.test after preprocessing and the original jest-hoist.test.ts.
What is wrong with it?

kulshekhar · 2017-06-03T16:01:42Z

Do we though? If the sourcemaps work in all other cases, I don't see it as a pressing issue.

I think at this stage we can add a note about this issue and track it in a separate issue.

I don't really understand what's going on in the install hook.

If you're referring to the code that injects require('ts-jest').install(), it is required to get sourcemaps to work in nodejs. It uses source-map-support which needs to be registered before any code runs.

I feel like we can test whether it works in all other cases with a test case that uses async/await transpiled to es3 - that mangles the source code completely.

All tests that check the line numbers do that, don't they? Or do you mean something else?

What is wrong with it?

I'm not really sure. It's just that I can't see any place where hoisting has modified the sourcemap.

kulshekhar · 2017-06-03T17:56:30Z

The sourcemap is generated only after all the plugins are executed

There's not much we can do about incorrect mappings that result from hoisting. A note about this will have to do for now.

This looks good for a merge. What do you think?

GeeWee · 2017-06-03T20:26:24Z

Strange. I remember seeing the sourcemap being transformed.
Everything babel transforms is via a plugin: https://babeljs.io/docs/plugins/
That obviously means the sourcemap is generated afterwards, I'd be more worried if it was generated before the transforms ;)

I agree with a note being enough - let's track this in a seperate issue and merge this in. The issue is reasonably low priority in my book, I think most people add their mock statements at the top anyway.

kulshekhar · 2017-06-04T06:21:42Z

That obviously means the sourcemap is generated afterwards, I'd be more worried if it was generated before the transforms ;)

I was hoping that they somehow updated the sourcemap (or tracked changes for use during sourcemap generation) as plugins modify the source code but that doesn't seem to be the case

GeeWee added 12 commits May 30, 2017 12:47

Updated dependencies, added regression tests and cleaned up tests

544a0a9

new babel regression test

5de92d0

uses babeltransformer always. Breaks sourcemaps

666c90e

all tests but hoist pass atm

b79a491

all tests pass now

b26a949

fixed a few things

b1a2224

maybe two spaces is right

87630d8

one?

580f841

ok we'll go with four

1e2a83a

line feed

d7e53a8

line feed

f8822db

format

609b533

GeeWee requested a review from kulshekhar May 31, 2017 10:00

GeeWee mentioned this pull request May 31, 2017

Jest.mock calls are not hoisted to the top #227

Closed

kulshekhar requested changes May 31, 2017

View reviewed changes

GeeWee added 2 commits May 31, 2017 14:13

format

7e927e3

Merge remote-tracking branch 'origin/master' into hoist-mock-calls

4179926

Conflicts: yarn.lock

commit failing test

f251ca9

kulshekhar added 6 commits June 3, 2017 17:04

Remove the mock calls from the test application file

31c9014

Update the expected line numbers in the test for hoisting.

45da28a

Force babel to generate inline sourcemaps

467a853

Add column numbers to the hoist test

541f407

Set sourceMaps for babel during declaration of the options

73ad65b

Fix typo in a comment

03c841e

kulshekhar approved these changes Jun 3, 2017

View reviewed changes

GeeWee commented Jun 3, 2017

View reviewed changes

kulshekhar merged commit deef466 into master Jun 3, 2017

kulshekhar deleted the hoist-mock-calls branch June 3, 2017 21:01

GeeWee mentioned this pull request Jun 4, 2017

jest.mock() calls underneath the code mangle the stacktraces #240

Closed

macklinu mentioned this pull request Jun 10, 2017

An in-range update of ts-jest is breaking the build 🚨 danger/danger-js#274

Closed

Hoist mock calls #239

Hoist mock calls #239

Conversation

GeeWee commented May 31, 2017

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

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

GeeWee commented May 31, 2017

kulshekhar commented Jun 1, 2017

bcruddy commented Jun 1, 2017

GeeWee commented Jun 1, 2017

kulshekhar commented Jun 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeeWee commented Jun 3, 2017

kulshekhar commented Jun 3, 2017

GeeWee commented Jun 3, 2017 • edited Loading

kulshekhar commented Jun 3, 2017

GeeWee commented Jun 3, 2017

kulshekhar commented Jun 3, 2017

GeeWee commented Jun 3, 2017

kulshekhar commented Jun 3, 2017

kulshekhar commented Jun 3, 2017

GeeWee commented Jun 3, 2017 • edited Loading

GeeWee commented Jun 3, 2017

kulshekhar commented Jun 3, 2017

kulshekhar commented Jun 3, 2017

GeeWee commented Jun 3, 2017

kulshekhar commented Jun 3, 2017

kulshekhar commented Jun 3, 2017 • edited Loading

GeeWee commented Jun 3, 2017

kulshekhar commented Jun 4, 2017

GeeWee commented Jun 3, 2017 •

edited

Loading

GeeWee commented Jun 3, 2017 •

edited

Loading

kulshekhar commented Jun 3, 2017 •

edited

Loading