Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Act warning in tests #368

Closed
nstepien opened this issue Jun 17, 2020 · 11 comments
Closed

Act warning in tests #368

nstepien opened this issue Jun 17, 2020 · 11 comments

Comments

@nstepien
Copy link
Contributor

nstepien commented Jun 17, 2020

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

  • Popper.js: 2.4.2
  • react-popper: 2.2.3
@FezVrasta
Copy link
Member

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

@amanmahajan7
Copy link

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
Copy link

AndrewOCC commented Jul 2, 2020

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
Copy link
Member

PRs to improve this are welcome

@ismay
Copy link

ismay commented Jul 22, 2020

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
Copy link

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
Copy link
Collaborator

atomiks commented Jan 11, 2022

@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
Copy link
Contributor

piecyk commented Jan 11, 2022

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
Copy link

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
Copy link
Collaborator

atomiks commented Dec 7, 2022

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: testing-library/react-testing-library#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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants