infinite loop when DOM mutation happens in waitFor callback

[drudv]

Jest hangs on the following sample test:

const { waitFor } = require("@testing-library/dom");

describe("test", () => {
  it("test", async () => {
    let value = 1;

    setTimeout(() => {
      value = 2;
    });

    await waitFor(() => {
      // both these lines are important: it's necessary to do a mutation and to fail in the first iteration
      // It works normally, if we comment one of these lines
      document.body.setAttribute("data-something", 'whatever');
      expect(value).toEqual(2);
    });

    console.log("execution never comes here");
  });
});

Two conditions have to be met:

• there should be a DOM mutation in waitFor callback
• first execution of waitFor callback should fail

Expected result:
waitFor should resolve the promise after a successful iteration regardless whether there were DOM mutations or not

Current result:
waitFor calls the callback infinitely even if subsequent iterations don't throw any exception.

Environment:
@testing-library/dom@7.26.4
jest@26.6.2

[kentcdodds]

Thanks for this @drudv. Perhaps this is related to the change in #801. Cc @romain-trotard

[romain-trotard]

Hi guys.
I have tested before the release and it doesn't work too.
I wonder if waitFor is made for making mutation. Or just assert things.
Because making the mutation outside of the waitFor works well. The following code works:

describe("test", () => {
    it("test", async () => {
        let value = 1;

        setTimeout(() => {
            value = 2;
        });

        setTimeout(() => {
            document.body.setAttribute("data-something", 'whatever');
        }, 500);

        await waitFor(() => {
            expect(value).toEqual(2);
        });

        console.log("execution never comes here");
    });
});

(The waitFor fails the first time and pass the second one)
In my projects, I just used waitFor for waitings things to appeared (just expect), that I can't do with find* queries. What do you think @kentcdodds ?

[drudv]

Maybe I should provide some details on how we came to this problem. There was the following code in our tests:

await wait(() => {
    const submitButton = getByTestId(...);
    expect(submitButton.disabled).toBeFalse();
    fireEvent.click(submitButton);
});

At the first glance it doesn't do any DOM mutations. But there was a listener for click events that cause changes in the DOM. Since we were using Jest 24 with MutationObserver shim, the problem occurred only sometimes and led to failures in waits of other tests in the module. We were trying to catch it for few months because we were not able to stably reproduce it and most of the time it was failing in CI. After switching to Jest 26 with the latest JSDOM which supports MutationObserver it always fails in that block, so we were able to identity the problem and fix it with:

const submitButton = await waitFor(() => {
    const submitButton = getByTestId(...);
    expect(submitButton.disabled).toBeFalse();
    return submitButton;
});
fireEvent.click(submitButton);

However, it's strange that such innocent code (although I agree it's not great that it produces DOM mutations as a side effect) hangs the whole Jest.

[kentcdodds]

Oh, yeah doing mutations in waitFor was never a good idea. We should document this better, but we're not going to put any effort into supporting this. The number of times your callback is called is non-deterministic. Don't put interactions in that callback.

[drudv]

I agree that it's better not to put interactions and we should mention it in the docs. But IMO waitFor needs to ensure that it will stop checking results after the provided timeout. In this case the timeout is being ignored for the moment. Moreover it blocks the whole Jest to finish and it's hard to figure out where the problem is.

Maybe I will try to go deeper and prepare some solution for the problem later.

[kentcdodds]

That makes sense to me @drudv. This definitely sounds like a bug 👍 Thanks for any assistance you can offer!

[marcosvega91]

I don't know if we can do something for this, I think that we can only update the doc. The problem is that mutating the dom inside the callback will cause an infinite recursive check. When the attribute is set MutationObserver will call again the callback causing the issue. I have created this small repo using only MutationObserver

https://repl.it/repls/DrearyIroncladRegression

[marcosvega91]

The same behavior is also happening in the browser https://jsfiddle.net/g3sdnkyu/, so I'm pretty sure that we can also add a note in the doc

[nickserv]

Could we possibly add a warning that detects a loop, or should this only be a documentation change?

[marcosvega91]

I don't how we can detects this loop. We can in someway make it works wrapping the callback in a setTimeout or a Promise.resolve. I don't have in mind a better solution

[kentcdodds]

I wonder what impact putting the callback in the next tick of the event loop would have on perf 🤔 @romain-trotard's investigation showed that removing a simple setImmediate made a significant impact in performance 🧐

[romain-trotard]

Personnaly, I would prefer to just update the documentation. And clarify that we should not do mutation or interaction in waitFor.

[kentcdodds]

I'd love to have the perf improvement and help people avoid making this mistake if possible.

Either way, I think it's safe to document this right now.

[SerkanSipahi]

I do not really understand how await waitFor(() => false); console.log('Foo') work. In my case waitFor waits for nothing. No matter if the result of the callback is true or false, the promise (waitFor) will always be resolved and Foo is logged. Have I understood something wrong here?

some env infos:

• real browser with karma + jasmine.
• angular extension of the testing-library.
  -- waitFor is re-exported from testing-library. Therefore the issue should be reported here 👍

[ryuuji3]

The very first step of waitFor with or without fake timers is to check the callback. (https://github.com/testing-library/dom-testing-library/blob/master/src/wait-for.js#L56 or https://github.com/testing-library/dom-testing-library/blob/master/src/wait-for.js#L96)
waitFor doesn't consider booleans, its either callback throws (try again) or doesn't throw (success). (https://github.com/testing-library/dom-testing-library/blob/master/src/wait-for.js#L146) If you need to verify against boolean then your callback should use toBeTruthy()
eg.

await waitFor(() => expect(condition).toBeTruthy())

[SerkanSipahi]

@ryuuji3 Thanks. At first sight it was not clear to me that an exception has to be thrown for it to work. I was overwhelmed by the documentation and could not find the information you describe immediately. Later I found out by debugging it myself. Unfortunately I hadn't thought to report this here anymore.

[timdeschryver]

Because ATL invokes an Angular detection cycle within the waitFor callback, we might end up in an infinite loop because this leads to a mutation of the DOM in some cases (testing-library/angular-testing-library#230).

Would it be an option to keep track of the execution time inside waitFor ourselves (when fake timers aren't used) in this case. If the timeout limit is reached, we could then throw an error. This won't be as precise as the current setTimeout, but it will provide a fallback to prevent infinite loops in cases when DOM mutations are happening inside the waitFor callback.

If we want to add this to DTL, I can create a PR for this and we can take it from there.
I tried this implementation before creating this comment, and it doesn't affect the current tests.

[timdeschryver]

Another possibility would be to send the trigger (interval timer or observer) to the callback.
This way consumers could use the trigger to branch off their logic.

For ATL specific, we can ignore an Angular change detection cycle when the callback is invoked by the observer.