Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance issue with the byRole query causing timeout errors #698

Open
MatheusPoliCamilo opened this issue Jul 14, 2020 · 20 comments
Open

Comments

@MatheusPoliCamilo
Copy link

MatheusPoliCamilo commented Jul 14, 2020

  • @testing-library/jest-dom version: 5.11.0
  • node version: 12.14.1
  • yarn version: 1.21.1

What you did:

I converted several tests of the project I work on, which were using manual queries with container and querySelector, to use screen prioritizing the query byRole as recommended.

What happened:

There are some timeout errors when I run some tests after the conversion to use screen and the byRole selector.

image

Reproduction:

I made a small example to demonstrate the difference in ms in a small case, like a table with 8 cells, when using the query byRole compared to byText.

Problem description:

At the moment there is a certain performance problem with the byRole selector, which even causes a timeout problem in tests, something that does not happen with other selectors such as byText.

Below is the time it takes to run a test on the project I work on when using the byRole query (in which there is a timeout error in some moments for taking too long).

image
image

Then, the time it takes to perform the same test, but using the byText selector. I've run the test several times this way and with this selector there is no timeout, in addition to being much faster.

image
image

Suggested solution:

I believe that for now the solution would be not to suggest the query byRole as a priority, at least until there are some more performance improvements to get closer to the other selectors and avoid timeouts.

I apologize because I did not get to look at the code in depth to try to understand a little if there is a way to bring about an improvement in byRole. I am also aware of the benefits that this selector brings and I am willing to waste more time to run the tests to use it, however the time difference is still large and causing timeout errors. Thank you in advance!!!

@sousajunior
Copy link

I have the same problem.

@eps1lon
Copy link
Member

eps1lon commented Jul 15, 2020

If you're confident that you can manually verify the checks ByRole provides then I suggest switching to other queries. ByRole considers the a11y tree, styles and the accname spec. These checks are expensive but have a higher confidence that what you're testing is actually what a screen reader would "see".

It's like any other tool: If you don't need the features then you should switch to a tool with smaller feature set. If test performance is a bigger issue for you than test confidence then ByRole is not the right tool.

One thing: A real-world repro would help us a lot improving performance. We've had success in the past with good repros since they help us identify bottlenecks. Screenshots of code don't help us very much. Small examples that don't reproduce the issue are also not helpful.

@gnapse
Copy link
Member

gnapse commented Jul 15, 2020

Shouldn't this issue be moved to @testing-library/dom?

@eps1lon eps1lon transferred this issue from testing-library/jest-dom Jul 15, 2020
@eps1lon
Copy link
Member

eps1lon commented Jul 15, 2020

What's also very important is the environment e.g. which browser or if you're running from node (via e.g. jest) what version of jsdom (yarn why jsdom or npm ls jsdom).

@MatheusPoliCamilo
Copy link
Author

Thank you very much for responding! I will try to reproduce the code we have to make it clearer.

jsdom version: 16.2.2
Running jest version 26.1.0 on node.

@MatheusPoliCamilo MatheusPoliCamilo changed the title Performance issue with the byRole query Performance issue with the byRole query causing timeout errors Jul 17, 2020
@jihchi
Copy link

jihchi commented Aug 8, 2020

I've made a contrived repo here: https://github.com/jihchi/698_byrole_performance_issue

there is a 3rd-party datetime picker and it would take ~1 second when you opened the calendar and pick a specific date in the calendar by using getByRole('button', { name }).

@idanen
Copy link
Collaborator

idanen commented Oct 4, 2020

Tried a PR adding a test like:

test('reasonable performance', () => {
  const {getByRole, getByLabelText} = render(
    `<label><input type="checkbox" name="checker" /> check</label>`,
  )
  let start = Date.now()
  getByLabelText(/check/i)
  let end = Date.now()
  const byLabelTextTime = end - start
  start = end
  getByRole('checkbox', {name: /check/i})
  end = Date.now()
  const byRoleTime = end - start

  expect(byRoleTime).toBeLessThan(5 * byLabelTextTime)
})

I think this gives a roughly good start.
I saw we're using getComputedStyle pretty heavily, which might be a bottleneck.
Tried to manage some caching mechanism like we do with isSubtreeInaccessible, which showed small improvement, but not sure it's a good direction.

I'd be happy to continue if anyone has better idea of where might be a bottleneck, or how can I profile it.

(does anyone see any value in a PR with just the failing test?)

