Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Preserve order of jest calls when hoisting #10536

Merged
merged 12 commits into from
Oct 29, 2020

Conversation

tgriesser
Copy link
Contributor

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
Copy link
Member

SimenB commented Sep 20, 2020

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 🙂

@SimenB SimenB requested a review from jeysal September 21, 2020 16:00
@tgriesser
Copy link
Contributor Author

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
Copy link
Member

SimenB commented Sep 21, 2020

@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
Copy link
Contributor Author

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

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

e2e/babel-plugin-jest-hoist/__tests__/integration.test.js Outdated Show resolved Hide resolved
@tgriesser
Copy link
Contributor Author

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.

Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

e2e/babel-plugin-jest-hoist/__tests__/integration.test.js Outdated Show resolved Hide resolved
@SimenB
Copy link
Member

SimenB commented Oct 19, 2020

@tgriesser thoughts on @jeysal's suggestion?

@tgriesser
Copy link
Contributor Author

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
Copy link
Member

SimenB commented Oct 23, 2020

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
Copy link
Contributor

jeysal commented Oct 23, 2020

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

@jeysal jeysal force-pushed the tgriesser/fix/hoisting-order branch from ecdee37 to 9560104 Compare October 28, 2020 20:15
@jeysal
Copy link
Contributor

jeysal commented Oct 28, 2020

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

@jeysal
Copy link
Contributor

jeysal commented Oct 28, 2020

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

@jeysal jeysal requested a review from SimenB October 28, 2020 20:20
This was referenced Mar 12, 2021
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants