Skip to content

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

y-hsgw
Copy link
Member

@y-hsgw y-hsgw commented Aug 14, 2025

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:

document.getElementById('hoge').click();

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!

Copy link

codecov bot commented Aug 14, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.37%. Comparing base (1467a18) to head (1139feb).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1064      +/-   ##
==========================================
- Coverage   96.45%   96.37%   -0.09%     
==========================================
  Files          50       50              
  Lines        2709     2675      -34     
  Branches     1124     1096      -28     
==========================================
- Hits         2613     2578      -35     
- Misses         96       97       +1     

☔ 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.

@y-hsgw y-hsgw changed the title fix(rule): narrow event-handler detection to Testing Library query results to avoid false positives fix(no-node-access): narrow event-handler detection to Testing Library query results to avoid false positives Aug 14, 2025
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.

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?

@Belco90 Belco90 changed the title fix(no-node-access): narrow event-handler detection to Testing Library query results to avoid false positives fix(no-node-access): narrow detection to Testing Library query Aug 15, 2025
@Belco90 Belco90 changed the title fix(no-node-access): narrow detection to Testing Library query fix(no-node-access): narrow detection to Testing Library queries Aug 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants