fix(prefer-query-by-disappearance): allow get and find query variants when passed in directly

[Lukas-Kullmann]

Checks

• [x] I have read the contributing guidelines.

Changes

Currently, the plugin reports

await waitForElementToBeRemoved(screen.getByText('abc'))

as a violation. This PR removes this check and also allows the findBy variants.

Context

When passing in an element directly, waitForElementToBeRemoved actually checks whether the element is there initially. If it is not, it will throw an error. This corresponds with the getBy variants of the queries.

I figured to also remove the check for the findBy variants since this is really a problem to solve by typescript (and the type definitions of the "main" testing library).

If anything, we should report waitForElementToBeRemoved(screen.queryByText('abc')) as a violation. However, I also think that this is something that the typescript definitions of the "main" testing library should solve (by constraining what can and cannot be passed into waitForElementToBeRemoved). So I did not add this check here.

[G-Rath]

This seems to fly directly in the face of the rule per it's documentation:

Using queryBy* queries in a waitForElementToBeRemoved yields more descriptive error messages

And the examples of that difference is:

// TestingLibraryElementError: Unable to find an element by: [data-testid="loader"]
await waitForElementToBeRemoved(screen.getByTestId('loader'));

// The element(s) given to waitForElementToBeRemoved are already removed.
// waitForElementToBeRemoved requires that the element(s) exist(s) before waiting for removal.
await waitForElementToBeRemoved(screen.queryByTestId('loader'));

To me, the error provided when using queryByTestId seems more natural even if it could be technically incorrect (i.e. if you've typo'd your selector, then it's not actually that it's already been removed); meanwhile you seem to be arguing for the opposite without an actual reason for why that's better (to me, you've implied something along the lines of "we'll fail earlier and that's a good thing", but you've not outright said that).

Could you expand on what the actual advantage you think there is in using getByText for this situation?

[Lukas-Kullmann]

I should have made this a bit more clear, sorry for that.

I think that you are right. The error message when using queryBy* is more descriptive. There is no arguing about that.

However, the reason why I would use getBy* is the case that we are discussing here is that, in my opinion, there is not really an overlap of getBy* and queryBy*. I always use getBy* when I expect the element to be there at this exact moment. And I always use queryBy* when I expect it not to be there at this moment. So if waitForElementToBeRemoved expects the element to be there initially, I naturally use getBy*. This is also the reason why I think that it's fine to use queryBy* in the callback variant. The callback will be called repeatedly and eventually, I expect the element not to be there any more. That's why I use queryBy* there.

I guess in the end this is then a matter of personal preference. And I can always disable the rule. Thanks for the feedback. 🙂