refactor: use micromatch for matching files to be reported

[Belco90]

Relates to #198

After discussing this with @MichaelDeBoey and rethinking about using micromatch instead of regexp, I decided to switch this since ESLint.getFileName() returns not just the filename itself but its path (I had to put a console.log in an alpha version to verify this). Since I have the whole path then, I can match more than just the filename for reporting Testing Library files or not, so the plugin will match same as Jest by default: files under __tests__ folder or files with .spec or .test suffix.

[MichaelDeBoey]

Looking at the tests now, I think we should just remove the file-patterns rule? 🤔
We already said that we could actually do that by using ESLint itself and just do a file/path override.

Looking at the tests here, it really feels wrong to me to have to add the file-pattern setting in each test or to provide the fileName.

Also: we now have 2 ways of telling "apply these rules to these files"

[MichaelDeBoey]

After getting some more information by @Belco90, I came to the conclusion that I misunderstood why we sometimes have set the setting of filename rule.

LGTM! 👊

[Belco90]

I'm gonna clarify couple of things so everyone has the proper context.

The filename property in the tests is just the way to simulate the name of the file being reported. For example, a test like this:

{
  settings: { 'testing-library/file-patterns': ['**/?(*.)+(not-matching).[jt]s?(x)'] },
  code: `const button = screen.getByRole('button')`,
  filename: 'project/src/MyComponent.test.ts'
}

it's literally simulating the following case:

// MyComponent.test.ts, located at "project/src/"
const button = screen.getByRole('button')

// .eslintrc
settings: { 'testing-library/file-patterns': ['**/?(*.)+(not-matching).[jt]s?(x)'] }

So the filename is not anything the user has to setup, but a way to simulate an actual filename for testing within an ESLint test case.

Having said that, why adding this file-patterns setting? I need to give explain some background here. One of the new features on v4 is Aggressive Reporting. This is an opted-in behavior to assume things that could be Testing Library utils are actually Testing Library utils. This behavior was motivated by the discussion in #222

What's the purpose of this Aggressive Reporting then? It will assume:

• everything matching a Testing Library util naming pattern will be considered as something to be reported, no matter if has been imported from @testing-library/react or somewhere-else
• every function including render on its name will be considered as a Testing Library render.

Why?

• it's pretty common for Testing Library users to re-export all utils from their own module. If we only look for utils imported from @testing-library/module then we will be opting-out everything coming from a custom module (which usually is test-utils). Having a setting to set your custom modules wouldn't help too much if users don't realize about they have to set their custom modules in order to be reported. Instead, with Aggressive Reporting we don't care from which module the utils are imported and just assumed they have to be reported, so we don't silence potential errors of utils imported from custom modules.
• the most common reason for creating a custom module is to have a custom render. Usually, this custom render overwrites the default one, so it's called render too. But it's also common to have custom renders like renderWithRedux and so (it's not a good practices, but that's not the point here). So similar to the previous point, if we look only for render as a reportable Testing Library render util we could be silencing errors coming from custom renders, and users could be missing this if they had to set their custom renders up in some setting. Again, the Aggressive Reporting will assume everything named including "render" is a valid render.

This is Aggressive Reporting already implemented in v4: it assumes things that are not 100% sure if could be related to Testing Library are actually related. So far so good. But what if someone needs to disable this for any reason? That's where the global settings come into play. The two first settings we introduced are:

• "testing-library/utils-module": string - this setting allows you to indicate the module file name from where you'd like to consider utils re-exported. This opts-out the first behavior of Aggressive Reporting, so it only considers @testing-library/* and your custom utils-module as reportable modules. Let's say you set this setting like: "testing-library/utils-module": "test-utils": This will report all Testing Library utils found imported from @testing-library/react and test-utils but not from whatever or my-testing-module.
• "testing-library/custom-renders": string[] - this setting allows you to indicate which are the list of names the be reported as render. This opts-out the second behavior of Aggressive Reporting, so it only considers render and the list of given names as reportable renders. Let's say you set this setting like `I'm gonna clarify couple of things so everyone has the proper context.

The filename property in the tests is just the way to simulate the name of the file being reported. For example, a test like this:

