fix: getTestInfo detects NewExpression and CallExpression depending on ruleTesterConstructors settings - #657
Conversation
…ending in RuleTester
| (node.callee.type === 'MemberExpression' && | ||
| node.callee.property.type === 'Identifier' && | ||
| node.callee.property.name === 'RuleTester')) | ||
| node.callee.property.name.endsWith('RuleTester'))) |
There was a problem hiding this comment.
Not sure whether endsWith is the correct check here
The function to create a RuleTester in typescript-eslint is called createRuleTesterWithTypes, which, IMO, is a reasonable name
Maybe we can add support for settings['eslint-plugin'].additionalRuleTesterConstructors or something similar instead?
(Anyway, if it's too much work, we can probably just rename the function on our end 😄)
There was a problem hiding this comment.
What do you think should be the check? I'm not sure I should try to parse all used methods to find what they return, nor all classes to see what they extend.
There was a problem hiding this comment.
I like the idea! something like:
{
settings: {
"eslint-plugin": {
"RuleTesterConstructors": ["RuleTester"] // the default, can be configured to other names.
}
}
}There was a problem hiding this comment.
we just need to check the function names - ESLint is designed to check individual files; if the rule tester is defined in another file, static analysis is not possible.
There was a problem hiding this comment.
That is a good idea. Do I create two options, one for constructors (ruleTester = new ClassName()) and one for functions (ruleTester = creatorFunction())?
Where in the docs do I put the info for the shared settings? Can every rule related to RuleTester override the settings?
|
@aladdin-add @StyleShit I would like to know four things:
|
|
I don’t really see the necessity for this at the moment. We can skip it for now, and if needed, we can always add it later. |
aladdin-add
left a comment
There was a problem hiding this comment.
Thanks for updating! the changes look good to me, we just need:
- revert the change: a8e9dbd
- add a section in docs.
There was a problem hiding this comment.
Pull request overview
This PR extends the getTestInfo static analysis utility so it can recognize additional ways of constructing a RuleTester (including factory calls), and updates tests to cover configurable constructor matching and avoid new false positives in “not a RuleTester” fixtures.
Changes:
- Added support for treating both
NewExpressionandCallExpressionas potentialRuleTesterconstructions, driven by a newsettings['eslint-plugin'].ruleTesterConstructorsoption. - Implemented settings extraction/validation helpers and threaded the resolved options through
getTestInfo’s recursive AST scanning. - Expanded and adjusted test coverage for settings-driven behavior; updated “Not RuleTester” fixtures to avoid suffix-based matches.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/utils.ts | Adds settings parsing/validation and expands RuleTester-construction detection logic. |
| tests/lib/utils.ts | Expands getTestInfo tests across multiple settings permutations and constructor patterns. |
| tests/lib/rules/test-case-shorthand-strings.ts | Renames “Not RuleTester” fixture to avoid new suffix-based detection. |
| tests/lib/rules/test-case-property-ordering.ts | Same fixture rename for suffix-based detection avoidance. |
| tests/lib/rules/prefer-output-null.ts | Same fixture rename for suffix-based detection avoidance. |
| tests/lib/rules/no-only-tests.ts | Same fixture rename for suffix-based detection avoidance. |
| tests/lib/rules/no-identical-tests.ts | Same fixture rename for suffix-based detection avoidance. |
| tests/lib/rules/consistent-output.ts | Same fixture rename for suffix-based detection avoidance. |
Suppressed comments (1)
tests/lib/utils.ts:1142
- Same as above:
JSON.stringify(settings)in the test title won’t includeRegExpvalues, which can lead to duplicate/unclear test names. Use a replacer to preserve regexes.
it(`${testSource} with options ${JSON.stringify(settings)}`, () => {
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
getTestInfo detects NewExpression and CallExpression with name ending in RuleTestergetTestInfo detects NewExpression and CallExpression depending on ruleTesterConstructors settings
|
I have WebStorm instead of VSCode, so I have no idea what to do with the ToC of the Readme |
What is the purpose of this pull request?
Extends
getTestInfoto captureNewExpressionandCallExpressionwith name ending inRuleTesterWhat changes did you make? (Give an overview)
CallExpressionis now a valid option to create aRuleTesterAccepted class and function names end in
RuleTesterinstead of only beingRuleTesterRelated Issues
Fixes #654
Is there anything you'd like reviewers to focus on?
💖