Add rules from "Common mistakes with React Testing Library"

[nickmccurdy]

There are some great suggestions in Common mistakes with React Testing Library, a few of which we could probably check statically:

1. Using wrapper as the variable name for the return value from render
2. Wrapping things in act unnecessarily (might be tricky, but maybe we could filter AST nodes and only error if something is used that isn't from testing-library)
3. Having multiple assertions in a single waitFor callback
   

Already implemented but should maybe be enabled by default:

1. Using cleanup
2. Using container to query for elements
3. Passing an empty callback to waitFor

Related issues: #133 #134 #162

[Belco90]

Hey Nick, thanks for bringing this discussion here!

Indeed, there are some other common mistakes we can prevent. From your list of new rules:

1. This one is an easy one to implement. Just something like no-render-wrapper or something similar would be enough. Should be a fixable rule? I'd rather not, as we would have to rename all appearances of wrapper const defined by the user and we can mess up the code
2. Not really sure about this one. Can we really check this statically? I'm afraid about throwing errors for something that is actually fine.
3. We were discussing around this one when implementing the prefer-find-by one. I think it needs more discussion to clarify the exact behavior for it. Do we care if there are several expects but also other stuff? Should be fixable? What if it's fixable the first thing within waitFor callback isn't an expect? (I guess in this case we won't automatically fix it).

About already implemented ones:

1. We didn't enable no-manual-cleanup for different reasons you can find in rule description. Basically 1) it depends on your testing framework and 2) can be skipped with a env var.
2. Do we have a rule for preventing querying from container? It's been around lately, but we didn't implement anything for know IIRC.
3. Something similar to the cleanup one: we didn't enable no-wait-for-empty-callback by default because it's still allowed to pass an empty callback. It can be really annoying rule for people migrating to new waitFor method. Could be a good idea to enable it by default when waitFor is forced to have non-empty callback tho.

[nickmccurdy]

1. This one is an easy one to implement. Just something like no-render-wrapper or something similar would be enough. Should be a fixable rule? I'd rather not, as we would have to rename all appearances of wrapper const defined by the user and we can mess up the code

Alternatively, could we possibly extend/reuse/configure prefer-destructuring so it only applies to wrapper? if that were possible, we might be able to just rely on its autofixing. If not, I'd be fine with a simple rule to check the name without an autofix.

2. Not really sure about this one. Can we really check this statically? I'm afraid about throwing errors for something that is actually fine.

I think the main issue would be that we'd need to keep this updated pretty frequently. It would be annoying to have an incorrect act recommendation just because some new API isn't in the whitelist. Is this still worth the effort?

3. We were discussing around this one when implementing the prefer-find-by one. I think it needs more discussion to clarify the exact behavior for it. Do we care if there are several expects but also other stuff? Should be fixable? What if it's fixable the first thing within waitFor callback isn't an expect? (I guess in this case we won't automatically fix it).

I was thinking the logic would just error if there was more than one expect, without an autofix as that could alter the behavior of the test. However this wouldn't handle functions that call other expectations, or alternative expectation libraries that don't have an expect function. Is still still worth the effort as well?

1. We didn't enable no-manual-cleanup for different reasons you can find in rule description. Basically 1) it depends on your testing framework and 2) can be skipped with a env var.

👍

2. Do we have a rule for preventing querying from container? It's been around lately, but we didn't implement anything for know IIRC.

Is prefer-screen-queries supposed to do this?

3. Something similar to the cleanup one: we didn't enable no-wait-for-empty-callback by default because it's still allowed to pass an empty callback. It can be really annoying rule for people migrating to new waitFor method. Could be a good idea to enable it by default when waitFor is forced to have non-empty callback tho.

I thought the blog was suggesting that this was bad practice either way, regardless of the API change.

[Belco90]

Alternatively, could we possibly extend/reuse/configure prefer-destructuring so it only applies to wrapper? if that were possible, we might be able to just rely on its autofixing. If not, I'd be fine with a simple rule to check the name without an autofix.

Mmm I'm not sure. This even looks like 2 different rules: prefer-destructuring and no-render-wrapper. With the destructuring one you are no even allowed to get render results under a const. With the no wrapper one, you can but it just doesn't allow you to name it incorrectly. I'd go for just throwing an error at render result named as wrapper, but preferring destructuring could be another interesting rule.

I think the main issue would be that we'd need to keep this updated pretty frequently. It would be annoying to have an incorrect act recommendation just because some new API isn't in the whitelist. Is this still worth the effort?

Yeah that's my concern. I think it would imply too much effort for having a brittle check.

I was thinking the logic would just error if there was more than one expect, without an autofix as that could alter the behavior of the test. However this wouldn't handle functions that call other expectations, or alternative expectation libraries that don't have an expect function. Is still still worth the effort as well?

I think implementing exactly this without the autofix is completely worth it. I've found tons of tests in the codebase of the company I work for with more than one expect within waitFor callback. This rule would have save lot of time and prevent some errors. In my opinion, this is the most important one from the list you shared and shouldn't be that complex to implement.

Is prefer-screen-queries supposed to do this?

Not really. You can check in the rule description it says "Given the screen component does not expose utility methods such as rerender() or the container property, it is correct to use the render response in those scenarios." (this description could be improved tho)

We had some discussion around that tho, and we even tried to prevent using container from render results to use the one within screen instead... without checking testing library docs 🤦 . container can be only retrieved from render results, so prefer-screen-queries doesn't take care of this.

We have some other interesting issues or comments around this:

• New Rule: Disallow DOM exploration properties #62
• screen.container undefined #156 (comment)

I thought the blog was suggesting that this was bad practice either way, regardless of the API change.

You are absolutely right here, didn't think about this has been a good practice even before waitFor. We can enable this by default then.

[nickmccurdy]

Mmm I'm not sure. This even looks like 2 different rules: prefer-destructuring and no-render-wrapper. With the destructuring one you are no even allowed to get render results under a const. With the no wrapper one, you can but it just doesn't allow you to name it incorrectly. I'd go for just throwing an error at render result named as wrapper, but preferring destructuring could be another interesting rule.

The wording of the heading in the article is a little misleading, it's more about advising you to use destructuring than to not use wrapper. I'd prefer to just encourage destructuring as it's more accurately following this advice (and prevents a variable with a different name being used), but if that's not possible we could just check if the name is wrapper and autofix with destructuring. I'm not that great with implementing ESLint rules though, so I'd appreciate any insights about how this could be implemented (especially if we could reuse the destructuring rule or if we would have to reimplement parts of it).

Not really. You can check in the rule description it says "Given the screen component does not expose utility methods such as rerender() or the container property, it is correct to use the render response in those scenarios." (this description could be improved tho)

We had some discussion around that tho, and we even tried to prevent using container from render results to use the one within screen instead... without checking testing library docs 🤦 . container can be only retrieved from render results, so prefer-screen-queries doesn't take care of this.

We have some other interesting issues or comments around this:

• New Rule: Disallow DOM exploration properties #62
• #156 (comment)

I'm thinking of a new rule that would error on methods like querySelector that are often used instead of get/query functions, as there might be other reasons why you'd need to access the container in a test (such as assertions). It's more about using RTL productively without relying on DOM internals. #62 is a similar, but I'm thinking specifically about banning container methods and replacing them with get/query functions.

[Belco90]

Ok, to sum up what we got so far:

• New prefer-destructuring-render rule. I wouldn't go for autofix at first, but the implementation itself should be easy by mirroring similar behavior from ESLint prefer-destructuring one. It can get more complex that it seems if there is a function wrapping the render method tho, something like:

const setUp = () => render(<Foo />);

const utils = setUp();

👆 this should throw an error

• New single-wait-for-assert rule. Proposal name for the rule to avoid more than one expect within waitFor callback. No autofix.
• New no-container rule. It forbids to query using container directly. No autofix. Should suggest how to replace container with a query tho? I don't know how far we can go helping the user here.
• Add no-wait-for-empty-callback rule to recommended preset.

[gndelia]

• New single-wait-for-assert rule. Proposal name for the rule to avoid more than one expect within waitFor callback. No autofix.

I think I can start with this one in the incoming days. We can track it on #133

[nickmccurdy]

Since we've agreed on the overall direction here, I'm closing this discussion and replacing it with some new issues and pull requests I'm listing below, along with the original issues from my original post.

• New prefer-destructuring-render rule. I wouldn't go for autofix at first, but the implementation itself should be easy by mirroring similar behavior from ESLint prefer-destructuring one. It can get more complex that it seems if there is a function wrapping the render method tho, something like:

const setUp = () => render(<Foo />);

const utils = setUp();

👆 this should throw an error

#170

• New no-container rule. It forbids to query using container directly. No autofix. Should suggest how to replace container with a query tho? I don't know how far we can go helping the user here.

#171

• Add no-wait-for-empty-callback rule to recommended preset.

#168

I've also recommended prefer-screen-queries separately, as it's mentioned in the article but I forgot to include it in this issue: #169

[Belco90]

Thanks for organizing these tickets and discussion @nickmccurdy !

[gndelia]

Asking here, just not to create a new ticket:

should we also make prefer-wait-for recommended for the v4? That should help users to enforce the usage of waitFor instead of the deprecated APIs

[nickmccurdy]

I believe the rationale of not including it by default was to make it easier for users that are still on a legacy version (where this new API doesn't exist yet). I'd be fine with changing this to assume people are using an up-to-date version of the library if we can reach an agreement though