Create-react-app: 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.