Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.8k
chore(eslint-plugin): consistently useit in tests#10680
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Changes a few occurrences of `test(name, fn)` to use `it(name, fn)` forconsistency with the rest of the codebase.
Thanks for the PR,@43081j! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently onhttps://opencollective.com/typescript-eslint. |
netlifybot commentedJan 18, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
✅ Deploy Preview fortypescript-eslint ready!
To edit notification comments on pull requests, go to yourNetlify site configuration. |
nx-cloudbot commentedJan 18, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
View yourCI Pipeline Execution ↗ for commitae4333c.
☁️Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM!
it in testsit in testsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
-0.5 from me, but if the rest of the team disagrees I don't mind this being merged at all. Thanks for sending!
| expect(areOptionsValid(exampleRule, ['value-a'])).toBe(true); | ||
| }); | ||
| describe('returns false for invalid options', () => { | ||
| test('bad enum value', () => { | ||
| it('bad enum value', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
🤔 I actually liketest() here overit(). The test name doesn't grammatically read out as a full sentence. I've personally kind of gravitated towards:
it(): for most tests, if there's at all a way to phrase it as"it X when Y" or something like thattest(): as a fallback if there's no logic, just a descriptor - like"test X"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't really care which one we use as long as it is one, and not two
If you can get the others to agree, I'm happy to update
I don't think you should end up with both. What you say makes sense but, if anything, suggests you would prefer havingtest throughout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh don't get me wrong, I thinkit() is the right choice almost all of the time. Just mypersonal nitpicky preference is to usetest() inspecific situations. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
To be honest, my preference is actually suite/test/assert. But the most consistent thing we can do right now isit it seems
As a middle ground, id prefer to later move from the now-consistentit totest if anything
Oh, I apologize, I made a mistake here. I had assumed that this was anecessary change for moving to vitest, because of its inclusion in#10579. However, now that I realize that vitest has
Sincere apologies again to@43081j for my misunderstanding and for requesting this PR only to close it 🙁 And thank you to@JoshuaKGoldberg for pointing out my mistake! |
Changes a few occurrences of
test(name, fn)to useit(name, fn)for consistency with the rest of the codebase.PR Checklist