@jihchi
Copy link

jihchi commented Oct 4, 2020

According to my investigation, @eps1lon has summarized the causes pretty clear and I agree with him:

If you're confident that you can manually verify the checks ByRole provides then I suggest switching to other queries. ByRole considers the a11y tree, styles and the accname spec. These checks are expensive but have a higher confidence that what you're testing is actually what a screen reader would "see".

It's like any other tool: If you don't need the features then you should switch to a tool with smaller feature set. If test performance is a bigger issue for you than test confidence then ByRole is not the right tool.

Simply say, you may encounter slowness of ByRole API and/or eventually a timeout occurs when you have a large amount of DOM elements.

@idanen
Copy link
Collaborator

idanen commented Oct 4, 2020

I totally agree with @eps1lon , but if there is something to do to improve performance we should give it a try, shouldn't we?
The difference between getByRole and getByLabelText is pretty big (notice in the test I compared to 5 times the duration and it's still longer than that), so I think we should at least make some effort to reduce it

@jihchi
Copy link

jihchi commented Oct 4, 2020

@idanen For sure, I would definitely love to see any improvements of ByRole API!

@ryuuji3
Copy link
Contributor

ryuuji3 commented Nov 21, 2020

I believe that the accessibility checks are nice, but in some cases you don't need them- take for example a UI framework. That's one of the times where you absolutely do want these checks.
But then say you have an application that uses the framework- if the only thing that needs to verified is application behavior, and speed is absolutely important for development.

I think it very much depends on your needs. Not to mention some people have separate pipelines for these checks (eg. lighthouse)

@petermarcoen
Copy link

I also experience this problem but only on my CI runners, not on my local machine.

I understand that byRole might take more time but then I would prefer a global setting to increase the default timeout of the findBy -queries, otherwise I have to litter all my tests with , {timeout: 2000}

@frankandrobot
Copy link

@kentcdodds any updates on this? We are still hitting this 3 years later with @testing-library/dom v8.11.3 and @testing-library/jest-dom v5.16.5.

However, I did see there is a v6 for jest-dom. But not sure if that addresses this issue.

@HugoPoi
Copy link

HugoPoi commented Nov 5, 2023

I can reproduce this issue with @testing-library/dom@9.3.1, I have some dom taking like more 600ms to evaluate screen.getByRole('row', name: /Stuff/) on a 4Ghz desktop cpu. I will publish a repo next week to reproduce.

  let start = new Date()
  screen.getByRole('row', { name: rowRegex })
  let duration = (new Date()) - start
  console.log(rowRegex, duration)
  start = new Date()
  screen.getAllByRole('row')
  duration = (new Date()) - start
  console.log('all row', duration)
  console.log
    /Snape/i 680
  console.log
    all row 20

Yeahhhh you read well near 700ms to find one row matching an innerText vs 20 ms to get all the row. And 20ms for just listing some row is already very slow for just traversing a small tree like maybe 1000 nodes.

@HugoPoi
Copy link

HugoPoi commented Nov 7, 2023

As promise serious testing materials https://github.com/HugoPoi/bug-perf-demo-getByRole/tree/main

@alexkrolick
Copy link
Collaborator

alexkrolick commented Nov 8, 2023

I'm curious if the same query takes a similarly long time in Playwright. They implement all the Testing Library queries natively.

I have also noticed performance differences between JSDOM with RTL vs Shadow DOM TL, which patches JSDOM to allow traversing shadow roots. Somehow the patches seem to avoid an expensive serialization step (or something)

@MatanBobi
Copy link
Member

@alexkrolick I doubt it. Based on the research I did, it looks like our major bottleneck is getComputedStyles from JSDOM and querySelectorAll... These are known to be much slower in JSDOM rather than the browser. Also, their implementation is quite different from what I saw last time I checked.

@HugoPoi
Copy link

HugoPoi commented Nov 8, 2023

Yes maybe the issue reside in JSDom but the method causing lags is computeAccessibleName at the top. I don't yet understand all intrication beyond this slowness but getcomputedStyles should be fast if we don't have any style in the Dom. I have started to read the code behind computeAccessibleName and seems to be a bear implementation of the w3c specification but without any performance optimization in mind.

Copy link

github-actions bot commented Nov 9, 2023

Uh oh! @MatheusPoliCamilo, the image you shared is missing helpful alt text. Check your issue body.

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

1 similar comment

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests