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

Warn if describe returns a value #7852

Merged
merged 11 commits into from Mar 7, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Expand Up @@ -12,6 +12,8 @@
- `[jest-config]` Print error information on preset normalization error ([#7935](https://github.com/facebook/jest/pull/7935))
- `[jest-haste-map]` Add `skipPackageJson` option ([#7778](https://github.com/facebook/jest/pull/7778))
- `[jest-get-type]` Add `isPrimitive` function ([#7708](https://github.com/facebook/jest/pull/7708))
- `[jest-circus/jest-jasmine2]` Warn if describe returns a value ([#7852](https://github.com/facebook/jest/pull/7852))
- `[jest-util]` Add `isPromise` ([#7852](https://github.com/facebook/jest/pull/7852))

### Fixes

Expand Down
41 changes: 41 additions & 0 deletions e2e/__tests__/__snapshots__/declarationErrors.test.ts.snap
@@ -0,0 +1,41 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`warns if describe returns a Promise 1`] = `
" console.log
● Test suite failed to run

Returning a Promise from \\"describe\\" is not supported. Tests must be defined synchronously.
Returning a value from \\"describe\\" will fail the test in a future version of Jest.

9 | 'use strict';
10 |
> 11 | describe('Promise describe warns', () => {
| ^
12 | it('t', () => {});
13 | return Promise.resolve();
14 | });

at Object.describe (__tests__/describeReturnPromise.test.js:11:1)

"
`;

exports[`warns if describe returns something 1`] = `
" console.log
● Test suite failed to run

A \\"describe\\" callback must not return a value.
Returning a value from \\"describe\\" will fail the test in a future version of Jest.

9 | 'use strict';
10 |
> 11 | describe('describe return warns', () => {
| ^
12 | it('t', () => {});
13 | return 42;
14 | });

at Object.describe (__tests__/describeReturnSomething.test.js:11:1)

"
`;
31 changes: 31 additions & 0 deletions e2e/__tests__/declarationErrors.test.ts
@@ -0,0 +1,31 @@
/**
* 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.
*/

import runJest from '../runJest';

const normalizeCircusJasmine = (str: string) =>
str
.replace(/console\.log .+:\d+/, 'console.log')
.replace(/.+addSpecsToSuite (.+:\d+:\d+).+\n/, '');

it('warns if describe returns a Promise', () => {
const result = runJest('declaration-errors', [
'describeReturnPromise.test.js',
]);

expect(result.status).toBe(0);
expect(normalizeCircusJasmine(result.stdout)).toMatchSnapshot();
});

it('warns if describe returns something', () => {
const result = runJest('declaration-errors', [
'describeReturnSomething.test.js',
]);

expect(result.status).toBe(0);
expect(normalizeCircusJasmine(result.stdout)).toMatchSnapshot();
});
14 changes: 14 additions & 0 deletions e2e/declaration-errors/__tests__/describeReturnPromise.test.js
@@ -0,0 +1,14 @@
/**
* 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';

describe('Promise describe warns', () => {
it('t', () => {});
return Promise.resolve();
});
14 changes: 14 additions & 0 deletions e2e/declaration-errors/__tests__/describeReturnSomething.test.js
@@ -0,0 +1,14 @@
/**
* 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';

describe('describe return warns', () => {
it('t', () => {});
return 42;
});
5 changes: 5 additions & 0 deletions e2e/declaration-errors/package.json
@@ -0,0 +1,5 @@
{
"jest": {
"testEnvironment": "node"
}
}
38 changes: 36 additions & 2 deletions packages/jest-circus/src/index.ts
Expand Up @@ -5,8 +5,10 @@
* LICENSE file in the root directory of this source tree.
*/

import chalk from 'chalk';
import {bind as bindEach} from 'jest-each';
import {ErrorWithStack} from 'jest-util';
import {formatExecError} from 'jest-message-util';
import {ErrorWithStack, isPromise} from 'jest-util';
import {Global} from '@jest/types';
import {
BlockFn,
Expand Down Expand Up @@ -63,7 +65,39 @@ const _dispatchDescribe = (
mode,
name: 'start_describe_definition',
});
blockFn();
const describeReturn = blockFn();

// TODO throw in Jest 25
if (isPromise(describeReturn)) {
console.log(
formatExecError(
new ErrorWithStack(
chalk.yellow(
'Returning a Promise from "describe" is not supported. Tests must be defined synchronously.\n' +
'Returning a value from "describe" will fail the test in a future version of Jest.',
),
describeFn,
),
{rootDir: '', testMatch: []},
{noStackTrace: false},
),
);
} else if (describeReturn !== undefined) {
console.log(
formatExecError(
new ErrorWithStack(
chalk.yellow(
'A "describe" callback must not return a value.\n' +
'Returning a value from "describe" will fail the test in a future version of Jest.',
),
describeFn,
),
{rootDir: '', testMatch: []},
{noStackTrace: false},
),
);
}

dispatch({blockName, mode, name: 'finish_describe_definition'});
};

Expand Down
36 changes: 34 additions & 2 deletions packages/jest-jasmine2/src/jasmine/Env.ts
Expand Up @@ -31,7 +31,9 @@ WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
/* eslint-disable sort-keys */

import {AssertionError} from 'assert';
import {ErrorWithStack} from 'jest-util';
import chalk from 'chalk';
import {formatExecError} from 'jest-message-util';
import {ErrorWithStack, isPromise} from 'jest-util';
import queueRunner, {
Options as QueueRunnerOptions,
QueueableFn,
Expand Down Expand Up @@ -415,12 +417,42 @@ export default function(j$: Jasmine) {
currentDeclarationSuite = suite;

let declarationError: null | Error = null;
let describeReturnValue: null | Error = null;
try {
specDefinitions.call(suite);
describeReturnValue = specDefinitions.call(suite);
} catch (e) {
declarationError = e;
}

// TODO throw in Jest 25: declarationError = new Error
if (isPromise(describeReturnValue)) {
console.log(
formatExecError(
new Error(
chalk.yellow(
'Returning a Promise from "describe" is not supported. Tests must be defined synchronously.\n' +
'Returning a value from "describe" will fail the test in a future version of Jest.',
),
),
{rootDir: '', testMatch: []},
{noStackTrace: false},
),
);
} else if (describeReturnValue !== undefined) {
console.log(
formatExecError(
new Error(
chalk.yellow(
'A "describe" callback must not return a value.\n' +
'Returning a value from "describe" will fail the test in a future version of Jest.',
),
),
{rootDir: '', testMatch: []},
{noStackTrace: false},
),
);
}

if (declarationError) {
this.it('encountered a declaration exception', () => {
throw declarationError;
Expand Down
25 changes: 25 additions & 0 deletions packages/jest-util/src/__tests__/isPromise.test.ts
@@ -0,0 +1,25 @@
/**
* 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.
*/

import isPromise from '../isPromise';

describe('not a Promise: ', () => {
test.each([undefined, null, true, 42, '1337', Symbol(), [], {}])(
'%p',
value => {
expect(isPromise(value)).toBe(false);
},
);
});

test('a resolved Promise', () => {
expect(isPromise(Promise.resolve(42))).toBe(true);
});

test('a rejected Promise', () => {
expect(isPromise(Promise.reject().catch(() => {}))).toBe(true);
});
2 changes: 2 additions & 0 deletions packages/jest-util/src/index.ts
Expand Up @@ -21,6 +21,7 @@ import ErrorWithStack from './ErrorWithStack';
import getFailedSnapshotTests from './getFailedSnapshotTests';
import installCommonGlobals from './installCommonGlobals';
import isInteractive from './isInteractive';
import isPromise from './isPromise';
import setGlobal from './setGlobal';
import deepCyclicCopy from './deepCyclicCopy';
import convertDescriptorToString from './convertDescriptorToString';
Expand All @@ -46,6 +47,7 @@ export = {
getFailedSnapshotTests,
installCommonGlobals,
isInteractive,
isPromise,
pluralize,
preRunMessage,
replacePathSepForGlob,
Expand Down
14 changes: 14 additions & 0 deletions packages/jest-util/src/isPromise.ts
@@ -0,0 +1,14 @@
/**
* 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.
*/

// capture global.Promise before it may potentially be overwritten
SimenB marked this conversation as resolved.
Show resolved Hide resolved
const Promise: any = global.Promise;
SimenB marked this conversation as resolved.
Show resolved Hide resolved

// see ES2015 spec 25.4.4.5, https://stackoverflow.com/a/38339199
const isPromise = (candidate: unknown): candidate is Promise<unknown> =>
jeysal marked this conversation as resolved.
Show resolved Hide resolved
Promise.resolve(candidate) === candidate;
export default isPromise;