Error trace cryptic when a method reference not passed to afterEach

[anuragkapur]

🐛 Bug Report

The error trace generated when a method reference is not passed as expected to afterEach is cryptic and hard to debug.

To Reproduce

import React from 'react';
import {cleanup} from 'react-testing-library';
import 'jest-dom/extend-expect';

afterEach(cleanup());

test("Hello World", () => {});
// several other tests but they they are not relevant to this example and have been excluded to keep the question simple

Expected behavior

An appropriate error message that suggests that the problem is in the afterEach line so that as a developer I can know what went wrong and fix it, without manually guessing which of my 100 lines of test code is problematic.

Link to repl or repo (highly encouraged)

https://repl.it/@anuragkapur/ProudDependablePhysics

Run npx envinfo --preset jest

Paste the results here:

  System:
    OS: macOS High Sierra 10.13.6
    CPU: x64 Intel(R) Core(TM) i7-6660U CPU @ 2.40GHz
  Binaries:
    Node: 8.2.0 - /usr/local/bin/node
    npm: 5.3.0 - /usr/local/bin/npm

[thymikee]

Would you like to contribute this change? We're happy to review once ready :)

[natealcedo]

@thymikee Hey! I'd love to take a look into this. Could you possibly point me in the right direction to get started? :)

[SimenB]

jest-jasmine: https://github.com/facebook/jest/blob/32e26490ecf170d9976e437c3416d77610463d03/packages/jest-jasmine2/src/jasmine/Env.js#L505-L539
jest-circus: https://github.com/facebook/jest/blob/32e26490ecf170d9976e437c3416d77610463d03/packages/jest-circus/src/index.js#L42-L48

[natealcedo]

Hey @SimenB, thanks for pointing me in the right direction. From what I've observed so far, this extends to any of the hook functions. IE, beforeEach, afterEach, etc.

What I've tried so far is do a type check in jest/packages/jest-jasmine2/src/jasmine/Env.js, something like this.

  this.beforeEach = function(beforeEachFunction, timeout) {
      if (typeof beforeEachFunction !== 'function') {
        throw new Error(
          `Invalid argument ${beforeEachFunction}. It must be a callback function.`,
        );
      }
      currentDeclarationSuite.beforeEach({
        fn: beforeEachFunction,
        timeout() {
          return timeout || j$._DEFAULT_TIMEOUT_INTERVAL;
        },
      });
    };

So far in some cases where the inputs are a string or undefined, it works as expected. But when the input is a number or a boolean, it instead goes into jasmine_async and doesnt throw an error. What I mean by this is, in actual usage, is throws a type error TypeError: wrappedFn.call is not a function and in the test cases, it doesnt throw anything at all.

Having said that, how strict should we be with checking the argument being provided into the hook function since the behaviour differs based on arguments passed.

Here are some sample tests to test the Env.js changes.
On a side note: Is this the right way to test the hook functions?

describe('hooks error testing', () => {
  // Works 
  it('it throws error when a callback function is not passed in beforeEach', () => {
    const input = 'sdfdsf';
    expect(() => beforeEach(input)).toThrowError(
      `Invalid argument ${input}. It must be a callback function.`,
    );
  });

// Doest throw anything 
  it('it throws error when a callback function is not passed in beforeEach', () => {
    const input = 1;
    expect(() => beforeEach(input)).toThrowError(
      `Invalid argument ${input}. It must be a callback function.`,
    );
  });
});

Actual usage of the updated code

// Throws TypeError: wrappedFn.call is not a function
//  at Object.asyncFn (../jest/packages/jest-jasmine2/build/jasmine_async.js:63:37)

describe("test", () => {
  beforeEach(1);

  it("should work", () => {
    expect(2).toEqual(2);
  });
});

Would love your input on how we should proceed!

[SimenB]

@natealcedo sorry I never got back to you. However, I think this has been solved by #6917, and this I'm closing this. Do you agree?

[natealcedo]

Hey @SimenB, thanks for reaching out. I actually don't think this is solved satisfactorily enough. I had similar looking code as #6917 however jest will only throw the helpful error message if you pass in a string in the hook functions. Numbers, symbols, arrays. I didn't proceed further as I didn't have enough context. Fixing the code just in jest-jasmine didn't fix it and I didn't know where to proceed next.

I can replicate the behaviour where this will not work.

jest/packages/jest-jasmine2/src/__tests__/hooks_error.test.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(() => {
        // This doesnt work for anything other than strings. (numbers, symbols, etc)
		// Throws TypeError: wrappedFn.call is not a function
        global[fn](2);
      }).toThrowError(
        'Invalid first argument, param. It must be a callback function.',
      );
    },
  );
}); 

Having said that, its not really a big bug. More like a quality of life improvement. I defer to your judgement here as to whether or not you believe this warrants further investigation. :)

[SimenB]

Good point! It's fixed for circus, but not jasmine.

Moving the checks into jasmine-light.js instead of Env.js should fix it. Wanna send a PR doing that?

[natealcedo]

@SimenB I'd love to send in a PR. Will do when I get the chance to do so!

[natealcedo]

Let's close this issue now since I believe this is solved satisfactorily enough with #6931 :)

[SimenB]

Right!

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.