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

waitForElement can leave its MutationObserver running for all future tests #357

Closed
RoystonS opened this issue Sep 21, 2019 · 3 comments
Closed

Comments

@RoystonS
Copy link
Contributor

  • DOM Testing Library version: 5.5.0
  • node version: 10.16.0
  • npm (or yarn) version: yarn 1.16.0

Relevant code or config:

  async function waitForListToBeReady(id: number) {
      console.log('getCancelButton', id);
      await waitForElement(() => {
        console.log('running waitForElement predicate', id);
        return getRowCount() === 44;
      });
      console.log('waitForElement', id, 'completed');
    }

    bdd.it('runs some stuff in test1', async () => {
      doSomeSetup();
      await waitForListToBeReady(1);
      expect(something).to.be.true;
    });
    bdd.it('runs some stuff in test2', async () => {
      doSomeSetup();
      await waitForListToBeReady(2);
      expect(something).to.be.true;
    });
    bdd.it('runs some stuff in test3', async () => {
      doSomeSetup();
      await waitForListToBeReady(3);
      expect(something).to.be.true;
    });
  });

What you did:

Used waitForElement in one test, with the Intern testing framework.

What happened:

Every test that calls waitForElement causes another DOM MutationObserver to be created, and not cleaned up, and every DOM change causes all prior waitForElement predicates to be re-evaluated. The tests get slower and slower.

The output:

getCancelButton 1
running waitForElement predicate 1
waitForElement 1 completed
running waitForElement predicate 1
getCancelButton 2
running waitForElement predicate 2
running waitForElement predicate 1
waitForElement 2 completed
running waitForElement predicate 1
running waitForElement predicate 2
getCancelButton 3
running waitForElement predicate 3
running waitForElement predicate 1
running waitForElement predicate 2
waitForElement 3 completed
running waitForElement predicate 1
running waitForElement predicate 2
running waitForElement predicate 3
running waitForElement predicate 1
running waitForElement predicate 2
running waitForElement predicate 3
[repeat a couple of hundred times as the test framework writes its output to the DOM]

This shows that, despite the fact that waitForElement has completed, it's still listening for DOM changes and still re-evaluating its predicate.

Reproduction:

See above. A full repro would require setting up an Intern deployment, which I can do, but it'll take a little longer.

Problem description:

The tests get slower and slower as the number of DOM MutationObservers accumulates, especially as the existing test predicates start to cause exception throws.

Suggested solution:

I'm not sure. Looking at the code, it looks like waitForElement uses setImmediate to queue up the .disconnect call of the MutationObserver, but it looks like, as my tests run, nothing ever yields sufficiently to allow this callback to run, so the .disconnect call gets pushed back until all the tests complete. (I'm running on Chrome, which doesn't have native setImmediate, so DTL is using a polyfill which uses postMessage to simulate the effect.)

Removing the setImmediate call would cause the MutationObserver to be shut down synchronously, and that would fix this issue, but I don't know why the setImmediate call is there: presumably somebody added it for a good reason?

@kentcdodds
Copy link
Member

Hi @RoystonS,

Well, I wasn't sure why it was needed either, but I think there's probably something we can do to solve #99 (which is why it was added) and your issue as well.

I'm not certain of the best way to go about this. Maybe if we use setTimeout instead?

If you could prototype something that works for your situation and then make a PR for that, we'll give it a look.

@kentcdodds kentcdodds added needs discussion We need to discuss this to come up with a good solution needs investigation and removed needs discussion We need to discuss this to come up with a good solution labels Jan 21, 2020
@kentcdodds
Copy link
Member

I think this is solved in beta. Would you mind giving it a try?

npm install @testing-library/dom@beta
# npm install @testing-library/react@beta

Thanks!

kentcdodds pushed a commit that referenced this issue Mar 4, 2020
Closes #413
Closes #357

BREAKING CHANGE: MutationObserver is supported by all major browsers and recent versions of JSDOM. If you need, you can create your own shim (using @sheerun/mutationobserver-shim) and attach it to the window.
kentcdodds pushed a commit that referenced this issue Mar 4, 2020
Closes #413
Closes #357

BREAKING CHANGE: MutationObserver is supported by all major browsers and recent versions of JSDOM. If you need, you can create your own shim (using @sheerun/mutationobserver-shim) and attach it to the window.
kentcdodds pushed a commit that referenced this issue Mar 12, 2020
Closes #413
Closes #357

BREAKING CHANGE: MutationObserver is supported by all major browsers and recent versions of JSDOM. If you need, you can create your own shim (using @sheerun/mutationobserver-shim) and attach it to the window.
@kentcdodds
Copy link
Member

🎉 This issue has been resolved in version 7.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

2 participants