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

CHANGELOG.md

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

packages/babel-plugin-jest-hoist/src/__tests__/__snapshots__/hoistPlugin.test.ts.snap

@@ -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');
+  _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;
+}
+
+`;

packages/babel-plugin-jest-hoist/src/__tests__/hoistPlugin.test.ts

@@ -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');
+          return class Mocked extends Actual {
+            constructor() {
+              super();
+              jest.spyOn(this, 'add');
+            }
+          };
+        });
+      `),
+      formatResult,
+      snapshot: true,
+    },
   },
   /* eslint-enable */
 });

packages/babel-plugin-jest-hoist/src/index.ts

@@ -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
@@ -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;
   }
@@ -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;
@@ -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});
 
@@ -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) {