Disallow the use of `queryBy*` queries for assertions

[thomaslombart]

I've seen people using queryBy as assertions in their tests:

screen.queryByText("Account")

This is not the right way to use it because the test won't fail whether the text is there or not.

Would it be possible to extend the prefer-explicit-assert rule in order to disallow the use of queryBy as assertions as well?

[Belco90]

I think it would make sense to be part of prefer-explicit-assert.

[Belco90]

This should be really easy to implement on v4, but it hasn't been refactored yet.

[thomaslombart]

@Belco90 I'll refactor the rule and add the enhancement then 🙌

[Belco90]

That would be awesome. I'm gonna put that rule on WIP assigned to you on #198 then.

Please double check that issue as that rule has a side note to remove its option, which is not necessary anymore. You can check prefer-presence-queries or await-async-query to get an idea about how we are reporting the queries now. You'll see that:

• we have detection helpers to know if an identifier is a particular Testing Library query
• we don't check anymore if coming from render method or Testing Library itself (aggressive query reporting)
• we don't differentiate between built-in and custom queries: if it matches a particular Testing Library query pattern, it's reported (aggressive query reporting)

Let me know if you have any questions or need help to hook the rule with the new detection mechanism!

[thomaslombart]

I was about to implement that enhancement, and there's something that bugged me. The goal of this enhancement is to disallow this kind of statement:

screen.queryByText("My button")

It can happen if someone is mistaken about getBy and queryBy queries. But if something ever happens, what should we suggest to the user? Maybe he doesn't like explicit assertions and wants to use screen.getByText in the end.

Then, maybe that rule should be implemented in prefer-presence-queries. When the user uses screen.queryByText, it's suggested to use screen.getByText. And if the user has prefer-explicit-assert enabled, then the rule will fire.

What do you think, @Belco90?

[Belco90]

Mmm not sure to be honest, but definitely something to be handled combining prefer-presence-queries and prefer-explicit-assert.

I think we should just suggest in prefer-explicit-assert to wrap screen.queryByText("My button") within some assertion, no matter which one. Then, if that assertion is checking element is present (e.g. expect(screen.queryByText("My button") ).toBeInTheDocument()) and prefer-presence-queries is enabled, this one should be the one reporting that the wrong assertion was used, not prefer-explicit-assert. This way, we don't mix their scopes so prefer-presence-queries only checks if assertion used corresponds to presence type checked, and prefer-explicit-assert only makes sure an assertion is used for standalone queries (no matter which assertion is used).

Thoughts?

[gndelia]

I think we should just suggest in prefer-explicit-assert to wrap screen.queryByText("My button") within some assertion, no matter which one. Then, if that assertion is checking element is present (e.g. expect(screen.queryByText("My button") ).toBeInTheDocument()) and prefer-presence-queries is enabled, this one should be the one reporting that the wrong assertion was used, not prefer-explicit-assert. This way, we don't mix their scopes so prefer-presence-queries only checks if assertion used corresponds to presence type checked, and prefer-explicit-assert only makes sure an assertion is used for standalone queries (no matter which assertion is used).

I agree, each rule should take care of their own purpose (one about not using the query in isolation, the other about preferring one query over the other) and then if they want the combinated result, they should enable both. In that sense, we should probably recommend both to be enabled together (maybe link each other in the readme), if they are not part of the base recommended ruleset

[thomaslombart]

That makes sense! I won't have the bandwidth to work on that right now, but I'll do it at some point 🙂

[Belco90]

Closing due to inactivity.