Bug: no-dom-import false negatives with several Testing Library imports

[Belco90]

Have you read the Troubleshooting section?

Yes

Plugin version

5.3.3

ESLint version

v8.14.0

Node.js version

v16.15.0

package manager and version

npm v8.9.0

Operating system

macOS v12.3.1

Bug description

If there are several Testing Library imports, no-dom-import doesn't report @testing-library/dom imports if they are other than the first occurrence between Testing Library imports.

Steps to reproduce

This should be reported but it's not:

import { render } from '@testing-library/react'
import { screen } from '@testing-library/dom'
// ^ this import is not reported since it's not the first Testing Library import

import { App } from '../App'

it('should render a basic demo', () => {
  render(<App />)
  expect(screen.getByText('Hello Parcel + React!')).toBeInTheDocument()
})

Error output/screenshots

N/A

ESLint configuration

N/A

Rule(s) affected

no-dom-import

Anything else?

This is related to how we store the imported Testing Library module in the internal helpers. This only allows one Testing Library module, so the first one found it's the chosen one for later checks.

I remember mentioning this in another issue/PR, but I can't remember which one. This is not that simple to fix since we need to store a list of Testing Library modules used rather than a single one, updating all the helpers provided internally too.

After we allow storing several Testing Library modules in our core helpers, we will be able to fix the actual issue reported.

Do you want to submit a pull request to fix this bug?

Yes

[Belco90]

Actually, it doesn't need to update the internal helpers if we just look for the corresponding imports within this rule. I'll give it a try later.

[sjarva]

@Belco90 I noticed that you had said that you'd like to submit a PR to fix this. Would you mind if I submit one, since you haven't done one yet?

[Belco90]

@Belco90 I noticed that you had said that you'd like to submit a PR to fix this. Would you mind if I submit one, since you haven't done one yet?

I couldn't work on this, so please feel free to submit a PR to fix it! 🙏

[sjarva]

Alright, the PR is now open! I noticed that other rules (prefer-wait-for and no-manual-cleanup) also use the old util function that returns just the first Testing Library import. I didn't refactor them to use this new util that returns all Testing Library imports, because I wasn't sure if there are false negatives with the old implementation. If a refactor is needed, I can work on that too, either on a new PR or #657.

[github-actions]

🎉 This issue has been resolved in version 5.7.1 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[Belco90]

Alright, the PR is now open! I noticed that other rules (prefer-wait-for and no-manual-cleanup) also use the old util function that returns just the first Testing Library import. I didn't refactor them to use this new util that returns all Testing Library imports, because I wasn't sure if there are false negatives with the old implementation. If a refactor is needed, I can work on that too, either on a new PR or #657.

Refactoring those rules would be amazing! Better to open an issue for that so we can discuss the approach properly.