-
Notifications
You must be signed in to change notification settings - Fork 153
fix(no-node-access): narrow detection to Testing Library queries #1064
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(no-node-access): narrow detection to Testing Library queries #1064
Conversation
Codecov Report
✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.41%. Comparing base (1467a18) to head (f749e12).
Additional details and impacted files
@@ Coverage Diff @@ ## main #1064 +/- ## ========================================== - Coverage 96.45% 96.41% -0.05% ========================================== Files 50 50 Lines 2709 2675 -34 Branches 1124 1096 -28 ========================================== - Hits 2613 2579 -34 Misses 96 96
☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.
🚀 New features to boost your workflow:
- ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
- 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this approach, which is in the same direction of getting rid of the aggressive reporting (I'm still writing an RFC for it), so we only report for Testing Library utils.
the direct element retrieval itself (in this example, getElementById) is already reported, so the lack of a report here should not be an issue.
I'm not sure about what rule would report document.getElementById('hoge').click();, @y-hsgw can you clarify this?
Thanks! I’ll proceed with this approach and focus on increasing the test coverage.
I'm not sure about what rule would report document.getElementById('hoge').click();,
You’re right—that was my mistake; I misunderstood. We don’t currently report document.getElementById('hoge').click(). Sorry for the confusion on my part 🙇♂️
I’ve increased the test coverage and completed the implementation. Please review.
Uh oh!
There was an error while loading. Please reload this page.
Checks
Changes
This PR changes the rule to only report native DOM event method calls (e.g.
click(),select()...) when the receiver is an element obtained via Testing Library queries (e.g.screen.getBy*,findBy*,queryBy*, chains).Previously: the rule broadly reported any event handler method that was not fireEvent or userEvent.
Now: the rule reports only when event handler methods are called on results of Testing Library queries.
This reduces false positives while keeping the intended guidance.
Background
The prior, wide-scoped detection caused false positives when non–Testing Library APIs exposed methods with the same names as DOM event handlers.
Related reports:
Limitation: This rule does not report cases where an element is obtained directly from document, for example:
However, we consider this acceptable because, in our setup, the direct element retrieval itself (in this example,
getElementById) is already reported, so the lack of a report here should not be an issue.This is a proposal PR. Please review, and if there are no concerns, I’ll proceed to broaden the test coverage afterward!