Warn if describe returns a value (#7852)

* Fail test suite if describe returns a Promise

* copyright header

* fix for overwritten global.Promise

* fix e2e test for Node 6

* only console.warn for now

* stack trace

* remove circus stack entry

* snapshot test

* console.warn => console.log

* remove Env.js after rebase

* fix jasmine2 after rebase

CHANGELOG.md

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

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)
+    
+"
+`;

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();
+});

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();
+});

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;
+});

e2e/declaration-errors/package.json

@@ -0,0 +1,5 @@
+{
+  "jest": {
+    "testEnvironment": "node"
+  }
+}

packages/jest-circus/src/index.ts

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

packages/jest-jasmine2/src/jasmine/Env.ts

@@ -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,
@@ -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;

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);
+});

packages/jest-util/src/index.ts

@@ -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';
@@ -46,6 +47,7 @@ export = {
   getFailedSnapshotTests,
   installCommonGlobals,
   isInteractive,
+  isPromise,
   pluralize,
   preRunMessage,
   replacePathSepForGlob,

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
+const Promise: any = global.Promise;
+
+// see ES2015 spec 25.4.4.5, https://stackoverflow.com/a/38339199
+const isPromise = (candidate: unknown): candidate is Promise<unknown> =>
+  Promise.resolve(candidate) === candidate;
+export default isPromise;