dom-testing-library: waitFor + getByRole causing severe delays

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.

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.

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.

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.

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.

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.

Did somebody tried with Happy DOM instead of jsdom to see if it helps?

I tried: https://github.com/tkrotoff/getByRolePerfIssue

• With jsdom:

 PASS  src/Form.test.tsx
  ✓ getByText() (473 ms)
  ✓ getByRole() (1454 ms)

• With Happy DOM:

 PASS  src/Form.test.tsx
  ✓ getByText() (161 ms)
  ✓ getByRole() (150 ms)

I can confirm that #1068 is a meaningful improvement. Pasting my comment from that thread:

I tried specifically adding a dep on @testing-library/dom to bump us to 8.11, and ran our longest UI test file, an “integration”-style test using next-page-tester.

Before:

 PASS  client/features/programDetails/characteristicsTab/tests/Characteristics.tests.tsx (107.118s)
  ProgramCharacteristicsContent
    √ Renders and allows editing Program Characteristics (98432ms)

Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        109.2s, estimated 124s

After:

 PASS  client/features/programDetails/characteristicsTab/tests/Characteristics.tests.tsx (90.958s)
  ProgramCharacteristicsContent
    √ Renders and allows editing Program Characteristics (82187ms)

Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        93.06s, estimated 106s

No other code changes.

So, this distinctly made some improvement.

(I still don’t think the test file should be taking anywhere this long to begin with, but I haven’t had time to really drill down and see what else is taking it so long.)

getByRole and getByText have entirely different meanings and different purposes so comparing between them is an unfair fight, as I mentioned above, ByRole has way more things to do. Unfortunately, I do not have the capacity at the moment to try and think of a way to improve the performance here, but it’s merely because we have a lot of DOM work to do. Visibility check might sound easy, but it requires calling getComputedStyle, which calculates the stylesheet for the DOM element, and for every parent of the element because visibility is inherited. There’s a lot to think about here. I’m not saying that there’s no way to improve it, I’m just saying that it’s not an easy task as one might think 😃

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

Hi @ahayes91! I don’t think this is something we’ll want to prevent (or advocate people to avoid). Using *ByRole is one of the best practices to see that our elements have the accessible role we need to make the app accessible. If we do have performance issues in *ByRole we should tackle them 😃 Thanks for the offer!

Had to close #1031 as I don’t have the patience. Hopefully someone else can get it done.

We’ve used vitetest before

While it does help test boot up, if the problem is Jsdom, it doesn’t provide speed ups to slow queries.

As for happy Dom, it’s too incomplete to use in production. I really wish you guys would stop recommending it 😜

On Nov 6, 2023 11:01 AM, Matan Borenkraout @.***> wrote:

@frankandrobothttps://github.com/frankandrobot, you’re not, and we’re 100% open for criticism. I’m not saying it is what it is, we are trying to workaround those, having said that, the major bottleneck here is usually JSDOM. Regarding your specific question:

1. If you don’t care about visibility check for example, you can use hiddenhttps://testing-library.com/docs/queries/byrole#hidden for example to avoid that check and get a tiny speedup. The rest is probably too related to the role selection and we can’t remove it.
2. I think that not using ByRole misses a bit of the accessibility checks and misses the point of our guiding principle.
3. I highly recommend trying vitest instead of jest and also happy-dom instead of jsdom for some quick wins 😃 Feel free to ask more questions, I’ll do my best to help.

— Reply to this email directly, view it on GitHubhttps://github.com/testing-library/dom-testing-library/issues/820#issuecomment-1795499954, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAGAGMNJ4OATI5N6BVALD2DYDEJXFAVCNFSM4TUQYKQKU5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCNZZGU2DSOJZGU2A. You are receiving this because you were mentioned.Message ID: @.***>

Looking for some opinions. Is the byRole query as vital when using a UI library like MUI, who pride themselves on having fully accessible components? We’re really starting to see significant performance issues within our tests and I have always promoted the use of byRole to ensure our code is accessibility friendly. However, if we can “trust” that these checks are being done for the most part by MUI, we may look into using byText more regularly in tests where performance is hit the most.

Is there a reason to have it scheduled for 10 ms? Could we not do this and achieve the same goal just faster?

I’m not so sure, consider this very common case:

const response = await fetch('api');
const json = await response.json();

the timeline would look something like this:

• fetch() is called in the main thread, it is awaited so the main thread is relinquished
• setTimeout immediately completes and queues its task
• fetch() resolves and queues its task
• the main thread is done
• the setTimeout task runs
• the fetch() task runs
• response.json() is called, main thread is relinquished again
• setTimeout immediately completes and queues its task
• response.json() resolves and queues its task
• the main thread is done
• the setTimeout task runs
• the response.json() task runs
• we finally get to render what we wanted

Some async operations depend on multiple tasks to run to completion to get what we want, but we run too much code in between.

Something similar actually happened to us with react-focus-lock, we had many userEvent.tab() before a waitFor, but react-focus-lock had many slow async tasks queued that we couldn’t get past response.json() in time.

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.

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

Update:

• @MatanBobi , even with happy-dom we’re still seeing huge speed differences between getByRole and getByText. 200ms vs 2sec!
• and on our CI environment, the difference becomes even worse, 1s vs 11 s!
• so definitely we still need a workaround