{
  settings: { 'testing-library/file-patterns': ['**/?(*.)+(not-matching).[jt]s?(x)'] },
  code: `const button = screen.getByRole('button')`,
  filename: 'project/src/MyComponent.test.ts'
}

it's literally simulating the following case:

// MyComponent.test.ts, located at "project/src/"
const button = screen.getByRole('button')

// .eslintrc
settings: { 'testing-library/file-patterns': ['**/?(*.)+(not-matching).[jt]s?(x)'] }

So the filename is not anything the user has to setup, but a way to simulate an actual filename for testing within an ESLint test case.

Having said that, why adding this file-patterns setting? I need to give explain some background here. One of the new features on v4 is Aggressive Reporting. This is an opted-in behavior to assume things that could be Testing Library utils are actually Testing Library utils. This behavior was motivated by the discussion in #222

What's the purpose of this Aggressive Reporting then? It will assume:

• everything matching a Testing Library util naming pattern will be considered as something to be reported, no matter if has been imported from @testing-library/react or somewhere-else
• every function including render on its name will be considered as a Testing Library render.

Why?

• it's pretty common for Testing Library users to re-export all utils from their own module. If we only look for utils imported from @testing-library/module then we will be opting-out everything coming from a custom module (which usually is test-utils). Having a setting to set your custom modules wouldn't help too much if users don't realize about they have to set their custom modules in order to be reported. Instead, with Aggressive Reporting we don't care from which module the utils are imported and just assumed they have to be reported, so we don't silence potential errors of utils imported from custom modules.
• the most common reason for creating a custom module is to have a custom render. Usually, this custom render overwrites the default one, so it's called render too. But it's also common to have custom renders like renderWithRedux and so (it's not a good practices, but that's not the point here). So similar to the previous point, if we look only for render as a reportable Testing Library render util we could be silencing errors coming from custom renders, and users could be missing this if they had to set their custom renders up in some setting. Again, the Aggressive Reporting will assume everything named including "render" is a valid render.

This is Aggressive Reporting already implemented in v4: it assumes things that are not 100% sure if could be related to Testing Library are actually related. So far so good. But what if someone needs to disable this for any reason? That's where the global settings come into play. The two first settings we introduced are:

• "testing-library/utils-module": string - this setting allows you to indicate the module file name from where you'd like to consider utils re-exported. This opts-out the first behavior of Aggressive Reported so it only considers @testing-library/* and your custom utils-module as reportable modules. Let's say you set this setting like: "testing-library/utils-module": "test-utils": this will report all Testing Library utils found imported from @testing-library/react and test-utils but not from whatever or my-testing-module.
• "testing-library/custom-renders": ["renderWithRedux", "renderAll"]: this will report only things coming from render, renderWithRedux and renderAll but not from renderWithProviders or other names

Of course these 2 settings are related, so if you set them both, only things from indicated renders imported from indicated Testing Library modules would be reported.

So this is everything about the Aggressive Reporting, its inspiration and how to opt it out. However, could this Aggressive Reporting be too much in some situations? It could be: rules like no-node-access are matching methods which can be used outside a test file, and they could be potentially reported in a regular file since Aggressive Reporting could assume those methods are coming from Testing Library.

This is why I thought about adding the 3rd setting modified on this PR: "testing-library/filename-pattern". This setting allows you to define which patterns your files should match to be reported by the plugin. By default it will match the same as Jest: files under __tests__ folder and files with suffix .spec or .test. This way we make sure we only report things from tests files right? Yes, but it's actually the opposite idea from previous two, where we assume everything is valid and then the user can opt-out those behaviors with some settings. This one is actually defining some config already, and the user can change it if necessary.

This is nice to only report tests files, but can silence errors again. ESLint already allows you to run specific configurations for a subset of files matched by glob pattern, so we are re-adding the same behavior which can lead to false negatives situations really easy: the user has a different pattern for their files (e.g. only files with suffix .testing-library) so the defaults enabled by our 3rd setting would silence the errors in their tests. This is actually the opposite of what we want to achive with the Aggressive Reporting, and it's the main reason to remove this setting.

So, after this massive explanation, what do you think we should do with this setting?

