Dom-testing-library: getByRole("progressbar") is pretty slow

[ValentinH]

• React Testing Library version: 10.0.4 (I'm using /react not /dom but I'm posting here as getByRole is implemented here)
• node version: 🤷 (codesandbox)
• npm (or yarn) version: 🤷 (codesandbox)

I've noticed that some of my tests were pretty slow to run (more than 2 seconds). After investigating, I've noticed that the following pattern was to source of the slowness:

await waitForElementToBeRemoved(() => screen.getByRole('progressbar'))

If I add a data-testid to my loader and switch to:

await waitForElementToBeRemoved(() => screen.getByTestId('loader'))

it's much faster.

Relevant code or config:

I'm sometimes displayed a CircularProgress from material-ui while fetching some data and I use this pattern to wait for it to be removed.

await waitForElementToBeRemoved(() => screen.getByRole('progressbar'))

example of implementation

if(isLoading) {
  return <CircularProgress className={classes.loader} data-testid="loader" />
}

What happened:

Here are 2 tests outputs where I only change the pattern mentioned above.

getByRole

getByTestId:

Reproduction:

I've create the following Sandbox: https://codesandbox.io/s/musing-gauss-e1ynf?file=/src/Demo.js

The numbers are much smaller but when running the tests several times we can see that the getByRole is slower when used on the CircularProgress.

Problem description:

I think that

await waitForElementToBeRemoved(() => screen.getByRole('progressbar'))

is the best pattern to test that a loader is displayed on screen. However, the slowness prevents us from using in all our tests as it quickly gets slow.

[eps1lon]

getByRole can get quite expensive in large trees since it currently checks every element while ByTestId is a simple querySelector. It also includes a lot more a11y related checks. For example it won't return inaccessible elements.

Since you actually remove the element from the DOM you can ignore the additional a11y tree checks and go with getByRole('progressbar', { hidden: false }).

On slow machines these additional checks can starve your CPU and you should increase the polling interval with waitForElementToBeRemoved(() => {}, { interval: 200 }) (try out which interval works best).

And last but not least: What version of jest or, more specifically, jsdom are you using? In later jsdom versions (or actual browsers) getByRole should be a lot faster.

For us: I think we should make the perf impact of hidden in ByRole clearer in the docs. Also mention too low polling rate on slow machines.

[ValentinH]

Thanks for the quick reply!

I've just tested the { hidden: false } and it doesn't have an impact in my example unfortunately. 😞
The thing is that in the sandbox example, the tree is not so big. What I don't get, it the difference when using CircularProgress and directly using its output div + svg.

I'm using version 25.2.7 of Jest.

[eps1lon]

I've just tested the { hidden: false } and it doesn't have an impact in my example unfortunately.

Have you tried increasing the interval? Would be helpful if you could provide a full repro. Otherwise it's unclear what makes this test so slow.

What I don't get, it the difference when using CircularProgress and directly using its output div + svg.

It's unclear where this difference is coming from. Could just be the CircularProgress doing more things and slowing the test down because of it.

[ValentinH]

Increasing the interval didn't help either.

I'm preparing a demo with my real use case :)

[ValentinH]

I've updated the demo with a more complex form and here's the result:

Sandbox: https://codesandbox.io/s/musing-gauss-e1ynf?file=/src/DemoForm.js

I've removed the CircularProgress so that we don't focus on it.

[eps1lon]

Great. I think this is somewhat expected with the current implementation. ByTestId takes 130ms while ByRole takes 200ms. This is 50% slower.

Might be nice to experiment with different implementations that don't loop over all nodes but convert the role directly to a selector and use that directly.

[ValentinH]

I've dug in the (transpiled) code and try to see where it was spending the most time. Surprisingly, this is not in queryAllByRole, but at this line: https://github.com/testing-library/dom-testing-library/blob/ca1b60895599351b38a6e8e6b5ca1ee4e3db481c/src/query-helpers.js#L62

Especially, the call to getMissingError(). Before this call every operations takes a few milliseconds. getMissingError takes almost 1 second on my example!

[ValentinH]

Digging even more, I'm landing on this line: https://github.com/testing-library/dom-testing-library/blob/master/src/queries/role.js#L116

