React-popper: Popper causing warnings when running tests

[mogusbi]

Reproduction demo

https://github.com/mogusbi/react-popper-test-error

Steps to reproduce the problem

1. Clone the repo
2. Install dependencies
3. Run npm test

What is the expected behavior?

Tests to run without any warnings being emitted

What went wrong?

Any test that contains a component that has popper causes this warning to be outputted.

This is happening when using hooks and when using the older render method.

console.error node_modules/react-dom/cjs/react-dom.development.js:88
      Warning: An update to Hook inside a test was not wrapped in act(...).

      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

Packages versions

• Popper.js: 2.3.2
• react-popper: 2.2.2

[FezVrasta]

We have a couple of those warnings in our test suite too, I don't know how to fix them, but I'm almost positive it's not an issue with the library itself, but with the way the tests are written.

[mogusbi]

I'm not so sure as these warnings were not appearing in version 1 of the library

[FezVrasta]

v1 didn't use React Hooks, those warnings appear only if the component is using them and the tests are not properly written

[FezVrasta]

Please take a look at this commit to see how I fixed the warnings in react-popper's tests, you should do something similar in your tests.

https://github.com/popperjs/react-popper/commit/76822a25ebb87592ad7faea7c3c407f5be1f2da0

[adamhooper]

@FezVrasta I think this should be re-opened....

By my reading, the warning is exactly right, and it's up to react-popper to help. The problem is: createPopper() runs its first update() asynchronously.

For instance, with this code and test (pseudo-code, just so we're all talking about the same thing):

// Popup.js
export default function Popup() {
  const [referenceElement, setReferenceElement] = React.useState(null)
  const [popperElement, setPopperElement] = React.useState(null)
  const { styles, attributes } = usePopper(referenceElement, popperElement)

  return (
    <>
      <div ref={setReferenceElement}>Reference</div>
      <div ref={setPopperElement} style={styles.popper} {...(attributes.popper || {})}>Pop!</div>
    </>
  )
}

// Popup.test.js

it('should mount', () => {
  act(() => {
    const wrapper = mount(<Popup />) // enzyme, @testing-library ... same problem
  })
  expect(true).to.be(true)
})

Here's what happens when the test is run:

1. Popup mounts. referenceElement and popperElement are null, so styles and attributes come back empty.
2. During mount, setReferenceElement() and setPopperElement() are called. That leads to a re-render with new referenceElement and popperElement. Now, usePopper() can take action. react-popper returns some styles and attributes ... and its internal createPopper() _schedules an update_ (this is the problem!)
3. The test finishes! act() returns. Popper never updated.
4. Popper updates ... which leads to a re-render with new styles and attributes. React warns, because the test executed out-of-order.

Indeed, @FezVrasta, there is a _very_ subtle trick in commit 76822a25ebb87592ad7faea7c3c407f5be1f2da0 that seems unintentional. I think it's erroneously hiding the warning.

The code added in that commit:

  await act(async () => {
    result = await render(

render() is synchronous, so await ought to do nothing, right? But of course, JavaScript is more nuanced than that! Calling await render(...) runs render() synchronously, and _then it flushes the Promise microtask queue_. I suspect that's the only reason update() finishes before the act() code block finishes.

How to solve?

Brainstorming, here are two ideas:

1. Leave as-is, and document the crap out of this. Nix the await render() from the test suite: instead, write an await null with a comment after render(). Document that all unit tests must wrap all actions in act() (unless they kick off a code path that calls forceUpdate()?), and explain that they need an await null in them.
2. Make react-popper prevent Popper from calling update(); instead, make react-popper call useLayoutEffect(forceUpdate) at the right times. For instance, react-popper would attach scroll-event handlers, and it would prevent @popperjs/core from doing so. I don't know how perilous this path would be ... but as a React user I mistrust all libraries that touch the DOM outside of event handlers and useLayoutEffect().)

Here's what I write in my own unit tests:

    wrapper.find('button.context-button').simulate('click')
    await act(async () => await null) // Popper update() - https://github.com/popperjs/react-popper/issues/350