fix: Preserve order of jest calls when hoisting

[tgriesser]

Summary

It looks like the changes to hoisting in #9806 have incorrectly ordered hoisted jest in the same block, because they are unshifted on the containing scope individually, rather than collectively after walking the AST.

Test plan

Added an integration test in e2e/babel-plugin-jest-hoist.

import a from '../__test_modules__/a';

// These will all be hoisted above imports
jest.enableAutomock();
jest.disableAutomock();

Before the change:

'use strict';

// These will all be hoisted above imports
_getJestObj().disableAutomock();

_getJestObj().enableAutomock();

// ... rest of spec

After change:

'use strict';

// These will all be hoisted above imports
_getJestObj().enableAutomock();

_getJestObj().disableAutomock();

// ... rest of spec

[SimenB]

This change makes sense to me, but CI is very unhappy. I'm assuming this somehow broke our of usage of jest.mock?

Also, please add a changelog entry 🙂

[tgriesser]

Haven't quite figured out how to adapt the Babel plugin here correctly. Tried a few things, including collecting the nodes during traversal and then adding the nodes on exit:

const onExitBlock = (node: NodePath<Block>) => {
  const hoistNodes = node.getData('__babelJestHoistNodes');
  if (hoistNodes) {
    node.unshiftContainer('body', hoistNodes);
  }
};

// ...
{
  post({path: program}: {path: NodePath<Program>}) {
    program.traverse({
      Block: {
        exit: onExitBlock,
      },
      CallExpression: callExpr => {
        const {
          node: {callee},
        } = callExpr;
        if (
          isIdentifier(callee) &&
          callee.name === this.jestObjGetterIdentifier?.name
        ) {
          const mockStmt = callExpr.getStatementParent();

          if (mockStmt) {
            const mockStmtNode = mockStmt.node;
            const mockStmtParent = mockStmt.parentPath;
            if (mockStmtParent.isBlock()) {
              mockStmt.remove();
              const hoistNodes =
                mockStmtParent.getData('__babelJestHoistNodes') ?? [];
              hoistNodes.push(mockStmtNode);
              mockStmtParent.setData('__babelJestHoistNodes', hoistNodes);
            }
          }
        }
      },
    });
    onExitBlock(program as NodePath<Block>);
  },
}

But for some reason the nodes aren't being added properly. Will try a few more things, but would love some eyes from anyone a little more versed in babel transforms.

[SimenB]

@jeysal @nicolo-ribaudo ideas? 😀 I still suck at AST transformations, so not much help here 😛

We should probably have some more direct unit tests for this like discussed in #9806 (comment)

[tgriesser]

@SimenB ok, tried a new approach which seems to work decent enough - added some unit tests as you mentioned and fixed an e2e test which seemed to only be passing due to the previously incorrect hoisting behavior.

[SimenB]

yay, thanks for adding a start for proper unit tests 👍

[tgriesser]

Sidenote: In adding the unit tests, and trying it out with some other issues around @jest/globals like #10218 and #10147, I feel like there's a number of cases that aren't fully covered by the globals change/refactor.

Hoping this will be a good starting place for addressing some of those issues.

[jeysal]

Thanks for the contribution, I agree that order preservation is a valuable guarantee to give!

Regarding the tests I also think it's important to build up a more maintainable test suite than the current one. I think long term we want to go towards exec tests, because snapshot tests like these will in large numbers become unmaintainable as well, because different but equally valid output in 20 snapshots is hard to review. However, this is a good start for sure.

Regarding the approach to fixing this in the Babel plugin using stateful logic, I'm not so sure. Considering that we may have a couple more edge cases to fix in the future as mentioned, I'm worried that the state of the 'tags' on nodes may become confusing. Have you considered the following rough structure, which unless I'm missing something should allow working statelessly close to the existing code:

program.traverse
  BlockStatement|Program
    let firstStatementOfBlock = ...
    block.traverse
      CallExpression
        (The existing code except mockStmtNode is inserted before firstStatementOfBlock)

[SimenB]

@tgriesser thoughts on @jeysal's suggestion?

[tgriesser]

Haven't had a chance to come back to this. If one of y'all want to take a stab at it, feel free.

[SimenB]

That's perfectly fine @tgriesser, thanks for getting this started!

@jeysal would you be up for finishing this? If not I'll just land as is as it's a bug fix

[jeysal]

Yep, I'll give it a shot, I'll have more time again from Nov I think

[jeysal]

Pushed a stateless solution that passes the added tests, just noticed it seems to fail one e2e test though, will TAL

[jeysal]

Nvm that was actually a mistake with the minor comment about moving things around in the test. The code appears to work fine

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