-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed mockReturnValue overriding mockImplementationOnce #8398
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8398 +/- ##
=========================================
- Coverage 62.34% 62.3% -0.04%
=========================================
Files 266 266
Lines 10736 10722 -14
Branches 2611 2608 -3
=========================================
- Hits 6693 6680 -13
Misses 3460 3460
+ Partials 583 582 -1
Continue to review full report at Codecov.
|
This does seem like the way more obvious implementation anyway, I'm wondering if there's a reason it was not done this way. |
I was just bitten by the same (almost) thing: expect(
jest
.fn()
.mockImplementationOnce(() => {
throw new Error('first call');
})
.mockReturnValueOnce('second call')
).toThrow('first call'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, the comment just brought this back on my radar. I'll request a few more reviews. This will have to wait some longer anyway though - because it's breaking it can only go in Jest 25.
packages/jest-mock/src/index.ts
Outdated
if (mockConfig.isReturnValueLastSet) { | ||
return mockConfig.defaultReturnValue; | ||
} | ||
|
||
// If mockImplementationOnce()/mockImplementation() is last set, | ||
// or specific return values are used up, use the mock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'or specific return values are used up' no longer seems accurate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Less code which fixes a bug? I like it! Also interested if this breaks FB, though
(Needs a rebase)
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
Fixes #8376. I have removed
defaultReturnValue
and insteadmockReturnValue
now usesmockImplementation
under the hood. I might be missing some context, but I think this is a much more straightforward way to manage the order of mock implementations.Test plan
Added related tests.