Test really slow because of MutationObserver not disconnecting directly

[romain-trotard]

• @testing-library/dom version: 7.26.3
• Testing Framework and version:
  @testing-library/react version: 11.1.0
  jest version: 26.6.1
• DOM Environment:
  jsdom version: 16.4.0

Hello guys :)

What happened:

My tests are really slow.

Reproduction:

I made a little codesandbox: https://codesandbox.io/s/mutationobserver-reproduction-pe9gf?file=/src/__tests__/index.js where you can see my problem.

The issue is that the callback of waitFor is called even when the element has been found:

The console log is printed thrice instead of one:

Problem description:

The MutationObserver is disconnected at the end of the test instead of directly when the onDone callback is called. I guess this is because the thread is not available until the end of the test.
These unnecessary callback's calls (in the MutationObserver) make my tests really slow as you can see below:

Suggested solution:

I have tested to directly disconnect the observer instead of inside the setImmediate method (https://github.com/testing-library/dom-testing-library/blob/master/src/wait-for.js#L97). I get really faster tests:

if (!usingFakeTimers) {
    clearInterval(intervalId)
    observer.disconnect()
}

Is there a reason to disconnect the observer inside setImmediate instead of synchronously ?
Am I doing something wrong ?
Or maybe I could make a PR to make the disconnection configurable globally (choice would be between synchronously and asynchronously) ?

[kentcdodds]

Hi @romain-trotard,

Thanks for this! I'd love to speed this up. Here's the PR that added the setImmediate behavior: #102

If you can figure out how to avoid the problem that PR solved and make it faster, I'd welcome a PR for that.

[romain-trotard]

Hi @kentcdodds.
Thank you for the link of the PR. I will look at it and will try to find a solution (and open a PR) :)

[romain-trotard]

I'm back, after an afternoon of investigation :)

The use of @sheerun/mutationobserver-shim make an infinite loop that #102 fixed.
The onDone function is executed line 45. In the onDone there is the observer.disconnect() in which the _timeout is cleared. But line 48 a new setTimeout is called: here is the infinite loop.

Since the version 13.2.0 of jsdom, there is an implementation of MutationObserver (so no more need of @sheerun/mutationobserver-shim). In this implementation, there is no problem to call the disconnection syncrhonously in the onDone callback.
Actually in my project I use jest in the latest version (26.6.1), which rely on the previous jsdom version since [email protected] :)
So I didn't have problems by removing setImmediate(() => observer.disconnect())...

I made all my tests on the https://github.com/grigasp/dom-testing-library-waitforelement-issue project and mine, with the three options presents here https://github.com/testing-library/dom-testing-library/releases/tag/v7.0.0, and did not see any problem to call both:

observer.disconnect()
setImmediate(() => observer.disconnect())

Like this, I guess, there is no breaking change for projects which do not use at least the version [email protected] (or a custom implementation of MutationObersver) and makes tests going faster on project using at least this jsdom version: actually I can't say for other projects but it's really faster for mine :)

For one test suite:

Before	After
65sec	35sec

For all test suites:

Before	After
105sec	70sec

(I have a lot of test which are utilities tests, not using dom-testing-library)

I will open a PR adding this, hoping that you do not see any problem :D

[marcosvega91]

Are we sure that this problem is still an issue ? waitForElement has been deprecated in favor of waitFor. mutationobserver-shim is not used anymore and the test was dropped some time ago. Maybe we can safely change that line? 🤔

[romain-trotard]

Actually, the problem of the PR #102 is now present in the waitFor implementation for people still using @sheerun/mutationobserver-shim on their own (option 3 of the release note https://github.com/testing-library/dom-testing-library/releases/tag/v7.0.0)

That's why I proposed to call both:

observer.disconnect()
setImmediate(() => observer.disconnect())

But if you guys think that we do not have to handle this case, we can also just replace the line.

[marcosvega91]

We can check if we are using @sheerun/mutationobserver-shim or jsdom MutationObserver and call observer.disconnect() or setImmediate(() => observer.disconnect()) ?

[romain-trotard]

Seems like the line is gonna finally be removed :D
Thank you Marco for your help

Just for your last comment, I'm interested to know how you would implement it ?
What I think:

• Add dependency for jsdom and @sheerun/mutationobserver-shim
• Get MutationObserver from both and alias them
• Know which one is our mutationObserver in the waitFor method with instanceof

Like this? Or is there another better solution?

[kentcdodds]

I don't think it's worth the complexity. People should run their code in an environment that has proper support for MutationObserver.

[romain-trotard]

Again thank you all I close the issue :)
Pr: #801