Skip to content
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

refactor(jest-mock)!: simplify usage of jest.fn generic type arguments #12489

Merged
merged 24 commits into from
Feb 26, 2022
Merged

refactor(jest-mock)!: simplify usage of jest.fn generic type arguments #12489

merged 24 commits into from
Feb 26, 2022

Conversation

mrazauskas
Copy link
Contributor

Closes #12479

Summary

jest.fn and jest.spyOn depend on Mock and MockInstance types. These take two generic type arguments: the returned value, parameters type. That works just fine internally, but makes usage hard for on the user side. For instance, here is a line from Jest docs:

const mockAdd = jest.fn() as jest.MockedFunction<typeof add>;

All looks fine at first glance, but the thing is that type returned by jest.fn is Mock and it is slightly different from MockedFunction. As it was pointed out in the link issue, to have the right type currently user should do something like this:

const mockAdd = jest.fn<ReturnType<typeof add>, Parameters<typeof add>>();

This PR makes it possible to type jest.fn in much more simple way (which is also more precise than type casting using MockedFunction):

const mockAdd = jest.fn<typeof add>();

Looks like I made it work as expected. Have to take a second look tomorrow at a couple of anys here and there. Seems like internally they are necessary. On user side, everything should default to unknown.

Test plan

It was priceless to have type tests in place! All should pass.

@mrazauskas
Copy link
Contributor Author

mrazauskas commented Feb 25, 2022

Trying out types while looking through examples in docs. It might be good idea to add a tab with TypeScript flavoured examples. Just trying out what is possible and how it works.

With current types:

const asyncMock = fn<Promise<string>, []>()
  .mockResolvedValue('default')
  .mockResolvedValueOnce('first call')
  .mockResolvedValueOnce('second call');

const mock = fn<number, []>()
  .mockReturnValue(42)
  .mockReturnValueOnce(35)
  .mockReturnValueOnce(12);

With types of this branch (more verbose, but also more clear):

const asyncMock = fn<() => Promise<string>>()
  .mockResolvedValue('default')
  .mockResolvedValueOnce('first call')
  .mockResolvedValueOnce('second call');

const mock = fn<() => number>()
  .mockReturnValue(42)
  .mockReturnValueOnce(35)
  .mockReturnValueOnce(12);

Of course, this is only necessary if fn() lacks implementation, otherwise everything is typed (also mockResolvedValue throws TS error with sync implementation):

const asyncMock = fn(async () => 'default')
  .mockResolvedValueOnce('first call')
  .mockResolvedValueOnce('second call');

const mock = fn(() => 42)
  .mockReturnValueOnce(35)
  .mockReturnValueOnce(12);

@mrazauskas
Copy link
Contributor Author

Just push docs with TS examples. Preview is here – https://deploy-preview-12489--jestjs.netlify.app/docs/next/mock-function-api

(Might be there is better way to setup tabs. I have zero experience with Docusaurus, so just followed their docs.)

expect(mockedFunction).toHaveBeenCalled()

Expected mock function "mockedFunction" to have been called, but it was not called.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The actual message is (feels to be redundant, or?):

Expected number of calls: >= 1
Received number of calls:    0

Copy link
Member

Choose a reason for hiding this comment

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

really? that's a regression then. The point is that we print the mockName provided

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The name gets printed. The line expect(mockedFunction).toHaveBeenCalled() is correct, but Expected mock function "mockedFunction" to have been called, but it was not called prints as Expected number of calls... I will double check.

Copy link
Member

Choose a reason for hiding this comment

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

Aha. Still a regression, we shouldn't be printing numbers when we're expecting any. Makes sense when we expect none, tho

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright. Would be better to leave the line in place and to open an issue? So we don’t forget to look at it. Right?

Copy link
Member

Choose a reason for hiding this comment

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

yeah 👍

@SimenB
Copy link
Member

SimenB commented Feb 26, 2022

Loving the docs changes! 👍 Good idea to use tabs

export interface Mock<T, Y extends Array<unknown> = Array<unknown>>
type UnknownFunction = (...args: Array<unknown>) => unknown;

