Jest.mock calls are not hoisted to the top

[GeeWee]

• Issue

Jest.mock('foo') calls are not hoisted to the top of the module as they're meant to, unless allowSyntheticDefaults is set to true.
This is because in jest, they are hoisted automatically by babel-jest:

https://github.com/facebook/jest/blob/master/packages/babel-jest/src/index.js
line 68

 presets: ((options && options.presets) || []).concat([jestPreset]),

and the jestPreset is:
https://github.com/facebook/jest/blob/master/packages/babel-preset-jest/index.js

https://github.com/facebook/jest/blob/master/packages/babel-preset-jest/index.js

I suggest we always run the transpiled code through babel-jest, but we only add the plugin 'transform-es2015-modules-commonjs' if allowSyntheticDefaults is set to true.

This will align with the expected behaviour of vanilla jest.

I think this will also indirectly fix things like #120 which currently only work when allowSyntheticDefaults is set to true, and fixes the workaround needed in #90

[GeeWee]

Currently blocked by #211 - running everything through babel will break coverage for everyone

[kulshekhar]

Just adding a note to remove the check for allowSyntheticDefaultImports when using babel to hoist mock calls

[GeeWee]

Yes. I have this almost finished in a local branch. Would love if someone had time to do a few extra process-regression tests, just with snapshots.

[GeeWee]

I added regression tests. Work is ongoing in https://github.com/kulshekhar/ts-jest/tree/hoist-mock-calls

[GeeWee]

I am unsure as to how to test whether the sourcemaps actually line up. Do you have any suggestions @kulshekhar ?

[kulshekhar]

I had the same problem at the beginning. Here's how I approached it. Write a test in typescript that results in an error (in the test or in the part that the test is testing). See if the line number in the final output matches the actual source file.

So in the current setup, if the tests that check the line numbers pass, the sourcemaps are working properly.

Also, you might want to update your editor settings to add a new line character at the end of the files because it is currently removing it.

[kulshekhar]

Is checking transpiled code against a snapshot is a good idea? I've seen the outputs vary across transpiler versions and it might cause hard to debug failures in CI

[GeeWee]

Hm, that makes sense. I see that the current changes result in the line numbers being right, but the character offset being off. It makes sense as a strategy I guess, although it seems a little hacky.

I'll get the editor fixed.

I'm not sure, don't you think it'll be fine if we pin our dependencies? If there's minor differences between transpiler versions we should just be able to update them. I'd really like the safety net of knowing you don't fuck anything up acidentally. Are there other alternatives?

[kulshekhar]

I see that the current changes result in the line numbers being right, but the character offset being off

I think that's a known issue with babel jest. Currently we're checking the line and column number in the tests. With babel jest, we should probably only check the line number.

Edit: I can swear I read something like this somewhere but can't quite find it. Consider this to not be the case till we get an authoritative source!

Edit2: This is only applicable when setting retainLines to true in babel. That's usually needed when sourcemaps can't be used. Since that's not the case here, we should aim to get both, the row number & the column number, right.

Are there other alternatives?

I think a test that actually uses hoisting would be less brittle. So something that would break if hoisting didn't work but would pass if it did.

[GeeWee]

I think you're thinking of #230 maybe?

I have some tests lying around that uses hoisting, but I haven't comitted them yet. I'm mostly just a little scared that if we start playing with hooks that we don't have a way of sanity checking refactors. Does it matter the tests are brittle if they're easy to update? I'm on the fence about this atm.

[kulshekhar]

found it - I had read that in babel's documentation for retainLines.

I'm mostly just a little scared that if we start playing with hooks that we don't have a way of sanity checking refactors.

That's the advantage of testing like we're currently doing - by running jest on mini projects (that's what the tests essentially are). It doesn't matter what we have inside as long as the external behavior is what we want.

Does it matter the tests are brittle if they're easy to update? I'm on the fence about this atm.

The test for hoisting should be as simple as the simplest test we have. It's just that the code in that mini-project (test) will use something that needs hoisting.

So basically, copy the tests/simple folder, replace the content of Hello.ts with the code that you have and add a test in tests/__tests__ that runs jest on this new folder

[GeeWee]

That's the advantage of testing like we're currently doing - by running jest on mini projects (that's what the tests essentially are). It doesn't matter what we have inside as long as the external behavior is what we want.

Hm, yeah alright. It just seems a little hacky to check the sourcemaps via error lines, but I guess it could work.

The test for hoisting should be as simple as the simplest test we have. It's just that the code in that mini-project (test) will use something that needs hoisting.

Yeah definitely. I did all of that on another pc and forgot to push.. 😭

[GeeWee]

nice catch with the babel documentation.

Should be resolved in #239 - i ended up removing the regression tests, I think you were right.