Add toHaveBeenCalledOnceWith matcher

[thibautsabot]

What

Add toHaveBeenCalledOnceWith matcher

Why

To fix the second part of #138

Housekeeping

• Unit tests
• Documentation is up to date
• No additional lint warnings
• Typescript definitions are added/updated where relevant

[codecov]

Codecov Report

Merging #430 (b5096b0) into main (5097d2a) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #430   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           71        72    +1     
  Lines          582       594   +12     
  Branches       148       151    +3     
=========================================
+ Hits           582       594   +12     

Impacted Files	Coverage Δ
src/matchers/toHaveBeenCalledOnceWith.js	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[keeganwitt]

I think it'd be more flexible to do this using the existing matchers. For example,

const arg = 1;
mock(arg);
expect(mock).toHaveBeenCalledOnce();
expect(mock.mock.calls[0][0]).toEqual(arg);
// or expect(mock.mock.lastCall[0]).toEqual(arg);
// or expect(mock.mock.invocationCallOrder[0]).toEqual(arg);

Using this pattern, you can use toEqual() or your choice of any matcher instead of only equals().

[thibautsabot]

Hey @keeganwitt! Thank you for taking the time to comment the PR.

I do agree that having a toHaveBeenCalledOnceWith helper will only use the basic equality.
However I feel like this is what most helpers are doing (like the official toHaveBeenCalledWith)

Use .toHaveBeenCalledWith to ensure that a mock function was called with specific arguments. The arguments are checked with the same algorithm that .toEqual uses.

[keeganwitt]

Thank you for the suggestion and the PR (sorry I forgot to say that earlier)!

The fact Jest's own toHaveBeenCalledWith uses equals() is a good point. Lemme see if I can get another opinion. Our goal is to add matchers that add enough value to be worth adding, and not every conceivable matcher. I'm still getting a feel for where to draw the line.

[G-Rath]

I think it makes sense to have this matcher if we've got toHaveBeenCalledOnce as it is a natural complement & it doesn't add that much (I'd actually think we could try and deduplicate the code with the non-with matcher, but that doesn't have to be in this PR), but I would expect it to try and use the same logic as toHaveBeenCalledWith, whatever that is, since this is meant to be in effect the same matcher just combined with a count checked.

[G-Rath]

I'm trying to find the underlying matcher in jest which I think is here: https://github.com/facebook/jest/blob/51fa61964f0b8f4d06db93a014b49b21ec53ea71/packages/expect/src/spyMatchers.ts#L553

I think we should confirm if the checks there are the equivalent to this.equal.

[Fouzyyyy]

Hi there

I thought about having a util function in our company codebase that is called toHaveBeenCalledOnceWith and under the hood, it calls toHaveBeenCalledTimes(1) and toHaveBeenCalledWith()

How is this different from the proposed implementation here?

[keeganwitt]

It would do the same thing. So would toBeCalledTimes(1) and toHaveBeenNthCalledWith(1, expected).

[keeganwitt]

I'm wondering if it'd maybe be better to change this to toHaveBeenCalledNTimesWith, where the equivalent of the current implementation would be toHaveBeenCalledNTimesWith(1, expected). Thoughts?

[monokrome]

@keeganwitt I think that toHaveBeenCalledNTimesWith could be a separate matcher, but I'd hate to conflate it with this feature personally. In my personal experience, toHaveBeenCalledOnceWith is a much more common use-case.

Also, if we wanted to consider toHaveBeenCalledNTimesWith, we're going to have to have the discussion of how the expected argument works. "Can I pass multiple arguments as ...expected for specifying expectations for each call?", "Does it just match every call expecting the same call if I only provide one?", etc. are things that would still need ironed out. At that point, it seems questionable whether it's easier to use this matcher or we should just manually check each call consecutively.

Also, I personally think that toHaveBeenCalledOnceWith feels "discoverable". If I needed toHaveBeenCalledNTimesWith, I wouldn't have found it by just expecting it to be there / guessing the name. I think that says something about toHaveBeenCalledOnceWith having value even if it ended up just wrapping a toHaveBeenCalledNTimesWith matcher.

That feels like a completely separate discussion / issue to me.

[monokrome]

@thibautsabot FYI it looks like this was approved 😺

[keeganwitt]

I'm trying to find the underlying matcher in jest which I think is here: https://github.com/facebook/jest/blob/51fa61964f0b8f4d06db93a014b49b21ec53ea71/packages/expect/src/spyMatchers.ts#L553

I think we should confirm if the checks there are the equivalent to this.equal.

I'm pretty sure those are the same.

[wiese]

I like that this matcher exists but am surprised by it only supporting one expected value. Native .toHaveBeenCalledWith() and its siblings all support multiple arguments. I think there is value in consistency and it would add capabilities. Can somebody shed some light on the reasoning, please?

Ironically, #518 moved things in the opposite direction.

[keeganwitt]

#518 made the type match the actual implementation. #517 would change the implementation to accept more than just 1 argument.