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)!: make jest.mocked() helper to always return deeply mock-typed object #12516

Closed
wants to merge 3 commits into from
Closed

Conversation

mrazauskas
Copy link
Contributor

Summary

Currently jest.mocked() helper takes second optional argument, which allows to add mock-types to the deeply nested members of the passed object. The default is false (or, not deep). In contrary jest-mock mocking implementation does not have any options for deep/shallow mocking. If I get it right, all members are always mocked recursively.

Why the helper is not deep by default? Is there any performance penalty? The jest.mocked() was copied recently from ts-jest library. So I went to their repo to see what was the motivation? Here is the PR adding mocked() kulshekhar/ts-jest#774, but it has no reasons mentioned.

To me it seems like this second argument is not needed. It adds unnecessary complexity to the code (and usage). So perhaps the shallow implementation could be simply removed.

Alternatively it is possible to switch the defaults – keeping it always deep and allowing to enable shallow mode. But what would be the use case? Not sure it that is needed.

Test plan

Added a quick type test to make sure an example in docs really works.

@SimenB
Copy link
Member

SimenB commented Feb 28, 2022

@huafu @ahnpnl opinions? 🙂

(I'd go for less options if possible of course, but it might serve a purpose)

@huafu
Copy link
Contributor

huafu commented Feb 28, 2022

That's a long long time I wrote the code for mocked(), but I'm pretty sure there are performance related reasons that I made it an option.
So I'd go for any default, but the option need to be available. For big objects with deep nested other objects I believe intensive use of mocked with deep enabled may cause performance downsides.

Update: I forgot that it was only for typings, so yeah, I'm not aware of latest typescript improvements and it might not have any performance downsides anymore...

@SimenB
Copy link
Member

SimenB commented Feb 28, 2022 via email

@mrazauskas
Copy link
Contributor Author

Recently I had a chance to run one test suite agains different versions of TS starting with v3.8 and up. The performance improvements were very notable on current versions. Actually it felt like system froze when I ran tests on TS v3.8. Just an observation, that’s not a performance test of course.

@SimenB
Copy link
Member

SimenB commented Mar 1, 2022

I guess one use case is

const realObject = {
  thing: {foo: () => ''},
  foobar: () => 'baz',
};

const mocked: typeof realObject = {
  ...realObject,
  foobar: jest.fn(),
};

const mockedObject = jest.mocked(mocked);

in this case, if we default to deep, then the types will be wrong. types would already be wrong if we add another top level property and don't override it, so maybe not a big deal? But e.g. Jest's automock is shallow

@mrazauskas
Copy link
Contributor Author

mrazauskas commented Mar 1, 2022

Yes, but I am not sure if it is a good idea to use mocked() like this. I would do:

const realObject = {
  foobar: (a: string) => 'baz',
  thing: {foo: () => ''},
};

const partlyMocked = {
  ...realObject,
  // with implementation
  foobar: jest.fn((a: string) => 'zab'),
  // or like this if it must be `unknown`
  // foobar: jest.fn(),
  // or like this if types are needed for `.mockImplementation()` later
  // foobar: jest.fn<typeof realObject.foobar>(),
};

// const mockedObject = jest.mocked(mocked);

partlyMocked.thing.foo(); // works

partlyMocked.foobar.mock.calls[0][0]; // also works

const returnValue = partlyMocked.foobar.mock.results[0];

if (returnValue.type === 'return') {
  returnValue.value; // it is here
}

@ahnpnl
Copy link
Contributor

ahnpnl commented Apr 10, 2022

The performance will impact only on tsc, I don’t see any problems for Jest itself.

@mrazauskas
Copy link
Contributor Author

But e.g. Jest's automock is shallow

@SimenB Coming back here just to understand how to move on with #12727.

Sorry. Last time my eye missed your note that Jest's jest.mock() is shallow. So better jest.Mocked should be shallow to stay in sync with jest.mock()? And how do you think, do we need jest.MockedDeep?


Hm.. But looking at the example from docs (see below): foo.a.b.c.hello() is nested inside an object and jest.mock() is mocking it. The test is passing and jest.isMockFunction(mockedFoo.a.b.c.hello) returns true. So perhaps jest.Mocked<typeof foo> should do the same as mocked(foo, true)? Got lost ;D

// foo.ts
export const foo = {
  a : {
    b: {
      c: {
        hello: (name: string) => `Hello, ${name}`,
      },
    },
  },
}
// foo.test.ts
import { foo } from './foo'
jest.mock('./foo')

const mockedFoo = jest.mocked(foo, true)

test('deep', () => {
  mockedFoo.a.b.c.hello('me')
  expect(mockedFoo.a.b.c.hello.mock.calls).toHaveLength(1)
})

@mrazauskas
Copy link
Contributor Author

Here is the plan. jest.Mocked will be wrapping object types with with mock definitions deeply (see #12727). It is a newly added type, not a change. Hence this will be a way to find out if in the real world there are any reasons to have a shallow version as well.

Closing this since it is not relevant at the moment.

@mrazauskas mrazauskas closed this Apr 26, 2022
@mrazauskas mrazauskas deleted the refactor-mocked branch April 26, 2022 05:46
@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 27, 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.

5 participants