However, I'm using 7.3.0 and seeing that you changed the implementation in 7.4.0. Thus, I need to check if it has changed the behaviour. 🙂

[ValentinH]

Still the same behavior on 7.5.1.

I'm now checking this line: https://github.com/testing-library/dom-testing-library/blob/429de04d023f2c49af2cb7b10d6daa537d417999/src/role-helpers.js#L134

isInaccessible gets called 30 times when getByRole rejects and each call last around 30 ms.

[ValentinH]

Funnily, if I do getByRole('progressbar', { hidden: true }) it solves my issue as it does not call isInaccessible. 😄

[kentcdodds]

Hi @ValentinH! Thanks for all this digging.

I believe that isInaccessible has historically been some bottleneck code. I'm not sure of the best way to speed it up, but if anyone has ideas that would be pretty sweet :)

[ValentinH]

Yes, I'm trying to see what part of it is the slowest part :)

[ValentinH]

Not a big surprise, but most of the time is spent on

window.getComputedStyle(element).display

[ValentinH]

Using the same caching mechanism than in https://github.com/testing-library/dom-testing-library/blob/752ff667d52efd452273232a6a48319a207c66c2/src/queries/role.js#L42-L49 makes my tests almost 2 times faster.

They are still 5-6 times slower than getByTestId though.

One idea I have is: do we really need to build the whole "role" error message when using waitElementToBeRemoved() ? Indeed, we are only checking the error name in it: https://github.com/testing-library/dom-testing-library/blob/752ff667d52efd452273232a6a48319a207c66c2/src/wait-for-element-to-be-removed.js#L36

[ValentinH]

I could make it even faster by using the same cache idea but for the window.getComputedStyle:

const getComputedStyleCache = new WeakMap()
function cachedGetComputedStyle(element) {
  if (!getComputedStyleCache.has(element)) {
    getComputedStyleCache.set(element, window.getComputedStyle(element))
  }

  return getComputedStyleCache.get(element)
}

As this is used both in isInaccessible and isSubtreeInaccessible, we cache even more.

[eps1lon]

Funnily, if I do getByRole('progressbar', { hidden: true }) it solves my issue as it does not call isInaccessible.

Oh right. I had it backwards.

But I think it would be a good idea to skip the error helpers when running in a waitFor*. That would speed things up.

[ValentinH]

I don't see a way to skip the error in this case without explicitly passing a (new) option to getByRole(). Do you have an idea?

[drewschrauf]

I ran into this today while trying to debug why a mocked network request was so slow. I'll try to put together a minimal reproduction but the Cliffs Notes is:

My app code was making a network request and on success would navigate to another route. I was waiting for the new route to load with screen.findByRole('heading', {name: 'xxx'}). Using console.time around the fetch call was showing ~1900ms elapsed. A similar request that would 400 was taking around ~22ms to display the error message. Swapping to screen.findByText('xxx') brought the elapsed time for the former down to ~25ms. I took a look at the profiler and it seems that *ByRole is starving my CPU. I tried both {hidden: true} and {interval: 500} with no improvement. This is in a toy app with a pretty tiny DOM.

The only real difference between the tests seems to be the route change. I'll try to put together a minimal reproduction this weekend to narrow it down.

[kentcdodds]

If I understand correctly were determining the accessibility of everything before matching. Is that right? If so... What if we find the match first and then determine if it's accessible?

[eps1lon]

Is that right? If so... What if we find the match first and then determine if it's accessible?

Pretty sure this is what we do already.

[eps1lon]

I don't see a way to skip the error in this case without explicitly passing a (new) option to getByRole(). Do you have an idea?

We could flip a global variable and then do something like runWithDiagnosticsDisabled(callback) inside waitFor* methods.

[ValentinH]

Is that right? If so... What if we find the match first and then determine if it's accessible?

Pretty sure this is what we do already.

What I understand is that we are searching for all the roles in the container tree in order to explain why the role that was requested was not found.

[ValentinH]

@eps1lon is there already a similar pattern in the existing codebase, if yes I can work on a PR to implement the light error message when run within a waitFor*. 🙂

Actually, I think it should only be for waitForElementToBeRemoved(), for the other ones, the full error message makes sense.

