(feat:no-unnecessary-act): adds no-unnecessary-act rule, closes #259 #292

alessbell · 2021-03-17T13:18:27Z

I was starting to spin my wheels a bit so I wanted to open the PR to get some feedback 🙂 Closes #259.

I'm having trouble detecting all three of these cases:

empty functions
arrow functions with block bodies
arrow functions with concise bodies

My interpretation of the business logic is:
Find all instances of act (regardless of how act is imported, ReactTestUtils.act should be treated the same as act) and check whether the function passed to act:

has an empty body = act unnecessary
contains a non-Testing Library function call = act is permitted

Five test cases are still failing. Will circle back around to this later today but wanted to open it up for any and all advice, thanks @Belco90 !

Belco90 · 2021-03-18T11:50:50Z

Nice! I'll take a look during the weekend.

It would be nice if you can update your branch again with v4 branch since we already merged the fix for running the pipeline correctly.

…-library into pr/no-unnecessary-act

MichaelDeBoey · 2021-03-19T10:48:18Z

docs/rules/no-unnecessary-act.md

+Examples of **incorrect** code for this rule:
+
+```js
+import { fireEvent, act } from '@testing-library/react';


Use @testing-library/user-event in order to draw attention to the act problem and not have separate problems in the example

Suggested change

import { fireEvent, act } from '@testing-library/react';

import { act } from '@testing-library/react';

import userEvent from '@testing-library/user-event';

I would actually use both examples: with fireEvent to clarify it's a Testing Library util already wrapped with act, and userEvent to clarify the same since it's imported from a different package.

MichaelDeBoey · 2021-03-19T10:48:42Z

docs/rules/no-unnecessary-act.md

+import { fireEvent, act } from '@testing-library/react';
+
+it('Should have foo', () => {
+  act(() => fireEvent.click(el));


Suggested change

act(() => fireEvent.click(el));

act(() => userEvent.click(el));

MichaelDeBoey · 2021-03-19T11:01:46Z

docs/rules/no-unnecessary-act.md

+
+## Rule Details
+
+This rule disallows the usage of `act` when using it to wrap Testing Library functions, or functions with an empty body, which suppresses valid warnings. For more information, see [https://kcd.im/react-act](https://kcd.im/react-act).


Suggested change

This rule disallows the usage of `act` when using it to wrap Testing Library functions, or functions with an empty body, which suppresses valid warnings. For more information, see [https://kcd.im/react-act](https://kcd.im/react-act).

This rule disallows the usage of `act` when using it to wrap Testing Library

functions, or functions with an empty body, which suppresses valid warnings. For

more information, see https://kcd.im/react-act.

tests/lib/rules/no-unnecessary-act.test.ts

Belco90

Thanks for your PR @alessbell ! I'm flagging it as WIP until you finish it.

It looks good so far! However, I've left some comments about how to detect renamed act properly. You may need to change the implementation of the CallExpression listener a bit to get this properly.

Regarding the detection of empty functions, you can get some inspiration from no-wait-for-empty-callback where we actually report empty functions :)

I don't know if it could be interesting to report the act here in two different ways:

for act with empty functions: look for a CallExpression, then check it's an act to be reported, and finally check if its callback is empty
for act containing non Testing Library calls: look for any CallExpression, then check it's not coming from Testing Library (you have a helper available for it), and if you find one then flag the closest parent act called as valid. I'm not sure if this approach is easier or not than the opposite one (look for CallExpression that are act to be reported and then check for calls inside its callback) you are following to be honest, it's just an idea in case it helps. Remember eslint-utils is part of @typescript-eslint/experimental-utils we use, in case there is any function there it helps you to detect something.

Apart from the comments I've added throughout the PR, I think you should add more tests (specially without aggressive reporting, you can see the no-debug PR that I mentioned in one comment) and some extra examples in the rule doc.

Belco90 · 2021-03-20T16:32:46Z

lib/index.ts

@@ -39,6 +40,7 @@ const rules = {
  'no-promise-in-fire-event': noPromiseInFireEvent,
  'no-render-in-setup': noRenderInSetup,
  'no-side-effects-wait-for': noSideEffectsWaitFor,
+  'no-unnecessary-act': noUnnecessaryAct,


There is a snapshot that will fail because of this new rule. You can update it just running npm run test:update

Belco90 · 2021-03-20T16:33:45Z

lib/index.ts

@@ -39,6 +40,7 @@ const rules = {
  'no-promise-in-fire-event': noPromiseInFireEvent,
  'no-render-in-setup': noRenderInSetup,
  'no-side-effects-wait-for': noSideEffectsWaitFor,
+  'no-unnecessary-act': noUnnecessaryAct,


I think we should enable this one for react rules preset. What do you think?

Belco90 · 2021-03-20T16:42:57Z

lib/rules/no-unnecessary-act.ts

+export type MessageIds = 'noUnnecessaryAct';
+type Options = [];
+
+const ACT_EXPRESSION_QUERY = 'CallExpression[callee.name=/^(act)$/]';


I realized after migrating most of the rules to v4 that this limits the way we try to find the corresponding utils. This won't report act usages if the user renames the import like import { act as renamedAct } from '@testing-library/react'

Instead, it's better to just create a listener for CallExpression nodes and inside check if it's named act with the available helpers. You'll probably have to add a new isActUtil helper, in the same way I did it for debug one here when refactoring no-debug: https://github.com/testing-library/eslint-plugin-testing-library/pull/293/files#diff-bac511bc63f362d7483df411edf8aba96b1ae5949b4c15843a02d44804b59376

You can create this new isActUtil copy&pasting that method and replacing "debug" by "act" in that detect-testing-library-utils.ts file, and then use it inside your rule in a CallExpression listener callback to check if that node is a valid act. Then you can basically follow the same pattern I followed in no-debug about finding the deepest identifier node so you get act no matter if called like act() or rtl.act().

The isTestingLibraryUtil called inside isActUtil, isDebugUtil and the other helpers will detect automatically for you if such node is imported from Testing Library and gets its renamed value if some.

Not sure if I'm explaining myself here really well, probably easier if you check the idea in that PR and let me know if you want me to go over any particular point 😅

tests/lib/rules/no-unnecessary-act.test.ts

Belco90 · 2021-03-20T17:06:50Z

tests/lib/rules/no-unnecessary-act.test.ts

+      `,
+      errors: [
+        {
+          messageId: 'noUnnecessaryAct'


I forgot to mention: can you check line and column for reported errors?

Belco90 · 2021-03-20T17:21:16Z

tests/lib/rules/no-unnecessary-act.test.ts

+
+const ruleTester = createRuleTester({
+  ecmaFeatures: {
+    jsx: true


Our createRuleTester already enables jsx so you don't need to set it here :)

gndelia · 2021-03-26T15:14:17Z

lib/rules/no-unnecessary-act.ts

+        body.every((node: TSESTree.ExpressionStatement) => {
+          if (
+            isCallExpression(node.expression) &&
+            isMemberExpression(node.expression.callee) &&
+            isCallExpression(node.expression.callee.object) &&
+            helpers.isNodeComingFromTestingLibrary(node.expression.callee)
+          ) {
+            return true;
+          }
+          return false;
+        });


really minor suggestion, but you can drop the if clause if you want and just return the expression

Suggested change

body.every((node: TSESTree.ExpressionStatement) => {

if (

isCallExpression(node.expression) &&

isMemberExpression(node.expression.callee) &&

isCallExpression(node.expression.callee.object) &&

helpers.isNodeComingFromTestingLibrary(node.expression.callee)

) {

return true;

}

return false;

});

body.every((node: TSESTree.ExpressionStatement) =>

isCallExpression(node.expression) &&

isMemberExpression(node.expression.callee) &&

isCallExpression(node.expression.callee.object) &&

helpers.isNodeComingFromTestingLibrary(node.expression.callee)

);

Belco90 · 2021-03-29T07:43:40Z

@alessbell I just realized this PR has been closed because it pointed to v4 branch which has been closed in favor of beta branch. I can't reopen it myself, but if you can edit the base branch to be beta it should be fine. Also, you can create another PR too. Sorry about it, I didn't know the branch change would cause this :(

We have already a beta released, so I have more spare time now. Can I help with something here? After having to migrate 20+ rules from the plugin, I really feel into writing ESLint rules so ping me if you want me to give you a hand here!

alessbell · 2021-04-02T12:52:53Z

@Belco90 thanks for your patience, and offer to help! It's not letting me edit the base branch for some reason, so I think I'll open a new PR this weekend and will let you know if I have any questions once I've addressed the comments left here 👍

alessbell added 2 commits March 17, 2021 08:26

(feat:no-unnecessary-act): adds no-unnecessary-act rule, closes #259

d0f6a85

Refactor into single function

e4df19c

Merge branch 'v4' of github.com:testing-library/eslint-plugin-testing…

560054c

…-library into pr/no-unnecessary-act

MichaelDeBoey requested changes Mar 19, 2021

View reviewed changes

Belco90 requested changes Mar 20, 2021

View reviewed changes

Belco90 reviewed Mar 20, 2021

View reviewed changes

gndelia reviewed Mar 26, 2021

View reviewed changes

Belco90 closed this Mar 28, 2021

Belco90 deleted the branch testing-library:v4 March 28, 2021 18:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(feat:no-unnecessary-act): adds no-unnecessary-act rule, closes #259 #292

(feat:no-unnecessary-act): adds no-unnecessary-act rule, closes #259 #292

alessbell commented Mar 17, 2021

Belco90 commented Mar 18, 2021

MichaelDeBoey Mar 19, 2021

Belco90 Mar 20, 2021

MichaelDeBoey Mar 19, 2021

MichaelDeBoey Mar 19, 2021

Belco90 left a comment •

edited

Loading

Belco90 Mar 20, 2021

Belco90 Mar 20, 2021

Belco90 Mar 20, 2021

Belco90 Mar 20, 2021

Belco90 Mar 20, 2021

gndelia Mar 26, 2021

Belco90 commented Mar 29, 2021

alessbell commented Apr 2, 2021

	import { fireEvent, act } from '@testing-library/react';
	import { act } from '@testing-library/react';
	import userEvent from '@testing-library/user-event';

	act(() => fireEvent.click(el));
	act(() => userEvent.click(el));


		## Rule Details

		This rule disallows the usage of `act` when using it to wrap Testing Library functions, or functions with an empty body, which suppresses valid warnings. For more information, see [https://kcd.im/react-act](https://kcd.im/react-act).

(feat:no-unnecessary-act): adds no-unnecessary-act rule, closes #259 #292

(feat:no-unnecessary-act): adds no-unnecessary-act rule, closes #259 #292

Conversation

alessbell commented Mar 17, 2021

Belco90 commented Mar 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Belco90 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Belco90 commented Mar 29, 2021

alessbell commented Apr 2, 2021

Belco90 left a comment •

edited

Loading