feature request: new rule; no-render-in-setup

[benmonro]

imo, it would be nice to have a rule that doesn't allow the usage of render in setup functions (before, beforeEach, etc)

Would also be nice to have the ability to configure custom render functions, we have one for renderWithContext for example.

I've seen people do code like this:

beforeEach(() => {
  render(<MyComponent />);
});

it("Should have foo", () => {
  expect(screen.getByText("foo")).toBeInTheDocument();
});

it("Should have bar", () => {
  expect(screen.getByText("bar")).toBeInTheDocument();
});

it("Should have baz", () => {
  expect(screen.getByText("baz")).toBeInTheDocument();
});

imo this should just be a single test. why render it N times when nothing is changing in the render?

[Belco90]

Thanks for your idea @benmonro. Are you interested in implementing this one yourself?

Ability to support custom render functions will be added globally on version 4: #198

[benmonro]

Possibly. I've been quite busy lately not sure when I can get to it. I'd someone else wants it that'd be awesome

[alessbell]

👋 I'm interested in picking this up. Ok if I assign myself @Belco90 ?

[Belco90]

@alessbell Sure! That'd be amazing, thanks ✨

[Belco90]

🎉 This issue has been resolved in version 3.5.0 🎉

The release is available on:

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

Your semantic-release bot 📦🚀

[kevinbeal]

What is the rationale for this rule?

[Belco90]

What is the rationale for this rule?

https://kentcdodds.com/blog/avoid-nesting-when-youre-testing

(We should add this to the rule's doc)

[kevinbeal]

@Belco90 Thanks for getting back so fast. Not sure I share Kent's concern, but I'm glad to have read it :). Obviously, I can just disable the rule if I want to.

[Floby]

The author's explanation of the blog post linked above is almost uniquely expressed as a personal taste. This rule seems a bit too specific to be applied by default as an error, given that the error message and associated doc doesn't mention the cause for such a rule.

let me submit a pull request about this =)

[bvandercar-vt]

imo, it would be nice to have a rule that doesn't allow the usage of render in setup functions (before, beforeEach, etc)

Would also be nice to have the ability to configure custom render functions, we have one for renderWithContext for example.

I've seen people do code like this:

beforeEach(() => {
  render(<MyComponent />);
});

it("Should have foo", () => {
  expect(screen.getByText("foo")).toBeInTheDocument();
});

it("Should have bar", () => {
  expect(screen.getByText("bar")).toBeInTheDocument();
});

it("Should have baz", () => {
  expect(screen.getByText("baz")).toBeInTheDocument();
});

imo this should just be a single test. why render it N times when nothing is changing in the render?

Your example is a badly coded example. There is no rule that can prevent badly written tests like your case.

render in beforeEach is used when the state of the component is needed to be reset, which is incredibly common in testing.

We all agree that in your example, it should all be in one test. But just because you have this one-off case of a badly written test suite doesn't mean there should be a rule to prevent using render in beforeEach, when there are much more likely, frequent, and acceptable cases where people do that, and should be allowed to do it.

[bvandercar-vt]

The author's explanation of the blog post linked above is almost uniquely expressed as a personal taste. This rule seems a bit too specific to be applied by default as an error, given that the error message and associated doc doesn't mention the cause for such a rule.

let me submit a pull request about this =)

I agree with everything in this sentiment.

In addition, the author states:

This pattern:

test('whatever', () => {
  const foo = someThing()
  // use foo
})

makes for a WAY simpler testbase than:

let foo
beforeEach(() => {
  foo = someThing()
})

test('whatever', () => {
  // use foo
})

but this is just badly written code to begin with. What about the common case where no variable is assigned to?

beforeEach(() => {
  render(<Component />)
})

I see no reason this shouldn't be allowed.

It just seems like we are taking away from the entire intent of the existence of the beforeEach hook here.

[Belco90]

Some rules prevent actual errors, others are recommendations. This rule won't prevent an actual error, it's just considered a best practice.

The example illustrated is a common scenario. It's true that there could be a better example. However, it doesn't matter if you need to reset the component between tests, or pass several props or none at all. The recommendation is to avoid abstractions in tests to make it simple to follow and maintain.

If you don't agree with the explanation given, or you found a scenario in your codebase to allow this pattern, it's as simple as turning off the rule.