[ValentinH]

I guess I can use the globalObj in https://github.com/testing-library/dom-testing-library/blob/master/src/helpers.js#L1
and create 2 functions to read /write the flag: toggleDiagnosticsInErrors() and shouldPrintDiagnosticsInErrors(). What do you think?

[kentcdodds]

I think we really need to be careful here. I want to give it a little bit of extra thought...

[ValentinH]

Yes this would be quite hacky and just a workaround for a deeper issue: isInaccessible being slow.

[ValentinH]

If you decide that we can go this way, I'm ready to make a PR for the 2 following things:

• replaced the existing cachedIsSubtreeInaccessible() by the cachedGetComputedStyle() I mentioned above. As we cache more, it's even faster.
• implement the "global" shouldPrintDiagnosticsInErrors() check in the getMissingError() of Role queries when run within waitForElementToBeRemoved().

[kentcdodds]

Ok, so I think the simplest solution is to default hidden to true and make the default configurable. I know the arguments against this that @eps1lon will have (we've been through this before) but I'm convinced that opting into this kind of a performance drain in exchange for the confidence is better than having to opt out of it. I worry that people will switch from ByRole to a suboptimal query due to performance issues and I don't think that's wise.

Because this is a breaking change, we should do this in two phases.

1. Make it configurable and release that as a minor version
2. Change the default and release that as a major version (maybe batched with other major changes in we can think of any).

Anyone want to work on phase 1?

[eps1lon]

I thought I already said that I'd be for skipping this check by default :smile:

This is what we're currently doing in Material-UI:

1. hidden: true by default locally when using JSDOm
2. hidden: false by default in CI or when using an actual browser (getComputedStyle is way faster in a browser)
3. explicitly set hidden on assertions where it actually matter

Step 3 is needed to pass/fail the same assertions locally compared to CI.

The underlying assumption is that

1. we rarely have tests where we accidentally hide elements
2. immediate feedback is more important locally than in CI (again assuming your CI runs minutes not seconds).

But you need to be aware that we only tests components and not apps. For tests of complete pages (or when using a browser) I'd still recommend to keep this check by default.

[ValentinH]

@kentcdodds is there already a way to configure some defaults globally?

I agree with the point about making it faster. In my case, we already did what you mentioned: we switched to getByTestId, which is less clean.

[kentcdodds]

Here's how we do defaults + config with waitFor: https://github.com/testing-library/dom-testing-library/blob/429de04d023f2c49af2cb7b10d6daa537d417999/src/wait-for.js#L21

Here's where the config is: https://github.com/testing-library/dom-testing-library/blob/master/src/config.js

And here's where we'd want the config value to be retrieved: https://github.com/testing-library/dom-testing-library/blob/429de04d023f2c49af2cb7b10d6daa537d417999/src/role-helpers.js#L121

That should be enough to get anyone going.

@eps1lon, sorry I must have missed the comment where you said changing the would work for you. I like the setup you have and think that kind of setup would be awesome to have documented (with an example repo). Seems pretty straightforward.

[ValentinH]

Actually there I already a defaultHidden config so I guess the PR would only be to assign it as the default value for getRoles().
I can take care of this.

[kentcdodds]

Oh yeah, I forgot about the defaultHidden thing 😅

[ValentinH]

I actually noticed while implementing the PR that it was already done in the getMissingError() function which is the one that was slow in my report:
https://github.com/testing-library/dom-testing-library/blob/master/src/queries/role.js#L113

Thus, setting the global config with:

import { configure } from '@testing-library/react'

configure({ defaultHidden: true })

solves the issue.

Thus, I guess only the last step is necessary: switching the default value to true?

[domarmstrong]

Setting hidden makes only a small difference in a test suite for a moderately complex DOM tree. I'm seeing a single test suite take 83 seconds with hidden: false, 67 seconds with hidden: true and 6 seconds using more convoluted queries and test ids. For me that is still unusably slow, which is sad because the api is significantly better than the queries to work around it. That is 1 test suite of hundreds, so you can imagine using role queries is not an option for any scale.

[eps1lon]

It would help a great deal if somebody could provide actual code with which we could test possible improvements. Otherwise we can't meaningful work on improving performance.

[ValentinH]

@eps1lon I think the demo I shared above is already showing a big difference on a quite complex form: https://codesandbox.io/s/musing-gauss-e1ynf?file=/src/DemoForm.js

I've added some content around the form to make the tests even slower though.

This is my real use case except I have a bunch of providers on top.

[eps1lon]

codesandbox.io/s/musing-gauss-e1ynf?file=/src/DemoForm.js

I don't see much of a slowdown there. It's two times slower which is expected. It's even lower when using getByRole(role, { hidden: true }) We can't have ByRole be as fast as ByTestId since ByRole has much for features (one of them being confidence).

[tommarien]

@eps1lon I might have some code which i probably could share with you, which leads to unstable tests in parallel because we are doing a await findByRole('alert') which can eat up de timeout of 1000ms very fast, but it's client code so i can only give you the a link to private repo where you have read access to. Let me know if you want it ;) (it costs some time to set up)

[eps1lon]

@tommarien That'd be great.

[ValentinH]

@eps1lon in my screenshot, it's 8 times slower.

Indeed, using{hidden: true} is the solution for me. This is why Kent was suggesting to make it the default.

[tommarien]

@eps1lon I'll try to set it up this evening and let you know ;)

[tommarien]

@eps1lon I've setup the repro repo, i've kept as much as possible in because i did not want to remove the problem in some way ( you have a pending collaborator access invite). I've left where to look in the readme ;)

[tommarien]

@eps1lon Any update on this issue ?

[kentcdodds]

:tada: This issue has been resolved in version 7.6.0 :tada:

The release is available on:

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

Your semantic-release bot :package::rocket:

[domarmstrong]

This doesn't actually improve the performance of getByRole though does it? Just when using with waitFor. I was not using waitFor and getByRole was still unusably slow in a complex form component.

[eps1lon]

I was not using waitFor and getByRole was still unusably slow in a complex form component.

Please open a new issue (since this issue was concerned about ByRole in waitFor) and fill out the template. Having a repro like the one @tommarien provided helps a lot.

[tommarien]

I've just update testing-library/dom in the reproduction repo and see times go from 280ms on average to 38ms ;), so thanks a lot 👍 . If next release of testing-libs dom dependency to > 7.6.0 than everyone can benefit from this :). @eps1lon Thanks and i'll remove the repro repo today

[ctoppel]

I am also seeing very slow times in a large complex DOM (>800ms/getByRole). @domarmstrong Did you open a new issue already?

[kentcdodds]

Hi @ctoppel,

I'll just echo what @eps1lon said earlier:

Please open a new issue (since this issue was concerned about ByRole in waitFor) and fill out the template. Having a repro like the one @tommarien provided helps a lot.

Thanks!

[dbertella]

Hey guys I don't know if anybody else opened a new issue but today I had this exact issue, one of my test suite started to have systematic timeouts after updating to new version of testing library and updating my queries to getByRole
I run a bit of profiling and those are my findings

Let's say I have an Input component similar to this

<Input
  {...el.input}
  placeholder="My El"
  data-testid="MyEl"
/>

In my tests I'm running this:

console.log('pre getByTestId', performance.now())
screen.getByTestId('MyEl')
console.log('after getByTestId', performance.now())
console.log('pre getByRole', performance.now())
screen.getByRole('textbox', { name: /My El/ })
console.log('after getByRole', performance.now())
console.log('pre getByText', performance.now())
screen.getByText(/My El/)
console.log('after getByText', performance.now())

Those are my results before without configure({ defaultHidden: true })

pre getByTestId 10829.295586
after getByTestId 10835.550812
pre getByRole 10836.638695
after getByRole 12147.583377
pre getByText 12148.72231
after getByText 12153.527319

Those are the results after configure({ defaultHidden: true })

pre getByTestId 6450.390944
after getByTestId 6454.832352
pre getByRole 6457.684077
after getByRole 7683.902226
pre getByText 7693.771436
after getByText 7698.411979

In general for me getByRole is slower it's 1 / 2 sec vs 5ms with the other queries

[atsikov]

https://github.com/eps1lon/dom-accessibility-api/pull/323
This PR improves getByRole performance by up to 20% by removing one extra getComputedStyle call