feat: add toBeVisible as a presence query

[CodingItWrong]

Checks

• I have read the contributing guidelines.

Changes

• Adds toBeVisible as a presence query, so that expect(screen.queryBy…).toBeVisible() will be recommended to be changed to expect(screen.getBy…).toBeVisible()

Context

Fixes #728

Questions

I am not fully confident if this PR yet handles the proposed behavior in #728 — both of the following should produce an error:

/* 5 */ expect(screen.queryByText("a")).toBeVisible();
/* 7 */ expect(screen.queryByText("a")).not.toBeVisible();

This is different from other presence queries, where expect(screen.queryBy…).not.toBeVisible() is correct.

Could a review please check if that is or isn't working as desired?

Since I just copied the toBeInTheDocument tests, I would think that the tests are not specifying that this is the case. But I don't have enough confidence in my understanding of the test suite to be sure. Also, I wasn't able to figure out how to run my local branch in a real React project; I got the error "ESLint couldn't find the plugin" when I tried yarn publish, yalc, and just specifying a package.json path to my local plugin folder.

[Belco90]

@CodingItWrong I didn't realize about these two cases were supposed to throw an error:

/* 5 */ expect(screen.queryByText("a")).toBeVisible();
/* 7 */ expect(screen.queryByText("a")).not.toBeVisible();

This is different from other presence queries, where expect(screen.queryBy…).not.toBeVisible() is correct.

This is basically the main reason, let me give you some context.

From the point of view of the rule, reporting case 5 but not reporting case 7 is expected. The goal of the rule is to encourage specific matchers for checking presence or absence. If a matcher is related to presence, then using get* queries with a positive matcher is correct (e.g. expect(screen.getByText("a")).toBeInTheDocument()). Otherwise, using query* queries or negated matcher will produce an error (e.g. expect(screen.queryByText("a")).toBeInTheDocument() and expect(screen.getByText("a")).not.toBeInTheDocument()). This basically means the matchers are always associated with either presence or absence, and the other usage will be considered as invalid.

On the other hand, from the point of view of toBeVisible matcher, case 5 and 7 are expected to be reported because we need an existing element to assert if it's visible or not. Then, this matcher can't be aligned with the concept of presence or absence from this rule, since the matcher must find an existing element before doing the assertion. What we want to do is encourage always using get* queries with toBeVisible matcher, which is different from the behavior described for this rule.

I'd say it doesn't make sense to include toBeVisible in prefer-presence-queries since the behavior doesn't match, so I'd close this PR. That doesn't mean there is no room for the desired behavior for toBeVisible matcher, but it should be placed in a new rule. 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"}]
     }],
  }
}

Then, this new rule would report the matchers given if found used with other queries than the valid ones defined.

What do you think?

[CodingItWrong]

@Belco90 Your assessment sounds totally right; I think it makes sense for this behavior to be implemented as a separate rule. Especially because there is so much already going on in the existing prefer-presence-queries rule. Plus, the new rule will be simpler, since we don't need to check presence vs absence.

I'm closing this PR and I've updated #728 to note the plan to create a new rule. I will shoot to work on that soon.