Move rules settings to ESLint shared config

[Belco90]

As discussed in different issues and PRs, we need to find a better approach for giving the user the possibility to define which custom render methods they use for Testing Library, or which custom modules they are importing render method.

This change will imply some breaking changes, so v4 would be the perfect opportunity to address this.

The idea is to move specific rules options duplicated across the plugin to ESLint shared settings.

You can add settings object to ESLint configuration file and it will be supplied to every rule that will be executed. This may be useful if you are adding custom rules and want them to have access to the same information and be easily configurable.

For that reason, this seems the proper approach for avoid duplicating rules configs.

What things do we need to setup there?

0. As pre-requisite we need to find a way to hook enhanced rules listener with detection helpers for all rules implementations. This has two purposes: extract the common checks (e.g. is imported from Testing Library? is a valid filename?) into a shared create rule function which already prevent rules to be executed if common checks are not met (this allows rules to focus on particular checks for the rule itself without worrying about global checks), and receive a set of detection helpers to detect Testing Library utils like "isQuery" or "isRender" (this allows rules to detect Testing Library patterns easier, reducing duplicated code, complexity and errors).

1. New shared setting to setup a custom module where Testing Libraries can be imported from other than the official package itself. There is an example here in Testing Library docs. + "aggressive reporting" by default when no custom module set + add corresponding detection helper

// render method from custom module
import { render } from 'test-utils';
// ...
const view = render(<Foo />) // corresponding rules will get setting to check if this render method has to be reported

2. New shared setting to setup the name pattern of the files desired to be analyzed (by default with same config from jest) + add corresponding detection helper THIS WAS FINALLY DISCARDED

// assume the user config the plugin to just report errors from files named *.test.js

// foo.spec.js
screen.findByText('something'); // <-- we don't report anything here since the file name doesn't match with desired filename pattern

3. Moving current renderFunctions option from existing rules to shared setting, so users can config there any custom render function they want to be considered consider by eslint-plugin-testing-library to be reported + add corresponding detection helper:

// custom render method from outside
import { renderWithTheme } from 'test-utils';
// ...
const view = renderWithTheme(<Foo />) // corresponding rules will get shared setting to check if this render method has to be reported

// custom inline render method
import { render } from '@testing-library/framework';
const setUp = (props) => <Foo {...props} />
// ...
const view = setUp() // corresponding rules will get shared setting to check if this render method has to be reported

4. Apply aggressive reporting to Testing Library queries so we assume every method starting by (get|query|find)(All)?By are related to Testing Library itself, doesn't matter if they are built-in ones (e.g. getByText) or custom ones (e.g. getByIcon) + remove current rules options for customizing extra queries + add corresponding detection helper

import { render, screen } from 'test-utils';

// ...
render(<MyComponent />);
screen.findByIcon('search'); // <-- we report this for not being awaited even if it's not a built-in query

Motivation

• We can't guess other custom methods the user uses as render if called other than that so better to set it up globally (applies to 3)
• We can't guess if something belongs to Testing Library if it's imported from a module other than "testing-library" one. For this by default we consider Testing Library always imported and all matching methods as coming from it. Then, the user can setup their custom module from where they reexport Testing Library utils, so the plugin will just report errors if utils are imported from "testing-library" or custom modules. (applies to 1)
• "Aggressive reporting" is motivated by this comment so we don't opt-out the reporting if the user has custom utils not coming directly from Testing Library. Instead, if they run into issues because the plugin is reporting things out of the expected scope, they can 1) customize the methods considered as render and 2) strict the scope of analyzed files to those matching their desired pattern (applies to 2, 3)
• Related to "aggressive reporting", instead of checking just official queries combinations from Testing Library, we will check them by pattern so if they match a Testing Library pattern we report them. Again, we avoid opting-out reporting if the user has a custom query (e.g. getByIcon) but they forgot to set it up. Instead, we check if they follow TL query naming pattern and report them as any built-in query. There is nothing to configure here, for now at least. (applies to 4)

Extra things

• Update README to explain global settings
• Update README to explain "aggressive mode", its motivation and how to restrict its scope
• Update rules docs to delete references to removed options

---

Tasks

New reporting + settings requirements

• Hooking detection helpers into rule creator + fake rule to test detection mechanism: Move rules settings to ESLint shared config: part 1 - utils detection #237
• Setting for module to import TL + aggressive TL import detection + detection helper (requirement 1): Move rules settings to ESLint shared config: part 2 - check imports #239
• Setting for filename pattern + detection helper (requirement 2): Move rules settings to ESLint shared config: part 3 - check reported file name #244, refactor: use micromatch for matching files to be reported #296 removed in refactor: remove mechanism to match files to be reported #297
• Setting for custom render methods + detection helper (requirement 3): Move rules settings to ESLint shared config: Aggressive Render Reporting + refactor render-result-naming-convention #280, refactor(render-result-naming-convention): refine checks to decide if coming from Testing Library #282
• Aggressive query reporting + detection helpers (requirement 4): Move rules settings to ESLint shared config: refactor prefer-presence-queries rule #252, Move rules settings to ESLint shared config: refactor await-async-query and no-await-sync-query #260

