Make restoreMocks, resetMocks, clearMocks default

[mockdeep]

🚀 Feature Proposal

The restoreMocks, resetMocks, and clearMocks settings should be enabled by default.

Motivation

We've spent a lot of time debugging tests due to mocks leaking behavior between tests. We added jest.resetAllMocks() to our test helper file a while back and that made a huge difference. But recently I discovered a lingering test spy was causing false positives in other tests that followed it. I also needed to add jest.restoreAllMocks().

I noticed there are config options, so then I moved them to our jest config. (Per #7068, restoreAllMocks() does not also resetAllMocks() as the docs suggest.)

Pitch

I think the common case would be for people to expect clean state between tests, and not having that by default might be causing a lot of confusion and false positives in tests. I'm not aware of a valid use case for not clearing/restoring all mock state after each test. If there is, it seems like it's probably pretty rare, and could potentially be configured as needed.

[SimenB]

I think it makes sense to set those to true by default. Super breaking, but as long as we call it out in the blog post I think that's fine.

@jeysal @thymikee thoughts?

[jeysal]

Not sure true is a good default. As soon as you have one test that needs to retain state, you need to set it to false again and change all other tests to manually reset. Also, mocks lose their initial implementations so you will need to do mockImplementation etc in a beforeEach hook or get highly confused why your jest.fn().mockImplementation() is being lost.

[mockdeep]

As soon as you have one test that needs to retain state

What sort of state do you need to retain between tests? If anything, this seems like an anti-pattern to me. But if it's the sort of thing you know you're going to need, it seems like you're already going to have to deal with the issue of manually resetting as things are now.

mocks lose their initial implementations so you will need to do mockImplementation etc in a beforeEach hook

This actually seems like a good restriction to me. I don't know if it is information that can be inferred, but it would be super helpful if Jest were to warn or throw an error when something like mockImplementation was called outside of a test context like it or beforeEach.

highly confused why your jest.fn().mockImplementation() is being lost.

We've actually seen the inverse of this a lot, so changing the default is probably a wash at worst. Right now, when somebody overrides the mockImplementation in one of the tests they can lose a lot of time trying to figure out why a change in one test is suddenly causing a lot of later tests to act strangely.

Bottom line for me is that tests should be independent of each other and not have some persistent state between them that could impact whether they pass or fail. As much as possible, the test framework should ideally be configured in such a way to strongly encourage this. I'm still not sure if there are valid cases for persisting state across tests, but if there are, it seems like that should be the longer road to step around framework defaults.

[bisubus]

I expected them to be Jest defaults when it emerged, at least clearMocks and restoreMocks. But now it seems a bit late to enable them. I agree this would be "super breaking". My suggestion is to make a soft transition, officially recommend restoreMocks and clearMocks and set them to true in newly generated jest --init configs, with default values remaining false. Not resetMocks for certain, it's destructive.

clearMocks is a good thing that prevents tests from contaminating each other. This will break tests that were already badly written.

restoreMocks makes mocks in beforeAll and top-level spies invalid (that Jest favoured them for a long time is the sad reality):

// fetch accidentally starts to do real requests after first test
spyOn(global, 'fetch').mockResolvedValue(...)

beforeAll(() => {
  // same here
  spyOn(global, 'fetch').mockResolvedValue(...)
});

It may be good but only for old-fashioned style with mandatory beforeEach (beforeAll wasn't initially available in Jasmine):

beforeEach(() => {
  spyOn(global, 'fetch').mockResolvedValue(...)
});

As for default resetMocks, it's worse. A big show stopper is that resetMocks ruins ___mocks___ that are supposed to provide reusable module mocks:

module.exports = {
  // I don't want reset them, ever
  foo: jest.fn().mockReturnValue('foo'),
  bar: jest.fn(() => 'bar')
};

[jeysal]

For reasons bisubus explained, restoreMocks and resetMocks can be quite destructive and I don't see them becoming a default.
As for clearMocks, which is somewhat widely used, while enabling it by default is very breaking, and even breakages with prior warning we want to keep to a minimum, I could imagine nudging people towards considering it for their project. This could be more hints to the config option where we're talking about mocks (I believe the mockClear docs already have this). It could also be a --init suggestion as suggested by @bisubus. Also given Simen's comment, I think that this kind of change would be pretty uncontroversial, so I think we'd welcome PRs for that.

[ericbiewener]

I agree with making clearMocks: true the default, but it would also be nice to add the ability to override that for a given test suite, something like jest.clearMocks(false) at the top of a test suite, or inside a describe block. That would let it remain turned on even when a rare use-cases arises for not clearing mocks in a given suite/decribe. This would address @jeysal's comment.

[vcarl]

It would be really fantastic if clearMocks returned the mock to the state found after it was first configured. This seems like an achievable thing for manual mocks from __mocks__, and I burned a solid amount of time trying to figure out why the mock implementation I was setting wasn't around to be used when my test started running.

[pke]

so are you saying @vcarl that clearMocks removes all implementation from mocks in mocks? That does not make any sense, those mocks should be protected. Only mocks modified inside tests should be reset.

[vcarl]

That was 6 months ago, I don't recall the specifics of the situation anymore

[pke]

Anyway, implementations done inside mocks should not be reset under any circumstance.

I fear there are a lot of seriously broken tests out there because of all of this strange default mock leaking behaviour of jest.

[skovhus]

Maintaining mock state between tests is a footgun. So making clearMocks default to true makes a lot of sense to me – despite this being a breaking change.

@SimenB are you still in favour of changing this?

[sgb-io]

Yes, this is a very breaking change, but I personally believe there is a fundamental issue here that warrants the change. IMO, it is far too easy to write buggy tests that give a false assurance of working and tested software, due to subtle mock leakage between tests.

Ever had to modify unit tests and have to spend hours debugging why things broke when you made some chages, only to discover something was leaking, and a change of order or the existence of a new test was the cause? I have worked on dozens of codebases in the past several years and had this problem. It's a nightmare to debug and I wouldn't expect a trusted tool such as Jest to let me do slightly wild stuff like that, by default.

I want to have high confidence that my tests are passing because they're testing what I expected it to, using the input and mocks I expected it to use. Even if my test code is more verbose and explicit.

I hope @SimenB and @mrazauskas will consider this change once again

[pke]

This change needs to happen to fix all the broken jest tests out there. The fallout may be massive but people currently don't know their tests, and by that, their code, is potentially broken. +1 for changing the defaults. Should increase major version for that to stay true to semver though.

[cangSDARM]

Is there any further plan for this issue? It has been opened awhile

[github-actions]

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

[bisubus]

The issue is still relevant.

[hernang87]

This issue is still relevant.