testing-library / dom-testing-library

🐙 Simple and complete DOM testing utilities that encourage good testing practices.
https://testing-library.com/dom
MIT License
3.26k stars 466 forks source link

`waitForElementToBeRemoved` can time out even when the element has been removed from the `document` #1204

Open ericbiewener opened 1 year ago

ericbiewener commented 1 year ago

Relevant code or config:

await waitForElementToBeRemoved(screen.getByText("Loading"));

What you did:

Executed test

What happened:

Test fails with error Timed out in waitForElementToBeRemoved.

Reproduction:

Not sure how to dependably repro it, but when it occurs it seems to occur deterministically.

Problem description:

Suggested solution:

Just check if document.contains(element) rather than capturing the parent in the first place (of course, I assume there is a reason for not taking this simple approach).

ericbiewener commented 1 year ago

This might only be happening with <svg> elements. The Loading text I referenced is in a <title> element inside an <svg>. Perhaps it is happening in other scenarios as well, but so far I'm only aware of seeing it happen in our codebase on these SVG loaders.

eps1lon commented 1 year ago

It doesn't look like this bug report has enough info for one of us to reproduce it.

Please provide a CodeSandbox (https://react.new), or a link to a repository on GitHub.

Here are some tips for providing a minimal example: https://stackoverflow.com/help/mcve

ericbiewener commented 1 year ago

Since I suspect a race condition, that makes it difficult to reproduce a real world example. However, the problem can be forced by simply calling waitForElementToBeRemoved on an element that was never added to the DOM in the first place:

  const parent = document.createElement("div");
  const child = document.createElement("div");
  parent.appendChild(child)
  await waitForElementToBeRemoved(child);

Here's that code running in an actual test:

ddolcimascolo commented 1 year ago

Hello everyone, we have this in CI too, and this is not reproducible locally on developer machines. It happens randomly in CI but always on the same few tests.

Not sure what I can provide to further reproduce...

ddolcimascolo commented 1 year ago

I just noticed by looking at https://testing-library.com/docs/guide-disappearance/#waiting-for-disappearance that waitForElementToBeRemoved accepts a function returning the element to monitor. We're used to pass an element directly, that we first get through a getBy/find By query.

Maybe that's related?

EDIT: Yeah, the code's definitely not doing the same thing depending on whether a callback is passed or an element directly. Will implement this, give CI a few days (we usually reproduce once or twice a day) and report here.

ddolcimascolo commented 1 year ago

Hey guys, been running fine for one day. 🤞

kgregory commented 1 year ago

I ran into this because I figured that the first of these is preferable when I already have the element from a previous query:

await waitForElementToBeRemoved(dialog);
await waitForElementToBeRemoved(() => screen.queryByRole('dialog'));

Turns out that might not be true if the provided element has a parent.

I've put a reproduction together [codesandbox].

I took the MUI Alert Dialog demo and wrote a simple test for it:

The test suite consists of two nearly identical tests - the difference is in how we assert that the dialog is no longer in the document:

To simulate the behavior of the element being removed from the document prior to the invocation of waitForElementToBeRemoved, I've simply doubled up those lines. The first occurrence will succeed, the second will occur after the element has been removed.

The first test is timing out because MUI's dialog renders its element with role="dialog" within another element that is ultimately removed when the dialog is unmounted. So the dialog has a parent and when we provide that element to waitForElementToBeRemoved, the checks for parent existence and parent.contains(element) are successful in the callback that is provided to waitFor in this scenario (see wait-for-element-to-be-removed.js)

joeplaa commented 1 year ago

Hello everyone, we have this in CI too, and this is not reproducible locally on developer machines. It happens randomly in CI but always on the same few tests.

Had the same issue this week. I "fixed" it by increasing the timeout. This seems to us more like a workaround, but at least we can continue for now.

await waitForElementToBeRemoved(() => screen.queryByRole('dialog', { name: 'MyDialog' }), { timeout: 30000 });
ValentinH commented 11 months ago

I'm experiencing the same and it happens when these 2 conditions are met:

If the element is a DivElement, then it correctly throws the "The element(s) given to waitForElementToBeRemoved are already removed. waitForElementToBeRemoved requires that the element(s) exist(s) before waiting for removal." error. It doesn't for SVGs for some reasons 🤔

Here's the code I'm using: it's basically a helper we use at the beginning of our test to wait for the loader to be removed:


export const waitForLoaderToBeRemoved = async () => {
  try {
    const loader = await screen.findByTestId('my-loader');
    await waitForElementToBeRemoved(loader);
  } catch (e: any) {
    if (e.message.includes('already removed')) {
      // sometimes the loader has already been removed when we try to check for it!
      return;
    }
    throw e;
  }
};
ValentinH commented 11 months ago

I fixed my above code by adding document.contains(loader) to it:


export const waitForLoaderToBeRemoved = async () => {
  try {
    const loader = await screen.findByTestId('my-loader');
    if(!document.contains(loader)) return
    await waitForElementToBeRemoved(loader);
  } catch (e: any) {
    if (e.message.includes('already removed')) {
      // sometimes the loader has already been removed when we try to check for it!
      return;
    }
    throw e;
  }
};
oncet commented 8 months ago

Having the same problem, for now I will use this instead:

expect(form).not.toBeInTheDocument()
isimmons commented 1 week ago

It helps if you can control response time. I'm using msw and the delay method set explicitly to 1500 ms. Strange thing is my test for a loading spinner fails at waitForElementToBeRemoved at around 1490 ms if I set the timeout for it.

I set it to 1600 to be safe. It is working for me both ways, getting the spinner element first or using the callback method as long as I explicitly add the timeout.

const spinner = await screen.findByLabelText(/loading/i);
expect(spinner).toBeInTheDocument();

await waitForElementToBeRemoved(spinner, {
  timeout: 1600,
});