expose TestUtils.act() for batching actions in tests

[threepointone]

act() for testing react components

---

// React DOM
import { act } from 'react-dom/test-utils';

// React Native
import { act } from 'react-test-renderer';

React, like other libraries, doesn't guarantee synchronous ordering and execution of it's own work. eg - calling this.setState() inside a class doesn't actually set the component state immediately. In fact, it might not even update the state of the component in the same 'tick'. This isn't a problem when it comes to 'users'; they interact with React surfaces asynchronously, giving react and components plenty of time to 'resolve' to particular states.

However, tests are unique in that people write code, usually sequential, to interact with React components, and some assumptions they make won't hold true. Consider - with React's new useEffect hook, a component can run a side effect as soon as it 'starts up'. As a contrived example -

function App(props) {
  useEffect(() => {
    props.callback();
  });
  return null;
}

Let's say you write a test for it like so -

let called = false;
render(
  <App
    callback={() => {
      called = true;
    }}
  />,
  document.body
);
expect(called).toBe(true); // this fails, it's false instead!

The test would fail, which seems counterintuitive at first. But the docs explain it - "The function passed to useEffect will run after the render is committed to the screen." So while the effect has been queued into it's scheduler, it's up to React to decide when to run it. React only guarantees that it'll be run before the browser has reflected changes made to the dom to the user (ie - before the browser has 'painted' the screen)

You may be tempted to refactor this like so -

// don't do this!
function App(props) {
  useLayoutEffect(() => {
    props.callback();
  });
  return null;
}

This would "work" in that your test would pass, but that's because you've explicitly using a render blocking effect where it possibly wasn't required. This is bad for a number of reasons, but in this context, it's bad because we're changing product behavior just to fix a test.

What can we do better?

Well, React could expose a helper, let's call it act, that guarantees the sequential execution of it's update queue. Let's rewrite the test -

let called = false;
act(() => {
  // this 'scope' is safe to interact with the React component,
  // rendering and clicking as you please
  render(
    <App
      callback={() => {
        called = true;
      }}
    />,
    document.body
  );
});
// at this point, we can guarantee that effects have been executed,
// so we can make assertions
expect(called).toBe(true); // this passes now!
act(() => {
  // further interactions, like clicking buttons, scrolling, etc
});
// more assertions

Note - React still doesn't synchronously execute the effect (so you still can't put your expect statements inside act), but it does guarantee to execute all enqueued effects right after act is called.

This is a nice mental model for separation of concerns when testing components - "React, here's a batch of code I'd like you to run at one go", followed by more code to test what React has actually 'done'.

[threepointone]

cc @kentcdodds ^

[threepointone]

moving it into TestUtils
Edit - done.

[gaearon]

Possible follow up: warn if we detect setState on a Hook in jsdom environment?

[sebmarkbage]

If we're going to warn for setState on Hook outside of this thing, we should do it before release.

[threepointone]

Noted. I'll send a followup in a bit.

[sebmarkbage]

We shouldn't expose private APIs to do this in the production build since we'll have to live with those for longer. The goal is to get rid of all the other exports so it's zero private exports. We only get there if we stop going the other direction. Let's just use the hack in the test utils.

We should bikeshed the name a bit more. interact is the wrong word because not all of these are interactions, and even for things we've called interactions in the past we should rename (e.g. to "discrete events" rather than interactive events).

[sebmarkbage]

This function should move to ReactTestUtils and instead using the ReactDOM.render(null...) trick to flush the passive effects. That way we don't have to add any unnecessary invasive APIs to the production ReactDOM. We can add private APIs once we have the new bundle that is exclusively for testing.

[threepointone]

agreed, I felt icky doing this. will change,

[kentcdodds]

I think this looks good. We'll probably just re-export the interact function in react-testing-library.

warn if we detect setState on a Hook in jsdom environment?

I'm not sure I understand this.

[threepointone]

We should bikeshed the name a bit more. interact is the wrong word

strong agree. it felt wrong right there. some options - run, execute, batch, batchAndRun. I would have also liked do

[gaearon]

I'm not sure I understand this.

We want to add a warning when you setState on a Hook-using component in jsdom outside of this "interact" scope. This is because you're usually testing the wrong behavior that doesn't occur in practice due to batching.

