-
-
Notifications
You must be signed in to change notification settings - Fork 4.9k
fix: correct RuleTester typings #20105
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
Conversation
✅ Deploy Preview for docs-eslint canceled.
|
|
There is now a merge conflict. |
|
In the 2025-11-13 TSC Meeting, it was decided to add type definitions and documentation for the static Lines 2236 to 2238 in d5fc247
|
fasttime
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! Leaving open for @DMartens to verify.
lib/types/index.d.ts
Outdated
| export namespace RuleTester { | ||
| interface ValidTestCase { | ||
| interface ValidTestCase | ||
| extends Omit<Linter.Config, "name" | "basePath" | "files" | "ignores"> { |
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 following Linter.Config properties maybe should also be excluded:
linterOptions: this would only make sense for rules for inline configurationplugins: only if we want to address alanguageorprocessorby its name instead of passing it in directlyrules: we want to test the rule in isolation (an edge case would be rules which have a side-effect likecontext.markVariableAsUsed)
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’m fine with excluding linterOptions, plugins, and rules as well, but would like @fasttime’s opinion 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.
All config object keys can be also used in test cases. Excluding some of them from the typings is more a philosophical matter than a technical one. Now, linterOptions, plugins, and rules are not currently included in ValidTestCase, so I think we shouldn't add them if the team has any reservations. That said, I'd be in favor of including them if users request them for a specific use case at some point.
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.
Recently we decided to make the typings of data stricter in eslint/rewrite#310 even though other values (e.g. objects) can be stringified without error and there is no runtime check for this.
IIRC these properties were not allowed to be set in test cases before the introduction of the "flat" config.
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.
IIRC these properties were not allowed to be set in test cases before the introduction of the "flat" config.
linterOptions is a new option in flat config, so it was not allowed in test cases with the old RuleTester in ESLint v8. By contrast, plugins was allowed as an array of strings, and rules, in its current format, was already allowed prior to flat config. I checked the v8 code, and it looks like RuleTester has always worked the same way: moving all non-test-case properties to a config object, and using that config (after validation) to lint the provided code:
eslint/lib/rule-tester/rule-tester.js
Lines 718 to 735 in 83914ad
| /* | |
| * Assumes everything on the item is a config except for the | |
| * parameters used by this tester | |
| */ | |
| const itemConfig = { ...item }; | |
| for (const parameter of RuleTesterParameters) { | |
| delete itemConfig[parameter]; | |
| } | |
| /* | |
| * Create the config object from the tester config and this item | |
| * specific configurations. | |
| */ | |
| config = merge( | |
| config, | |
| itemConfig | |
| ); |
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.
You are right. I think I thought that because I never saw those properties in rule test files.
fasttime
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. Reapproving.
|
The open question was whether we want to include uncommon configuration options like |
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
What changes did you make? (Give an overview)
Is there anything you'd like reviewers to focus on?