eslint-plugin-testing-library icon indicating copy to clipboard operation
eslint-plugin-testing-library copied to clipboard

Make sure `no-unnecessary-act` detects unnecessary `act` called before the `render` method

Open fabiendem opened this issue 3 years ago • 10 comments

What rule do you want to change?

no-unnecessary-act

Does this change cause the rule to produce more or fewer warnings?

More warnings

How will the change be implemented?

I don't have a plan to implement the change yet. The idea is to improve the rule no-unnecessary-act so it can detect unnecessary act which are called before a render. https://github.com/testing-library/eslint-plugin-testing-library/blob/main/docs/rules/no-unnecessary-act.md Those unnecessary make the codebase noisy.

Example code

act(() => {
  someSetupMethod();
});

render(<Example />);

How does the current rule affect the code?

This unnecessary act is not detected

How will the new rule affect the code?

This unnecessary act is detected

Anything else?

No response

Do you want to submit a pull request to change the rule?

Yes, but need help

fabiendem avatar Feb 17 '22 16:02 fabiendem

Hey @fabiendem! Thanks for the suggestion, it's an interesting one. It would be great if you finally make some time and implement it yourself. I'll try to help as much as I can!

Belco90 avatar Feb 18 '22 08:02 Belco90

@Belco90 cheers. I use a lot of ESLint rules but I have never written one.

  • Do you have a list of good resources to start with?
  • Can you think of a similar constraint in the existing rules ("do not do X before Y"), so I can see examples?
  • Any other hint is welcomed 🙏

Thanks

fabiendem avatar Feb 18 '22 12:02 fabiendem

I see. Some resources to help you with this:

  • The Code Transformation and Linting with ASTs course is the one I used to learn about how to work with AST to build this plugin
  • I think there is no similar constraint as the one you want to apply. There are others about "do not do X inside Y callback" but they won't work in the same way. I can suggest you check no-render-in-setup and render-result-naming-convention which are related to the render method so you can see how to work around that
  • You can find some contributing guidelines in this project that will lead you through some parts and point to a few utils. We have tons of utils you can take advantage of, like the isRenderUtil or the isRenderVariableDeclarator, so you don't need to manually look for render methods but rely on those utils.

Belco90 avatar Feb 18 '22 12:02 Belco90

Thank you 👍

fabiendem avatar Feb 18 '22 12:02 fabiendem

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Apr 19 '22 17:04 stale[bot]

Feature is still relevant, I didn't have a chance to tackle it yet.

fabiendem avatar Apr 20 '22 10:04 fabiendem

This may be tricky to check tho. What if the developer is rendering the component in the beforeEach hook? This rule won't find any valid render within the tests so all acts could be potentially reported.

Belco90 avatar Apr 20 '22 11:04 Belco90

We have this rule in place: https://github.com/testing-library/eslint-plugin-testing-library/blob/main/docs/rules/no-render-in-setup.md So this is not an issue for us. But surely not everyone will have this rule enabled.

But even, we would want to detect any act before a render call. We can start by detecting this in the same blocks. Then detect acts in the before* blocks as well.

fabiendem avatar Apr 20 '22 11:04 fabiendem

Rules must not depend on each other, so we have to assume this scenario is possible.

We can start by detecting this in the same blocks. Then detect acts in the before* blocks as well.

LGTM! So this rule would report acts for the current criteria OR for acts called before a render within the test block (ignoring them if no render is found).

Belco90 avatar Apr 20 '22 11:04 Belco90

Rules must not depend on each other, so we have to assume this scenario is possible.

yeah definitely

LGTM! So this rule would report acts for the current criteria OR for acts called before a render within the test block (ignoring them if no render is found).

yeah seems legit

fabiendem avatar Apr 20 '22 15:04 fabiendem