fix: correct overeager hoisting in babel-plugin-jest-hoist

[jsnajdr]

Unit test with a snapshot that demonstrates a bug, a regression introduced in #13188.

If I call jest.spyOn, or something else from the jest namespace, inside a jest.mock factory, it will get hoisted to the top of the block. But sometimes the hosting is bad. I'm providing two examples:

jest.mock('some-module', () => {
  const module = jest.requireActual('some-module');
  jest.spyOn(module, 'add');
  return module;
});

will be transformed to:

_getJestObj().mock('some-module', () => {
  _getJestObj().spyOn(module, 'add');
  const module = jest.requireActual('some-module');
  return module;
});

where the jest.spyOn call will be moved to a place where the module variable isn't defined yet.

Second example is a class constructor where the call is moved before super:

jest.mock('some-module', () => {
  const Actual = jest.requireActual('some-module');
  return class Mocked extends Actual {
    constructor() {
      super();
      jest.spyOn(module, 'add');
    }
  };
});

is turned into:

_getJestObj().mock('some-module', () => {
  const Actual = jest.requireActual('some-module');
  return class Mocked extends Actual {
    constructor() {
      _getJestObj().spyOn(module, 'add');
      super();
    }
  };
});

where instantiating the Mocked class will always fail with:

Must call super constructor in derived class before accessing 'this'

FYI @nicolo-ribaudo: before #13188 the jest.spyOn calls weren't touched, they were not transformed to _getJestObj().spyOn and not hoisted.

I think we want to hoist only jest.mock calls, not any other jest.* ones?

[SimenB]

I think we want to hoist only jest.mock calls, not any other jest.* ones?

Correct. Are you up for sending a PR fixing this? Tests demonstrating the issue is of course a great start 😀

[jsnajdr]

Are you up for sending a PR fixing this?

Yes, I should be able to fix this.

[jsnajdr]

Hi @SimenB 👋 I converted this PR into a full fix 🙂

The transform works in two steps: first, in a visitor, it looks at jest.* expressions and converts the jest identifiers into getJestObj(). No hoisting yet. Before #13188 this visitor converted only calls like jest.mock, where the function matched the FUNCTIONS tests. After #13188 it also converts calls that have a parent node that's jest.mock-like. We want that.

The second step (in post) is to find the getJestObj() calls and hoist the expressions that contain them. But we don't want to hoist them all. Only the ones that match the original "matches FUNCTIONS" condition. The ones that match the new "parent" condition, we don't want to hoist them.

I fixed this by changing the return value of extractJestObjExprIfHoistable to include a hoist: boolean flag. And then adding only getJestObj() calls that have hoist: true to a hoistedJestGetters set. Only the members of this set are then hoisted in post, in the visitCallExpr visitor.

It's quite similar to the hoistedVariables set. Populated during the first step (visitor), used and hoisted during the second step (post and visitVariableDeclarator).

The unit tests are now passing, both old ones and the ones I added. Let me know what you think.

[SimenB]

nice! can you add a changelog entry as well?

[SimenB]

@nicolo-ribaudo it'd be awesome if you have time to give this a quick look! 🙂

[jsnajdr]

can you add a changelog entry as well?

Done!

[SimenB]

somewhat surprised jest.requireActual isn't replaced with _getJestObj().requireActual. But I guess that's not a change from this PR?

I.e. question 2 from #13188. I think we should always transform, but we should not always hoist

[jsnajdr]

I think this is another imperfection introduced in #13188. The transforming code visits only ExpressionStatements, but this one is a VariableDeclaration.

Even the code that looks at ExpressionStatement will only look at the top-most node and detect a top-level jest.* CallExpression. If it's nested, like in:

expect(jest.foo()).toBe('bar');

then the jest.foo call also won't be found, although the statement is ExpressionStatement.

The visitor and the extractJestObjExprIfHoistable would need to significantly be reworked to fix all this.

The source of the problem is that the original code was intended to transform only standalone statements like

jest.mock( ... );

or

jest.disableAutomock();

but now #13188 greatly expanded the scope to work with all possible jest.* usages at all possible places, and it's far from covering all possible cases.

[jsnajdr]

@SimenB I pushed a fix to this branch and it seems to work 🙂 Instead of visiting only ExpressionStatements and searching for CallExpressions as their immediate children, I'm visiting all CallExpressions, and if they have a jest.* format and are inside a hoisted jest.mock call, I'm replacing jest with getJestObj().

One case that is controversial is one where a hoistable jest.mock call is not an immediate child of an ExpressionStatement, but is nested somewhere inside:

jest.mock('module', () => {
  require('x');
  expect(jest.mock('otherModule')).toBe(1);
});

Should the entire expect statement be hoisted before the require?

I decided to not hoist these, and hoist only call expressions that satisfy the callExpr.parentNode.isExpressionStatement() condition.

The main reason is that allowing them to be hoisted opens a new can of worms, where one statement can contain multiple hoistable calls:

expect(jest.mock('a')).toEqual(jest.mock('b'));

Trying to transform this code with the current plugin leads to the plugin crashing, with an infinite loop and stack overflow inside @babel/traverse. I didn't debug this.

[SimenB]

this is going away from your original fixes, so I'm happy to merge this as-is as my comment is very much an edge case 🙂

[SimenB]

this one should be hoisted, I think... would have to assign the inner case and stuff tho, so maybe not...

Can we throw an error in this case? jest.mock should not be the argument of a function.

[SimenB]

ergh, latest changes broke some other mocking, so reverting to the previous version is probably a good idea 😅

[jsnajdr]

ergh, latest changes broke some other mocking, so reverting to the previous version is probably a good idea

The thing that broke are chained mock calls, like

jest
  .mock( 'module1' )
  .mock( 'module2' )
  .mock( 'module3' )

I reverted the branch to the state it had yesterday, with just the original fix.

[SimenB]

wonderful! thanks for this great contribution (and patience with me 😀)

[SimenB]

https://github.com/facebook/jest/releases/tag/v29.5.0

[kpruden]

I think this change may have broken a case I'm relying on.

I'm using jest.mock to mock out a module, using a helper function that is imported from a different file:

// Top of file
import {
  makeAPIMocks as mockMakeAPIMocks,
} from '../../../src/api/mock';

jest.mock('../../../src/api/clients', () => mockMakeAPIMocks());

...

This was working in 29.4.2, but is broken in 29.5.0

[SimenB]

Hmm, I'm not sure that was supposed to work. You can use jest.requireActual within your callback

[jsnajdr]

This is a typical code pattern that is supposed to trigger an error:

The module factory of `jest.mock()` is not allowed to reference any out-of-scope variables.
Invalid variable access: mockMakeAPIMocks

It's strange that it worked for you in 29.4.2. I tested this both in 29.4.2 and in 29.5.0 and it fails in both versions with the same message.

You need to require or requireActual the src/api/mock module inside the jest.mock factory body. It can't reference any outside variables, because the jest.mock statement needs to be able to move freely around the source module.

[kpruden]

That's interesting. I do not get this error. I don't get any error at all from Jest. What appears to be happening is the mock simply does not get set up, which results in my tests failing.

I will try requireActual and see if that resolves it.

[github-actions]

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.