-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
feat(eslint-plugin-internal): [no-dynamic-tests] new internal Lint rule to ban dynamic syntax in generating tests #11323
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(eslint-plugin-internal): [no-dynamic-tests] new internal Lint rule to ban dynamic syntax in generating tests #11323
Conversation
|
Thanks for the PR, @nayounsang! 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 project configuration. |
|
View your CI Pipeline Execution ↗ for commit c973039
☁️ Nx Cloud last updated this comment at |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11323 +/- ##
========================================
Coverage 90.68% 90.69%
========================================
Files 516 517 +1
Lines 52051 52176 +125
Branches 8598 8632 +34
========================================
+ Hits 47202 47319 +117
- Misses 4835 4843 +8
Partials 14 14
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
JoshuaKGoldberg
left a comment
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, thanks! Only the one request for an added test is blocking IMO. Everything else is nitpicks that can be ignored if you don't like them. 🙂
JoshuaKGoldberg
left a comment
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!
kirkwaiblinger
left a comment
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 think that if we intend to use this ever, we should enable it already now for all the files that don't currently have violations. (Maybe even this is a good chance for us to try out the eslint bulk suppressions feature)?
Also, please do enable the rule on the codebase and have a look at the reports to see whether they make sense and would make sense to a contributor.
Thanks!
This comment was marked as resolved.
This comment was marked as resolved.
JoshuaKGoldberg
left a comment
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. +1 to @kirkwaiblinger's suggestions on improving the reporting of the rule. I'll want to hear from Kirk before merging too.
kirkwaiblinger
left a comment
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.
(see comment)
JoshuaKGoldberg
left a comment
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.
Looks great to me, thanks for jumping on this! 🙌
I'll note that the rule isn't completely covered by unit tests. But since it's internal & specific to this repo (for now?) I think that's fine.

PR Checklist
Overview
no-dynamic-tests
Ban these:
For test case object, It validate these keys recursively:
code,error.No checks are performed on other keys.
Applied the rules to the test file in
eslint-plugin&eslint-plugin-internal. Bulk suppression feature was applied.Create the script to update suppression.