wait* utilities take more time since 9.2.0 (running into Timeout)

[kissthom]

• react-testing-library version: 9.2.0
• react version: 16.9.5
• node version: 12.6.0
• yarn version: 1.19.0

Relevant code or config:

What you did:

Upgraded from version 9.1.4 to 9.2.0.

What happened:

Some of our tests which are using the "wait*" utilities provided by the library are running into jest timeout after the upgrade. In some cases waiting for a dom element to appear takes 7-8x times more (~3 seconds) as before with version 9.1.4.

Reproduction:

I've created a simple example to reproduce the problem although with this example the difference is not so critical as in our application.

https://codesandbox.io/s/rtl-wait-slow-g7c21

Steps to reproduce:

1. Run the tests and check out the results in the console (average time for waiting for select.options elements to appear 40-60ms)
2. Switch the rtl version to 9.2.0
3. Run the tests and check out the results again (2-3 times slower) (average time for waiting for select.options elements to appear 110-150ms)

Any help is appreciated.

Problem description:

Suggested solution:

[kentcdodds]

I'm guessing the difference has nothing to do with the @testing-library/react version but actually the @testing-library/dom version. Could you add @testing-library/dom to your dependencies and using the process of elimination determine which version of @testing-library/dom starts causing the issue for you? That would be a huge help.

[kissthom]

Thanks. I'm on it.

[kissthom]

I was able to pinpoint the problem.
The performance problem occurs with the the version 6.4.0 of @testing-library/dom.

Although my assumption, that the problem has something to do with the wait* utilities, was wrong because it only occurs when the "role-queries" are used.
In the version 6.4.0 this issue (#352) has been solved which added an additional filtering (the first filter in the code below) to the dom nodes which you are currently querying.

(https://github.com/testing-library/dom-testing-library/blob/master/src/queries/role.js)

return Array.from(container.querySelectorAll('*'))
    .filter(element => {
      return hidden === false ? isInaccessible(element) === false : true
    })
    .filter(node => {
      const isRoleSpecifiedExplicitly = node.hasAttribute('role')

      if (isRoleSpecifiedExplicitly) {
        return matcher(node.getAttribute('role'), node, role, matchNormalizer)
      }

      const implicitRoles = getImplicitAriaRoles(node)

      return implicitRoles.some(implicitRole =>
        matcher(implicitRole, node, role, matchNormalizer),
      )
    })

And as I see the isInaccessible function can be expensive if the count of dom nodes is high because it will traverse all the parents of every dom node you are calling with.
I also tested it locally by writing a very simple test and if I remove the mentioned filter from the implementation the performance gets better.
It also helps if you switch the order of the filters.

A simple test to measure the performance:

test('measuring queryALlByRole performance', () => {
  let componentToRender = '<div role="test-role">Test content</div>';
  for (let i = 0; i <= 100; i++) {
    componentToRender = `<div>${componentToRender}</div>`
  }

  const { queryAllByRole } = render(componentToRender)

  const before = performance.now();
  queryAllByRole('test-role')
  const after = performance.now();

  console.log('Performance', after-before)
})

Let me know if I can help you any further.

[kentcdodds]

Thank you very much for that analysis! I'm going to transfer this issue to dom-testing-library and ask @eps1lon to give it a look.

[eps1lon]

Yeah that query can be quite expensive for which I don't really have an easy solution. We can probably optimize it a bit but the general runtime implication of traversing the whole tree will always be there.

So I have some ideas how to make this faster but this is not a priority for me. If somebody wants to work on this then I have a couple of suggestions:

• don't use querySelectorAll but traverse top-down and skip subtrees we know will be inaccessible (aria-hidden, hidden)
• cache isAccessible results (I think we have O(n^2) calls it to where n is the number of nodes)

As to your perspective I would generally ask if you really need waitFor queries. Personally I don't find them very useful since I'd like to see explicitly in the test how long something takes in my UI. So far any waiting I used could be replaced with explicitly advancing the timer and then making the assertion. I think this applies to your repro really well. At least I would like my test to fail if it takes 1s instead of .1s to transition. Now this applies mostly to your transitions but not really if you're waiting for some API. Maybe you should rather wait a fixed amount and then expect a certain state. After all your user doesn't want to look for 20s at a spinner either. But I understand that there are some use cases where wait makes more sense.

Another option may be to reduce to polling time i.e. is it important to assert every frame (16ms) or is every 10 frames (160ms) enough?

And last but not least you can always disabled this check queryByRole('textobox', { hidden: false }) if you're waiting for something to disappear. If you want to make sure that the queried items are accessible you'll have to wait until the isAccessible matcher is done in jest-dom.

Either way thank you for taking the time and providing the repro.

[eps1lon]

Ok I think I'll prioritize this more since this will be used in an isAccessible matcher and I want to expand functionality of byRole anyway. I'm not a fan of adding more features before existing ones haven't gotten a polish.

[kissthom]

Hi @eps1lon.
First, thank you for taking a look at it, for the explanation and tips and of course generally for the work on this awesome library.

This issue to solve really does not seem to be simple because I think you will need both mechanism what you suggested (traversing top-down, skipping inaccessible subtrees + caching) to be added to be able to reduce querying time in all cases.

In my particular case I'm waiting for some state change to happen asynchronously after a fake API call and after that there is also a transition involved (provided by third party lib) which I also have to wait for.
In the browser itself there is no noticeable performance problem with it, so at the time of writing I'm not worrying about how long the transition takes...but that time will definitely come :). But your argument is totally valid of course.

Another option may be to reduce to polling time i.e. is it important to assert every frame (16ms) or is every 10 frames (160ms) enough?

I'm not really understand this. How could it be useful to me and how could I apply it? Could you maybe show me some link or example?

And last but not least you can always disabled this check queryByRole('textobox', { hidden: false })

Indeed, thanks.
Or I think I can use other queries in my case.
Now that I understand the problem I'm pretty confident that I can use some workaround to solve the issues in our tests.

Ok I think I'll prioritize this more since this will be used in an isAccessible matcher and I want to expand functionality of byRole anyway. I'm not a fan of adding more features before existing ones haven't gotten a polish.

👍
If you'd like to I would be happy to help with it.

Should this issue maybe renamed? Because as I pointed out earlier, my original assumption was wrong and it's about the role-queries, not the wait* utilities.

[kentcdodds]

🎉 This issue has been resolved in version 6.8.1 🎉

The release is available on:

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

Your semantic-release bot 📦🚀