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.

[nickmccurdy]

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?

[nickmccurdy]

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!

[nickmccurdy]

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.

[nickmccurdy]

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 https://github.com/testing-library/dom-testing-library/issues/698#issuecomment-658714611), 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.

[nickmccurdy]

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).

[alexkrolick]

I would definitely try item 3 from @eps1lon's checklist for this particular case - the visibility calculation is expensive in large DOM trees

getByRole(role, { hidden: true })

[kentcdodds]

Thanks for the input here everyone. I think some docs here would go a long way.

@herecydev once you've tried and had success with one of these recommendations, would you mind working on improvement the documentation to help others who may face this in the future?

[eps1lon]

I would definitely try item 3 from @eps1lon's checklist for this particular case - the visibility calculation is expensive in large DOM trees

To touch on another potential optimization point: In Material-UI we set the default value to true (fast option) in local development and to false (slow) in CI. The reason being that you rarely accidentally query for inaccessible elements. If you do, it should be obvious in the test which we'll check during code review (since an explicit hidden option stands out). In 1015 ByRole calls we only ever need to explicitly specify the hidden state in 25 cases. So in the vast majority of cases we just check the hidden state for confidence not because the test is actually about the hidden state.

The other part is that CI can be a little bit slower (since it already takes a couple of minutes). However, for local development you want snappy feedback so every millisecond counts.

That might be worth documenting. Though we shouldn't integrate that into the library since we don't know whether people have CI or if every commit passes through it.

[herecydev]

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.

Thanks Sebastian, to address these;

1. On the latest version
2. Can you elaborate on this one, we're using RTL with jest (and JSDOM). Is there a way to configure other environments, if so what would you recommend?
3. We're already setting this to true globally
4. I've already mentioned that I'm getting mixed messages with this one. @kentcdodds has clearly called this out as a common mistake: https://kentcdodds.com/blog/common-mistakes-with-react-testing-library#using-the-wrong-query
5. Similar to above, I want to look for a link that also has some text. Is this not preferable to combine into a single query

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 }).

I just want to stress this is nothing to do with timers. Everything is mocked. We're using MSW here, but the *ByRole query is taking longer than the waitFor and so waitFor times out.

[alexkrolick]

Can you elaborate on this one, we're using RTL with jest (and JSDOM). Is there a way to configure other environments, if so what would you recommend?

ie Puppeteer, Cypress

How big is the DOM tree being queried in this test?

[meastes]

I was seeing similar performance issues in one of my tests as well. My app is using Material UI and I was seeing about 6 seconds per query with getByRole() making my total test time about 20 seconds. After switching the queries to getByLabelText() the test time went down to 600ms - HUGE difference. The component under test is a Drawer with about 50 checkboxes in it.

I agree that a small performance cost is worth the confidence, but it becomes unusable when that cost is about 6 seconds. I definitely agree in having some documentation updates to give visibility into this tradeoff.

[eps1lon]

I just want to stress this is nothing to do with timers. Everything is mocked.

Why are you using waitFor if everything is mocked? If everything is mocked a single jest.runAllTimers should be faster than waitFor. Could you run npx testing-library-envinfo react and share the results?

Can you elaborate on this one, we're using RTL with jest (and JSDOM). Is there a way to configure other environments, if so what would you recommend?

We're using mocha + karma with chrome headless and browserstack. Bundling the full test suite with webpack and running is as fast as running it with JSDOM. So there's a breakpoint where mocha + karma becomes faster if you get bundle time low enough. We don't have as severe problems with JSDOM so I suspect that this might be a real time saver for you.

However, I don't think it's appropriate for local development where you just run a subset of the tests.

I was seeing about 6 seconds per query with getByRole()

6 full seconds for each call of getByRole? Could you run npx testing-library-envinfo react and share the results? If you could share a repro I might be able to understand the issue. Having a repro for these problems is important since that's the only way we can meaningfully work on the performance. Make sure you

[shan-du]

We are having similar performance issues with getByRole. I did some comparison fetching the exact same element using getByText usually takes around 200~300ms, but with getByRole it takes upwards of 40 seconds!!!

I know that both of these durations are a bit long anyways due to the DOM we are testing, but the point here is that relative to each other, getByRole is significantly slower, which is just ridiculous.

In the end I had to give the selectors a container scope like container.getByRole instead of screen.getByRole, then the amount of time reduces drastically, but in comparison, it is still a lot slower than getByText.

I'm really not sure what the reason is and perhaps it should really be mentioned in the documentations.

[herecydev]

@eps1lon sorry I can't be of much help now, this issue was quite painful and we move over to using cypress where this isn't a problem for us, as well as providing a suite of other benefits.

[eps1lon]

I did some comparison fetching the exact same element using getByText usually takes around 200~300ms, but with getByRole it takes upwards of 40 seconds!!!

@shan-du Please share a reproducible example. Otherwise we can't help you with this problem.

[alexkrolick]

I wonder if it has something to do with the MutationObserver optimizations in waitFor hogging too much of the thread for the more expensive getByRole DOM traversal

[meastes]

Here's a reproducible example. It's not quite as bad as my original project (about 5 seconds per getByRole call in the example instead of 6 seconds), but still shows that there's a huge performance issue. Hopefully this is helpful in debugging the issue!

https://github.com/meastes/get-by-role-example

 PASS  src/components/DrawerExample.test.tsx (22.395 s)
  DrawerExample
    selecting
      ✓ defaults checkboxes to checked when selectedColumns is specified (840 ms)
      ✓ defaults checkboxes to checked when selectedColumns is specified (getByRole) (17729 ms)

Test Suites: 1 passed, 1 total
Tests:       2 passed, 2 total
Snapshots:   0 total
Time:        23.846 s, estimated 30 s
Ran all test suites matching /DrawerExample/i.

[radfahrer]

I just ran into this and found that switching from findByRole with name: to findByText saved me over a second per test. I understand that *byRole is more specific, and thus more reliable for getting the correct element. However, as far as "confidence" goes, I'm going to advise my team to avoid findByRole. The reason I found this performance issue is because I'm working on fixing a flaky test suite. The delay introduced by findByRole is causing our test suite to time out intermittently and therefore reduces confidence in our tests.

I would highly recommend improving the performance of *byRole if possible.

[Duncan-Alexander-Coutts]

Here's a reproducible example. It's not quite as bad as my original project (about 5 seconds per getByRole call in the example instead of 6 seconds), but still shows that there's a huge performance issue. Hopefully this is helpful in debugging the issue!

https://github.com/meastes/get-by-role-example

 PASS  src/components/DrawerExample.test.tsx (22.395 s)
  DrawerExample
    selecting
      ✓ defaults checkboxes to checked when selectedColumns is specified (840 ms)
      ✓ defaults checkboxes to checked when selectedColumns is specified (getByRole) (17729 ms)

Test Suites: 1 passed, 1 total
Tests:       2 passed, 2 total
Snapshots:   0 total
Time:        23.846 s, estimated 30 s
Ran all test suites matching /DrawerExample/i.

Apologies, not adding much to the conversation but my team has also been having problems with this. I can vouch that @meastes example is similar to what my team is seeing.

[mmnoo]

Just weighing in with something that worked for me. Its hacky, I dont fully understand why it works, and I dont like it but it really sped up my test suite after the other things I tried didnt make a difference.

My integration/e2e-ish tests that rendered my whole app were the slow ones causing problems, and especially slow were test in files with many other tests in the same file. I broke up those files to only have 1-3 tests (whatever it took to get the suite to run under 30 seconds), and that did the trick. I dont know why this works, but there seems to be an issue somewhere in the testing stack when multiple tests are in the same file.