floating-ui / react-popper

🍿⚛Official React library to use Popper, the positioning library
https://popper.js.org/react-popper/
MIT License
2.51k stars 225 forks source link

Act warning in tests #368

Closed nstepien closed 4 years ago

nstepien commented 4 years ago

Reproduction demo

import React, { useState } from 'react';
import { render, act } from '@testing-library/react';
import { usePopper } from 'react-popper';

function MyComponent() {
  const [reference, setReference] = useState();
  const [popper, setPopper] = useState();
  const { styles, attributes } = usePopper(reference, popper);

  return (
    <>
      <div ref={setReference} />
      <div ref={setPopper} style={styles.popper} {...attributes.popper} />
    </>
  );
}

test('act warning', () => {
  render(<MyComponent />);
});

test('no act warning with a poor workaround', async () => {
  render(<MyComponent />);
  await act(() => Promise.resolve());
});

Steps to reproduce the problem

  1. Run test using usePopper

What is the expected behavior?

No act warning, or a way to address it without feeling like a cheap workaround.

What went wrong?

  console.error
    Warning: An update to MyComponent 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
        in MyComponent

Any other comments?

This is probably coming from the use of useEffect (useIsomorphicLayoutEffect) + state update. https://kentcdodds.com/blog/fix-the-not-wrapped-in-act-warning

Packages versions

FezVrasta commented 4 years ago

Take a look at the react-popper tests, they work just fine

amanmahajan7 commented 4 years ago

It is possible that the underlying issue may have not been fixed. react-popper is using the following ways to fix the warnings

https://github.com/popperjs/react-popper/blob/master/src/Popper.test.js#L55

await waitFor(() => {});

https://github.com/popperjs/react-popper/blob/master/src/usePopper.test.js

await act(async () => {
   await rerender({ referenceElement, popperElement });
});

Some of them are unnecessary as per testing-library best practices https://kentcdodds.com/blog/common-mistakes-with-react-testing-library#passing-an-empty-callback-to-waitfor https://kentcdodds.com/blog/common-mistakes-with-react-testing-library#wrapping-things-in-act-unnecessarily

From what I suspect it is happening because of the following reason https://github.com/popperjs/react-popper/blob/master/src/usePopper.js#L60

We provide modifiers to the createPopper function and it may be calling fn hence setting the state before the component is destroyed.

https://github.com/popperjs/popper-core/blob/master/src/index.js#L282

I also noticed the warnings are fixed if we manually unmount the component which further validates this theory.

Thank you for looking into this

AndrewOCC commented 4 years ago

I'm also encountering this same issue running tests with the Render Props version of popper – we're on React 16.8.x so we don't have access to async act() to mitigate the issue.

I've included a basic example test below that raises the error:

import React, { useState } from 'react';
import { render } from '@testing-library/react';
import { Manager, Popper, Reference } from 'react-popper';
function BasicPopper () {
  return(
    <Manager>
      <Reference>{({ ref }) => <div ref={ref}>Reference element</div>}</Reference>
        <Popper>
          {({ ref, style }) => (
            <div ref={ref} style={style}>
              foo bar
            </div>
          )}
        </Popper>
    </Manager>
  )
};
test('act warning', () => {
  render(<BasicPopper />);
});
FezVrasta commented 4 years ago

PRs to improve this are welcome

ismay commented 4 years ago

From what I suspect it is happening because of the following reason https://github.com/popperjs/react-popper/blob/master/src/usePopper.js#L60

We provide modifiers to the createPopper function and it may be calling fn hence setting the state before the component is destroyed.

@amanmahajan7 I suspect that this is the case as well. I'm dealing with a similar issue with a popper that's also supplied with modifiers.

@FezVrasta I think this issue should be reopened. It's not resolved yet and an open issue will catch the attention of potential contributors more than a closed one.

m7kvqbe1 commented 2 years ago

This appears to still be an issue in the new 'floating-ui' library.

We recently adopted this and refactored in a hope this issue might have been solved as a side effect.

+1 to re-open this issue please.

atomiks commented 2 years ago

@m7kvqbe1 do you have a reproduction when the warning occurs?

Every time you perform a state update for Floating UI, if you're using RTL, you should be using waitFor() like this:

userEvent.click(getByTestId('button'));
await waitFor(() => expect(...).toBe(...));
piecyk commented 2 years ago

Every time you perform a state update for Floating UI, if you're using RTL, you should be using waitFor() like this:

Or a find* query. For example: const userAddress = await findByLabel(/address/i), checkout

How do I fix "an update was not wrapped in act(...)" warnings?

from https://testing-library.com/docs/react-testing-library/faq

Basic this cannot be solved as Popper/Floating UI is updating state "outside" of react.

theKashey commented 1 year ago

A good solution for a problem could be in configuring update debounce to be synchronous in testing mode.

And good news - this code just don't exists in floating-ui (modern popper) and updating is the best solution.

atomiks commented 1 year ago

The same issue also exists in Floating UI as it's an async function (to support async platform methods) which can't be made sync.

I think the core of the issue is that testing library should change the timing of its cleanup function but likely won't happen: https://github.com/testing-library/react-testing-library/issues/1073

It's a pain but there are two solutions (although I see the act warning get printed if the test fails, for the first one at least)

theKashey commented 1 year ago

The problem exists not only with "late" cleanup, but also with "continuous" render (as per @AndrewOCC example)

 render(<BasicPopper />);

// from @atomiks example about. This is an incorrect usage of userEvents as they are all async (now)
// userEvent.click(getByTestId('button'));
// this is correct
await userEvent.click(getByTestId('button'));
// ^^^ 💥 popper side effect will pop in the first and nearest await

await waitFor(() => expect(...).toBe(...));

In order to be "testable" all side effects have to be ended in render. Or, if framed differently, flushed by act or by any other command exported from floating-ui