Skip to content

feat(@jest/mock): Add withImplementation - #13281

Merged
SimenB merged 16 commits into
jestjs:mainfrom
jeppester:feature/with-implementation
Sep 23, 2022
Merged

feat(@jest/mock): Add withImplementation#13281
SimenB merged 16 commits into
jestjs:mainfrom
jeppester:feature/with-implementation

Conversation

@jeppester

Copy link
Copy Markdown
Contributor

Summary

A follow-up for #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.

For temporarily overriding mock implementations.
@jeppester
jeppester force-pushed the feature/with-implementation branch from ecb89f1 to c4cb677 Compare September 19, 2022 09:06
Comment thread packages/jest-mock/src/index.ts Outdated
Comment thread packages/jest-mock/src/index.ts Outdated
Comment thread packages/jest-mock/README.md
Comment thread packages/jest-mock/src/__tests__/index.test.ts Outdated
@mrazauskas

Copy link
Copy Markdown
Contributor

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.

Comment thread packages/jest-mock/src/index.ts Outdated
Comment thread packages/jest-mock/src/index.ts Outdated
Comment thread docs/MockFunctionAPI.md Outdated
@jeppester

Copy link
Copy Markdown
Contributor Author

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

Comment thread packages/jest-mock/__typetests__/mock-functions.test.ts Outdated
Co-authored-by: Tom Mrazauskas <tom@mrazauskas.de>
@jeppester

Copy link
Copy Markdown
Contributor Author

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

@mrazauskas

Copy link
Copy Markdown
Contributor

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

@SimenB

SimenB commented Sep 23, 2022

Copy link
Copy Markdown
Member

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 SimenB left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice stuff!

Comment thread CHANGELOG.md Outdated
Comment thread docs/MockFunctionAPI.md Outdated
Comment thread packages/jest-mock/README.md Outdated
Comment thread packages/jest-mock/src/__tests__/index.test.ts Outdated
Comment thread packages/jest-mock/src/__tests__/index.test.ts Outdated
Comment thread packages/jest-mock/src/__tests__/index.test.ts
Comment thread packages/jest-mock/src/index.ts Outdated
Comment on lines +793 to +794
typeof returnedValue === 'object' &&
returnedValue !== null &&

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
typeof returnedValue === 'object' &&
returnedValue !== null &&
returnedValue != null &&
typeof returnedValue === 'object' &&

probably doesn't matter, but easier to bail out early

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Like I mentioned in one of my previous comments I copied this verbatim from jest circus.

So there's probably a nice tiny performance enhancement waiting to be made there as well:
https://github.com/facebook/jest/blob/a20fd859673800c50f8f089cfb4a87faec119525/packages/jest-circus/src/utils.ts#L271-L275

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha, the linked SO answer says not to use it 🙈

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened #13314

jeppester and others added 5 commits September 23, 2022 14:24
Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>
Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>
Remove redundant `async`

Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>
Change promise detection expression to bail out earlier for `null` and `undefined`

Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>