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: disallow hook definitions in tests #9957

Merged
merged 10 commits into from
May 4, 2020
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
32 changes: 22 additions & 10 deletions e2e/__tests__/nestedTestDefinitions.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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".',
);
},
);
18 changes: 18 additions & 0 deletions e2e/nested-test-definitions/__tests__/nestedHookInTest.js
Original file line number Diff line number Diff line change
@@ -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
});
});
46 changes: 31 additions & 15 deletions packages/jest-circus/src/eventHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member Author

Choose a reason for hiding this comment

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

pushing into the errors array is essentially what happened before, except we don't go through the handler again into the error case

}

const describeBlock = makeDescribe(blockName, currentDescribeBlock, mode);
Expand Down Expand Up @@ -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(
Copy link
Member Author

Choose a reason for hiding this comment

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

no need to mutate the message in asyncError, this is sync code

'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;
Expand All @@ -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;
}

Expand Down
2 changes: 1 addition & 1 deletion packages/jest-types/src/Circus.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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>;
Copy link
Member Author

Choose a reason for hiding this comment

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

it was any before... Array<any> is more accurate, but still not particularly good 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

This is weird, now it can be an array of tuples? Look at TestError

Copy link
Member Author

@SimenB SimenB May 4, 2020

Copy link
Member Author

Choose a reason for hiding this comment

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

you can see it used in formatNodeAssertErrors.ts

fn?: TestFn;
invocations: number;
mode: TestMode;
Expand Down