Test fail for optional parameters in "toHaveBeenCalledWith"

[jay-medina]

Do you want to request a feature or report a bug?
Report a bug

What is the current behavior?
Unit test fails when an optional parameter isn't explicitly passed to toHaveBeenCalledWith. Within the terminal, nothing is printed out unless the user is explicit to pass in either undefined or something to fail on purpose.

Example is in TypeScript but it is reproducible in JavaScript as well. The repository below has both examples.

Example:
File:

import { logger } from './helper';

export function testFunction(message: string, filename?: string) {
  logger(message, filename);
}

Test:

jest.mock('./helper', () => ({ logger: jest.fn() }));

import { testFunction } from './testFile';
import { logger } from './helper';

describe('testFile', () => {
  describe('when invoked', () => {
    beforeEach(() => {
      testFunction('errorMsg');
    });

    it('fails to print the error line', () => {
      expect(logger).toHaveBeenCalledWith('errorMsg');
    });
  });
});

The output:

If the current behavior is a bug, please provide the steps to reproduce and
either a repl.it demo through https://repl.it/languages/jest or a minimal
repository on GitHub that we can yarn install and yarn test.


Repo: https://github.com/mrfunkycold/jest-demo
There is a typescript and javascript version. Easiest to just execute npm run watch:test and run all the tests to see the failures.

What is the expected behavior?
Not exactly sure. I would have expected the output to either do one of the following:

1. Pass
2. Tell me the failing line has been passed with less than expected parameters.
3. Something better?

Please provide your exact Jest configuration and mention your Jest, node,
yarn/npm version and operating system.

OS: MacOS 10.12.6
Jest: 22.0.4 (though this has failed for earlier versions)
typescript: 2.6.2
node: v8.4.0
npm: 5.6.0

[thymikee]

I think we could pass undefined explicitly so it's easier to test such fns, what do you think @SimenB @cpojer?

[cpojer]

Yeah, we could do that, and use function.length or something to pad it.

[SimenB]

Feels like a footgun, doesn't it? I would prefer it to not be the default if added, I like being explicit. Can use expect.anything()

This feels more like a bug with the toHaveBeenCalledWith matcher, in that it doesn't include information about actual invocation.

https://github.com/facebook/jest/blob/ca5d0fc1378609bfc3d84c5cc0ffb76aa2e93e0f/packages/expect/src/spy_matchers.js#L83-L84

Not sure why not... Can dig into it tomorrow

[cpojer]

Yeah, I’m fine with either: padding undefined values at the end or improving the error message.

[jay-medina]

@cpojer @thymikee I lean towards @SimenB . I would have expected the toHaveBeenCalledWith to fail and say "Hey you are calling the mock with one parameter where it expects three".

Anyway, Thanks for taking a look into this!

[jessecarfb]

It seems weird to me that a test author should be forced to specify optional parameters when a function does not require them. I'd expect the test to pass - and padding with undefined seems like it would provide the expected behavior. @SimenB, can you elaborate why you see it as a footgun? What about a case where there's an optional parameter that sets a default value? I understand your viewpoint of wanting to be explicit, but IMO that's an argument against optional params in an api in general rather than jest's treatment of such apis.

[jessecarfb]

I guess the concern would be jest saying that a test passed when required parameters weren't actually supplied. I'm still not fully convinced though since I don't think it's jest's job to be a linter, and taking a step back, I think it makes sense for the test to pass in this scenario.

[SimenB]

const spy1 = jest.fn();
const spy2 = jest.fn();

spy1('hello', 'world');
spy2('hello', undefined);

expect(spy1).toHaveBeenCalledWith('hello');
expect(spy2).toHaveBeenCalledWith('hello');

In your suggestion, only the first assertion would fail, not the second. I disagree undefined should be treated special here.

If so, we should have a toHaveBeenCalledWithExactly which has the current behaviour (whilst fixing the bad error message on missed undefineds), but that would be super breaking.

[jessecarfb]

That makes sense, thanks for the example @SimenB. I'll publish a PR that has a better error message.

[SimenB]

No worries. Sorry about the late response, I somehow missed your replies in here.

[SimenB]

#5547

[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.