fix: disallow hook definitions in tests (#9957)

jestjs · May 4, 2020 · d30a586 · d30a586
1 parent 3375ac3
commit d30a586
Show file tree

Hide file tree

Showing 7 changed files with 78 additions and 27 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -12,6 +12,7 @@
 - `[jest-circus, jest-console, jest-jasmine2, jest-reporters, jest-util, pretty-format]` Fix time durating formatting and consolidate time formatting code ([#9765](https://github.com/facebook/jest/pull/9765))
 - `[jest-circus]` [**BREAKING**] Fail tests if a test takes a done callback and have return values ([#9129](https://github.com/facebook/jest/pull/9129))
 - `[jest-circus]` [**BREAKING**] Throw a proper error if a test / hook is defined asynchronously ([#8096](https://github.com/facebook/jest/pull/8096))
+- `[jest-circus]` Throw more descriptive error if hook is defined inside test ([#9957](https://github.com/facebook/jest/pull/9957))
 - `[jest-circus]` [**BREAKING**] Align execution order of tests to match `jasmine`'s top to bottom order ([#9965](https://github.com/facebook/jest/pull/9965))
 - `[jest-config, jest-resolve]` [**BREAKING**] Remove support for `browser` field ([#9943](https://github.com/facebook/jest/pull/9943))
 - `[jest-haste-map]` Stop reporting files as changed when they are only accessed ([#7347](https://github.com/facebook/jest/pull/7347))

diff --git a/e2e/__tests__/__snapshots__/circusDeclarationErrors.test.ts.snap b/e2e/__tests__/__snapshots__/circusDeclarationErrors.test.ts.snap
@@ -16,6 +16,7 @@ FAIL __tests__/asyncDefinition.test.js
       14 |   });
       15 | });
 
+      at eventHandler (../../packages/jest-circus/build/eventHandler.js:143:11)
       at test (__tests__/asyncDefinition.test.js:12:5)
 
   ● Test suite failed to run
@@ -30,6 +31,7 @@ FAIL __tests__/asyncDefinition.test.js
       15 | });
       16 | 
 
+      at eventHandler (../../packages/jest-circus/build/eventHandler.js:112:11)
       at afterAll (__tests__/asyncDefinition.test.js:13:5)
 
   ● Test suite failed to run
@@ -44,6 +46,7 @@ FAIL __tests__/asyncDefinition.test.js
       20 | });
       21 | 
 
+      at eventHandler (../../packages/jest-circus/build/eventHandler.js:143:11)
       at test (__tests__/asyncDefinition.test.js:18:3)
 
   ● Test suite failed to run
@@ -57,5 +60,6 @@ FAIL __tests__/asyncDefinition.test.js
       20 | });
       21 | 
 
+      at eventHandler (../../packages/jest-circus/build/eventHandler.js:112:11)
       at afterAll (__tests__/asyncDefinition.test.js:19:3)
 `;
diff --git a/e2e/__tests__/nestedTestDefinitions.test.ts b/e2e/__tests__/nestedTestDefinitions.test.ts
@@ -42,16 +42,28 @@ test('print correct error message with nested test definitions inside describe',
   expect(cleanupRunnerStack(summary.rest)).toMatchSnapshot();
 });
 
-test('print correct message when nesting describe inside it', () => {
-  if (!isJestCircusRun()) {
-    return;
-  }
+(isJestCircusRun() ? test : test.skip)(
+  'print correct message when nesting describe inside it',
+  () => {
+    const result = runJest('nested-test-definitions', ['nestedDescribeInTest']);
 
-  const result = runJest('nested-test-definitions', ['nestedDescribeInTest']);
+    expect(result.exitCode).toBe(1);
 
-  expect(result.exitCode).toBe(1);
+    expect(result.stderr).toContain(
+      'Cannot nest a describe inside a test. Describe block "inner describe" cannot run because it is nested within "test".',
+    );
+  },
+);
 
-  expect(result.stderr).toContain(
-    'Cannot nest a describe inside a test. Describe block "inner describe" cannot run because it is nested within "test".',
-  );
-});
+(isJestCircusRun() ? test : test.skip)(
+  'print correct message when nesting a hook inside it',
+  () => {
+    const result = runJest('nested-test-definitions', ['nestedHookInTest']);
+
+    expect(result.exitCode).toBe(1);
+
+    expect(result.stderr).toContain(
+      'Hooks cannot be defined inside tests. Hook of type "beforeEach" is nested within "test".',
+    );
+  },
+);
diff --git a/e2e/nested-test-definitions/__tests__/nestedHookInTest.js b/e2e/nested-test-definitions/__tests__/nestedHookInTest.js
@@ -0,0 +1,18 @@
+/**
+ * Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+'use strict';
+
+const {getTruthy} = require('../index');
+
+test('test', () => {
+  expect(getTruthy()).toBeTruthy();
+
+  beforeEach(() => {
+    // nothing to see here
+  });
+});
diff --git a/packages/jest-circus/src/eventHandler.ts b/packages/jest-circus/src/eventHandler.ts
@@ -39,9 +39,12 @@ const eventHandler: Circus.EventHandler = (
       const {currentDescribeBlock, currentlyRunningTest} = state;
 
       if (currentlyRunningTest) {
-        throw new Error(
-          `Cannot nest a describe inside a test. Describe block "${blockName}" cannot run because it is nested within "${currentlyRunningTest.name}".`,
+        currentlyRunningTest.errors.push(
+          new Error(
+            `Cannot nest a describe inside a test. Describe block "${blockName}" cannot run because it is nested within "${currentlyRunningTest.name}".`,
+          ),
         );
+        break;
       }
 
       const describeBlock = makeDescribe(blockName, currentDescribeBlock, mode);
@@ -90,16 +93,25 @@ const eventHandler: Circus.EventHandler = (
       break;
     }
     case 'add_hook': {
-      const {currentDescribeBlock, hasStarted} = state;
+      const {currentDescribeBlock, currentlyRunningTest, hasStarted} = state;
       const {asyncError, fn, hookType: type, timeout} = event;
-      const parent = currentDescribeBlock;
 
-      if (hasStarted) {
-        asyncError.message =
-          'Cannot add a hook after tests have started running. Hooks must be defined synchronously.';
-        state.unhandledErrors.push(asyncError);
+      if (currentlyRunningTest) {
+        currentlyRunningTest.errors.push(
+          new Error(
+            `Hooks cannot be defined inside tests. Hook of type "${type}" is nested within "${currentlyRunningTest.name}".`,
+          ),
+        );
+        break;
+      } else if (hasStarted) {
+        state.unhandledErrors.push(
+          new Error(
+            'Cannot add a hook after tests have started running. Hooks must be defined synchronously.',
+          ),
+        );
         break;
       }
+      const parent = currentDescribeBlock;
 
       currentDescribeBlock.hooks.push({asyncError, fn, parent, timeout, type});
       break;
@@ -109,14 +121,18 @@ const eventHandler: Circus.EventHandler = (
       const {asyncError, fn, mode, testName: name, timeout} = event;
 
       if (currentlyRunningTest) {
-        throw new Error(
-          `Tests cannot be nested. Test "${name}" cannot run because it is nested within "${currentlyRunningTest.name}".`,
+        currentlyRunningTest.errors.push(
+          new Error(
+            `Tests cannot be nested. Test "${name}" cannot run because it is nested within "${currentlyRunningTest.name}".`,
+          ),
+        );
+        break;
+      } else if (hasStarted) {
+        state.unhandledErrors.push(
+          new Error(
+            'Cannot add a test after tests have started running. Tests must be defined synchronously.',
+          ),
         );
-      }
-      if (hasStarted) {
-        asyncError.message =
-          'Cannot add a test after tests have started running. Tests must be defined synchronously.';
-        state.unhandledErrors.push(asyncError);
         break;
       }
 

diff --git a/packages/jest-circus/src/formatNodeAssertErrors.ts b/packages/jest-circus/src/formatNodeAssertErrors.ts
@@ -43,7 +43,7 @@ const formatNodeAssertErrors = (
   state: Circus.State,
 ): void => {
   if (event.name === 'test_done') {
-    event.test.errors = event.test.errors.map((errors: Circus.TestError) => {
+    event.test.errors = event.test.errors.map(errors => {
       let error;
       if (Array.isArray(errors)) {
         const [originalError, asyncError] = errors;

diff --git a/packages/jest-types/src/Circus.ts b/packages/jest-types/src/Circus.ts
@@ -214,7 +214,7 @@ export type TestError = Exception | [Exception | undefined, Exception]; // the e
 export type TestEntry = {
   type: 'test';
   asyncError: Exception; // Used if the test failure contains no usable stack trace
-  errors: TestError;
+  errors: Array<TestError>;
   fn?: TestFn;
   invocations: number;
   mode: TestMode;