From 2983c35c00ab2ea3d9be72a02f96b8ed65425f0a Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Fri, 17 Apr 2020 13:14:51 +0200 Subject: [PATCH 1/8] fix: disallow hook defintions in tests --- CHANGELOG.md | 1 + e2e/__tests__/nestedTestDefinitions.test.ts | 14 ++++++++++++++ .../__tests__/nestedHookInTest.js | 18 ++++++++++++++++++ packages/jest-circus/src/eventHandler.ts | 9 ++++++++- 4 files changed, 41 insertions(+), 1 deletion(-) create mode 100644 e2e/nested-test-definitions/__tests__/nestedHookInTest.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 6bef176537ff..09db28aeb3f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,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-config, jest-resolve]` [**BREAKING**] Remove support for `browser` field ([#9943](https://github.com/facebook/jest/pull/9943)) ### Chore & Maintenance diff --git a/e2e/__tests__/nestedTestDefinitions.test.ts b/e2e/__tests__/nestedTestDefinitions.test.ts index 0a26d4fc14c5..4eaa8f8ae0ac 100644 --- a/e2e/__tests__/nestedTestDefinitions.test.ts +++ b/e2e/__tests__/nestedTestDefinitions.test.ts @@ -55,3 +55,17 @@ test('print correct message when nesting describe inside it', () => { 'Cannot nest a describe inside a test. Describe block "inner describe" cannot run because it is nested within "test".', ); }); + +test('print correct message when nesting a hook inside it', () => { + if (!isJestCircusRun()) { + return; + } + + 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 new file mode 100644 index 000000000000..d7b403d9842c --- /dev/null +++ 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 index 87a2061a649f..fbe74caea584 100644 --- a/packages/jest-circus/src/eventHandler.ts +++ b/packages/jest-circus/src/eventHandler.ts @@ -88,8 +88,15 @@ const eventHandler: Circus.EventHandler = ( break; } case 'add_hook': { - const {currentDescribeBlock, hasStarted} = state; + const {currentDescribeBlock, currentlyRunningTest, hasStarted} = state; const {asyncError, fn, hookType: type, timeout} = event; + + if (currentlyRunningTest) { + throw new Error( + `Hooks cannot be defined inside tests. Hook of type "${type}" is nested within "${currentlyRunningTest.name}".`, + ); + } + const parent = currentDescribeBlock; if (hasStarted) { From 7f38a269a1fd18dd8bb111f52643228fe4ac595f Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Mon, 4 May 2020 16:15:09 +0200 Subject: [PATCH 2/8] push into unhandled --- packages/jest-circus/src/eventHandler.ts | 47 ++++++++++++++---------- 1 file changed, 28 insertions(+), 19 deletions(-) diff --git a/packages/jest-circus/src/eventHandler.ts b/packages/jest-circus/src/eventHandler.ts index 28f0163b83c4..b3b0a45fcf0c 100644 --- 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}".`, + state.unhandledErrors.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); @@ -94,19 +97,21 @@ const eventHandler: Circus.EventHandler = ( const {asyncError, fn, hookType: type, timeout} = event; if (currentlyRunningTest) { - throw new Error( - `Hooks cannot be defined inside tests. Hook of type "${type}" is nested within "${currentlyRunningTest.name}".`, + state.unhandledErrors.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.', + ), ); - } - - 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); break; } + const parent = currentDescribeBlock; currentDescribeBlock.hooks.push({asyncError, fn, parent, timeout, type}); break; @@ -116,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}".`, + state.unhandledErrors.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; } From d612eab6e95a8e7c4af304e07325288cfa248a19 Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Mon, 4 May 2020 16:16:39 +0200 Subject: [PATCH 3/8] update snap --- .../nestedTestDefinitions.test.ts.snap | 9 +++---- e2e/__tests__/nestedTestDefinitions.test.ts | 25 +++++++++---------- 2 files changed, 16 insertions(+), 18 deletions(-) diff --git a/e2e/__tests__/__snapshots__/nestedTestDefinitions.test.ts.snap b/e2e/__tests__/__snapshots__/nestedTestDefinitions.test.ts.snap index daba859d0094..95a191204304 100644 --- a/e2e/__tests__/__snapshots__/nestedTestDefinitions.test.ts.snap +++ b/e2e/__tests__/__snapshots__/nestedTestDefinitions.test.ts.snap @@ -2,10 +2,9 @@ exports[`print correct error message with nested test definitions inside describe 1`] = ` FAIL __tests__/nestedTestWithinDescribe.js - in describe - ✕ outer test - ● in describe › outer test + + ● Test suite failed to run Tests cannot be nested. Test "inner test" cannot run because it is nested within "outer test". @@ -22,9 +21,9 @@ FAIL __tests__/nestedTestWithinDescribe.js exports[`print correct error message with nested test definitions outside describe 1`] = ` FAIL __tests__/nestedTestOutsideDescribe.js - ✕ outer test - ● outer test + + ● Test suite failed to run Tests cannot be nested. Test "inner test" cannot run because it is nested within "outer test". diff --git a/e2e/__tests__/nestedTestDefinitions.test.ts b/e2e/__tests__/nestedTestDefinitions.test.ts index 4eaa8f8ae0ac..5200a49aa0e3 100644 --- a/e2e/__tests__/nestedTestDefinitions.test.ts +++ b/e2e/__tests__/nestedTestDefinitions.test.ts @@ -42,19 +42,18 @@ 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; - } - - const result = runJest('nested-test-definitions', ['nestedDescribeInTest']); - - 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".', - ); -}); +(isJestCircusRun() ? test : test.skip)( + 'print correct message when nesting describe inside it', + () => { + const result = runJest('nested-test-definitions', ['nestedDescribeInTest']); + + 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".', + ); + }, +); test('print correct message when nesting a hook inside it', () => { if (!isJestCircusRun()) { From 2e6b642624766a870c5a51df8dbc247a56c3f061 Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Mon, 4 May 2020 16:20:11 +0200 Subject: [PATCH 4/8] push error into current test --- .../__snapshots__/nestedTestDefinitions.test.ts.snap | 9 +++++---- packages/jest-circus/src/eventHandler.ts | 6 +++--- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/e2e/__tests__/__snapshots__/nestedTestDefinitions.test.ts.snap b/e2e/__tests__/__snapshots__/nestedTestDefinitions.test.ts.snap index 95a191204304..daba859d0094 100644 --- a/e2e/__tests__/__snapshots__/nestedTestDefinitions.test.ts.snap +++ b/e2e/__tests__/__snapshots__/nestedTestDefinitions.test.ts.snap @@ -2,9 +2,10 @@ exports[`print correct error message with nested test definitions inside describe 1`] = ` FAIL __tests__/nestedTestWithinDescribe.js + in describe + ✕ outer test - - ● Test suite failed to run + ● in describe › outer test Tests cannot be nested. Test "inner test" cannot run because it is nested within "outer test". @@ -21,9 +22,9 @@ FAIL __tests__/nestedTestWithinDescribe.js exports[`print correct error message with nested test definitions outside describe 1`] = ` FAIL __tests__/nestedTestOutsideDescribe.js + ✕ outer test - - ● Test suite failed to run + ● outer test Tests cannot be nested. Test "inner test" cannot run because it is nested within "outer test". diff --git a/packages/jest-circus/src/eventHandler.ts b/packages/jest-circus/src/eventHandler.ts index b3b0a45fcf0c..590e66ca3a13 100644 --- a/packages/jest-circus/src/eventHandler.ts +++ b/packages/jest-circus/src/eventHandler.ts @@ -39,7 +39,7 @@ const eventHandler: Circus.EventHandler = ( const {currentDescribeBlock, currentlyRunningTest} = state; if (currentlyRunningTest) { - state.unhandledErrors.push( + currentlyRunningTest.errors.push( new Error( `Cannot nest a describe inside a test. Describe block "${blockName}" cannot run because it is nested within "${currentlyRunningTest.name}".`, ), @@ -97,7 +97,7 @@ const eventHandler: Circus.EventHandler = ( const {asyncError, fn, hookType: type, timeout} = event; if (currentlyRunningTest) { - state.unhandledErrors.push( + currentlyRunningTest.errors.push( new Error( `Hooks cannot be defined inside tests. Hook of type "${type}" is nested within "${currentlyRunningTest.name}".`, ), @@ -121,7 +121,7 @@ const eventHandler: Circus.EventHandler = ( const {asyncError, fn, mode, testName: name, timeout} = event; if (currentlyRunningTest) { - state.unhandledErrors.push( + currentlyRunningTest.errors.push( new Error( `Tests cannot be nested. Test "${name}" cannot run because it is nested within "${currentlyRunningTest.name}".`, ), From b212f99b400bb6a697a5eff6157961e1687451a8 Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Mon, 4 May 2020 16:23:21 +0200 Subject: [PATCH 5/8] fix type of testErrors --- packages/jest-types/src/Circus.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/jest-types/src/Circus.ts b/packages/jest-types/src/Circus.ts index e1291bfe1764..963a6a61b537 100644 --- 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; fn?: TestFn; invocations: number; mode: TestMode; From 861fbbf2209967fd9aa062b3998c84c82d40ee04 Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Mon, 4 May 2020 16:24:40 +0200 Subject: [PATCH 6/8] another one --- e2e/__tests__/nestedTestDefinitions.test.ts | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/e2e/__tests__/nestedTestDefinitions.test.ts b/e2e/__tests__/nestedTestDefinitions.test.ts index 5200a49aa0e3..ab000c325e19 100644 --- a/e2e/__tests__/nestedTestDefinitions.test.ts +++ b/e2e/__tests__/nestedTestDefinitions.test.ts @@ -55,16 +55,15 @@ test('print correct error message with nested test definitions inside describe', }, ); -test('print correct message when nesting a hook inside it', () => { - if (!isJestCircusRun()) { - return; - } - - const result = runJest('nested-test-definitions', ['nestedHookInTest']); +(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.exitCode).toBe(1); - expect(result.stderr).toContain( - 'Hooks cannot be defined inside tests. Hook of type "beforeEach" is nested within "test".', - ); -}); + expect(result.stderr).toContain( + 'Hooks cannot be defined inside tests. Hook of type "beforeEach" is nested within "test".', + ); + }, +); From 7be051495ad5a857bda23980801ec37f25791877 Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Mon, 4 May 2020 16:41:00 +0200 Subject: [PATCH 7/8] remove extraneous type --- packages/jest-circus/src/formatNodeAssertErrors.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/jest-circus/src/formatNodeAssertErrors.ts b/packages/jest-circus/src/formatNodeAssertErrors.ts index d9dbdee0bb8e..10ffc8d1e25f 100644 --- 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; From 011f5930cf257d94891ed1025868953471b8dd25 Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Mon, 4 May 2020 17:23:09 +0200 Subject: [PATCH 8/8] update snap --- .../__snapshots__/circusDeclarationErrors.test.ts.snap | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/e2e/__tests__/__snapshots__/circusDeclarationErrors.test.ts.snap b/e2e/__tests__/__snapshots__/circusDeclarationErrors.test.ts.snap index 83a0e4eab83d..bbaca7ca6978 100644 --- 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) `;