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)!: change the default jest.mocked helper’s behaviour to deep mocked #13125

Merged
merged 14 commits into from
Aug 13, 2022
Merged

refactor(jest-mock)!: change the default jest.mocked helper’s behaviour to deep mocked #13125

merged 14 commits into from
Aug 13, 2022

Conversation

mrazauskas
Copy link
Contributor

Split from #12727

Summary

It seems to me that by default jest.mocked() should wrap the deep methods of passed object. Currently it requires and argument: jest.mocked(someObject, true). Also note that without reading the docs it isn’t clear what true does.


Consider this example:

import { expect, jest, test } from "@jest/globals";
import { someObject } from "./someObject";

jest.mock("./someObject");

test("is mock function?", () => {
  expect(jest.isMockFunction(mockObject.one.more.time)).toBe(true);
});

The test will pass. Seems like jest.mock() is mocking deeply nested methods. So I think jest.mocked() should do the same by default (and jest.Mocked<T> as well).

Possibly shallow mocked definitions were implemented to improve performance of TS compiler. Or to work around some limitation, i.e. handling of recursive types. If I got it right, ts-jest implemented jest.mock() in the early days of TypeScript v3. Performance and handling of recursive types improved a lot in TS v4.

Perhaps it’s time to swap the defaults? I mean, to make the behaviour of jest.mocked() deep by default and to allow opting out with an option: jest.mocked(someObject, {shallow: true}).

Test plan

Type tests added.

@mrazauskas mrazauskas changed the title refactor(jest-mock)!: refactor jest.mocked defaults refactor(jest-mock)!: change the default jest.mocked helper’s behaviour to deep mocked Aug 12, 2022
@SimenB
Copy link
Member

SimenB commented Aug 12, 2022

Mye only question is:

import thing from 'thing';

jest.mock('thing');

const mockedThing = jest.mocled(thing);

Would thing be equivalent to mockedThing? Or would they be different? I.e. would one be deep (assuming thing has deep objects) and one the opposite?

@mrazauskas
Copy link
Contributor Author

mrazauskas commented Aug 13, 2022

Current behaviour:

Screenshot 2022-08-13 at 08 38 27

The test is passing. Hence mockedTime is a mock function, but jest.mocked() didn’t type it as such (needs true to be passed).

Swapping the defaults makes this example work without type errors. Behaviour becomes aligned between jest.mock() and jest.mocked(). Is this what you are asking? (;

@SimenB
Copy link
Member

SimenB commented Aug 13, 2022

Swapping the defaults makes this example work without type errors. Behaviour becomes aligned between jest.mock() and jest.mocked(). Is this what you are asking? (;

Yep!

Comment on lines 529 to 530
import fetch from 'node-fetch';
import {expect, jest, test} from '@jest/globals';
import type {fetch} from './fetch';
Copy link
Member

Choose a reason for hiding this comment

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

this looks wrong - a type import will fail the typeof below. But why not keep the node-fetch import?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm.. Checked it. type works, TS does not complain. Similar to the above: https://github.com/facebook/jest/blob/2f4340cb1da435a9b147e238f7034f99653b8070/docs/MockFunctionAPI.md?plain=1#L500-L505

I was not happy that ESLint (import/order) is pushing to keep 'fetch' import above '@jest/globals'. Probably some ignore comment could fix this.

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.

awesome, thanks!

@SimenB SimenB merged commit 7d8c313 into jestjs:main Aug 13, 2022
@mrazauskas mrazauskas deleted the refactor-jest.mocked-defaults branch August 13, 2022 10:50
@SimenB
Copy link
Member

SimenB commented Aug 19, 2022

@mrazauskas
Copy link
Contributor Author

Just checked. The example from #13125 (comment) now does not give any type error. Sweet. Thanks!

@SimenB
Copy link
Member

SimenB commented Aug 25, 2022

@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 Sep 25, 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.

3 participants