Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(prefer-explicit-assert): only enforce assertion when presence/absence matcher #238

Merged
merged 1 commit into from
Oct 16, 2020
Merged

fix(prefer-explicit-assert): only enforce assertion when presence/absence matcher #238

merged 1 commit into from
Oct 16, 2020

Conversation

skovy
Copy link
Collaborator

@skovy skovy commented Oct 13, 2020

Problem

This is a follow-up improvement to issue #218.

The original implementation was flawed. With the way this was implemented, any getBy* query will trigger a linting error if it's not using toBeInTheDocument (assuming the config defined the assertion to use as toBeInTheDocument). However, there are still valid cases to use a different assertion. For example, if you want to check if a button is disabled (toBeDisabled) or maybe check if an anchor has the appropriate href attribute (toHaveAttribute).

With the way the rule was implemented, both of these examples would trigger as violations (since they're not using toBeInTheDocument) but they're both still valid uses of getBy* and thus are false positives.

Solution

Now, this rule only triggers if the matchers used are toBeInTheDocument, toBeTruthy, toBeDefined, not.toBeNull, or not.toBeFalsy and doesn't match the configured assertion. These matches already existed as a constant for another rule (prefer-presence-queries), so I moved them to utils so they could be reused. This rule also had somewhat related logic for handling negated matchers. This logic was adapted for this rule.

@skovy skovy marked this pull request as ready for review October 13, 2020 03:15
@@ -144,14 +152,44 @@ ruleTester.run(RULE_NAME, rule, {
code: `expect(getByText('foo')).toBeDefined()`,
options: [
{
assertion: 'toBeInDocument',
assertion: 'toBeInTheDocument',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one 💯

Belco90
Belco90 previously approved these changes Oct 15, 2020
Copy link
Member

@Belco90 Belco90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking really good! Thanks also for taking the time to refactor and extract those utils, really appreciated.

@Belco90 Belco90 dismissed their stale review October 15, 2020 15:35

Should rule's doc be updated?

@Belco90
Copy link
Member

Belco90 commented Oct 15, 2020

@skovy Should rule's doc be updated to reflect this change?

@skovy
Copy link
Collaborator Author

skovy commented Oct 15, 2020

I think the current docs are accurate, but I can update with more details about the specific logic and thinking around this rule.

@skovy skovy changed the title fix(prefer-explicit-assert): only enforce assertion when truthy fix(prefer-explicit-assert): only enforce assertion when presence/absence matcher Oct 16, 2020
@skovy
Copy link
Collaborator Author

skovy commented Oct 16, 2020

@Belco90 updated the docs, let me know if that was roughly what you had in mind or something else?

Copy link
Member

@Belco90 Belco90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, that's more than enough. Thanks!

@Belco90 Belco90 merged commit f78720d into testing-library:master Oct 16, 2020
@Belco90
Copy link
Member

Belco90 commented Oct 16, 2020

🎉 This PR is included in version 3.9.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants