Endless loop in waitFor

[jbchr]

Hello 👋,,

we have the issue that some of our testing-library tests get stuck in an endless loop but only when they fail.

I could track down the issue to the waitFor function and to all functions that use it under the hood such as findBy*.

Furthermore, I think the issue is related to the internal usage of MutationObserver in waitFor by DTL to trigger the callback inside waitFor. But also how ATL wraps the waitFor function.

One idea i have that it might be related to this issue in DTL that suggests that you shouldn't do DOM mutations inside the waitFor but this is what can happen in the detectChanges call:
https://github.com/testing-library/angular-testing-library/blob/main/projects/testing-library/src/lib/testing-library.ts#L314

You can reproduce the issue in this simple test:

@Component({
  selector: 'atl-fixture',
  template: `
    <button [ngClass]="classes">Load</button>
  `,
})
class LoopComponent {
  get classes() {
    return {
      someClass: true,
    };
  }
}

test('does not end up in a loop', async () => {
  await render(LoopComponent);

  await waitForATL(() => {
    expect(true).toEqual(false);
  });
});

This should time out but is stuck in a loop. If I manually remove the mutationObserver code in the waitFor function from testing-library-dom the issue is gone. However, I think this issue still belongs to this library as it is angular specific.

Edit: This issue in the angular repository might also indicate what is going wrong here

Edit2: I'm pretty sure now it works like this: ATL detectChanges causes CD cycle => get classes() returns new object => DOM mutation => mutationObserver triggers => DTL triggers callback => triggers ATL detectChanges again. My solution for now is to remove MutationObserver code from DTL using patch-package as I prefer slower tests to reliable tests. We could open request to DTL to make mutationObserver optional, or maybe you have a better idea. I'm also willing to create a PR if we can come up with a feasible solution.

[timdeschryver]

Hi @jbchr , thanks for creating this issue.
I think all of your observations are correct here.

From my perspective removing the MutationObserver would be the last resort though.
It would also be hard to run that check outside of ZoneJS because that's Angular specific.
Inside the waitFor, there's a timeout that in this case never times out.
I think we should investigate this first.

I also think, that you can tweak the observer options so it doesn't end up in an infinite loop.

  await waitFor(
    () => {
      expect(true).toEqual(false);
    },
    {
      mutationObserverOptions: { childList: false, attributes: false, subtree: false, characterData: false },
    },
  );

[jbchr]

Hey @timdeschryver,

thanks a lot for your quick reply and for taking this up 🙏

I also think, that you can tweak the observer options so it doesn't end up in an infinite loop.

Yes, but I don't really like this approach for various reasons:

• It's "soft"-disabling MutationObserver, so I'd rather go with really disabling it
• One of childList, attributes, or characterData has to be set to true according to spec, so you have to pick your poison (I think attributes has probably the lowest chance to trigger it if only listens to container)
• This option can not be set globally so I either have to wrap all the functions by myself or monkey patch the library as well.

Inside the waitFor, there's a timeout that in this case never times out.
I think we should investigate this first.

While I see where you are coming from (also saw your comment on this issue) I don't think this is a good approach as the loop will be triggered anyway, which means it will fire a lot of unnecessary times, it even has the chance to grow exponentially if multiple children of the container are mutated. So if you want to go this route maybe it would also make sense to at least throttle the detectChanges calls inside the callback or have a throttle mechanism inside DTL.

[timdeschryver]

Those are valid points and concerns @jbchr , thanks for your thoughts.

After tinkering on this today, I guess it would make sense in our case to discard the mutation observer because the DOM would only change during a change detection cycle. Right?

I'm little hesitant though, because I'm afraid that some tests will take longer to run.
Where the mutation observer caught a change previously, waitFor would need to wait on the next interval before it can make the assertion with this change. I'll have to verify this though.

Another idea:
Maybe invoking the waitFor callback with a trigger (if the callback was triggered by an interval, or by the mutation observer)? That way, we could only invoke a change detection cycle when it was triggered by the interval, and not by the observer.

EDIT: After some tests, I think we need to keep the mutation observer.
Some tests will be faster with it. Some will also fail without it, for example https://github.com/testing-library/angular-testing-library/blob/main/apps/example-app/src/app/examples/13-scrolling.component.spec.ts, maybe this can be fixed...

[jbchr]

EDIT: After some tests, I think we need to keep the mutation observer.
Some tests will be faster with it. Some will also fail without it, for example https://github.com/testing-library/angular-testing-library/blob/main/apps/example-app/src/app/examples/13-scrolling.component.spec.ts, maybe this can be fixed...

Ah yes, seems waitForElementToBeRemoved to rely on mutationObserver maybe it can be fixed. But yes, that some tests can be slower seems the case, you could increase polling interval which would still be better than having this loops.

Maybe invoking the waitFor callback with a trigger (if the callback was triggered by an interval, or by the mutation observer)? That way, we could only invoke a change detection cycle when it was triggered by the interval, and not by the observer.

That seems like a good idea 👌! Only has to minor downsides:

• requires some changes in DTL to pass around the parameter (which I hope would be accepted by them)
• callback will be invoked twice for each cylce (one initially and one when mutationObserver triggers it)
  but I think that seems to be best solution for now

[github-actions]

🎉 This issue has been resolved in version 10.8.3 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[jbchr]

@timdeschryver thanks a lot for the fix 🙌 But as a heads up I had to add setImmediate polyfill to our setup, as it seems not to be part of jest-environment-jsdom anymore in v27

[timdeschryver]

@jbchr thanks! I only tried jest 26 😅.
I think this is just a temporary fix until something better comes up.

So, if I understand correctly you're using Jest 27 for your angular tests without troubles? That's good to know, because I had a negative when it just got released.