jest icon indicating copy to clipboard operation
jest copied to clipboard

feat(@jest/mock): Add withImplementation

Open jeppester opened this issue 3 years ago • 2 comments

Summary

A follow-up for https://github.com/facebook/jest/pull/9270

It's often useful to override mock implementations for specific tests, and mockImplementationOnce is there to help out.

It is however not a very elegant solution for situations where the mock will get called multiple times:

  • It's not intuitive that you can call the method multiple times to "plan" for multiple calls
  • The number of calls to the mock might be unimportant to the test. mockImplementationOnce makes the number of calls unecessarily important.
  • The number of calls to the mock becomes part of the "arrange" phase of the test rather than the "assert" phase.
  • mockImplementationOnce will bleed into the next test if the implementation does not end up getting called.

This PR implements a new method withImplementation which sets a temporary default implementation which will be available within a callback, after which the default implementation will be set back to what it was before. If the callback returns a promise, withImplemenation will return a promise that can be awaited in the test.

I had to add a couple of @ts-expect-error comments to get the returned value of withImplementation (promise or void) match the return value of the callback implementation. If you know a better way of achieving this, let me know.

Also, I could potentially be useful to print a warning if the callback does not trigger a call to the mock - in which case withImplementation was redundant. What do you think about that idea?

Test plan

I've added unit tests for the feature. Let me know if I need to add more tests.

jeppester avatar Sep 19 '22 08:09 jeppester

Could you add type tests, please? For completeness and to make sure that generic types will work as expected for the user.

The tests live in this file. To run them build the library and run yarn test-types. Remember to rebuild after each change. If you are using IDE, ignore the red errors the test file. These are caught by expectError.

mrazauskas avatar Sep 19 '22 15:09 mrazauskas

I believe all the feedback has been addressed now. Let me know if I need to do more.

jeppester avatar Sep 20 '22 19:09 jeppester

@mrazauskas I believe there are no issues left. What is the next step from here?

jeppester avatar Sep 22 '22 06:09 jeppester

Right, all looks good to me. So we have to wait for @SimenB, because only he is able to merge PRs.

mrazauskas avatar Sep 22 '22 06:09 mrazauskas

Also, I could potentially be useful to print a warning if the callback does not trigger a call to the mock - in which case withImplementation was redundant. What do you think about that idea?

I like that idea!

SimenB avatar Sep 23 '22 11:09 SimenB

I might be wrong but to me it seems that the failing job random and not related to the PR. If you agree with that assumption, can I get you to rerun the job?

If that job succeeds I believe this PR is ready to merge. - that is unless you have more comments.

Btw. thank you for the thorough, kind, and useful reviews :+1:, it's a great experience to contribute.

jeppester avatar Sep 23 '22 13:09 jeppester

Yeah, you can safely ignore that failure 👍

I'll land #13314, then merge main into this PR, then I'll land this one.

Thanks for the patience and great contribution! 🎉

SimenB avatar Sep 23 '22 13:09 SimenB

https://github.com/facebook/jest/releases/tag/v29.1.0

SimenB avatar Sep 28 '22 07:09 SimenB

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[bot] avatar Oct 30 '22 00:10 github-actions[bot]