-
-
Notifications
You must be signed in to change notification settings - Fork 80
feat: add assertion options to RuleTester #137
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
Changes from all commits
3d8c0ba
e25685f
641dabb
05a971e
19762be
d27f172
69af370
cd33bfc
9a07ded
ad0293b
b720c9d
eef7306
429e0a3
aa12816
55b1ec7
ae0c11d
ab96b31
e378647
5f137b0
c3a09c4
e3aff7b
50b7323
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,365 @@ | ||
| - Repo: https://github.com/eslint/eslint | ||
| - Start Date: 2025-07-10 | ||
| - RFC PR: https://github.com/eslint/rfcs/pull/137 | ||
| - Authors: ST-DDT | ||
|
|
||
| # Rule Tester Assertions Options | ||
|
|
||
| ## Summary | ||
|
|
||
| Add options that control which assertions are required for each `RuleTester` test case. | ||
|
|
||
| ## Motivation | ||
|
|
||
| In most eslint(-plugin)s' rules the error assertions are different from each other. | ||
| Adding options that could be set/shared when configuring the rule tester would ensure a common base level of assertions is met throughout the (plugin's) project. | ||
| The options could be defined on two levels. On `RuleTester`'s `constructor` effectively impacting all tests, or on the test `run` method itself, so only that set is affected. | ||
|
|
||
| ## Detailed Design | ||
|
|
||
| ```ts | ||
| ruleTester.run("rule-name", rule, { | ||
| assertionOptions?: { | ||
| /** | ||
| * Require message assertion for each invalid test case. | ||
| * If `true`, each error must extend `string | RegExp | { message: string | RegExp } | { messageId: string }`. | ||
| * If `'message'`, each error must extend `string | RegExp | { message: string | RegExp }`. | ||
| * If `'messageId'`, each error must extend `{ messageId: string }`. | ||
| * | ||
| * @default false | ||
| */ | ||
| requireMessage?: boolean | 'message' | 'messageId'; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there should be an extra assertion to disallow using a number for errors (maybe named
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just for clarification, so instead of
You have to use A)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is dependent on the other assertions.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO this is entirely/mostly covered by adding
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @DMartens Are you suggesting that
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So if I dont like that. Because then it behaves like requireMessage/location isnt set in the first place.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can handle the option as
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The requirement for errors to include either a Given that ESLint has supported this behavior by default, I tend to agree with @DMartens that it makes sense to maintain the possibility of specifying a number for
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I agree that we should make a change here. To me, if someone wants to use I think
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is solved with the mentioned additional assertion |
||
| /** | ||
| * Require full location assertions for each invalid test case. | ||
| * If `true`, each error must extend `{ line: number, column: number, endLine?: number | undefined, endColumn?: number | undefined }`. | ||
| * `endLine` or `endColumn` may be absent or undefined, if the observed error does not contain these properties. | ||
| * | ||
| * @default false | ||
| */ | ||
| requireLocation?: boolean; | ||
| }, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another helpful check would be
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would this imply
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also considered requiring the data block (if needed) into
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would not imply
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure whether I understand the last comment. IMO requireData results in AFAICT The current code only checks for data in the messageId block. Which makes sense to me, as data is used to fill in the placeholders in its message. Most messageId checks are very broad as most rules only have one or two messages. So also checking for data is kind of relevant. Which is why I would implicitly attach it to the requireMessageId option.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This option (
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree a
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, the scope of this RFC is already well-defined and I don't think we should be expanding at this point in time. @DMartens feel free to open an issue to start the discussion around a |
||
| valid: [ /* ... */ ], | ||
| invalid: [ /* ... */ ], | ||
| }); | ||
| ``` | ||
|
|
||
| ### requireMessage | ||
|
|
||
| If `requireMessage` is set to `true`, the invalid test case cannot consist of an error count assertion only, but must also include a message assertion. (See below) | ||
| This can be done either by providing only a `string`/`RegExp` message, or by using the `message`/`messageId` property of the error object in the `TestCaseError` (Same as the current behavior). | ||
| If `true`, each error must extend `string | RegExp | { message: string | RegExp } | { messageId: string }`. | ||
| If `'message'`, each error must extend `string | RegExp | { message: string | RegExp }`. | ||
| If `'messageId'`, each error must extend `{ messageId: string }`. | ||
|
|
||
| ```ts | ||
| ruleTester.run("rule-name", rule, { | ||
| assertionOptions: { | ||
| requireMessage: true, | ||
| }, | ||
| invalid: [ | ||
| { | ||
| code: "const a = 1;", | ||
| errors: 1, // ❌ Message isn't being checked here | ||
| }, | ||
| { | ||
| code: "const a = 2;", | ||
| errors: [ | ||
| "Error message here.", // ✅ we only check the error message | ||
| ], | ||
| }, | ||
| { | ||
| code: "const a = 3;", | ||
| errors: [ | ||
| { | ||
| message: "Error message here.", // ✅ we check the error message and potentially other properties | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }); | ||
| ``` | ||
|
|
||
| ### requireLocation | ||
|
|
||
| If `requireLocation` is set to `true`, the invalid test case's error validation must include all location assertions such as `line`, `column`, `endLine`, and `endColumn`. | ||
ST-DDT marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| `endLine` or `endColumn` may be absent or `undefined`, if the observed error does not contain these properties. | ||
|
|
||
| ```ts | ||
| ruleTester.run("rule-name", rule, { | ||
| assertionOptions: { | ||
| requireLocation: true, | ||
| }, | ||
| invalid: [ | ||
| { | ||
| code: "const a = 1;", | ||
| errors: 1, // ❌ Location isn't checked here | ||
| }, | ||
| { | ||
| code: "const a = 2;", | ||
| errors: [ | ||
| "Error message here.", // ❌ Location isn't checked here | ||
| ], | ||
| }, | ||
| { | ||
| code: "const a = 3;", | ||
| errors: [ | ||
| { | ||
| line: 1, // ❌ Location isn't fully checked here | ||
| column: 1, | ||
| }, | ||
| ], | ||
| }, | ||
| { | ||
| code: "const a = 4;", | ||
| errors: [ | ||
| { | ||
| line: 1, // ✅ All location properties have been checked | ||
| column: 1, | ||
| endLine: 1, | ||
| endColumn: 12, | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }); | ||
| ``` | ||
|
|
||
| ## Implementation Hint | ||
|
|
||
| ```patch | ||
| diff --git a/lib/rule-tester/rule-tester.js b/lib/rule-tester/rule-tester.js | ||
| index dbd8c274f..b39631faa 100644 | ||
| --- a/lib/rule-tester/rule-tester.js | ||
| +++ b/lib/rule-tester/rule-tester.js | ||
| @@ -555,6 +555,10 @@ class RuleTester { | ||
| * @param {string} ruleName The name of the rule to run. | ||
| * @param {RuleDefinition} rule The rule to test. | ||
| * @param {{ | ||
| + * assertionOptions?: { | ||
| + * requireMessage?: boolean | "message" | "messageId", | ||
| + * requireLocation?: boolean | ||
| + * }, | ||
| * valid: (ValidTestCase | string)[], | ||
| * invalid: InvalidTestCase[] | ||
| * }} test The collection of tests to run. | ||
| @@ -563,6 +567,14 @@ class RuleTester { | ||
| * @returns {void} | ||
| */ | ||
| run(ruleName, rule, test) { | ||
| + if (!test || typeof test !== "object") { | ||
| + throw new TypeError( | ||
| + `Test Scenarios for rule ${ruleName} : Could not find test scenario object`, | ||
| + ); | ||
| + } | ||
| + | ||
| + const { requireMessage = false, requireLocation = false } = | ||
| + test.assertionOptions ?? {}; | ||
| const testerConfig = this.testerConfig, | ||
| requiredScenarios = ["valid", "invalid"], | ||
| scenarioErrors = [], | ||
| @@ -582,12 +594,6 @@ class RuleTester { | ||
| ); | ||
| } | ||
|
|
||
| - if (!test || typeof test !== "object") { | ||
| - throw new TypeError( | ||
| - `Test Scenarios for rule ${ruleName} : Could not find test scenario object`, | ||
| - ); | ||
| - } | ||
| - | ||
| requiredScenarios.forEach(scenarioType => { | ||
| if (!test[scenarioType]) { | ||
| scenarioErrors.push( | ||
| @@ -1092,6 +1098,10 @@ class RuleTester { | ||
| } | ||
|
|
||
| if (typeof item.errors === "number") { | ||
| + assert.ok( | ||
| + !requireMessage && !requireLocation, | ||
| + "Invalid cases must have 'errors' value as an array", | ||
| + ); | ||
| if (item.errors === 0) { | ||
| assert.fail( | ||
| "Invalid cases must have 'error' value greater than 0", | ||
| @@ -1137,6 +1147,10 @@ class RuleTester { | ||
|
|
||
| if (typeof error === "string" || error instanceof RegExp) { | ||
| // Just an error message. | ||
| + assert.ok( | ||
| + requireMessage !== "messageId" && !requireLocation, | ||
| + "Invalid cases must have 'errors' value as an array of objects e.g. { message: 'error message' }", | ||
| + ); | ||
| assertMessageMatches(message.message, error); | ||
| assert.ok( | ||
| message.suggestions === void 0, | ||
| @@ -1157,6 +1171,10 @@ class RuleTester { | ||
| }); | ||
|
|
||
| if (hasOwnProperty(error, "message")) { | ||
| + assert.ok( | ||
| + requireMessage !== "messageId", | ||
| + "The test run requires 'messageId' but the error (also) uses 'message'.", | ||
| + ); | ||
| assert.ok( | ||
| !hasOwnProperty(error, "messageId"), | ||
| "Error should not specify both 'message' and a 'messageId'.", | ||
| @@ -1170,6 +1188,10 @@ class RuleTester { | ||
| error.message, | ||
| ); | ||
| } else if (hasOwnProperty(error, "messageId")) { | ||
| + assert.ok( | ||
| + requireMessage !== "message", | ||
| + "The test run requires 'message' but the error (also) uses 'messageId'.", | ||
| + ); | ||
| assert.ok( | ||
| ruleHasMetaMessages, | ||
| "Error can not use 'messageId' if rule under test doesn't define 'meta.messages'.", | ||
| @@ -1242,21 +1264,43 @@ class RuleTester { | ||
| if (hasOwnProperty(error, "line")) { | ||
| actualLocation.line = message.line; | ||
| expectedLocation.line = error.line; | ||
| + } else if (requireLocation) { | ||
| + assert.fail( | ||
| + "Test error must specify a 'line' property.", | ||
| + ); | ||
| } | ||
|
|
||
| if (hasOwnProperty(error, "column")) { | ||
| actualLocation.column = message.column; | ||
| expectedLocation.column = error.column; | ||
| + } else if (requireLocation) { | ||
| + assert.fail( | ||
| + "Test error must specify a 'column' property.", | ||
| + ); | ||
| } | ||
|
|
||
| if (hasOwnProperty(error, "endLine")) { | ||
| actualLocation.endLine = message.endLine; | ||
| expectedLocation.endLine = error.endLine; | ||
| + } else if ( | ||
| + requireLocation && | ||
| + hasOwnProperty(message, "endLine") | ||
| + ) { | ||
| + assert.fail( | ||
| + "Test error must specify an 'endLine' property.", | ||
| + ); | ||
| } | ||
|
|
||
| if (hasOwnProperty(error, "endColumn")) { | ||
| actualLocation.endColumn = message.endColumn; | ||
| expectedLocation.endColumn = error.endColumn; | ||
| + } else if ( | ||
| + requireLocation && | ||
| + hasOwnProperty(message, "endColumn") | ||
| + ) { | ||
| + assert.fail( | ||
| + "Test error must specify an 'endColumn' property.", | ||
| + ); | ||
| } | ||
|
|
||
| if (Object.keys(expectedLocation).length > 0) { | ||
| ``` | ||
|
|
||
| ## Documentation | ||
|
|
||
| This RFC will be documented in the RuleTester documentation, explaining the new options and how to use them. | ||
| So mainly here: https://eslint.org/docs/latest/integrate/nodejs-api#ruletester | ||
| Additionally, we should write a short blog post to announce for plugin maintainers, to raise awareness of the new options and encourage them to use them in their tests. | ||
|
|
||
| ## Drawbacks | ||
|
|
||
| This proposal adds slightly more complexity to the RuleTester logic, as it needs to handle the new options and enforce the assertions based on them. | ||
| Currently, the RuleTester logic is already deeply nested, so adding more options may make it harder to read and maintain. | ||
|
|
||
| Due to limitations of the `RuleTester`, the error message does not point to the correct location of the test case. | ||
| While this is true already, it gets slightly more relevant, since now it might report missing properties, that cannot be searched for in the test file. | ||
mdjermanovic marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| See also: https://github.com/eslint/eslint/issues/19936 | ||
|
|
||
| If we enable the `requireMessage` and `requireLocation` options by default, it would be a breaking change for existing tests that do not follow these assertion requirements yet. | ||
| It is not planned to enable `requireMessage` or `requireLocation` by default. | ||
|
|
||
| ## Backwards Compatibility Analysis | ||
|
|
||
| This change should not affect existing ESLint users or plugin developers, as it only adds new options to the RuleTester and does not change any existing behavior. | ||
|
|
||
| ## Alternatives | ||
|
|
||
| ### Constructor based options | ||
|
|
||
| ```ts | ||
| new RuleTester(testerConfig: {...}, assertionOptions: { | ||
| /** | ||
| * Require message assertion for each invalid test case. | ||
| * If `true`, each error must extend `string | RegExp | { message: string | RegExp } | { messageId: string }`. | ||
| * If `'message'`, each error must extend `string | RegExp | { message: string | RegExp }`. | ||
| * If `'messageId'`, each error must extend `{ messageId: string }`. | ||
| * | ||
| * @default false | ||
| */ | ||
| requireMessage: boolean | 'message' | 'messageId'; | ||
| /** | ||
| * Require full location assertions for each invalid test case. | ||
| * If `true`, each error must extend `{ line: number, column: number, endLine?: number | undefined, endColumn?: number | undefined }`. | ||
| * `endLine` or `endColumn` may be absent, if the observed error does not contain these properties. | ||
| * | ||
| * @default false | ||
| */ | ||
| requireLocation: boolean; | ||
| } = {}); | ||
| ``` | ||
|
|
||
| ### Options Setter Method | ||
|
|
||
| Alternatively add an `setAssertionOptions()` method independently of the constructor and test method. | ||
|
|
||
| ### ESLint RuleTester Lint-Rule | ||
|
|
||
| As an alternative to this proposal, we could add a eslint rule that applies the same assertions, but uses the central eslint config. | ||
| While this would apply the same assertions for all rule testers, it would be a lot more complex to implement and maintain, | ||
| it requires identifying the RuleTester calls in the codebase and might run into issues if the assertions aren't specified inline but via a variable or transformation. | ||
ST-DDT marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| ### RuleTester Estimated Error Location | ||
|
|
||
| The following issue makes it easier to identify the exact/estimated error location (outside of/more precise than `RuleTester#run`): | ||
|
|
||
| - https://github.com/eslint/eslint/issues/19936 | ||
|
|
||
| ## Open Questions | ||
|
|
||
| 1. ~~Is there a need for disabling scenarios like `valid` or `invalid`?~~ No, unused scenarios can be omitted using empty arrays. If needed, this option can be added later on. | ||
| 2. ~~Should we use constructor-based options or test method-based options? Do we support both? Or global options so it applies to all test files?~~ Currently, the method level options (variant 2) is the prefered one. | ||
| 3. ~~Should we enable the `requireMessage` and `requireLocation` options by default? (Breaking change)~~ No | ||
| 4. ~~Do we add a `requireMessageId` option or should we alter the `requireMessage` option to support both message and messageId assertions?~~ Just `requireMessage: boolean | 'message' | 'messageid'` | ||
| 5. ~~Should we add a `strict` option that enables all assertion options by default?~~ No, currently not planned. | ||
| 6. ~~Should we inline the options?~~ No, this might cause issues with alphabetical object properties. | ||
| 7. ~~What should the otpions be called?~~ We name it `assertionOptions` to avoid issues with alphabetical object properties. e.g. `invalid, options, valid` | ||
|
|
||
| ## Help Needed | ||
|
|
||
| English is not my first language, so I would appreciate help with the wording and grammar of this RFC. | ||
ST-DDT marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| I'm able to implement this RFC, if we decide to go with options instead of a new rule. | ||
|
|
||
| ## Frequently Asked Questions | ||
|
|
||
| ### Why | ||
|
|
||
| Because it is easy to miss a missing assertion in RuleTester test cases, especially when many new invalid test cases are added. | ||
|
|
||
| ## Related Discussions | ||
|
|
||
| The idea was initially sparked by this comment: vuejs/eslint-plugin-vue#2773 (comment) | ||
|
|
||
| - https://github.com/vuejs/eslint-plugin-vue/pull/2773#discussion_r2176359714 | ||
|
|
||
| > It might be helpful to include more detailed error information, such as line, column, endLine, and endColumn... | ||
| > [...] | ||
| > Let’s make the new cases more detailed first. 😊 | ||
|
|
||
| The first steps have been taken in: eslint/eslint#19904 - feat: output full actual location in rule tester if different | ||
|
|
||
| - https://github.com/eslint/eslint/pull/19904 | ||
|
|
||
| This lead to the issue that this RFC is based on: eslint/eslint#19921 - Change Request: Add options to rule-tester requiring certain assertions | ||
|
|
||
| - https://github.com/eslint/eslint/issues/19921 | ||
Uh oh!
There was an error while loading. Please reload this page.