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-in-document): false positive on .toHaveLength(1) matcher with *AllBy* query #273

Closed
wants to merge 8 commits into from

Conversation

SevenOutman
Copy link
Contributor

@SevenOutman SevenOutman commented Oct 9, 2022

What:

Close #171

Why:

It's valid to use .toHaveLength(1) with *AllBy* queries to check for "exactly one match".

See conclusion of discussions at #171 (comment)

How:

Implement the truth table as described in the link above.

Checklist:

  • Documentation
  • Tests
  • Ready to be merged

@SevenOutman SevenOutman marked this pull request as ready for review October 9, 2022 08:31
@SevenOutman SevenOutman changed the title fix(prefer-in-document): make toHaveLength(1) with AllBy queries valid fix(prefer-in-document): false positive on .toHaveLength(1) matcher with *AllBy* query Oct 9, 2022
@G-Rath
Copy link
Collaborator

G-Rath commented Nov 8, 2022

hey sorry for the delay - would you mind rebasing this so that we've got the CI checks going? then I'll do a review

@SevenOutman
Copy link
Contributor Author

@G-Rath Sure, I have rebased it.

Copy link
Collaborator

@G-Rath G-Rath left a comment

Choose a reason for hiding this comment

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

Looks good - got some minor changes I'd like made, and you also need to rebase again (sorry 😅) and get CI passing (you're missing a bit of coverage)

* screen.<query>() -> <query>
*/
function getDTLQueryIdentifierNode(callExpressionNode) {
if (!callExpressionNode || callExpressionNode.type !== "CallExpression") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm pretty sure we can use optional chaining here:

Suggested change
if (!callExpressionNode || callExpressionNode.type !== "CallExpression") {
if (callExpressionNode?.type !== "CallExpression") {

Copy link
Contributor Author

@SevenOutman SevenOutman Nov 11, 2022

Choose a reason for hiding this comment

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

I applied the suggestion but it turned out that ESLint does not recognize optional chaining syntax in this project, so I undid the changes.

src/rules/prefer-in-document.js Show resolved Hide resolved
src/rules/prefer-in-document.js Outdated Show resolved Hide resolved
src/rules/prefer-in-document.js Show resolved Hide resolved
src/rules/prefer-in-document.js Show resolved Hide resolved
@SevenOutman
Copy link
Contributor Author

All right, I've merged main branch and updated the suggestion message.

@G-Rath
Copy link
Collaborator

G-Rath commented Jan 14, 2023

@SevenOutman if you're still interested in this, could you resolve the test failures?

@SevenOutman
Copy link
Contributor Author

@SevenOutman if you're still interested in this, could you resolve the test failures?

Sorry, I'm afraid I don't have enough mind bandwidth for this edit right now.

@EricKim987
Copy link
Contributor

Can I try taking over this pr? Or should I just create a new one? I really want to fix this. @G-Rath

@G-Rath
Copy link
Collaborator

G-Rath commented Jul 4, 2023

@EricKim987 go for it - I think it would be best to create a new PR though

@github-actions
Copy link

github-actions bot commented Aug 5, 2023

🎉 This issue has been resolved in version 5.0.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@SevenOutman SevenOutman deleted the doma/171 branch August 7, 2023 02:56
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.

prefer-in-document: .toHaveLength(1) vs .toBeInTheDocument()
4 participants