[Feature]: stop changing the behaviour of spies on mockFn.mockReset

[gerardolima]

🚀 Feature Proposal

I suggest mockFn.mockReset and jest.resetAllMocks to stop changing the behaviour of spies .

Motivation

The behaviour of mockFn.mockReset() is NOT intuitive: functions whose behaviour should only be observed by Jest actually are changed after the first call of jest.resetAllMocks().

In my current setup, I use to create spies and stubs on beforeAll and change their behaviour on beforeEach. This allows test cases to adapt the stubs according to their specific needs (i.e mockFn.mockReturnValue), without leaking changes into the following test cases.

When I created a spy-only mockFn from a method of an object, I wouldn't expect for the original behaviour to be erased when I called jest.resetAllMocks().

Example

const myObject = {foo: () => 'foo', bar: () => 'bar'}

const stubs: Record<string, jest.SpyInstance | jest.Mock> = {}
beforeAll(() => {
  stubs['myObject_foo'] = jest.spyOn(myObject, 'foo')
  stubs['myObject_bar'] = jest.spyOn(myObject, 'bar')
})
beforeEach(() => {
  // change behaviour of `myObject.foo`, but keep the original behaviour of `myObject.bar`
  stubs['myObject_foo'].mockReturnValue('POTATO!')
})

// all test cases start with the same "well known" setup defined in `beforeEach`
afterEach(() => { jest.resetAllMocks() })

test('run #1', () => {expect(myObject.bar).toEqual('bar')}) // this goes OK
test('run #2', () => {expect(myObject.bar).toEqual('bar')}) // this currently fails

Pitch

This is the behaviour of all other test suits I worked with. If it wasn't by a quick comment on the documentation*, I'd file this as a bug.

• = "Note that resetting a spy will result in a function with no return value"

[github-actions]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

[gerardolima]

Again, as far as I know, this behaviour is different from all test libraries derived from Jasmin. Wouldn't it worth to make this change?

[github-actions]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

[gerardolima]

Keeping this thread alive for a while, or until receiving a response that this behaviour change is not intended.

[github-actions]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

[feliperli]

Hey! I'll try to investigate and take a look at this issue :)

[feliperli]

@gerardolima Hey, just to double check if I understand what your problem is.

const myObject = {bar: () => 'bar'};

let barStub: any;

beforeAll(() => {
  barStub = jest.spyOn(myObject, 'bar');
});

test('run #1', () => {
  expect(myObject.bar()).toBe('bar');

  barStub.mockReturnValue('POTATO!');

  expect(myObject.bar()).toBe('POTATO!');

  jest.resetAllMocks();

  expect(myObject.bar()).toBe('bar');
});

In this example that I created, barStub will be receiving spyOn of myObject. Then, I modify the output with mockReturnValue to POTATO. Your point is that after jest.resetAllMocks, the following expect should return bar (the original value) instead of undefined

Is that it? Only trying to double check to see if I'm following the correct aproach

[gerardolima]

yes, @feliperli, your example shows exactly the behaviour I suggested.

[feliperli]

In that case @gerardolima, I believe the mockRestore functions (jest.mockRestore(), jest.restoreAllMocks()) would solve your problem, no? If you change my test resetAllMocks() to restoreAllMocks, the test will pass 🤔

What do you think? Does it helps you?

[gerardolima]

hey, @feliperli, if you tested, I believe you. Either way, though, don't you think it's strange changing the behaviour of a function which is never mocked on reset?

[feliperli]

@gerardolima Indeed. I'll see what I can do in a PR and see if the maintainers agree with it.

[SimenB]

@cpojer thoughts on this one?

[cpojer]

Yeah, I agree this is basically a bug in Jest and will appreciate a PR fixing it.

[feliperli]

@SimenB @cpojer Hey I actually made a PR to fix the issue. Could you guys take a look? #13692

[SimenB]

#13692

[SimenB]

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

[shlomo-artlist]

According to this change there's no case for using mockRestore. If it's a spied function it would be the same as mockReset (the issued change), and if it's a mock function (jest.fn()) it makes no sense to call restore at all. That's why I (and probably many others) assumed that the only use case where there would be a difference is in a spy with implementation:

• mockReset would clear and set a default mock stub.
• mockRestore would remove the spy and set the original behavior.

That's why I use mockReset everywhere in my code on spies with implementation - to make sure they go back to be default stubs.
But that was changed now, and now I can't upgrade any of my codebases to use the latest jest version.

I think the purpose of this change wasn't important to begin with, since it was fine so far and didn't cause any unfixable problems (just replace mockReset with mockRestore).

A real issue I think is important to notice after this change is that it's now impossible to reset mocks automatically without distinguishing between a mocked function that's mocked by jest.spyOn(...).mock... and a mock that's created by jest.fn(). They would behave differently when resetting them. If I now want to fix it I would create a wrapper that performs both of these on a mock:

• mock.mockClear();
• mock.mockImplementation()

Since I can definitely see why one would want to keep the old behavior, and I can't see a special need to keep the new behavior - I think this change should be reverted in the next major.

[mrazauskas]

Not sure I fully understand what you mean. Are you simply hitting #13808 or is that something else?

Could you drop an example, please?

[shlomo-artlist]

I think it's a different topic. They are complaining about resetAllMocks specifically. I am pointing out that the actual intended implementation here - mockReset, that is now restoring instead of setting a default stub - seems to be problematic.

Problems:

• jest.spyOn(...).mockImplementation() and jest.fn() are now doing different things on mockReset.
• mockRestore is now irrelevant. There's no use case for it.
• There is no built-in function to clear and stub a spy with mocked implementation. You now need to do both manually everywhere.

[mrazauskas]

jest.spyOn(...).mockImplementation() and jest.fn() are now doing different things on mockReset.

Shouldn’t we compare jest.fn() with jest.spyOn(...)? And also jest.fn().mockImplementation(...) with jest.spyOn(...).mockImplementation(...)?

test("resetAllMocks", () => {
  const obj = {
    two() {
      return 2;
    },
  };

  const mock = jest.fn();
  const spy = jest.spyOn(obj, "two");

  expect(mock()).toBe(undefined);
  expect(obj.two()).toBe(2);

  mock.mockImplementation(() => 4);
  spy.mockImplementation(() => 4);

  expect(mock()).toBe(4);
  expect(obj.two()).toBe(4);

  jest.resetAllMocks();

  expect(mock()).toBe(undefined);
  expect(obj.two()).toBe(2);
});

mockRestore is now irrelevant. There's no use case for it.

Hm.. I think it is as useful as jest.useRealTimers().

There is no built-in function to clear and stub a spy with mocked implementation. You now need to do both manually everywhere.

Do I get it right that you are using mockFn.mockReset() as a shorthand for mockFn.mockClear().mockImplementation(() => {})? Documentation of mockFn.mockReset() says: "Does everything that mockFn.mockClear() does, and also removes any mocked return values or implementations." When mocked implementations are removed, I think one should get exactly what was created by calling jest.spyOn(). If mockFn.mockReset() would not do this, one could equally argue that there is no build-in function to reset a spy to initial state.

[trivikr]

I came across this issue independently when working on vitest-codemod.

I agree with @shlomo-artlist, and have re-opened #13916 where we can continue discussing.

[gerardolima]

hey, @shlomo-artlist and @trivikr, I opened this issue because I was having side-effects on methods that had spies, but whose behaviour were never changed. Could you, please, explain how to address the issue described at the code sample at the top) prior to the changes done on the scope of this thread?

[shlomo-artlist]

Hey @gerardolima I think it's what @feliperli suggested here: #13229 (comment)

[github-actions]

This issue 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.