Rules pending to be refactored

✏️ - blocked by custom render setting + detection helper (requirement 3)
❗ - blocked by aggressive query reporting + detection helper (requirement 4)
🚧 - WIP by someone

• await-async-query ❗: Move rules settings to ESLint shared config: refactor await-async-query and no-await-sync-query #260
• await-async-utils: Move rules settings to ESLint shared config: refactor await-async-utils #263
• await-fire-event: Move rules settings to ESLint shared config: refactor await-fire-event #265
• consistent-data-testid: Move rules settings to ESLint shared config: part 5 - refactor consistent-data-testid rule #248
• no-await-sync-events: Move rules settings to ESLint shared config: refactor no-await-sync-events rule #302
• no-await-sync-query ❗: Move rules settings to ESLint shared config: refactor await-async-query and no-await-sync-query #260
• no-container ✏️ : Move rules settings to ESLint shared config: refactor no-container rule  #295
• no-debug ✏️ : Move rules settings to ESLint shared config: refactor no-debug rule  #293
• no-dom-import: Move rules settings to ESLint shared config: part 4 - refactor no-dom-import rule #247
• no-manual-cleanup: Move rules settings to ESLint shared config: part 6 - refactor no-manual-cleanup rule #249
• no-multiple-assertions-wait-for: Move rules settings to ESLint shared config: refactor no-wait-for-multiple-assertions rule #301
• no-node-access: Move rules settings to ESLint shared config: part 1 - utils detection #237
• no-promise-in-fire-event: Move rules settings to ESLint shared config: refactor no-promise-in-fire-event #266
• no-render-in-setup ✏️ : Move rules settings to ESLint shared config: refactor no-render-in-setup rule #299
• no-side-effects-wait-for: Move rules settings to ESLint shared config: refactor no-wait-for-side-effects rule #300
• no-wait-for-empty-callback: Move rules settings to ESLint shared config: refactor no-wait-for-empty-callback #284
• no-wait-for-snapshot: refactor: migrate no-wait-for-snapshot to v4 #271
• prefer-explicit-assert ❗ *remove custom query option too: refactor(prefer-explicit-assert): use new utils and remove custom query option #274
• prefer-find-by: refactor: migrate prefer-find-by to v4 #270
• prefer-presence-queries ❗: Move rules settings to ESLint shared config: refactor prefer-presence-queries rule #252
• prefer-screen-queries ✏️❗: Move rules settings to ESLint shared config: refactor prefer-screen-queries rule #285
• prefer-user-event: feat: add new settings for prefer-user-event #251
• prefer-wait-for refactor: prefer-wait-for with the new settings #255
• render-result-naming-convention ✏️ : Move rules settings to ESLint shared config: Aggressive Render Reporting + refactor render-result-naming-convention #280, refactor(render-result-naming-convention): refine checks to decide if coming from Testing Library #282

[gndelia]

I think it pretty much sums up.
We should probably consider this in the tests of all rules

[Belco90]

I think it pretty much sums up.
We should probably consider this in the tests of all rules

It sounds like a good idea. Also those two utils mentioned should have a nice bunch of unit tests. Our utils don't have tests so it's a good opportunity to start with some.

[Belco90]

I'm gonna start working on it as I finished the rule I was implementing.

[gndelia]

I'm gonna start working on it as I finished the rule I was implementing.

let me know if you need help, I just created a PR and I'm open to taking more tasks; otherwise, I'll grab v3 issues 😅

[renatoagds]

I'm open to help too if you need something 😊

[Belco90]

@gndelia @renatoagds thanks guys! I think I need to work on changing the config by myself. Then, I'll update rules currently using the renderFunction option to use the new mechanism. So at that point, I think you can help me applying the same mechanism to the remaining rules. I'll let you know when I achieve stuff needed for it.

[Belco90]

Hey guys. Sorry I haven't been active with this one, but I realized about something couple of days ago which I think implies more work than expected related to point 2).

Then add another key to shared setting for being able to setup a custom module where render can be imported from. There is an example here in Testing Library docs.

// render method from custom module
import { render } from 'test-utils';
// ...
const view = render(<Foo />) // corresponding rules will get shared setting to check if this render method has to be reported

There are more things affected here. Basically, everything could be imported from a custom module! That's actually what Testing Library docs suggests: reexporting everything from this custom module. I didn't realize about it, so technically it would be possible to import other stuff like screen or fireEvent:

// other testing library methods reexported from custom module
import { screen, fireEvent } from 'test-utils';
// ...
const button = screen.getByRole('button'); // this screen should be included to be reported too
fireEvent.click(button); // this fireEvent should be included to be reported too