• remove it: user could get errors reported in other files than tests ones. Solve it would be simple since they'll notice an undesired error reported, will open an issue/discussion here (or find some doc about this within the repo), and we will point them to ESLint overrides so they run eslint-plugin-testing-library only for their tests files.
• keep it: user wouldn't get errors reported in other files than tests ones, but can lead to silenced errors if they use a different tests location/pattern than the usual one and don't realize they need to set it up. This is dangerous since they might not ever notice about it. It also add double configuration in case they already use ESLint overrides since they'll have to indicate the same config in testing-library/filename-pattern setting

[Belco90]

@CreativeTechGuy pinging you here in case you have some feedback about it :)

[MichaelDeBoey]

I think @kentcdodds can give some valuable input on this idea too

[kentcdodds]

Long issue. What do you want my feedback on?

[CreativeTechGuy]

I think in the spirit of the aggressive reporting, it would make sense to report in all files. But I can see how a lot of people would really hate this especially if they upgrade to v4 and suddenly have hundreds of linting errors which might cause people to immediately downgrade again.

What about this compromise, custom lint error messages depending on the file path. This way the error sort of acts like a suggestion when it's less confident. If the file containing the error matches the condition you mention above (same as Jest) then the normal error message can be shown. It's fairly safe to assume in that context that it's a real error. But if the error appears in non .test or .spec files then I'd like to see a slightly different, less authoritative, error message with a link/info to update their settings if that was a false positive.

I think the worst thing that this aggressive reporting update could do is be wrong far too often that it becomes overly painful and people avoid using it. And for any new users, the documentation for these global settings should be front and center in the setup instructions.

That's my 2c at first glance. Great work on this! Excited to see it finally released. :)

[Belco90]

@CreativeTechGuy that's a really interesting approach indeed! What we can do is wrapping errors reported from outside those paths like Potential error: <error description>. If you think this is an error, please check <link to a .md in our repo explaining why it might not be an error, and how to setup the ESLint overrides>. This means we would leave the setting just in case, and we would reuse the mechanism for enhancing the report function (I think it should be easy to do so).

To be honest, I don't think the v4 will report almost anything on a regular file for most of the users. Even if we have this Aggressive reporting in place, the utils names must much with Testing Library ones and I don't think it's usual to find waitFor, getBy*, findBy*, queryBy*, render or fireEvent in a regular file. The only ones I'm concerned is debug from no-debug and DOM Node methods from no-node-access, and still shouldn't happen that often.

[kentcdodds]

To be clear, the objective here is basically to avoid this rule applying on non-test files?

If that's the case, then that's outside the scope of this package. The responsibility of which rules apply to which files should be within the person configuring ESLint. They should use ESLint's overrides functionality or place an .eslintrc within the directory they want to enable rules for. I've never heard of a plugin conditionally applying its rules based on whether a file matches some glob, and I wouldn't recommend it.

[Belco90]

To be clear, the objective here is basically to avoid this rule applying on non-test files?

@kentcdodds yeah it was basically that. I realized while writing the whole explanation around deciding to apply the rules based on file matching a pattern that it may be doing more harm than good.

I like the idea @CreativeTechGuy described tho, so we could give some hint to the user if the plugin is not sure about reporting a testing file, but I'll leave it as a nice to have to be discussed in the future after v4 is released.

I'm closing this issue then in favor of creating a new one removing this mechanism. Thanks guys, specially @MichaelDeBoey for starting to test an alpha of v4 and making me rethink about all this stuff!

PS: at least I can use my massive message here as a draft for writing some documentation for next version of the plugin 😅

[kentcdodds]

I know how it feels to put a bunch of work into something and then realize it's not the right direction. Real bummer. But I think you're making the right call 👍 Thanks for being receptive to the feedback!

[Belco90]

No problem. I'm more worried about having some sanity checks around this kind of decisions than delete code to be honest. I've been writing this new version for so long that I don't know if what I'm doing makes sense anymore 🙃 . Thanks for your help mates, really appreaciate it.

[MichaelDeBoey]

I like the idea @CreativeTechGuy described tho, so we could give some hint to the user if the plugin is not sure about reporting a testing file, but I'll leave it as a nice to have to be discussed in the future after v4 is released.

I think we could indeed add this as a nice to have after v4 is released.
Let's focus on having v4 released and making sane decisions about all (new) rules 👊