toBeCalledWith failure output is weird with multiple mismatched calls

[bandersongit]

🐛 Bug Report

test("reproduce issue", () => {
    let fn = jest.fn();
    fn(1);
    fn(2);
    fn(3);

    expect(fn).toBeCalledWith((5));
});

yields

Expected behavior

Maybe a list of what is called with? Something like the failure output for the toBeCalled() matcher

Run npx envinfo --preset jest

Paste the results here:

Environment:
  OS:  macOS High Sierra 10.13.6
  Node:  8.11.1
  Yarn:  1.12.1
  npm:  5.6.0
  Watchman:  4.9.4
  Xcode:  Xcode 9.4.1 Build version 9F2000
  Android Studio:  3.2 AI-181.5540.7.32.5056338

[SimenB]

I agree the output here isn't as good as it should be. @pedrottimark is this in your list?

[pedrottimark]

Ouch. Good timing. I looked at spyMatchers yesterday for the first time in about a year :(

At lot of work there. It seems like toHaveBeenCalledTimes will be our warm up exercise.

Yes, I agree that .not.toHaveBeenCalled is an analogy, with called args vertical instead of horizontal.

What do y’all think of first draft of improved report, with more complicated example calls:

Expected args: [5]
Received args: [1]
               [2, 'deux']
               [3, 'trois', 'drei']

[jeysal]

Seems quite intuitive 👍

[bandersongit]

@pedrottimark I like your suggestion a lot.

Also FWIW I ran into this while implementing Mock Matchers of my own in Rely, a Jest-inspired native Reason testing framework. I've been taking cues for matcher output pretty heavily from you guys to avoid bikeshedding and would love to know what you guys are thinking/take part in those discussions

[pedrottimark]

@bandersongit to discuss information design with 2 use cases instead of one sounds super!

To strike while the feedback is hot, current baseline at left and second draft improved at right

toBeCalledWith please ignore the difference in matcher name, it was a mistake:

At lower right of preceding examples, if expected arguments contain abstract criteria like asymmetric matchers, then received args might be useful to decide which is incorrect: code or test.

@jeysal our experiment continues to decide whether less is more in report for .not matcher ;)

toBeCalledTimes ditto, and which is clearer label Expected: or Expected calls: and so on?

At upper right of following examples, if there is a different number of calls, are the actual calls useful?

[SimenB]

At upper right of following examples, if there is a different number of calls, are the actual calls useful?

No, I don't think so? The stacks might have been to track down the invocation, but I think that's information overload

[jeysal]

and which is clearer label

Expected # calls: maybe unless you want to avoid any special characters

[bandersongit]

At upper right of following examples, if there is a different number of calls, are the actual calls useful?

No, I don't think so? The stacks might have been to track down the invocation, but I think that's information overload

I agree with Simen on that point, and don't have labelling opinions.

I find the indexing kind of confusing. The current behavior prints args/returns in order of "most recently called" and I think that labelling them 1:, 2:, etc. suggests a chronological ordering rather than reverse chronological.

Also something that has come up for me is issues with equality for .toBeCalledWith() and other matchers. Basically structural equality is used by default which doesn't work for things like floats (I know that in Jest you guys are using some smart equality implementation based on runtime type information, but even then you can run into issues with number comparison where you expected not to be called with 2 but were actually called with 1.999, which was close enough). My solution was to have "to be called with a value equal to" instead of "to be called with exactly", but even that has some issues. I could see reasonably printing the actual value(s) that were equal.

[jeysal]

3rd call:
2nd call:
1st call:

?

[bandersongit]

That's definitely clearer and I would favor that if we want to have labels (which I am neutral on)

[SimenB]

Whatever matches the ordering you'd use for expect().toHaveBeenNthCalledWith() should be the order. I think that's invocation order. And the numbering should match that as well