waitFor + getByRole causing severe delays

[herecydev]

• @testing-library/dom version: 7.26.6
• node version: 14.3.0

Relevant code or config:

it("Causes a big delay", async () => {
      // This fires a fetch request that is mocked using MSW (so it should be near instant)
      userEvent.click(screen.getByRole("button", { name: "Search"}));

      await waitFor(() => {
        //super fast call
        //screen.getByText("some_link_text");

        // causes massive delays
        screen.getByRole("link", { name: "some_link" });
      });
    });

What you did:

Running this test causes the fetch request promise to resolve a lot longer than usual (around 1.5 - 2s). I initially investigated this by wrapping console.time around the request.

I also had an additional test which looks very similar to the above, but instead of using getByRole it was using getByText to search for some text. That test took a fraction of the time:

What happened:

After changing getByRole to getByText or inserting a getByText query before the getByRole it resolved much faster. My working theory is that getByRole is so expensive that it was monopolising the waitFor loop and takes a long time to relinquish control to the pending promise that needs to resolve.

[nickserv]

The issue is that getByRole is fairly slow, but it's also syncronous, so waitFor keeps rerunning it and has to wait for each call to finish before other code can run. It could potentially use some performance improvements, but perhaps we could make it asyncronous (or add an alternative for back compatibility) so this can be run asyncronously.

[herecydev]

Thanks for the quick reply Nick, does that not suggest that any waitFor that is sufficiently slow would trigger this? If so, is waitFor problematic in it's design?

What do you think the best short term resolution for this?

[nickserv]

I'm guessing waitFor may be so eager to quickly rerun longer functions that it might actually be slowed down. You could try increasing the interval option, it defaults to only 50 milliseconds.

[herecydev]

For reference the getByRole was taking 400-500ms and would cause the waitFor to loop 3-4 times. The best fix (for time) is to insert a more trivial query before that (like a getByText) as that flushes that promise much faster.

Would you like me to document some of this? Feels like a sufficiently frustrating morning that I don't want others to go through!

[nickserv]

A higher interval might cause less loops. Even if it doesn't make this specific test run shorter, a longer interval could allow other tests to run in parallel more effectively. Can you try this out?

Alternatively, we could dynamically adjust the interval by default based on how long it takes each time. I'm not sure if that's worth the effort though.

[herecydev]

Setting an interval of 750 still caused 2 iterations to fail (~450ms), then the fetch promise resolves. There's one more failure (output still not rendered to the screen), finally it passes.

Setting an interval underneath the getByRole time causes it to fail as expected. I.e. setting it to 200ms will cause 3 iterations and then the waitFor will just give up (total time is 1s by default if I remember).

I guess it works, but you keep having to up this arbitrary interval to "match" however long the waitFor will take as the complexity grows. That doesn't feel great to me.

[nickserv]

Yea, I was thinking maybe waitFor could intelligently learn how long previous calls took and use that to adjust the interval. I'm not sure how well it would work in practice though. On the other hand, you may not need *byRole (see #698 (comment)), though PRs to improve performance would be welcome.

[herecydev]

The problem is when the knowledge has been propogated differently;

https://kentcdodds.com/blog/common-mistakes-with-react-testing-library#using-the-wrong-query
https://testing-library.com/docs/guide-which-query/#priority

There's absolutely nothing in the docs about the downsides of ByRole, and previously I've been happy to accept the small performance hit for the better testing methodology. But this issue is straight out problematic now.

I would love to get an action on how best proceed with this, do we:

1. auto-increment waitFor intelligently
2. Change inner workings of ByRole to be asynchronous
3. Provide documentation around this known issue with suitable guidance

etc.

[nickserv]

1 would be cool to experiment with but as I said previously, I'm not sure how difficult to implement and useful it would be, so it may not be worth it. I'm fine with either 2 or 3.

@kentcdodds @eps1lon What do you think about an asyncronous version of *byRole (2) to work around performance issues like this?

[kentcdodds]

One question to make sure I understand. What is your test waiting on that takes more than a single 50ms interval? Most/All tests using this utility should mock out any async code so it resolves on the next tick or two of the event loop 🤔

[kentcdodds]

Just noticed the screenshot, sorry about that. Looks like it's getting mocked by MSW, so I wouldn't expect this to take longer than a few milliseconds. Something is up there.

The *ByRole query is definitely the performance hog of the bunch, but it's a cost with the benefit of confidence which is why it's the top most recommended query. I suppose we could add a note to the docs warning about this. Probably a good idea. I'd love to make that query faster, but I still recommend it.

In this situation, there's something especially fishy going on. I think we'd need a reproduction to dive any deeper though.

[herecydev]

@kentcdodds yer to confirm MSW is registering is ~1ms so it's very fast. It's the *ByRole that's exceeding the waitFor interval window.

Reproduction will take some time, is it not clear that exceeding the waitFor interval window will cause this?

[eps1lon]

If you want to optimize tests for performance go through these steps until you're satisfied. They're ordered by their confidence impact (lower impact, higher ranked) then perf impact (higher impact, higher ranked).

1. Make sure you use the latest version of jsdom
2. don't use JSDOM but modern browsers
3. getByRole(role, { hidden: true }) (WARNING: includes inaccessible elements - avoids expensive visibility check)
4. getByTestId instead of getByRole
5. don't filter by name in getByRole (Can rely on order in getAllByRole. Order of elements in the DOM is in most cases not an implementation detail.)

Not using jsdom at all is the best solution since it has better perf and higher confidence.

[kentcdodds]

@eps1lon great walkthrough 👍 though I think I'd prefer to try another query before testIDs. I think it makes sense to add this to the docs.

[eps1lon]

though I think I'd prefer to try another query before testIDs. I think it makes sense to add this to the docs.

Definitely! It was easier to say since "use another query" might not be that helpful. If you're really blocked by perf problems I would just suggest a quick escape hatch and revisit it once the test breaks on unrelated changes (though that might be even harder to judge). It's just really hard to give actionable advise that people can apply to different scenarios.

I think the most generic advise is to re-evaluate your waitFor usage. If you're in a unit-test-like environment you should be able to mock the timers so that you can deterministically forward to the point where your assertion should be true (skipping the wasteful polling approach). In e2e tests (with real timers/requests) I'd like to see a reproduction since I would expect a browser to not have perf problems with getByRole(role, { name }).

And to give an optimistic outlook: The ByRole implementation is just a polyfill at the moment. Long term we'll use actual DOM APIs that are hopefully faster (see https://wicg.github.io/aom/spec/#computed-accessibility-tree).