I’ve spent the better part of a day taking a look at why getByRole is much slower. In our case, the slowdown is actually coming from dom-accessibility-api, currently used here:

https://github.com/testing-library/dom-testing-library/blob/84bf52f1b2b30f34e3a709e5bdd32e14623d6f60/src/queries/role.js#L150

If the textContent is pulled out of the node instead of using dom-accessibility-api to compute the accessible name, performance increases drastically.

Test time while using computeAccessibleName

should show all keys in the layout (3665 ms)

Test time while comparing element.textContent

should show all keys in the layout (300 ms)

In my test, that speeds my test up by 12 times!

Why? I’m not too sure, but it may have something to do with jsdom’s implementation of getComputedStyle, which historically has had poor performance. dom-accessibility-api’s computeAccessibleName function eventually calls window.getComputedStyle. I found an old bug with jsdom which mentioned that they had poor performance and the ticket was closed after a bit of finger-pointing. This was 5 years ago though, so it may not be relevant.

---

@kentcdodds - Would you support adding another option to the *ByRole selector which allows a user to specify that they are looking for text directly? This would allow the selector to bypass dom-accessibility-api for instances where a user knows the name that they are looking for. I’m happy to implement.

While a catch-all selector is nice to have, there are some serious performance gains to be had here while still maintaining confidence.

// Proposed use
getByRole('button', { text: 'Exact text match' });

// Proposed pseudo-code change
// https://github.com/testing-library/dom-testing-library/blob/84bf52f1b2b30f34e3a709e5bdd32e14623d6f60/src/queries/role.js#L150
    .filter(element => {
      if(options.text !== undefined) {
          return element.textContent === options.text
      }

      if (name === undefined) {
        // Don't care
        return true
      }

      return matches(
        computeAccessibleName(element, {
          computedStyleSupportsPseudoElements:
            getConfig().computedStyleSupportsPseudoElements,
        }),
        element,
        name,
        text => text,
      )
    })

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

I have used vitest and happy-dom for production use as a replacement to jest + jsdom or else I wouldn’t have recommended it. I can understand why some cases are still not supported, but you can definitely migrate some tests and run them side by side.

@cmacdonnacha, @mmnoo I gave a talk to explain what’s happening in ByRole query to understand what’s happening as part of it and understand why it may still have a purpose. I recommend giving it a look (though I’m a bit biased): https://www.youtube.com/watch?v=jNAMdsbdvlIs&ab_channel=ReactNext

It doesn’t account for implicit roles which is widely used.

For my org that’s likely the deal breaker. If we can make that work, this simpler version should be good enough. (2 can be done easily with a visibility check)

Yes, we’ve also tried happy-dom. In the past it just hasn’t been ready for prime time.

However, this morning, I just tried it on some complex tests that previously did not work with happy-dom. And i’m happy to report that all tests passed with the exception of an act warning

So i think definitely, we’re ready to start re-evaluating happy-dom again

Update:

• ran happy-dom on even more tests and are still seeing red on tests that work with jsdom… that is, failing tests due to an incomplete feature set
• plus see my comment below.
• conclusion: happy-dom is by no means the solution to this issue

Did somebody tried with Happy DOM instead of jsdom to see if it helps?

@MatanBobi true. You’re absolutely right. Thanks and sorry 😅

Btw, I dropped a message in your DMs on TW.

@berTrindade I’d love to answer this in details but I don’t think it should be a part of this issue. Can you please either open a stackoverflow question and link to it here or reach out on our Discord server or my twitter account and I’d love help 😃

TL;DR the test code is blocking the app code because it’s all on the same thread.

Another benchmark:

console.time('check');
let cancelButton = await screen.findByRole('button', { name: 'Cancel' });
console.timeEnd('check');

Result: check: 2.586s

console.time('check');
let cancelButton;
await waitFor(async () => {
  await new Promise((resolve) => {
    setTimeout(resolve, 100);
  });
  cancelButton = screen.queryByRole('button', { name: 'Cancel' });
  expect(cancelButton).toBeInTheDocument();
});
console.timeEnd('check');

Result: check: 1.096s

So yeah, delaying things is ~2x faster 🤔

We definitely could switch all of our *ByRole queries to *ByText queries, but it’d be a shame to do that. It would lower our confidence in our tests as the two do not query for the same thing, and we could have accidental a11y breaking changes (e.g. a junior dev switching a button to a div + onclick).

I do see the hesitancy to add any sort of non-end-user-focused options to such a user-focused query, but the performance hit here with jsdom is pretty substantial. Faster tests encourage more tests!

I did see that there is a helper function to get node text. I’d change my recommendation to getByRole('button', {nodeText: 'Hyperdrive!'}) to make it clear what is being selected.

Thanks for the thorough analysis @stabback!

I could be convinced of this, but I don’t know dom-accessibility-api at all (that’s @eps1lon’s domain) so maybe there’s something else that could happen in there to reduce the compromises that need to made here. The issue is that you wouldn’t be able to get a textbox with text because it doesn’t have a .textContent, it has a label which has a text content. So adding a second way to specify the text you’re looking for could lead to confusion.

I think that hidden: true (also globally configurable) as described above side-steps the computedStyle stuff which should speed things up and I think I’d prefer recommending that to the added confusion of an additional way to determine the text we’re looking for…

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?

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.

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.