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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from Mar 1, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
37e5300
Fix overeager hoisting in Babel plugin
jsnajdr Feb 24, 2023
7095444
Use 'this' in the class constructor test
jsnajdr Feb 27, 2023
ddac69b
Add changelog entry
jsnajdr Feb 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Expand Up @@ -13,6 +13,7 @@

### Fixes

- `[babel-plugin-jest-hoist]` Fix unwanted hoisting of nested `jest` usages ([#13952](https://github.com/facebook/jest/pull/13952))
- `[jest-circus]` Send test case results for `todo` tests ([#13915](https://github.com/facebook/jest/pull/13915))
- `[jest-circus]` Update message printed on test timeout ([#13830](https://github.com/facebook/jest/pull/13830))
- `[jest-circus]` Avoid creating the word "testfalse" when `takesDoneCallback` is `false` in the message printed on test timeout AND updated timeouts test ([#13954](https://github.com/facebook/jest/pull/13954))
Expand Down
56 changes: 56 additions & 0 deletions packages/babel-plugin-jest-hoist/src/__tests__/__snapshots__/hoistPlugin.test.ts.snap
View file
Open in desktop
Expand Up @@ -230,3 +230,59 @@ function _getJestObj() {
}

`;

exports[`babel-plugin-jest-hoist 11. jest.spyOn call on the imported module: 11. jest.spyOn call on the imported module 1`] = `

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


↓ ↓ ↓ ↓ ↓ ↓

_getJestObj().mock('some-module', () => {
const module = jest.requireActual('some-module');
Copy link
Member

@SimenB SimenB Feb 27, 2023 •
edited

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

_getJestObj().spyOn(module, 'add');
return module;
});
function _getJestObj() {
const {jest} = require('@jest/globals');
_getJestObj = () => jest;
return jest;
}

`;

exports[`babel-plugin-jest-hoist 12. jest.spyOn call in class constructor: 12. jest.spyOn call in class constructor 1`] = `

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


↓ ↓ ↓ ↓ ↓ ↓

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

`;
26 changes: 26 additions & 0 deletions packages/babel-plugin-jest-hoist/src/__tests__/hoistPlugin.test.ts
View file
Open in desktop
Expand Up @@ -151,6 +151,32 @@ pluginTester({
formatResult,
snapshot: true,
},
'jest.spyOn call on the imported module': {
code: formatResult(`
jest.mock('some-module', () => {
const module = jest.requireActual('some-module');
jest.spyOn(module, 'add');
return module;
});
`),
formatResult,
snapshot: true,
},
'jest.spyOn call in class constructor': {
code: formatResult(`
jest.mock('some-module', () => {
const Actual = jest.requireActual('some-module');
SimenB marked this conversation as resolved.
Show resolved Hide resolved
return class Mocked extends Actual {
constructor() {
super();
jest.spyOn(this, 'add');
}
};
});
`),
formatResult,
snapshot: true,
},
},
/* eslint-enable */
});
51 changes: 27 additions & 24 deletions packages/babel-plugin-jest-hoist/src/index.ts
View file
Open in desktop
Expand Up @@ -32,6 +32,7 @@ const JEST_GLOBALS_MODULE_NAME = '@jest/globals';
const JEST_GLOBALS_MODULE_JEST_EXPORT_NAME = 'jest';

const hoistedVariables = new WeakSet<VariableDeclarator>();
const hoistedJestGetters = new WeakSet<CallExpression>();
const hoistedJestExpressions = new WeakSet<Expression>();

// We allow `jest`, `expect`, `require`, all default Node.js globals and all
Expand Down Expand Up @@ -255,9 +256,12 @@ const isJestObject = (
return false;
};

const extractJestObjExprIfHoistable = (
expr: NodePath,
): NodePath<Expression> | null => {
type JestObjInfo = {
hoist: boolean;
path: NodePath<Expression>;
};

const extractJestObjExprIfHoistable = (expr: NodePath): JestObjInfo | null => {
if (!expr.isCallExpression()) {
return null;
}
Expand All @@ -276,31 +280,34 @@ const extractJestObjExprIfHoistable = (
const jestObjExpr = isJestObject(object)
? object
: // The Jest object could be returned from another call since the functions are all chainable.
extractJestObjExprIfHoistable(object);
extractJestObjExprIfHoistable(object)?.path;
if (!jestObjExpr) {
return null;
}

// Important: Call the function check last
// It might throw an error to display to the user,
// which should only happen if we're already sure it's a call on the Jest object.
let functionLooksHoistableOrInHoistable = FUNCTIONS[propertyName]?.(args);

const functionIsHoistable = FUNCTIONS[propertyName]?.(args) ?? false;
let functionHasHoistableScope = functionIsHoistable;
for (
let path: NodePath<Node> | null = expr;
path && !functionLooksHoistableOrInHoistable;
path && !functionHasHoistableScope;
path = path.parentPath
) {
functionLooksHoistableOrInHoistable = hoistedJestExpressions.has(
functionHasHoistableScope = hoistedJestExpressions.has(
// @ts-expect-error: it's ok if path.node is not an Expression, .has will
// just return false.
path.node,
);
}

if (functionLooksHoistableOrInHoistable) {
if (functionHasHoistableScope) {
hoistedJestExpressions.add(expr.node);
return jestObjExpr;
return {
hoist: functionIsHoistable,
path: jestObjExpr,
};
}

return null;
Expand Down Expand Up @@ -334,21 +341,23 @@ export default function jestHoist(): PluginObj<{
},
visitor: {
ExpressionStatement(exprStmt) {
const jestObjExpr = extractJestObjExprIfHoistable(
const jestObjInfo = extractJestObjExprIfHoistable(
exprStmt.get('expression'),
);
if (jestObjExpr) {
jestObjExpr.replaceWith(
callExpression(this.declareJestObjGetterIdentifier(), []),
if (jestObjInfo) {
const jestCallExpr = callExpression(
this.declareJestObjGetterIdentifier(),
[],
);
jestObjInfo.path.replaceWith(jestCallExpr);
if (jestObjInfo.hoist) {
hoistedJestGetters.add(jestCallExpr);
}
}
},
},
// in `post` to make sure we come after an import transform and can unshift above the `require`s
post({path: program}) {
// eslint-disable-next-line @typescript-eslint/no-this-alias
const self = this;

visitBlock(program);
program.traverse({BlockStatement: visitBlock});

Expand All @@ -368,13 +377,7 @@ export default function jestHoist(): PluginObj<{
varsHoistPoint.remove();

function visitCallExpr(callExpr: NodePath<CallExpression>) {
const {
node: {callee},
} = callExpr;
if (
isIdentifier(callee) &&
callee.name === self.jestObjGetterIdentifier?.name
) {
if (hoistedJestGetters.has(callExpr.node)) {
const mockStmt = callExpr.getStatementParent();

if (mockStmt) {
Expand Down