Proposal: Revert override of jest default `resetMocks`

[andyrichardson]

About

Hey what's up!

I found myself trying to work out why all my tests broke when upgrading react-scripts and then realized it's because of the change to enabling resetMocks by default.

I wanted to challenge this decision because, especially when react-scripts is targeted at newer users looking to use react, this causes fundamental jest examples online to break.

App-wide mocks

It also means app-wide mocks such as these don't work.

// some-mocked-module.js
export const useSomeHook = jest.fn(() => true);

// setupTests.js
jest.mock('some-mocked-module', () => ({
  useSomeHook: jest.fn(() => true)
}))

For new users, I can see why it would be very confusing.

[AlliterativeAlice]

This also breaks mock functions in manual mocks

[PeterMylemans]

Is it possible to reflect the impact of this change in defaults more explicitly in the release notes?

Manual mocks no longer working sent us down the rabbit hole until stumbling on this issue or any of the related issues.

[andyrichardson]

@petermylemans I also spent a chunk of time trying to work out the cause - but in fairness, they did document this under breaking changes on the 4.0.0 release.

Jest

We've upgraded to Jest 26 and now set resetMocks to true by default in the Jest config.

[PeterMylemans]

Fair enough! That's how I also figured it out.

I was just pointing out that for more basic users like me (and I suspect many other CRA users), the global and manual mocks thing might merrit an explicit mention as well.

[k-yle]

is there a way override resetMocks to be false in CRA 4.0? CRA ignores "resetMocks": false in the jest config

[lithammer]

@k-yle Are you sure? Adding this to package.json solved it for us using CRA 4.0.1:

  "jest": {
    "resetMocks": false
  }

[k-yle]

@lithammer apologies, our issue was caused by a different breaking change in jest 26.

"resetMocks": false does work

[ianschmitz]

The default of false has also tripped people up as well. Our sense is that true is the best starting place to encourage better testing practices. It's one of those settings where not everyone will agree (hence why we make it configurable). See https://github.com/facebook/create-react-app/pull/7899 for more context.

I'll leave this open to gather more feedback. If we were to change it back it wouldn't be until the next major version of CRA.

@kentcdodds thoughts? I know you were in favor of this default.

[kentcdodds]

Resetting a mock will reset it's mock implementation, this could make it confusing if you tried to do this:

beforeAll(() => {
  const originalError = console.error
  jest.spyOn(console, 'error')
  console.error.mockImplementation((...args) => {
    if (args?.[0]?.includes('something to ignore')) return
    originalError(...args)
  })
})

With resetMocks that mock implementation will only work for the first test, then it will get reset. The fix for this would be to require folks to do this:

beforeAll(() => {
  const originalError = console.error
  jest.spyOn(console, 'error')
})

beforeEach(() => {
  console.error.mockImplementation((...args) => {
    if (args?.[0]?.includes('something to ignore')) return
    originalError(...args)
  })
})

That works, but it can be confusing.

However, there are great reasons to get the mocks cleared between runs. So I think I'd suggest using clearMocks: true instead of resetMocks: true. The only big difference is that the mock implementation remains. This comes with its own set of trade-offs though because it means that if I do this:

beforeAll(() => {
  const originalError = console.error
  jest.spyOn(console, 'error')
})

test('some test thing', () => {
  console.error.mockImplementation((...args) => {
    if (args?.[0]?.includes('something to ignore')) return
    originalError(...args)
  })
})

test('some other test thing', () => {
  // with resetMocks, calling console.error will call the underlying console.error always
  // with clearMocks, calling console.error will call the mock implementation in the previous test 😬
})

With that context, I'm not certain which to suggest. I think resetMocks is probably the best between the two, but there are foot guns for both.

Last thing I'd say is that we should configure at least one of them. You never want to have the mock state preserved between tests. So it should at least do clearMocks.

[angel3cu]

It just feels like it will break any tests using beforeAll() with mocks. Are there plans to deprecate beforeAll() and only use beforeEach()?

[ianschmitz]

@angel3cu that's a question for the Jest team, but i can't see that ever happening.

[aarowman]

See https://github.com/jefflau/jest-fetch-mock/issues/184 for a similar situation. This comment sums it up: https://github.com/jefflau/jest-fetch-mock/issues/184#issuecomment-718058715.

It's an easy enough workaround, but definitely has bigger ramifications than just suggesting people use beforeEach(), because other libraries may depend on the default jest behavior too, not just the tests written in the project.

[CrispyBacon12]

Definitely agree with using clearMocks instead of resetMocks as the default. I'm all for encouraging better testing practices, but this seems to be also discouraging things that aren't necessarily inferior. What's so bad about providing mock implementations inside jest.mock?

Ran into this when trying to mock react-transition-group, similar to this example: https://testing-library.com/docs/example-react-transition-group/

Only being able to provide .mockImplementation() in beforeEach and not the mock block itself seems unnecessary, and gets awkward with TypeScript, since you lose the jest.Mock type and have to cast it somewhere.

[aarowman]

I also agree with using clearMocks instead of resetMocks.

Currently I had to disable this behavior in package.json after upgrading:

"jest": {
  "resetMocks": false
}

and I already have this in every test file:

  afterEach(() => {
    jest.clearAllMocks(); // clear all mock.calls and mock.instances (not implementations)
  });

This helps SOOO much to clear the mocks so I can explicitly test something and make sure the mock calls are correct, but it doesn't break if I want global mock behavior for that test suite, and I don't have to worry about clearing mocks after they're used.

It'd be nice not to need the clearAllMocks in each test file, but I definitely don't want to resetMocks every time

[liam-jones-lucout]

My issue with this is it's taking a strong positive action; removing mocks defined outside of a specific test (__mocks__/** for example) and making it into an implicit behaviour that can only be understood by close examination of the CRA documentation. Seems like a huge beginner/tired programmer trap.

It doesn't help that the Jest documentation for resetMock, (which you would need to know exists in order to realise it could be the cause of mock implementations disappearing) leads to a description with this line in it:

This is useful when you want to completely reset a mock back to its initial state.

[Bold added for emphasis]

To me, that line is quite ambiguous; does it mean the initial implementation of a jest.fn (i.e. nothing) or does it mean the initial implementation of a mock within my code? I appreciate this isn't an issue for everybody, but it seems like taking a change with such a strong impact on tests, making it implicit, and making it different to the Jest default is setting up people for failure. In order to diagnose issues caused by this one would need to:

1. Intuit that the issue they're having is caused by their mock implementations being reset, and not any other mock inclusion mistake
2. Know that there's a setting in Jest that causes Mock implementations to be completely wiped between tests
3. Understand that initial implementation means no implementation
4. Know that same setting is changed from its default value of false within CRA

I'm positing this here because I was fortunate enough to know 1-3. but without knowing about 4, I lost a number of hours trying to debug this.

[AuthorProxy]

please make it default options, I spent so much time at this