Create a rule to support enforcing getBy* for toBeVisible

[CodingItWrong]

(this issue has been edited to reflect an updated intent; it was originally a proposal to change prefer-presence-queries)

Name for new rule

prefer-query-matchers

Description of the new rule

The rule should somehow provide a way to enforce that toBeVisible() should always be called with a getBy* query in place of a queryBy*. This works differently than the prefer-presence-queries rule, because a negative there expects an element to not be present in the DOM. In my understanding not.toBeVisible() means "is present in the DOM but is not visible," and it fails if it receives a null: "received value must be an HTMLElement or an SVGElement".

Rather than coding for this specific matcher only, we currently plan to provide a configurable rule where you can configure which matchers require get and which require query.

Testing Library feature

The getBy* and queryBy* queries. It is prompted by the toBeVisible matcher from jest-dom.

Testing Library frameworks

getBy* and queryBy* are in the Core API and so are not specific to any framework.

What category of rule is this?

Other: a best-practice recommendation. It allows users to configure rules to enforce use of the queries that will produce the best failure messages.

Code Examples

For the following proposed options schema:

{
  "rules": {
    "prefer-query-matchers": ["error", { 
      "validEntries": [{ "query": "get", "matcher": "toBeVisible"}]
     }],
  }
}

With the following code:

/* 1 */ expect(screen.queryByText("a")).toBeVisible();
/* 2 */ expect(screen.getByText("a")).toBeVisible();
/* 3 */ expect(screen.queryByText("a")).not.toBeVisible();
/* 4 */ expect(screen.getByText("a")).not.toBeVisible();

1 and 3 would produce errors, and 2 and 4 would not.

If instead the query value was changed:

{ 
  "validEntries": [{ "query": "get", "matcher": "toBeVisible"}]
}

Then 2 and 4 would produce errors, and 1 and 3 would not.

Anything else?

We investigated handling this with the existing prefer-presence-queries rule, but since that rule has a presence/absence distinction and the motivating case toBeVisible does not, it is a better fit to handle it in its own standalone rule. That also allows users to configure handling for any number of matchers.

Do you want to submit a pull request to change the rule?

Yes

[Belco90]

Hi @CodingItWrong! Thanks for this suggestion, it makes sense to report toBeVisible as a presence matcher.

IIRC adding this matcher to the PRESENCE_MATCHERS should be enough. You could add the corresponding tests to prefer-presence-queries.test.ts, then include the new matcher in the PRESENCE_MATCHERS list and I believe it should work.

[CodingItWrong]

thanks @Belco90 — with that encouragement, I will try making this change, and see if the test suite + trying it out locally in an app gives the behavior I'd expect. I'll shoot to open up a PR either way 👍

[CodingItWrong]

Update: based on feedback on #730, we would like to implement this as a new rule rather than extending prefer-presence-queries. I plan to work on this soon.

[Belco90]

Thanks @CodingItWrong! Let us now if we can assist you somehow.

[CodingItWrong]

@Belco90 would you like me to edit this issue so that the title/description reflect the new plan?

[CodingItWrong]

@Belco90 When you said in #730 (comment)

I'd say we could create a new rule named like prefer-query-matchers, that receives through its options those valid entries of queries and matchers that must be used together (it could be also the other way around, so we define invalid query and matcher combinations, this is just an idea):

{
  "rules": {
    "prefer-query-matchers": ["error", { 
      "validEntries": [{ "query": "get", "matcher": "toBeVisible"}]
     }],
  }
}

Are you thinking that there would be no options configured by default, and that folks would have to specifically configure toBeVisible to require get?

If so, I sort of see the argument for that. But that would create a surprising difference with .toBeInTheDocument():

• Both toBeInTheDocument and toBeVisible are provided by jest-dom
• toBeVisible implies toBeInTheDocument, with additional checks for element visibility. This makes me think that toBeVisible should be preferred over toBeInTheDocument for positive checks, due to the TL guiding principle: users don't care if there is an invisible DOM element, they care if they can see the element.
• KCD's advice to prefer get over query isn't limited to toBeInTheDocument; he says "Only use the query* variants for asserting that an element cannot be found". https://kentcdodds.com/blog/common-mistakes-with-react-testing-library#using-query-variants-for-anything-except-checking-for-non-existence

Because of that, since eslint-plugin-testing-library is preconfigured to error on inappropriate query usage for toBeInTheDocument, I think it should also be preconfigured on query usage for toBeVisible, since there is an argument that it should be preferred for positive checks over toBeInTheDocument. Therefore, I think the new prefer-query-matchers should be configured by default to require get for toBeVisible positive and negative checks. It could still be configurable to set additional or different checks.

Thoughts?

[Belco90]

@Belco90 would you like me to edit this issue so that the title/description reflect the new plan?

Yes please, that would be great!

Are you thinking that there would be no options configured by default, and that folks would have to specifically configure toBeVisible to require get?

That was my idea indeed. I wanted to keep it simple and require some config from the user. We can leave some suggested config in the rule's doc.

I like the idea you suggest and agree with your explanation, but I'd say such preferences are debatable. Additionally, we would have to worry about the user overriding vs extending the initial provided config, which would make the first iteration more complicated.

[CodingItWrong]

OK thanks @Belco90, I'll proceed on that basis.

[Belco90]

@CodingItWrong Great, thanks! Keep in mind that my proposal was just my first idea. Feel free to repurpose the options or the implementation as you wish.

[CodingItWrong]

Just a progress note on this: @obsoke and I are pairing on implementing this, and we made good progress today. We think we will have a PR open in the next few weeks.

[github-actions]

🎉 This issue has been resolved in version 5.11.0 🎉

The release is available on:

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

Your semantic-release bot 📦🚀

[github-actions]

🎉 This issue has been resolved in version 6.0.0-alpha.15 🎉

The release is available on:

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

Your semantic-release bot 📦🚀