export interface Mock<T extends FunctionLike = UnknownFunction>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a need to keep anys in type FunctionLike = (...args: any) => any. It does not work with unknowns either internally, or something breaks in tests. Despite anys, this is not any (; It is AnyFunction and that’s a constrain. Passing anything what is not a function will throw TS error. To avoid anys leak to user side I used defaults of (...args: Array<unknown>) => unknown. This means that using fn() without any implementation will default to unknown, if implementation is provided types are inferred.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying to explain this in more detail, because I repeated this pattern several times. Internally all what’s necessary is to be sure that we have a function, hence simply any function works. It also works as a constrain. But the default on user side always is (...args: Array<unknown>) => unknown.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good 👍 probably worth a code comment?

@mrazauskas mrazauskas marked this pull request as ready for review February 26, 2022 09:48
@mrazauskas
Copy link
Contributor Author

mrazauskas commented Feb 26, 2022

Loving the docs changes! 👍 Good idea to use tabs

The credits goes to Playwright – https://playwright.dev/docs/intro#first-test

They are doing it in much cleaner way. I couldn’t figure out if they use some plugin or is this some React component? https://github.com/microsoft/playwright/blob/80a38a39c2c66cfb082ad1031dbddd537553380e/docs/src/intro-js.md?plain=1#L52

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.

yay!

changelog? 😀

docs/MockFunctionAPI.md Show resolved Hide resolved
expect(mockAdd).toBeCalledWith(1, 2);
});
```

### `jest.MockedFunction`
Copy link
Member

Choose a reason for hiding this comment

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

should we remove this as well? comes from @types/jest and not us

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. I took it out and will bring it back after jest.Mocked refactor. What about that note on Create React App? Just above?

Copy link
Member

Choose a reason for hiding this comment

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

yeah, kill it. only thing under the TypeScript heading should be either pointing to "getting started" or examples using jest from @jest/globals. All the other stuff (CRA, "see this repo", "jest is written in TS") should go, IMO

expect(mockAdd).toBeCalledWith(1, 2);
});
```
### `jest.fn(implementation?)`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the way, I double checked if the example works. All is correct. Will add it to examples folder later. TypeScript example will need more changes.

Copy link
Member

Choose a reason for hiding this comment

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

convert back to draft so I don't accidentally merge? 😀

@mrazauskas mrazauskas marked this pull request as draft February 26, 2022 12:30
@mrazauskas mrazauskas marked this pull request as ready for review February 26, 2022 15:22
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.

this is fantastic stuff @mrazauskas! Thank you so much for diving in and fixing this 👍

_Note: `jest.fn(implementation)` is a shorthand for `jest.fn().mockImplementation(implementation)`._

For example:
Both `jest.fn(fn)` and `jest.fn().mockImplementation(fn)` are equivalent. For instance, you can use `.mockImplementation()` to replace the implementation of a mock:
Copy link
Member

Choose a reason for hiding this comment

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

stick in :::tip? or note I guess

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea was to refactor this note into a sentence, which should sell .mockImplementation(). If both are equivalent, why choose the longer one? For instance, .mockResolveValue() or .mockRejectedValue() have similar sentence followed by example. That’s why I tweak the following example too. Does not work? Hm.. Read it all again. I deleted too much. Will fix it.

Copy link
Member

@SimenB SimenB Feb 26, 2022

Choose a reason for hiding this comment

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

I think the example change is good since it's shorter, but I think highlighting the fact the two approaches are the same is a good idea 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. I reverted most of text. It sounds good.

@mrazauskas
Copy link
Contributor Author

Thank you so much for diving in and fixing this 👍

It was fun to dive in. Also I am happy with the result. Internals are cleaner, usage is simplified and we have a few more type tests too ;D

@SimenB SimenB merged commit b0dc2e4 into jestjs:main Feb 26, 2022
@mrazauskas mrazauskas deleted the refactor-Mock-generic-type-arguments branch February 26, 2022 18:40
@bfaulk96
Copy link

When should we expect this in a release?

@SimenB
Copy link
Member

SimenB commented Feb 28, 2022

I can make one tomorrow (bedtime here now)

@bfaulk96
Copy link

Perfect! Haha sorry, not trying to rush you by any means, was just curious 😄

@SimenB
Copy link
Member

SimenB commented Mar 1, 2022

https://github.com/facebook/jest/releases/tag/v28.0.0-alpha.6

@github-actions
Copy link

github-actions bot commented Apr 1, 2022

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 Apr 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: Ergonomic way to jest.fn over a function type in TypeScript
4 participants