Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fixed mockReturnValue overriding mockImplementationOnce #8398

Merged
merged 7 commits into from
Aug 22, 2019

Conversation

grosto
Copy link
Contributor

@grosto grosto commented Apr 30, 2019

Summary

Fixes #8376. I have removed defaultReturnValue and instead mockReturnValue now uses mockImplementation 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.

@codecov-io
Copy link

codecov-io commented Apr 30, 2019

Codecov Report

Merging #8398 into master will decrease coverage by 0.03%.
The diff coverage is 90.9%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
packages/jest-mock/src/index.ts 76.87% <90.9%> (-0.65%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d387bcf...def5719. Read the comment docs.

@jeysal
Copy link
Contributor

jeysal commented Apr 30, 2019

This does seem like the way more obvious implementation anyway, I'm wondering if there's a reason it was not done this way.
@scotthovestadt can you find out if this will break FB?

@jablko
Copy link
Contributor

jablko commented Jul 15, 2019

I was just bitten by the same (almost) thing:

expect(
  jest
    .fn()
    .mockImplementationOnce(() => {
      throw new Error('first call');
    })
    .mockReturnValueOnce('second call')
).toThrow('first call');

Copy link
Contributor

@jeysal jeysal left a 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.

if (mockConfig.isReturnValueLastSet) {
return mockConfig.defaultReturnValue;
}

// If mockImplementationOnce()/mockImplementation() is last set,
// or specific return values are used up, use the mock
Copy link
Contributor

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

@jeysal jeysal added this to the Jest 25 milestone Jul 15, 2019
Copy link
Member

@SimenB SimenB left a 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)

@github-actions
Copy link

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mockImplementationOnce behaves differently than mockImplementation
6 participants