Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

new rule: no-multiple-assertions-wait-for #133

Closed
renatoagds opened this issue May 13, 2020 · 12 comments
Closed

new rule: no-multiple-assertions-wait-for #133

renatoagds opened this issue May 13, 2020 · 12 comments
Assignees
Labels
new rule New rule to be included in the plugin released on @beta released
Milestone

Comments

@renatoagds
Copy link
Contributor

Following the proposed in Kent at his post (https://kentcdodds.com/blog/common-mistakes-with-react-testing-library). I would like to suggest a new rule that detects multiple expect inside a wait-for:

 // wrong
await waitFor(() => {
  expect(a).toEqual('a')
  expect(b).toEqual('b')
})
 // correct
await waitFor(() => expect(a).toEqual('a'))
expect(b).toEqual('b')
@Belco90
Copy link
Member

Belco90 commented May 13, 2020

Hey Renato! Thanks for your proposal, it sounds good. Would like to give it a try? PRs are welcome!

@Belco90 Belco90 added the new rule New rule to be included in the plugin label May 13, 2020
@Belco90 Belco90 added this to the v4 milestone Jun 17, 2020
@Belco90
Copy link
Member

Belco90 commented Jun 17, 2020

What name do you prefer for this one?

  • no-multiple-expect-wait-for
  • single-wait-for-assert
  • something else

@nickserv
Copy link
Member

Maybe no-multiple-assertions-wait-for?

@gndelia
Copy link
Collaborator

gndelia commented Jun 17, 2020

should we also cover the deprecated methods? like wait and waitForElement?

@Belco90
Copy link
Member

Belco90 commented Jun 17, 2020

I think so. It should be easy after implemented for waitFor right?

@timdeschryver
Copy link
Member

While being easy, is it needed?
The linter will fix the deprecated methods to waitFor, and then this rule kicks in.

Just thinking out loud here 😅

@renatoagds
Copy link
Contributor Author

renatoagds commented Jun 17, 2020

@Belco90 @gndelia Hey guys! I have been working on this one since few days ago. Sorry for the delay.

I'm calling it no-multiple-expect-wait-for in my branch:

https://github.com/renatoagds/eslint-plugin-testing-library/tree/no-multiple-expect-wait-for

Do I need to stop working on this? Should I rename it?

@gndelia
Copy link
Collaborator

gndelia commented Jun 17, 2020

oh nono, that's great! Let's continue over your branch. You can continue with it, I can just take another item

We did not know you were at it. Awesome 🚀 🚀

you might want to join the debate here 😄

@renatoagds
Copy link
Contributor Author

renatoagds commented Jun 17, 2020

@gndelia Great! Will continue work on this.

you might want to join the debate here

Yes! Sorry for the delay, I read the discussion from #165 to up to date about the directions.

While being easy, is it needed?

@timdeschryver I have mixed feelings about.

Talking about myself, the project that I work are in top of migration for RTL v10 and even with lint fix we have a few points to double check, but we're thinking about use TL eslint to ensure good practices without wait for migration, in this case, could be good to support deprecated methods, but, since it's a opt-in rule, I could only enable it in the final of the migration.

I would suggest to start with waitFor and receive feedback if people are trying to use it with deprecated methods, and implement if necessary.

@nickserv nickserv assigned renatoagds and unassigned gndelia Jun 17, 2020
@Belco90
Copy link
Member

Belco90 commented Jun 18, 2020

@renatoagds sorry, didn't know you were still working on it! I'm happy with your suggestion to start only with waitFor and see what feedback we get. It's a good idea also to rename the rule to no-multiple-assertions-wait-for. Thanks!

@nickserv nickserv changed the title new rule: no-multiple-expect-wait-for new rule: no-multiple-assertions-wait-for Jun 18, 2020
@Belco90 Belco90 closed this as completed Jun 30, 2020
@github-actions
Copy link

🎉 This issue has been resolved in version 4.0.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link

🎉 This issue has been resolved in version 4.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new rule New rule to be included in the plugin released on @beta released
Projects
None yet
Development

No branches or pull requests

5 participants