In fact this is already a problem in classes. setState() from test won't work like a real setState() in a class click handler. But it's too late to fix in classes since everybody does that. With Hooks we have a chance to explain this is bad, and point to the recommended solution.

[sebmarkbage]

We shouldn't warn in all jsdom environments. Only test ones, such as jest. Sometimes jsdom can be used in some other esoteric use cases - e.g. to server render webcomponents.

[sebmarkbage]

@threepointone Instead of detecting if we're inside "interact()", we can detect if we're batching updates or in concurrent mode. I.e. if we're in sync mode. It's unfortunate because we won't warn if you use batchedUpdates() which won't properly flush the effects but at least we catch the common case. That way we don't need to expose and new APIs from the ReactDOM bundle.

[kentcdodds]

This makes sense. So correct me if I'm wrong but this basically means that if you interact with a component in any way that calls a state updater, you'll get a warning if it wasn't done within an interact callback (or whatever that ends up being called). That sounds good to me 👍 Makes the rules easier to follow. I'd love to play with this when it's ready!

[gaearon]

We shouldn't warn in all jsdom environments. Only test ones, such as jest

This is tricky because we don't have a way to detect. We can detect Jest by global.it or global.expect (or both). But not Ava which uses non-global helpers.

We could check NODE_ENV. But not everybody sets it. Also, we can't easily do this from inside our bundles because our build step would replace it. So it would need to be threaded through somehow.

[sebmarkbage]

I suppose any jsdom server renderer should be using batchedUpdates anyway.

[sebmarkbage]

Drop the inter. Just call it act. It's cleaner.

[sebmarkbage]

There is precedence in the arrange, act, assert style testing to call the phase where you are actually performing things "act" which is what you're meant to be doing in this callback. Followed by expect() calls later.

It's also short so it's not too bothersome to write all the time in all your tests.

I think it's a feature that it is non-descriptive about what actually this models. It's kind of a frame boundary but it's not always because they can also flush early. Event sequences move around a lot in various heuristics/polyfills/spec changes. So there isn't a clear semantic other than there is a bunch of work.

[sebmarkbage]

Can we reuse a single node? Maybe put an inline React element instead so we don't bail out.

[sebmarkbage]

I believe that in the nested case, these will flush at the outer act(...) boundary which makes sense. That way I can put these on the top level always, but I can also put them inside test utilities like "dispatch" helpers.

[gaearon]

Canonical jsdom detection jsdom/jsdom#1537 (comment)

[threepointone]

Question - Assuming the warning exists, how would I get this test to pass?

it('lets a ticker update', () => {
  function App(){
    let [toggle, setToggle] = useState(0)
    useEffect(() => {
      let timeout = setTimeout(() => {
        setToggle(1)
      }, 200);
      return () => clearTimeout(timeout)
    })
    return toggle
  }
  const el = document.createElement('div')
  act(() => {
    ReactDOM.render(<App />, el);
  })
  jest.advanceTimersByTime(250); // this warns!!!        
  expect(el.innerHTML).toBe("1")
})

for context - I wrote ensureBatchingAndScheduleWork in ReactFiberScheduler, that dispatchAction in ReactFiberHooks would call. Running this across our tests, I found a number of failures (because we use the pattern we want to prevent - getting a pointer to a hooks setState and calling it). filtering on only jsdom occurrences, that number got smaller, but led me to writing the above test. current work here - threepointone@ac7416d

[threepointone]

one workaround is to isolate the render call and the timer advance, so this passes the test -

act(() => {
  ReactDOM.render(<App />, container);
});
act(() => {
  jest.advanceTimersByTime(250); // doesn't warn
})

expect(container.innerHTML).toBe('1');

bit annoying though.

another alternative, also annoying -

act(() => {
  act(()=> {
    ReactDOM.render(<App />, container);  
  })      
  jest.advanceTimersByTime(250); // this warns!!!      
});

(putting them both in the same act call doesn't work, since the effect wouldn't have fired yet)

[kentcdodds]

Good point @threepointone. I can see how this complicates the testing story quite a bit. Even experienced React engineers will struggle with writing tests for effects like this 🤔