Combine multiple wait-for rules into one

[thomaslombart]

We have multiples rules related to the use of waitFor in the v4:

• no-wait-for-snapshot
• no-wait-for-empty-callback
• no-side-effects-wait-for
• no-multiple-assertions-wait-for

After reading Common mistakes with React Testing Library, it seems like waitFor must be used in one way: by putting a single assertion inside the callback.

For example:

await waitFor(() => expect(a).toEqual('a'))

I feel like the rules above could be simplified and combined into one single rule that makes sure you only expect one assertion in the callback. That would make our rules simpler and easier to maintain. Plus, the developer wouldn't have to enable multiple rules but one which would give him a better experience.

The downside I see is that these rules have different levels of criticality, some of them are included by default and others don't. Combining it into one would make the rule too opinionated (though we can still configure it in the options).

I don't know if there are other legit use cases for waitFor so any feedback on that is appreciated.

[Belco90]

Hey @thomlom! I like the main idea explained here and I'm happy to have this discussion. However, there have been tons of changes already for v4, which is not even finished yet and has been around for a while, so I don't want to increase the list of breaking changes for it. Instead, I'd like to leave rules combinations and standardization for a future v5. I was already thinking about combining ones related to await async stuff or extract the framework setting from no-dom-import and make it global to the plugin. But this needs more analysis and I think v4 has enough changes already so I'm gonna remove it from v4 milestone, at least until we have some discussion around all this.

About combining waitFor rules. I wouldn't be worried about the granularity of having several rules, as you mentioned that could be configured over rule options. Regarding your main point of "it seems like waitFor must be used in one way: by putting a single assertion inside the callback." this is not technically true by different reasons:

1. Not all assertions inside waitFor callbacks are valid. Snapshots matchers doesn't work fine inside waitFor, and ideally toBeInTheDocument should be avoided so waitFor + get* + toBeInTheDocument would be replaced by find* + toBeInTheDocument
2. There are some other things than assertions which are valid too. If you go to DOM Testing Library v7.0.0 release notes where waitFor was introduced, you can see another valid case:

However, if for some reason you cannot use a find query, then waitFor should be a find/replace for waitForElement:

- const element = await waitForElement(() => container.querySelector('.loading'))
+ const element = await waitFor(() => container.querySelector('.loading'))

3. Since waitFor replaces waitForDomChange, all the valid cases for the latter are valid for the former: https://testing-library.com/docs/dom-testing-library/api-async#waitfordomchange-deprecated-use-waitfor-instead

So it doesn't seem waitFor is exclusively intended to be used for assertions, although it would be the most common one. This doesn't mean the rules can't be standardized or combined (the names could be consistent starting by no-wait-for-*) but I'm not really sure how to combine them.

[thomaslombart]

Yes, I agree it'll be a lot of changes for v4. I also think the async stuff could be mutualized with other rules.

I didn't know about the other use cases for waitFor. I always see it used combined with an assertion, so intuitively, I thought this was its only purpose. I suggest we put this issue on hold and come back to it later when v4 will be released.

[Belco90]

I moved this issue to a discussion thread!