feat(no-render-in-setup): adds no-render-in-setup rule, closes #207

[alessbell]

Added a no-render-in-setup rule with tests to address #207.

The rule looks for uses of render (or a custom render function who name can be passed in via config options) inside of two "before hooks": beforeEach and beforeAll.

[gndelia]

Thanks for your first PR ❤️ 🚀

For the styling changes, we've seen something similar in other PRs, so I just opened a ticket to review that #210 so it's ok - don't worry about it.

For the code in general, it looks great to me.

Perhaps my only suggestion is about the error message (both in the rule and in the docs).

They said

(docs) This rule disallows the usage of render in setup functions (beforeEach or beforeAll) in favor of a single test with multiple assertions.
(rule error) 'Combine assertions into a single test instead of re-rendering the component.'

I used to work in a project where we would use render in beforeEach just to save common code, and then each (multiple) test would have its own path starting from the render (plus assertions). When we dropped that convention, we went to move the render into each test, rather than combine them into only a large one. Those tests were unrelated and large enough to combine them.
Because of that, I'd drop the combine assertions into a single test part and put something about moving the render closer to the assertions, or moving it into each test. Another option could just say why moving the render out of the beforeEach/beforeAll and leave open to the dev where to put it.

It's an open suggestion so I want to also hear everyone else's thoughts and yours too.

Other than that, everything looks good to me so we can probably merge this soon.

[alessbell]

Thanks @gndelia! I totally agree, and I've had a similar experience refactoring tests as the one you've described.

I'm also now wondering if this rule should only be applied to beforeEach if the intent is to prevent re-renders, since rendering in beforeAll could be used to organize assertions into different tests which seems more like a stylistic choice. E.g.:

(docs) This rule disallows the usage of render in the beforeEach setup function to prevent unnecessary re-renders.
(rule error) 'Move render out of beforeEach and closer to test assertions to prevent unnecessary re-renders.'

If the goal is moving render closer to assertions more generally:

(docs) This rule disallows the usage of render in setup functions (beforeEach or beforeAll) in favor of moving render closer to test assertions.
(rule error) 'Move render out of {{name}} and into individual tests.'

Curious to hear your and others' thoughts!

[Belco90]

Thanks for your first contribution! Looks good so far, I asked couple of naming improvements and adding some extra tests. As mentioned in one of the comments, let me know if you need help or more indications!

[Belco90]

@gndelia @alessbell I agree on updating the error message to make it more generic and not so attached to the assert combination.

Good point about if it makes sense to report when used within beforeAll. I think it makes sense for the rule to report in that case, but maybe with a different error message?

[gndelia]

I think I remember there were some issues with using beforeAll with RTL, because there's an implicit afterEach cleanup in every test (by default) that prevents from using render in a beforeAll successfully. But I am not sure if that's configurable, if it can be deactivated or even if that's on by default or what.

Assuming it can be used properly, I believe it makes sense to report it.
What about allowing a config to disable one or the other? The rule by default would trigger on both, but the user would be able to configure which one to disable (disabling both makes no sense - disable the rule in that scenario)
Not sure if it makes sense though. What are your thoughts @Belco90 @alessbell ?

[alessbell]

Thanks for the reviews @gndelia @Belco90!

I've attempted to address most of your comments, with the exception of this one regarding ignoring render methods that don't come from testing library. I will add that in a follow-on commit shortly :)

I've made the docs/error language more general, and added an enum config option allowTestingFrameworkSetupHook which accepts either beforeAll or beforeEach to be allowlisted. Good call @gndelia - RTL cites several ways to skip clean-up, so allowing render in beforeAll may make sense for some while still disallowing it in beforeEach. Let me know what you think!

[Belco90]

@alessbell awesome! The rule implementation is looking really good. No rush with the import check one. Thanks for your work!

[gndelia]

Thanks for applying the changes! Looks great for me. Thanks for your contribution! 🎸 🚀

[alessbell]

Thanks @gndelia! I believe I just have one more fix to make, and it's related to the commented out test that is still failing. Per @Belco90's advice, I will need to adjust the rule so that e.g.:

import { render } from 'imNoTestingLibrary';

beforeAll(() => {
  render(<Component/>)
})`,

does not trigger the error. I believe that's all that is left, and I should be able to add it soon :) Thanks for all the comments!

[alessbell]

Updated @gndelia @Belco90 :) LMK if there's anything else I should add/fix, have a great weekend!

[Belco90]

LGTM, thanks!

[Belco90]

I thought this one was already moved to node-utils.ts but that was done in v4 of course 😓

[Belco90]

@all-contributors please add @alessbell for code, tests and doc.

[allcontributors]

@Belco90

I've put up a pull request to add @alessbell! 🎉

[Belco90]

🎉 This PR is included in version 3.5.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