Add toHaveLastReturned and toHaveNthReturned spy matchers. #6371
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6371 +/- ##
==========================================
+ Coverage 63.47% 63.53% +0.05%
==========================================
Files 227 227
Lines 8677 8721 +44
Branches 4 3 -1
==========================================
+ Hits 5508 5541 +33
- Misses 3168 3179 +11
Partials 1 1
Continue to review full report at Codecov.
|
|
Looks like I forgot to add unit tests for "toHaveNthReturned" that actually tests the "nth" param part. Do not merge yet. |
|
I'm not sure we need these in core, there's no parity with the called matchers and the use cases for when you need |
|
Requiring parity with called matchers doesn't make sense, because the returned matchers test how the call completed (returned vs threw). We already have I plan to next work on "throw" matchers ( That's where there should really be parity: between the "return" and "throw" matchers. It definitely makes sense to have |
|
Big +1 on the throw matchers, I think all of the spy matchers should be:
IMO:
I'm not sure it makes sense to have |
|
See #6381 for more justification that exact parity between "called" and "returned" does not make sense. A function may have been called, but not yet returned.
How is it any different than the difference between The mismatch between returned vs call matchers for "nth" and "last" variants is because there is no need at all to ever test if the "nth" call or "last" call was called without testing the arguments. That is covered simply by the existing The same is not true for "returned", because the existence of a call does not guarantee a return (the call may not have returned yet, or it may have thrown). Testing if a mock has returned at all, or has returned N times also does not guarantee anything about whether the last or Nth call returned. |
|
Let's ignore parity with call matchers, it's a minor point and I agree with you that it shouldn't matter 👍 I see your point with called matchers never needing a last/nth without args. In the cases where you don't care about the returned/thrown value, only that it returned/threw anything, what's wrong with doing this: expect(mock).nthReturnedWith(1, expect.anything());
expect(mock).lastReturnedWith(expect.anything());
expect(mock).nthThrownWith(1, expect.anything());
expect(mock).lastThrownWith(expect.anything());Typically when there's been proposed matchers for something achievable with existing matchers like this, we recommend the "sugar" matchers be added to jest-extended. For example, I can see just as many use cases for:
But we wouldn't add these to core either. We have to draw the line somewhere so we don't bloat core. As a reference, we don't even have some matchers as common as |
|
I was not aware of Do you think it's worth updating the documentation to suggest the use of Somewhat related: Now that I've introduced the concept of a call that has not yet completed, what do you think about adding "completed" matchers that simply test if the mock has completed (either returned or threw)?
|
|
It will always be "completed", won't it? Inside the recursion itself it might not be, but that (shouldn't) be observable from the outside, should it? EDIT: not to say tracking "completness" is not good, but I think it's quite edge casey matchers. Seems great for jest-extended, though :) |
|
Not only recursion, but also nested function calls. It wouldn't be a common case, but could be useful for verifying something like whether or not a callback is called synchronously or asynchronously. Example: test("callback is called synchronously", (done) => {
const spy= jest.fn(doSomethingThenCallbackSynchronously);
spy(() => {
// Confirm that the callback is called synchronously
expect(spy).not.toHaveLastCompleted();
done();
});
});
test("callback is called asynchronously", (done) => {
const spy= jest.fn(doSomethingThenCallbackAsynchronously);
spy(() => {
// Confirm that the callback is called asynchronously
expect(spy).toHaveLastCompleted();
done();
});
});Of course, that's an over-simplified example that could be easily reworked to spy on the callback and test whether the callback has been called immediately after calling the outer function. I'm trying to imagine if there could be more complex situations where a callback is indirectly/asynchronously passed to some method of a class that for some reason is impractical to call directly in a unit test, but can be spied upon. If so, then testing from within the callback whether the outer spied-upon method has completed or not would be a practical way to confirm whether the callback is called synchronously or asynchronously. |
|
Since we're incredibly bad at following this up, this can make any breaking changes it wants to - the next release of Jest will be a major |
|
Still not crazy about adding all of these matchers - @UselessPickles do you feel strongly that they're in core and not jest-extended? |
|
I think it would be sufficient to update documentation of I still think there's some confusion about which matchers should exist for "called" vs "returned"/"thrown"/completed". Why have If/when "throw" matchers are implemented similar to the "return" matchers, then a similar tip on those matchers would be nice too. The "throw" matchers will be a bit messy because there's already a |
|
Hey @UselessPickles thanks a lot for opening this I don't think that we are going to land this in core for now, this would be awesome in jest-extended though could you please open this up over there? 😄 |
|
FWIW, I think it makes sense to trim out (move to jest-extended) some of the more specific matchers from Jest as well. IMO we have too many now |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
Adds "toHaveLastReturned" and "toHaveNthReturned" spy matchers to round out the collection of "return" spy matchers.
Test plan
Updated existing test cases for "toHaveReturned" to also test the new matchers.