Do we need to use react-dom/test-utils act() function?

[kentcdodds]

I'm not sure what's up here, but it looks like we don't need it after all. The test here is passing and I'm not seeing any warnings 🤔 I would expect this to fail.

It .... just works™️ ???

Help appreciated @threepointone @gaearon

[threepointone]

bit pooped today, so I'll write you failing tests tomorrow. but your test just got lucky :) if you hadn't fired the click, then the effect wouldn't have flushed, and effectCb wouldn't have been called. also, if you look closely, effectCb is supposed to fire twice (once on initial render, once after clicking the button). if render and fireEvent were wrapped with act(), then you'd see effectCb called twice. (and only once if you didn't click the button)

[kentcdodds]

Oh of course! My bad. Thanks @threepointone! I'll have this ready to go by the time y'all release hooks I think. Thanks for all the work you do man. Get some rest Sunil, you look tired.

[threepointone]

btw, the reason you didn't see a warning, is because event handlers are batched by default in react, so the setState 'happened' within a batch. but it doesn't flush effects early by default. hence the need to wrap fireEvent with act

[kentcdodds]

OOoooh, that makes a lot of sense, thanks!

What about callbacks? How are people supposed to handle things like:

useEffect(() => {
  const unsubscribe = socket.subscribe(changes => setSomeState(changes.someState))
  return unsubscribe
})

My inclination is to assume that in this situation people would mock the thing that's calling their callback and make sure they trigger a change within an act manually. Is that right? So something like:

act(() => {
  socketMock.fireChanges(fakeChanges)
})

Let me know if that's not right. react-testing-library will re-export act for this reason and will have docs/examples for this kind of scenario.

[threepointone]

that's right! also, fake timers are super useful, so you could run

// expect(a spinner to be shown)
act(() => {
  jest.runAllTimers() // or jest.advanceTimersByTime(period)
});
// expect(data to be on screen)

[kentcdodds]

Perfect. Thanks friend 🤗

[danielkcz]

Just tried to update mobx-react-lite package to React 16.8 and there is a bunch of failing tests now. I want to have a closer look later, but it might be actually related to this issue. Leaving this here as a case study for now.

Update: Seems that act does not solve most of the failing tests. It's something else related to new React version 😕

[threepointone]

Afk, so cant look at the logs. Could you show me an example of the failures/corresponding error/warning/log messages?

[danielkcz]

@threepointone Turns out it was a mistake on our side (see more in mobxjs/mobx-react-lite#57 (comment))

This PR should be merged I think because now every test is spilling this warning. I wanted to try it out, but I am failing to build RTL on Windows.

When testing, code that causes React state updates should be wrapped into act(...):
      
      act(() => {
        /* fire events that update state */
      });
      /* assert on the output */
      
This ensures that you're testing the behavior the user would see in the browser. Learn more at https://fb.me/react-wrap-tests-with-act

[threepointone]

now every test is spilling this warning

just to clarify, does this mean you were already using hooks in production?

[danielkcz]

@threepointone Well, that package is built solely around the hooks since they became a thing :) And I know a bunch of people that were using in production already. It's working just fine now, except those tests spilling warnings. I've opened a new issue about it: #281