Validate hook functions first argument (#6931)

* Move validation checks of callback function for jest hooks from Env.js to jasmine_light.js * Standardise error message for validating hook functions in circus similar to how jasmine_light does it * Update CHANGELOG * Change backtick to singlequote when not needed in circus and jasmine_light * Add types for test.each and describe.each * Use describe.each along with test.each for testing hook argument validation
jestjs · Sep 1, 2018 · 2182a3b · 2182a3b
1 parent 24c173e
commit 2182a3b
Show file tree

Hide file tree

Showing 7 changed files with 96 additions and 55 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -8,8 +8,8 @@
 
 - `[jest-resolve]` Only resolve realpath once in try-catch ([#6925](https://github.com/facebook/jest/pull/6925))
 - `[expect]` Fix TypeError in `toBeInstanceOf` on `null` or `undefined` ([#6912](https://github.com/facebook/jest/pull/6912))
-- `[jest-jasmine2]` Throw a descriptive error if the first argument supplied to a hook was not a function ([#6917](https://github.com/facebook/jest/pull/6917))
-- `[jest-circus]` Throw a descriptive error if the first argument supplied to a hook was not a function ([#6917](https://github.com/facebook/jest/pull/6917))
+- `[jest-jasmine2]` Throw a descriptive error if the first argument supplied to a hook was not a function ([#6917](https://github.com/facebook/jest/pull/6917)) and ([#6931](https://github.com/facebook/jest/pull/6931))
+- `[jest-circus]` Throw a descriptive error if the first argument supplied to a hook was not a function ([#6917](https://github.com/facebook/jest/pull/6917)) and ([#6931](https://github.com/facebook/jest/pull/6931))
 - `[expect]` Fix variadic custom asymmetric matchers ([#6898](https://github.com/facebook/jest/pull/6898))
 - `[jest-cli]` Fix incorrect `testEnvironmentOptions` warning ([#6852](https://github.com/facebook/jest/pull/6852))
 - `[jest-each]` Prevent done callback being supplied to describe ([#6843](https://github.com/facebook/jest/pull/6843))

diff --git a/flow-typed/npm/jest_v23.x.x.js b/flow-typed/npm/jest_v23.x.x.js
@@ -914,7 +914,19 @@ declare var describe: {
   /**
    * Skip running this describe block
    */
-  skip(name: JestTestName, fn: () => void): void
+  skip(name: JestTestName, fn: () => void): void,
+
+  /**
+   * each runs this test against array of argument arrays per each run
+   *
+   * @param {table} table of Test
+   */
+  each(
+    table: Array<Array<mixed>>
+  ): (
+    name: JestTestName,
+    fn?: (...args: Array<any>) => ?Promise<mixed>
+  ) => void,
 };
 
 /** An individual test unit */
@@ -984,7 +996,18 @@ declare var it: {
     name: JestTestName,
     fn?: (done: () => void) => ?Promise<mixed>,
     timeout?: number
-  ): void
+  ): void,
+  /**
+   * each runs this test against array of argument arrays per each run
+   *
+   * @param {table} table of Test
+   */
+  each(
+    table: Array<Array<mixed>>
+  ): (
+    name: JestTestName,
+    fn?: (...args: Array<any>) => ?Promise<mixed>
+  ) => void,
 };
 declare function fit(
   name: JestTestName,

diff --git a/packages/jest-circus/src/__tests__/hooks_error.test.js b/packages/jest-circus/src/__tests__/hooks_error.test.js
@@ -11,15 +11,27 @@
 
 const circus = require('../index.js');
 
-describe('hooks error throwing', () => {
-  test.each([['beforeEach'], ['beforeAll'], ['afterEach'], ['afterAll']])(
-    '%s throws an error when the first argument is not a function',
-    fn => {
-      expect(() => {
-        circus[fn]('param');
-      }).toThrowError(
-        'Invalid first argument, param. It must be a callback function.',
-      );
-    },
-  );
-});
+describe.each([['beforeEach'], ['beforeAll'], ['afterEach'], ['afterAll']])(
+  '%s hooks error throwing',
+  fn => {
+    test.each([
+      ['String'],
+      [1],
+      [[]],
+      [{}],
+      [Symbol('hello')],
+      [true],
+      [null],
+      [undefined],
+    ])(
+      `${fn} throws an error when %p is provided as a first argument to it`,
+      el => {
+        expect(() => {
+          circus[fn](el);
+        }).toThrowError(
+          'Invalid first argument. It must be a callback function.',
+        );
+      },
+    );
+  },
+);
diff --git a/packages/jest-circus/src/index.js b/packages/jest-circus/src/index.js
@@ -41,9 +41,7 @@ const _dispatchDescribe = (blockFn, blockName, mode?: BlockMode) => {
 
 const _addHook = (fn: HookFn, hookType: HookType, hookFn, timeout: ?number) => {
   if (typeof fn !== 'function') {
-    throw new Error(
-      `Invalid first argument, ${fn}. It must be a callback function.`,
-    );
+    throw new Error('Invalid first argument. It must be a callback function.');
   }
 
   const asyncError = new Error();

diff --git a/packages/jest-jasmine2/src/__tests__/hooks_error.test.js b/packages/jest-jasmine2/src/__tests__/hooks_error.test.js
@@ -9,15 +9,27 @@
 
 'use strict';
 
-describe('hooks error throwing', () => {
-  test.each([['beforeEach'], ['beforeAll'], ['afterEach'], ['afterAll']])(
-    '%s throws an error when the first argument is not a function',
-    fn => {
-      expect(() => {
-        global[fn]('param');
-      }).toThrowError(
-        'Invalid first argument, param. It must be a callback function.',
-      );
-    },
-  );
-});
+describe.each([['beforeEach'], ['beforeAll'], ['afterEach'], ['afterAll']])(
+  '%s hooks error throwing',
+  fn => {
+    test.each([
+      ['String'],
+      [1],
+      [[]],
+      [{}],
+      [Symbol('hello')],
+      [true],
+      [null],
+      [undefined],
+    ])(
+      `${fn} throws an error when %p is provided as a first argument to it`,
+      el => {
+        expect(() => {
+          global[fn](el);
+        }).toThrowError(
+          'Invalid first argument. It must be a callback function.',
+        );
+      },
+    );
+  },
+);
diff --git a/packages/jest-jasmine2/src/jasmine/Env.js b/packages/jest-jasmine2/src/jasmine/Env.js
@@ -503,12 +503,6 @@ export default function(j$) {
     };
 
     this.beforeEach = function(beforeEachFunction, timeout) {
-      if (typeof beforeEachFunction !== 'function') {
-        throw new Error(
-          `Invalid first argument, ${beforeEachFunction}. It must be a callback function.`,
-        );
-      }
-
       currentDeclarationSuite.beforeEach({
         fn: beforeEachFunction,
         timeout() {
@@ -518,12 +512,6 @@ export default function(j$) {
     };
 
     this.beforeAll = function(beforeAllFunction, timeout) {
-      if (typeof beforeAllFunction !== 'function') {
-        throw new Error(
-          `Invalid first argument, ${beforeAllFunction}. It must be a callback function.`,
-        );
-      }
-
       currentDeclarationSuite.beforeAll({
         fn: beforeAllFunction,
         timeout() {
@@ -533,12 +521,6 @@ export default function(j$) {
     };
 
     this.afterEach = function(afterEachFunction, timeout) {
-      if (typeof afterEachFunction !== 'function') {
-        throw new Error(
-          `Invalid first argument, ${afterEachFunction}. It must be a callback function.`,
-        );
-      }
-
       currentDeclarationSuite.afterEach({
         fn: afterEachFunction,
         timeout() {
@@ -548,12 +530,6 @@ export default function(j$) {
     };
 
     this.afterAll = function(afterAllFunction, timeout) {
-      if (typeof afterAllFunction !== 'function') {
-        throw new Error(
-          `Invalid first argument, ${afterAllFunction}. It must be a callback function.`,
-        );
-      }
-
       currentDeclarationSuite.afterAll({
         fn: afterAllFunction,
         timeout() {

diff --git a/packages/jest-jasmine2/src/jasmine/jasmine_light.js b/packages/jest-jasmine2/src/jasmine/jasmine_light.js
@@ -93,18 +93,38 @@ exports.interface = function(jasmine: Jasmine, env: any) {
     },
 
     beforeEach() {
+      if (typeof arguments[0] !== 'function') {
+        throw new Error(
+          'Invalid first argument. It must be a callback function.',
+        );
+      }
       return env.beforeEach.apply(env, arguments);
     },
 
     afterEach() {
+      if (typeof arguments[0] !== 'function') {
+        throw new Error(
+          'Invalid first argument. It must be a callback function.',
+        );
+      }
       return env.afterEach.apply(env, arguments);
     },
 
     beforeAll() {
+      if (typeof arguments[0] !== 'function') {
+        throw new Error(
+          'Invalid first argument. It must be a callback function.',
+        );
+      }
       return env.beforeAll.apply(env, arguments);
     },
 
     afterAll() {
+      if (typeof arguments[0] !== 'function') {
+        throw new Error(
+          'Invalid first argument. It must be a callback function.',
+        );
+      }
       return env.afterAll.apply(env, arguments);
     },