dom-testing-library icon indicating copy to clipboard operation
dom-testing-library copied to clipboard

waitForElementToBeRemoved error reporting improvement

Open john-james-gh opened this issue 5 years ago • 6 comments

Describe the feature you'd like:

    render(<Component/>);

    await waitForElementToBeRemoved(await findByRole('spinbutton'));

    const button = await findByRole('button', { name: 'Hey' });
    expect(button).toBeInTheDocument();

in case there are two "spinbutton" roles rendered at the same time in Component - the error message in the terminal will not be the usual

Found multiple elements with the role "somerole" and name "somename" (If this is intentional, then use the *AllBy* variant of the query (like queryAllByText, getAllByText, or findAllByText))

instead all that is logged in the terminal is

Timed out in waitForElementToBeRemoved.

in my opinion, it would be useful to have the same error handling for waitForElementToBeRemoved as there is with other selectors such as findByRole

Suggested implementation:

call the same error handler in case waitForElementToBeRemoved fails due to multiple hits

Describe alternatives you've considered:

const x = await waitForElementToBeRemoved(await findByRole('spinbutton'));
expect(x).not.toBeInTheDocument()

also logged in the terminal

Timed out in waitForElementToBeRemoved.

however this setup, in my opinion, doesn't really make much sense since waitForElementToBeRemoved implicitly checks whether an element is in the document, i'd say.

Teachability, Documentation, Adoption, Migration Strategy:

I wouldn't say this would be necessary

john-james-gh avatar Feb 17 '21 16:02 john-james-gh

Hey. I am unable to recreate this in a jest unit test. Please have a look at the diff in my draft PR to see if I've done this correctly. https://github.com/lwkchan/dom-testing-library/pull/1. The Found multiple elements is coming up for me, instead of the timeout.

lwkchan avatar Apr 11 '21 10:04 lwkchan

hi. thanks for taking the time for this

i put this little app together and it seems that it is only await findBy* that is misbehaving

https://github.com/alex-james-vukovity/test-wait-for-element-to-be-removed/blob/main/src/App.test.js

eg

  1. await waitForElementToBeRemoved(await screen.findAllByTestId("abc")) -> works as expected

  2. await waitForElementToBeRemoved(await screen.findByTestId("abc")) -> does not work as expected instead throws Unable to find an element by: [data-testid="abc"] and that's that, no sign of Found multiple elements

  3. await waitForElementToBeRemoved(getByTestId("abc")) -> tests fails but does throw Found multiple elements just like in your example app.

of note: in prod app that i maintain await waitForElementToBeRemoved(getByTestId("abc")) cant be used to wait for a content loader - it fails, instead await waitForElementToBeRemoved(await findByTestId("abc")) must be used

john-james-gh avatar Apr 13 '21 18:04 john-james-gh

I couldn't reproduce locally. was this issue solved?

ronmerkin avatar May 05 '21 12:05 ronmerkin

I got the same Timed out in waitForElementToBeRemoved. sometimes when doing await waitForElementToBeRemoved(await screen.findByRole("status"));. When I say sometimes, I mean that it does pass 1 time out of 10. The whole test suit normally takes 25s to pass, so I am skeptical that this is really a timeout for the failing 9/10 times.

I did find a work-around that works for me (but it looks ugly):

// Before (fails randomly during some test runs)
await waitForElementToBeRemoved(await screen.findByRole("status"));

// Workaround
let loading;
await waitFor(() => {
  loading = screen.getByRole("status");
});
await waitFor(() => {
  expect(loading).not.toBeInTheDocument();
});

Above workaround passes 10/10.

Extra edit: I tried the following, but it fails with the same error even to the workaround succeeds every time:

await waitForElementToBeRemoved(await screen.findByRole("status", { timeout: 10000 } ));

johachi avatar Feb 25 '22 07:02 johachi

@johachi findBy wait a few moments (I don't know the default) before it throws, which slows down the test. Have you tried using getBy?

await waitForElementToBeRemoved(() => screen.queryByRole("status"));

timdeschryver avatar Feb 25 '22 16:02 timdeschryver

@johachi findBy wait a few moments (I don't know the default) before it throws, which slows down the test. Have you tried using getBy?

await waitForElementToBeRemoved(() => screen.queryByRole("status"));

@timdeschryver , thank you for your suggestion, but it is not the test that is timing out, it is waitForElementToBeRemoved that is timing out. The promise from screen.findByRole("status") is resolved before it is passed to waitForElementToBeRemoved.

Difference between

// screen.findByRole is resolved and the result is passed to waitForElementToBeRemoved
await waitForElementToBeRemoved(await screen.findByRole("status"));

// the promise from screen.findByRole passed to waitForElementToBeRemoved before it is resolved.
await waitForElementToBeRemoved(screen.findByRole("status"));

johachi avatar Feb 28 '22 01:02 johachi