So this led me update point 2) to instead of:

2. hasTestingLibraryImportModule should check if any import from LIBRARY_MODULES or custom modules in shared setting is found

we would need to repurpose hasTestingLibraryImportModule to hasTestingLibraryUtilImport which receives an arg about what util name you want to check if it's imported or not:

// the user has setup 'test-utils' as custom testing library module in ESLint shared config
import { screen } from 'test-utils';
import { fireEvent } from '@testing-library/react;
import { render } from '@somewhere/else';

// ...
hasTestingLibraryUtilImport(node, 'screen') // true
hasTestingLibraryUtilImport(node, 'fireEvent') // true
hasTestingLibraryUtilImport(node, 'render') // false

I thought this would be enough, but what if the import is renamed? import { screen as testingScreen } from 'test-utils'; --> this would be a dangerous case as our util method would say screen is imported and we would look for that import name in the rules implementation, when we should look for testingScreen instead.

So I think the final approach for point 2) should be a getTestingLibraryUtilImport method which returns the final import name if found, or null otherwise:

// the user has setup 'test-utils' as custom testing library module in ESLint shared config
import { screen as testingScreen } from 'test-utils';
import { fireEvent } from '@testing-library/react;
import { render } from '@somewhere/else';

// ...
getTestingLibraryUtilImport(node, 'screen') // 'testingScreen'
getTestingLibraryUtilImport(node, 'fireEvent') // 'fireEvent'
getTestingLibraryUtilImport(node, 'render') // null

So getTestingLibraryUtilImport has a double usage for covering everything:

1. if the returned value is truthy, then corresponding method is considered as imported from Testing Library (either default package or custom module). If falsy, then there is not module imported from it.
2. if the returned value is a string, that's the final import name of the given imported method.

Thoughts?

[gndelia]

sorry for the delay in the response! It has been a couple of rough days 😅

I am not sure if alias are a problem in the import - when analyzing the AST you can still match them by the original name and then pick up the alias. From there you'd use that. I think some rules solve it in that way

I agree on all of what you say; my only doubt is about userEvent because it's a separated install from RTL but could be also re-exported in that custom utils import (I actually do so in a project I work for), but in reality, it comes from a separated package. It's not a tech problem per-se, but it might be confusing because the method is supposed to check the Testing Library Import only (at some point, whenever user event gets merged in RTL, this shouldn't be a problem anymore) but it'd be also checking for userEvent

Other than that, I agree with the proposal

[Belco90]

Well, maybe we can add a 3rd optional arg to getTestingLibraryUtilImport so it would be flexible enough to check if imported from custom module defined in shared ESLint settings or from user-event module. Something like:

getTestingLibraryUtilImport(node, 'screen') // 'testingScreen'
getTestingLibraryUtilImport(node, 'fireEvent') // 'fireEvent'
getTestingLibraryUtilImport(node, 'userEvent', ['@testing-library/user-event']) // 'userEvent'
getTestingLibraryUtilImport(node, 'userEvent') // null

If we go for this option, I'd move this to an object arg to avoid having 3 args.

[Belco90]

@gndelia @timdeschryver @thomlom I'm finding more issues/questions while developing this. I've created a simple draft with couple of functions and their expected usage, so you can see what are my intentions and the problems I found: #217

I'm gonna comment my code there.

[Belco90]

I love this comment from @CreativeTechGuy: #222 (comment)

I think this is the right way of checking Testing Library stuff on v4: rather than trying to scope rules to directly imported Testing Library utils by default, the plugin should be really aggressive and report everything matching Testing Library names by default, without caring about where they are imported from. Then, the user can config from which custom module they want to check the utils, or which are the custom queries to be included.

It doesn't change that much the implementation of those utils I still have pending. I hope I can check it this week and make some progress.

[gndelia]

Sorry for the very long delay. 😅
I read that ticket and I like the idea of the aggressiveness, as long as we support the full configuration to set the custom imports. Otherwise,we just get false positives and we also get users that would have to patch the code because of that.

Other than that, it would mean just changing the "Default behaviour" from what we've defined as the approach to take; is that correct?

[Belco90]

Exactly, it would mean just changing the default behavior from "report just Testing Library things" to "report everything matching Testing Library patterns". Then when user sets custom renders, queries or something else, we would switch to "report just Testing Library things + custom things".

[Belco90]

So as mechanism to make Testing Library rules seems close in #237 I've done an analysis over shared config we need and how they affect to all rules to be refactored. You can find info about this analysis in this notion doc I wrote down

I'll put this on corresponding GitHub issues or project, but first I need to address few of the things commented in that notion doc notes. After that, we can start splitting the job if someone could help 😅 . I can finish this myself otherwise, but I want to share with all you at least for some sanitization checks!

[MichaelDeBoey]

This one's done! 👊

Congrats to the team! 🥳🎉

[Belco90]

Finally! I'll add more details about next steps on v4 issue.