testing-library / dom-testing-library

πŸ™ Simple and complete DOM testing utilities that encourage good testing practices.
https://testing-library.com/dom
MIT License
3.25k stars 463 forks source link

define waitFor interval globally #1226

Closed TkDodo closed 1 year ago

TkDodo commented 1 year ago

Describe the feature you'd like:

I would like to set the interval option on waitFor, which defaults to a hardcoded 50ms to a different value, globally:

https://github.com/testing-library/dom-testing-library/blob/1fc17bec5d28e5b58fcdd325d6d2caaff02dfb47/src/wait-for.js#L25

This is already possible for the timeout value, which defaults to asyncUtilTimeout of the global configuration:

https://github.com/testing-library/dom-testing-library/blob/1fc17bec5d28e5b58fcdd325d6d2caaff02dfb47/src/wait-for.js#L22

Reasoning

I'm a maintainer of TanStack/query, where we have many tests that perform short, asynchronous operations. Our QueryFunctions usually sleep for 5-10ms, because if we don't do that, our internal batching will batch state updates together, which would mean we're missing loading states. It's also closer to what happens in real life - as Queries are asynchronous, they won't resolve immediately.

Our test pipeline takes about 2 minutes, even after we've switched from jest to vitest. It seems that we cannot optimize this any further on the test runner level, so my thinking is that because the waitFor interval is fixed to 50ms, if the result is not found in the first check, it will at least wait 50ms, even though in our cases, 10ms would likely be enough. This seems to compound over the amount of tests we have.

I can of course verify this by passing a custom interval to all usages, but I was thinking that a global setting for this value wouldn't hurt :)

Suggested implementation:

Add an asyncUtilInterval option to the configure function, and make sure waitFor picks this up instead of the hardcoded 50. Of course, we would still default that value to 50 to be backwards compatible.

Describe alternatives you've considered:

It's not clear to me why there isn't one config object that lets you default the whole waitForOptions? Adding a second option asyncUtilInterval will let us configure this option, but not others of waitFor like container or stackTraceError.

It might be better to introduce a new waitForOptions config to configure and deprecate the asyncUtilTimeout option in favour of:

configure({ waitForOptions: { timeout: 10_000, interval: 10 })

options passed to waitFor directly would then need to be shallow merged with the defaults. This would allow all waitForOptions to be configured globally.


I'm happy to implement either one of the suggested solutions, or any alternative you can come up with :)

eps1lon commented 1 year ago

The global config was a mistake in my opinion anyway since it breaks pretty easily with multiple versions. So I'd rather not add more stateful stuff to the module.

Seems to me this can be solved today with using composition. I usually recommend wrapping RTL into a custom one for apps and libraries anyway which solves any configuration scenarios I can think of. In your case you'd probably want to wrap waitFor and findBy.

But if the underlying problem is test speed, why not switch to fake timers?

TkDodo commented 1 year ago

The global config was a mistake in my opinion anyway since it breaks pretty easily with multiple versions. So I'd rather not add more stateful stuff to the module.

I understand πŸ‘

Seems to me this can be solved today with using composition. I usually recommend wrapping RTL into a custom one for apps and libraries anyway which solves any configuration scenarios I can think of. In your case you'd probably want to wrap waitFor and findBy.

yeah, we haven't done that, so I'd need to change all tests, and potentially also make sure (with a lint rule) that no one imports the methods directly from testing-library in the future, which seems like quite a bit of effort.

What do you think about mocking waitFor from testing-library and then forward it to the actual impl (with requireActual) and append the desired default options?

But if the underlying problem is test speed, why not switch to fake timers?

I'd also need to look into that, might be a viable option. Thank you πŸ™

TkDodo commented 1 year ago

I tested a bit more, and it seems like the mutationObserver does most of the work, and the interval rarely matters. So I think if I want another improvement, going with fake times is the best approach. Thanks again, and sorry for wasting your time here πŸ˜