-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
feat(rule-tester): stricter rule test validations #9086
feat(rule-tester): stricter rule test validations #9086
Conversation
Thanks for the PR, @Vinccool96! 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 on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 3bdc61e. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
aca9d06
to
bde1e6e
Compare
Never mind, there was way less things to fix than I thought. |
901db33
to
6e71280
Compare
06d3edf
to
5f55140
Compare
@auvred Is everything good to merge? |
5f55140
to
cbdf415
Compare
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.
Left a few minor requests but overall looks great!
Btw, there is no need to ping individuals for review. We have a queue and we get to things when we can. https://typescript-eslint.io/contributing/pull-requests
7ff0f76
to
c13178a
Compare
a2715a0
to
b121617
Compare
40ec555
to
e761ee1
Compare
e761ee1
to
271d694
Compare
271d694
to
c94ed24
Compare
Sorry for the force pushes, I'm used to do rebase updates |
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.
Did you check this comment? #9086 (comment)
We still miss this assertion
The assertion is here https://github.com/Vinccool96/typescript-eslint/blob/4eebf7df8d89a919fbc03b69c9405d118e9d066a/packages/rule-tester/src/RuleTester.ts#L1047-L1057, lines 1047 to 1057 |
On typescript-eslint/packages/rule-tester/src/RuleTester.ts Lines 874 to 884 in de18a6e
typescript-eslint/packages/rule-tester/src/RuleTester.ts Lines 1032 to 1042 in de18a6e
|
Alright, let me plop it back in |
The prodigal son has returned |
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.
Thanks for working on this!
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.
The tests for this looked like a real slog, thanks so much for doing this! 🙌
Just a couple of teeny suggestions. Nothing major from me. We can apply them before merging if you don't get a chance before our Monday release.
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.
Thanks! 🙌
Had to merge since the main branch had tests that would've failed with the stricter rule tester (tests added by me, what was I thinking!) |
PR Checklist
Overview
Brings in changes from eslint/eslint#17654. This is pretty much a copy & paste.