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

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

Open
ericbiewener opened this issue Jan 9, 2023 · 11 comments

Comments

@ericbiewener
Copy link

ericbiewener commented Jan 9, 2023

  • @testing-library/dom version: 8.11.2
  • Testing Framework and version: jest@27.4.7
  • DOM Environment: jsdom@16.7.0

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:

  • Times out because parent.contains(element) always returns true.
  • However, while debugging, I found that document.contains(element) and document.contains(parent) both return false.
  • In other words, parent has been removed from the document, but dom-testing-library doesn't check against that.
  • When this occurs, I can see that parent is of type HTMLDivElement. I would have expected it to be HTMLHtmlElement because of this while loop. Could it be that we have a race condition in which the element gets removed from the document prior to (or even during!) the execution of the while loop?

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

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

eps1lon commented Jan 18, 2023

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

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

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

ddolcimascolo commented Apr 25, 2023

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

Hey guys, been running fine for one day. 🤞

@kgregory
Copy link
Collaborator

kgregory commented Jun 7, 2023

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:

  • There's an open button
  • Clicking the open button presents a dialog
  • There is an agree button
  • Clicking the agree button closes the dialog

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:

  • The first test provides the results of the *ByRole('dialog') query to waitForElementToBeRemoved and times out if that dialog is no longer in the document.
  • The second test provides a callback and reports that the element has already been removed if that 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
Copy link

joeplaa commented Jun 24, 2023

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

ValentinH commented Sep 29, 2023

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

  • the element I'm watching is an SVGSVGElement
  • the element has already been removed from the DOM

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

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;
  }
};

brianlove added a commit to georgetown-cset/parat that referenced this issue Oct 18, 2023
Increase test timeout to hopefully resolve issue where
`waitForElementToBeRemoved` times out:
testing-library/dom-testing-library#1204
brianlove added a commit to georgetown-cset/parat that referenced this issue Oct 20, 2023
Re-query for the dialog element instead of reusing the prior element as
suggested in testing-library/dom-testing-library#1204 (comment)
brianlove added a commit to georgetown-cset/parat that referenced this issue Oct 24, 2023
Re-query for the dialog element instead of reusing the prior element as
suggested in testing-library/dom-testing-library#1204 (comment)
@oncet
Copy link

oncet commented Dec 11, 2023

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

expect(form).not.toBeInTheDocument()

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

No branches or pull requests

7 participants