From 7aa4b945ef453c0237551e6ec5a2f087c7439436 Mon Sep 17 00:00:00 2001 From: Bryan Mishkin <698306+bmish@users.noreply.github.com> Date: Sun, 1 Jan 2023 20:15:13 -0600 Subject: [PATCH 1/9] feat: support for testing invalid rule schemas and runtime exceptions --- designs/2023-test-rule-errors/README.md | 295 ++++++++++++++++++++++++ 1 file changed, 295 insertions(+) create mode 100644 designs/2023-test-rule-errors/README.md diff --git a/designs/2023-test-rule-errors/README.md b/designs/2023-test-rule-errors/README.md new file mode 100644 index 00000000..087ea0b2 --- /dev/null +++ b/designs/2023-test-rule-errors/README.md @@ -0,0 +1,295 @@ +- Repo: eslint/eslint +- Start Date: 2023-01-01 +- RFC PR: +- Authors: [bmish](https://github.com/bmish) + +# Support for testing invalid rule schemas and runtime exceptions in rules + +## Summary + + + +Enable rule authors to write unit tests for invalid rule schemas and runtime exceptions in rules. + +## Motivation + + + +In certain situations, an exception can be thrown when running an ESLint rule, including: + +1. ESLint throws an exception because a rule is configured incorrectly. That is, the user-provided options did not conform to a rule's schema (`meta.schema`). +2. A rule implementation (in the `create()` function) throws an exception. Examples of why this could happen: + - The rule threw a custom exception when additional input validation failed (typically used for a constraint that couldn't be represented/enforced by the schema). + - The rule threw a custom exception because an unhandled or unexpected situation occurred, such as a type of node the rule wasn't expecting to occur in a particular position, perhaps due an oversight, or due to a new JavaScript language feature that the rule doesn't support yet. + - The rule implementation has an unknown bug that causes it to unintentionally crash. + +Other than for unanticipated cases like an unknown bug in the rule, the user may want to write tests for these situations, but is unable to do so today. It's currently only possible to write passing test cases (rule runs successfully and does not report violations) or failing test cases (rule runs successfully and reports violations). + +As a result of this limitation, users wanting to promote rule quality by ensuring their rules have complete test coverage, i.e. all of the logic is exercised in tests, may be unable to do so. + +### Rule schemas + +Today, unit tests for a rule will ideally ensure that the rule behavior is correct for all possible combinations of valid rule options, but it is not currently possible to test that a rule correctly disallows invalid rules schemas. + +For simple schemas, testing might not be as critical, but testing becomes more important when rule schemas grow more complex. Some schemas can even reach 100 lines long, and often allow various formats, such as in [no-restricted-imports](https://eslint.org/docs/rules/no-restricted-imports) which allows either an array of strings or an array of objects. + +For example, with the rule [no-restricted-imports](https://eslint.org/docs/rules/no-restricted-imports), one might want to test that the rule schema fails validation when passed: + +- Something that isn't an array +- An empty array +- An array containing an item that isn't a string nor object +- An array containing an object that is missing required properties like `name`, or has unknown properties +- Any other invalid combinations of input + +Note that the goal is not to test that [ajv](https://github.com/ajv-validator/ajv) (which performs the validation using JSON Schema) itself works properly, as we can treat that third-party dependency as a blackbox, but instead to test that the rule author has written out their schema correctly. + +It can be tricky to get schemas to perfectly represent what the allowed input should be, especially when no automated testing exists around this today. Schemas are often too lenient, allowing extra input that the rule implementation ignores or doesn't handle properly. This can result in rules silently misbehaving, not respecting the wishes or intentions of the user's configuration, or just crashing. + +By enabling testing of schemas, we'll make it easier to write higher-quality schemas and rules, thus improving the user experience. + +## Detailed Design + + + +We will augment the error objects of invalid test cases to support a `fatal: true` property. This will indicate that the test case triggered an exception with the given message. + +Example with a schema validation error because a non-existent option is passed: + +```json +{ + "invalid": [ + { + "code": "foo();", + "options": [{ "checkFoo": true, "nonExistentOption": true }], + "errors": [ + { + "message": "Value {\"checkFoo\":true,\"nonExistentOption\":true} should NOT have additional properties.", + "fatal": true + } + ] + } + ] +} +``` + +To test a custom exception thrown by a rule, simply use the exception text as the message: `"message": "Some custom exception message from the rule."`. + +A convenience feature to go along with this is to allow `messageId` to be set to some fixed values like `SCHEMA_VALIDATION_ERROR` or `CUSTOM_RULE_ERROR`. This would be useful since most of the time, the user may not care about the exact text of the exception, but only that the particular type of exception was thrown. This also makes it easier to update the exception text without having to update the messages in all the test cases. + +New constraints on test cases with a fatal error: + +- `code` is not required (the code may be irrelevant when testing options) + - **Open Question**: If this is inconvenient or breaks too many assumptions, we can continue to require it but just let the user provide a dummy value like `code: "foo();"` +- Only one error object can be provided (can't have multiple exceptions at the same time) +- Cannot have `output` in the top-level object (no violation/autofix can result from a fatal error) +- Cannot have `suggestions` in the error object (suggestions are only for violations) +- This feature can only be used for testing actual user-facing exceptions, not for testing exceptions thrown by the rule tester itself, nor for testing syntax/parsing errors + +### Implementation + +This will require changes to: + +- lib/rule-tester/rule-tester.js +- lib/rule-tester/flat-rule-tester.js + +And new tests in: + +- tests/lib/rule-tester/rule-tester.js +- tests/lib/rule-tester/flat-rule-tester.js + +First, we need to adjust the existing test case validation asserts to allow the tweaked test case format when fatal errors are present and enforce the new constraints mentioned above. + +Then, when we are executing a test case with a fatal error, we need some way to to convert any ESLint-generated exception during schema validation and any rule-generated exception during rule execution into a standard test error object with a `LintMessage` with `fatal: true`. This will allow the message to be compared against the expected error message in the test case. + +We could approach this in one of two ways: + +1. Approach #1: In `runRuleForItem()`, when executing a fatal test case, pass a new option to `validate()` and `linter.verify()` to instruct them to return a message for the relevant exceptions instead of throwing them. But `linter.verify()` is part of ESLint's public API and it's not clear this option would be useful for anyone else. So we're disqualifying this approach as of now. +2. Approach #2: In `runRuleForItem()`, when executing a fatal test case, surround the `validate()` and `linter.verify()` calls with a try/catch block, and convert the relevant exceptions to messages. + +Approach #2 is promising, but there are still a number of issues to solve with it: + +1. Issue #1: We need some way to determine which exceptions to convert to messages, and which exceptions to let bubble up, as there are many irrelevant exceptions that can be thrown by these functions which users shouldn't be testing. +2. Issue #2: We need to omit the superfluous ESLint helper text from the error message returned, as ESLint helper text is subject to change, not part of the original exception from the schema validation or rule, and unnecessarily including these extra lines forces the user to always write out a tedious, multi-line string with them for the message in their test case (unless they use a regexp to match part of the message). Example of exceptions with the ESLint helper text: + + ```pt + rule-tester: + Configuration for rule "no-foo" is invalid: + Value {"checkFoo":true,"nonExistentOption":true} should NOT have additional properties. + ``` + + ```pt + Some custom exception message from the rule... + Occurred while linting :2 + Rule: "no-foo" + ``` + +A few ways we could solve these issues with approach #2: + +1. Fix #1: Do string matching on the ESLint helper text in the exception message to determine if it's one of the relevant exceptions to convert and return as a message. It could help to prefix the exception message with an error code for easier and less-brittle string matching. Note that this fix requires manually stripping the ESLint helper text, which is also brittle, but workable. The code would be similar to Fix #2 but with string matching instead of relying on `err.messageForTest`. + +2. Fix #2: Add a property to the relevant exceptions `err.messageForTest = '...';` at their source and then use this property to determine if it's one of the relevant exceptions to convert to a message and what the message should be. This is more robust than string matching, but the downside is we have to add a custom property to some exception objects, and it requires the source of these exceptions to have awareness of the rule tester, which is not ideal. Example code: + + ```js + // Inside the rule tester class. + function runRuleForItem(item) { + // ... + + // Only surround these calls with a try/catch if the current test case is for a fatal error. + try { + validate(...); + // or + messages = linter.verify(...); + } catch (err) { + if (err.messageForTest) { + // Return a message so this exception can be tested. + return { + messages: [{ + ruleId: ruleName, + fatal: true, + message: err.messageForTest, + }], + }; + } + + // Not one of the relevant exceptions for testing. + throw err; + } + } + ``` + +I'm interested to gather feedback on these approaches and preferences about what the implementation should look like. + +## Documentation + + + +We will document this change in the ESLint [RuleTester](https://eslint.org/docs/latest/developer-guide/nodejs-api#ruletester) section of the [Node.JS API](https://eslint.org/docs/latest/developer-guide/nodejs-api) page. + +Ideally, we can also draw attention to it with a paragraph and example usage in the blog post / release notes for the version it ships in. This is the least we can do to raise awareness of it for rule authors who may be interested to take advantage of it in new or existing rules. + +## Drawbacks + + + +- A user testing that their rule schema catches invalid options may end up including the message from JSON Schema in their test case (e.g. `Value "bar" should be boolean.`). This could make it more difficult for ESLint to upgrade its version of ajv / JSON Schema in the future, as an upgraded version could tweak messages and necessitate updating any test cases that include the message. This is a relatively minor concern, as it's unlikely that messages will change often, and it's easy to update test cases if they do. Two factors that could mitigate this: + - If [snapshot testing](https://github.com/eslint/eslint/issues/14936) is implemented in the future, it would become possible to fix test cases with a single command. + - We can encourage the use of `messageId` (discussed in [Detailed Design](#detailed-design)) for the majority of cases that don't care about including the exact message text. + +## Backwards Compatibility Analysis + + + +This new feature is a non-breaking change and has no impact unless a user chooses to use it. + +## Alternatives + + + +### Top-level `error` array + +Instead of using the `invalid` test cases to cover this new functionality, we could have a dedicated `error` array of test cases for testing exceptions: + +```json +{ + "error": [ + { + "code": "foo();", + "options": [{ "foo": true }], + "message": "some exception message...", + } + ] +} +``` + +This is similar to the [API](https://github.com/ember-template-lint/ember-template-lint/blob/master/docs/plugins.md#rule-tests) that the linter [ember-template-lint](https://github.com/ember-template-lint/ember-template-lint) uses for testing rule exceptions, which could be referred to as prior art. + +Pros: + +- Avoid overloading existing invalid test cases to serve additional purposes which could complicate the usage, documentation, and implementation logic +- Avoid confusing third-party tooling that may be analyzing (e.g. linting in the case of [eslint-plugin-eslint-plugin](https://github.com/eslint-community/eslint-plugin-eslint-plugin)) or running invalid test cases with the assumption that they all report violations + +Cons: + +- Not able to share as much code with invalid test cases +- Large change / addition to the API of rule tester, as opposed to the original proposal which essentially just adds a single `error: true` property + +## Open Questions + + + +1. Are we satisfied with the proposed [API](#detailed-design)? For transparency, there's an alternative design in the [Alternatives](#alternatives) section. +2. Have we come up with the ideal [implementation](#implementation)? +3. Are there other kinds of exceptions we want to allow to be tested? The current approach essentially defines an allowlist of exceptions that are testable so that users can't test irrelevant exceptions (and so that we don't unnecessarily expose exceptions that are subject to change in our API). Note that we can add more exceptions later if we determine a need. +4. The current proposal doesn't change the structure of test cases much, so it's not anticipated that it will have any meaningful impact on a future not-yet-designed [snapshot testing](https://github.com/eslint/eslint/issues/14936) feature, but it's worth considering. +5. The property `fatal: true` is used internally to represent parsing errors. Could overloading that cause any problem or confusion? Note that we don't want to allow testing syntax/parsing errors. Also note that parsing errors and the exceptions being tested are both fatal exceptions, so it could make sense to use the same property name, as long as it doesn't cause problems. `error` or `exception` are alternative names which mean the same thing, but having multiple property names that mean the same thing could also be confusing. + +## Help Needed + + + +I expect to implement this change. + +## Frequently Asked Questions + + + +## Related Discussions + + + +- - the issue triggering this RFC +- - related, recent RFC to make RuleTester more strict From beab520a163cc5ddf0a0a6245cc8063815bb98a8 Mon Sep 17 00:00:00 2001 From: Bryan Mishkin <698306+bmish@users.noreply.github.com> Date: Wed, 18 Jan 2023 20:36:55 -0500 Subject: [PATCH 2/9] switch to separate array of test cases --- designs/2023-test-rule-errors/README.md | 90 +++++++++++++++---------- 1 file changed, 53 insertions(+), 37 deletions(-) diff --git a/designs/2023-test-rule-errors/README.md b/designs/2023-test-rule-errors/README.md index 087ea0b2..2d7e72a1 100644 --- a/designs/2023-test-rule-errors/README.md +++ b/designs/2023-test-rule-errors/README.md @@ -59,22 +59,17 @@ By enabling testing of schemas, we'll make it easier to write higher-quality sch used. Be sure to define any new terms in this section. --> -We will augment the error objects of invalid test cases to support a `fatal: true` property. This will indicate that the test case triggered an exception with the given message. +Add support to the rule tester for a new `fatal` test case array in addition to `valid` and `invalid`. Example with a schema validation error because a non-existent option is passed: ```json { - "invalid": [ + "fatal": [ { "code": "foo();", "options": [{ "checkFoo": true, "nonExistentOption": true }], - "errors": [ - { - "message": "Value {\"checkFoo\":true,\"nonExistentOption\":true} should NOT have additional properties.", - "fatal": true - } - ] + "message": "Value {\"checkFoo\":true,\"nonExistentOption\":true} should NOT have additional properties." } ] } @@ -82,16 +77,34 @@ Example with a schema validation error because a non-existent option is passed: To test a custom exception thrown by a rule, simply use the exception text as the message: `"message": "Some custom exception message from the rule."`. -A convenience feature to go along with this is to allow `messageId` to be set to some fixed values like `SCHEMA_VALIDATION_ERROR` or `CUSTOM_RULE_ERROR`. This would be useful since most of the time, the user may not care about the exact text of the exception, but only that the particular type of exception was thrown. This also makes it easier to update the exception text without having to update the messages in all the test cases. +A convenience feature to go along with this is to allow an alternative property `source: 'SCHEMA_VALIDATION' | 'RULE_IMPLEMENTATION'` to be specified indicating the type of exception without indicating the exception text. +This would be useful since most of the time, the user may not care about the exact text of the exception, but only that the particular type of exception was thrown. +This also makes it easier to update the exception text without having to update the messages in all the test cases. +Alternate names for this property to consider: `messageType`, `errorType`. -New constraints on test cases with a fatal error: +Differences between fatal test cases and invalid test cases: - `code` is not required (the code may be irrelevant when testing options) - - **Open Question**: If this is inconvenient or breaks too many assumptions, we can continue to require it but just let the user provide a dummy value like `code: "foo();"` -- Only one error object can be provided (can't have multiple exceptions at the same time) -- Cannot have `output` in the top-level object (no violation/autofix can result from a fatal error) -- Cannot have `suggestions` in the error object (suggestions are only for violations) -- This feature can only be used for testing actual user-facing exceptions, not for testing exceptions thrown by the rule tester itself, nor for testing syntax/parsing errors +- There's a required `message` property instead of an `errors` property + +This feature can only be used for testing the following user-facing exceptions: + +- ESLint-generated exception during schema validation +- Rule-generated exceptions + +Note that this feature cannot be used for testing exceptions thrown by the rule tester itself, nor for testing syntax/parsing errors. + +This design is similar to the [API](https://github.com/ember-template-lint/ember-template-lint/blob/master/docs/plugins.md#rule-tests) that the linter [ember-template-lint](https://github.com/ember-template-lint/ember-template-lint) uses for testing rule exceptions, which could be referred to as prior art. + +Pros of this design vs. the [reuse `invalid` test cases](#reuse-invalid-test-cases) alternative: + +- Avoid overloading existing invalid test cases to serve additional purposes which could complicate the usage, documentation, and implementation logic +- Avoid confusing third-party tooling that may be analyzing (e.g. linting in the case of [eslint-plugin-eslint-plugin](https://github.com/eslint-community/eslint-plugin-eslint-plugin)) or running invalid test cases with the assumption that they all report violations + +Cons vs. the [reusing `invalid` test cases](#reuse-invalid-test-cases) alternative: + +- Not able to share as much code with invalid test cases +- Large change / addition to the API of rule tester, as opposed to just adding a single `error: true` property ### Implementation @@ -105,9 +118,9 @@ And new tests in: - tests/lib/rule-tester/rule-tester.js - tests/lib/rule-tester/flat-rule-tester.js -First, we need to adjust the existing test case validation asserts to allow the tweaked test case format when fatal errors are present and enforce the new constraints mentioned above. +First, we'll follow the pattern of the existing `valid` and `invalid` test case arrays to add support for a new `fatal` test case array in the rule tester. -Then, when we are executing a test case with a fatal error, we need some way to to convert any ESLint-generated exception during schema validation and any rule-generated exception during rule execution into a standard test error object with a `LintMessage` with `fatal: true`. This will allow the message to be compared against the expected error message in the test case. +Then, when we are executing a test case with a fatal error, we need some way to convert any ESLint-generated exception during schema validation and any rule-generated exception during rule execution into a standard test error object with a `LintMessage` with `fatal: true`. This will allow the message to be compared against the expected error message in the test case. We could approach this in one of two ways: @@ -193,7 +206,7 @@ Ideally, we can also draw attention to it with a paragraph and example usage in - A user testing that their rule schema catches invalid options may end up including the message from JSON Schema in their test case (e.g. `Value "bar" should be boolean.`). This could make it more difficult for ESLint to upgrade its version of ajv / JSON Schema in the future, as an upgraded version could tweak messages and necessitate updating any test cases that include the message. This is a relatively minor concern, as it's unlikely that messages will change often, and it's easy to update test cases if they do. Two factors that could mitigate this: - If [snapshot testing](https://github.com/eslint/eslint/issues/14936) is implemented in the future, it would become possible to fix test cases with a single command. - - We can encourage the use of `messageId` (discussed in [Detailed Design](#detailed-design)) for the majority of cases that don't care about including the exact message text. + - We can encourage the use of `source` property (discussed in [Detailed Design](#detailed-design)) for the majority of cases that don't care about including the exact exception text. ## Backwards Compatibility Analysis @@ -214,33 +227,37 @@ This new feature is a non-breaking change and has no impact unless a user choose projects have already implemented a similar feature. --> -### Top-level `error` array +### Reuse `invalid` test cases + +Augment the error objects of invalid test cases to support a `fatal: true` property. This will indicate that the test case triggered an exception with the given message. -Instead of using the `invalid` test cases to cover this new functionality, we could have a dedicated `error` array of test cases for testing exceptions: +Example with a schema validation error because a non-existent option is passed: ```json { - "error": [ + "invalid": [ { "code": "foo();", - "options": [{ "foo": true }], - "message": "some exception message...", + "options": [{ "checkFoo": true, "nonExistentOption": true }], + "errors": [ + { + "message": "Value {\"checkFoo\":true,\"nonExistentOption\":true} should NOT have additional properties.", + "fatal": true + } + ] } ] } ``` -This is similar to the [API](https://github.com/ember-template-lint/ember-template-lint/blob/master/docs/plugins.md#rule-tests) that the linter [ember-template-lint](https://github.com/ember-template-lint/ember-template-lint) uses for testing rule exceptions, which could be referred to as prior art. - -Pros: - -- Avoid overloading existing invalid test cases to serve additional purposes which could complicate the usage, documentation, and implementation logic -- Avoid confusing third-party tooling that may be analyzing (e.g. linting in the case of [eslint-plugin-eslint-plugin](https://github.com/eslint-community/eslint-plugin-eslint-plugin)) or running invalid test cases with the assumption that they all report violations +New constraints on test cases with a fatal error: -Cons: +- `code` is not required (the code may be irrelevant when testing options) +- Only one error object can be provided (can't have multiple exceptions at the same time) +- Cannot have `output` in the top-level object (no violation/autofix can result from a fatal error) +- Cannot have `suggestions` in the error object (suggestions are only for violations) -- Not able to share as much code with invalid test cases -- Large change / addition to the API of rule tester, as opposed to the original proposal which essentially just adds a single `error: true` property +During the RFC discussion, we decided against this alternative because fatal test cases are sufficiently-different from invalid test cases that they deserve to have their own separate array to avoid confusion. ## Open Questions @@ -255,11 +272,10 @@ Cons: you can remove this section. --> -1. Are we satisfied with the proposed [API](#detailed-design)? For transparency, there's an alternative design in the [Alternatives](#alternatives) section. -2. Have we come up with the ideal [implementation](#implementation)? -3. Are there other kinds of exceptions we want to allow to be tested? The current approach essentially defines an allowlist of exceptions that are testable so that users can't test irrelevant exceptions (and so that we don't unnecessarily expose exceptions that are subject to change in our API). Note that we can add more exceptions later if we determine a need. -4. The current proposal doesn't change the structure of test cases much, so it's not anticipated that it will have any meaningful impact on a future not-yet-designed [snapshot testing](https://github.com/eslint/eslint/issues/14936) feature, but it's worth considering. -5. The property `fatal: true` is used internally to represent parsing errors. Could overloading that cause any problem or confusion? Note that we don't want to allow testing syntax/parsing errors. Also note that parsing errors and the exceptions being tested are both fatal exceptions, so it could make sense to use the same property name, as long as it doesn't cause problems. `error` or `exception` are alternative names which mean the same thing, but having multiple property names that mean the same thing could also be confusing. +1. Have we come up with the ideal [implementation](#implementation)? +2. Are there other kinds of exceptions we want to allow to be tested? The current approach essentially defines an allowlist of exceptions that are testable so that users can't test irrelevant exceptions (and so that we don't unnecessarily expose exceptions that are subject to change in our API). Note that we can add more exceptions later if we determine a need. +3. It's unlikely but worth considering whether this feature would make it more difficult to support [snapshot testing](https://github.com/eslint/eslint/issues/14936). +4. The property `fatal: true` is used internally to represent parsing errors. Could overloading that cause any problem or confusion? Note that we don't want to allow testing syntax/parsing errors. Also note that parsing errors and the exceptions being tested are both fatal exceptions, so it could make sense to use the same property name, as long as it doesn't cause problems. `error`, `exception`, `throws` are alternative names which mean the same thing, but having multiple property names that mean the same thing could also be confusing. ## Help Needed From 96a5940cb80b7ba7bd03a5b3f1a2299d31380908 Mon Sep 17 00:00:00 2001 From: Bryan Mishkin <698306+bmish@users.noreply.github.com> Date: Fri, 20 Jan 2023 15:07:35 -0500 Subject: [PATCH 3/9] errorType property --- designs/2023-test-rule-errors/README.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/designs/2023-test-rule-errors/README.md b/designs/2023-test-rule-errors/README.md index 2d7e72a1..db6aca27 100644 --- a/designs/2023-test-rule-errors/README.md +++ b/designs/2023-test-rule-errors/README.md @@ -77,10 +77,10 @@ Example with a schema validation error because a non-existent option is passed: To test a custom exception thrown by a rule, simply use the exception text as the message: `"message": "Some custom exception message from the rule."`. -A convenience feature to go along with this is to allow an alternative property `source: 'SCHEMA_VALIDATION' | 'RULE_IMPLEMENTATION'` to be specified indicating the type of exception without indicating the exception text. +A convenience feature to go along with this is to allow an alternative property `errorType: 'schema' | 'rule'` to be specified indicating the type of exception without indicating the exception text. This would be useful since most of the time, the user may not care about the exact text of the exception, but only that the particular type of exception was thrown. This also makes it easier to update the exception text without having to update the messages in all the test cases. -Alternate names for this property to consider: `messageType`, `errorType`. +Alternate names for this property to consider: `messageErrorType`, `messageType`, `messageId`, `source`. Differences between fatal test cases and invalid test cases: @@ -206,7 +206,7 @@ Ideally, we can also draw attention to it with a paragraph and example usage in - A user testing that their rule schema catches invalid options may end up including the message from JSON Schema in their test case (e.g. `Value "bar" should be boolean.`). This could make it more difficult for ESLint to upgrade its version of ajv / JSON Schema in the future, as an upgraded version could tweak messages and necessitate updating any test cases that include the message. This is a relatively minor concern, as it's unlikely that messages will change often, and it's easy to update test cases if they do. Two factors that could mitigate this: - If [snapshot testing](https://github.com/eslint/eslint/issues/14936) is implemented in the future, it would become possible to fix test cases with a single command. - - We can encourage the use of `source` property (discussed in [Detailed Design](#detailed-design)) for the majority of cases that don't care about including the exact exception text. + - We can encourage the use of `errorType` property (discussed in [Detailed Design](#detailed-design)) for the majority of cases that don't care about including the exact exception text. ## Backwards Compatibility Analysis From 8359badafb9cd29611a23385bfb54fca66583e2d Mon Sep 17 00:00:00 2001 From: Bryan Mishkin <698306+bmish@users.noreply.github.com> Date: Sat, 21 Jan 2023 11:56:11 -0500 Subject: [PATCH 4/9] switch to error object with message and name --- designs/2023-test-rule-errors/README.md | 48 ++++++++++++++----------- 1 file changed, 27 insertions(+), 21 deletions(-) diff --git a/designs/2023-test-rule-errors/README.md b/designs/2023-test-rule-errors/README.md index db6aca27..9fe32303 100644 --- a/designs/2023-test-rule-errors/README.md +++ b/designs/2023-test-rule-errors/README.md @@ -69,23 +69,29 @@ Example with a schema validation error because a non-existent option is passed: { "code": "foo();", "options": [{ "checkFoo": true, "nonExistentOption": true }], - "message": "Value {\"checkFoo\":true,\"nonExistentOption\":true} should NOT have additional properties." + "error": { + "message": "Value {\"checkFoo\":true,\"nonExistentOption\":true} should NOT have additional properties.", + "name": "SchemaValidationError" + } } ] } ``` -To test a custom exception thrown by a rule, simply use the exception text as the message: `"message": "Some custom exception message from the rule."`. +To test a custom exception thrown by a rule, simply use the exception text as the message: `"message": "Some custom exception message from the rule."` and/or the name of the exception class as the `name`. + +The `error` object in the test case uses the `name` and `message` properties directly from the original exception object, while grouping them nicely together in the same structure as the original exception. Similar to `message` and `messageId` in `invalid` test cases, only one of `message` and `name` is required. If the user doesn't care to test the full exception `message` text, the exception class `name` can be used instead. + +Examples of how exceptions would be thrown with different exception classes that could be distinguished by `name`: -A convenience feature to go along with this is to allow an alternative property `errorType: 'schema' | 'rule'` to be specified indicating the type of exception without indicating the exception text. -This would be useful since most of the time, the user may not care about the exact text of the exception, but only that the particular type of exception was thrown. -This also makes it easier to update the exception text without having to update the messages in all the test cases. -Alternate names for this property to consider: `messageErrorType`, `messageType`, `messageId`, `source`. +- `throw new SchemaValidationError('...should NOT have additional properties.')` (custom error type thrown by schema validation) +- `throw new SomeCustomRuleError('...')` (custom error type thrown by the rule implementation) +- `throw new Error('...')` (basic error thrown by the rule implementation) -Differences between fatal test cases and invalid test cases: +Differences between `fatal` test cases and `invalid` test cases: - `code` is not required (the code may be irrelevant when testing options) -- There's a required `message` property instead of an `errors` property +- There's a required `error` object instead of an `errors` array This feature can only be used for testing the following user-facing exceptions: @@ -96,16 +102,6 @@ Note that this feature cannot be used for testing exceptions thrown by the rule This design is similar to the [API](https://github.com/ember-template-lint/ember-template-lint/blob/master/docs/plugins.md#rule-tests) that the linter [ember-template-lint](https://github.com/ember-template-lint/ember-template-lint) uses for testing rule exceptions, which could be referred to as prior art. -Pros of this design vs. the [reuse `invalid` test cases](#reuse-invalid-test-cases) alternative: - -- Avoid overloading existing invalid test cases to serve additional purposes which could complicate the usage, documentation, and implementation logic -- Avoid confusing third-party tooling that may be analyzing (e.g. linting in the case of [eslint-plugin-eslint-plugin](https://github.com/eslint-community/eslint-plugin-eslint-plugin)) or running invalid test cases with the assumption that they all report violations - -Cons vs. the [reusing `invalid` test cases](#reuse-invalid-test-cases) alternative: - -- Not able to share as much code with invalid test cases -- Large change / addition to the API of rule tester, as opposed to just adding a single `error: true` property - ### Implementation This will require changes to: @@ -146,7 +142,7 @@ Approach #2 is promising, but there are still a number of issues to solve with i A few ways we could solve these issues with approach #2: -1. Fix #1: Do string matching on the ESLint helper text in the exception message to determine if it's one of the relevant exceptions to convert and return as a message. It could help to prefix the exception message with an error code for easier and less-brittle string matching. Note that this fix requires manually stripping the ESLint helper text, which is also brittle, but workable. The code would be similar to Fix #2 but with string matching instead of relying on `err.messageForTest`. +1. Fix #1: Do string matching on the ESLint helper text in the exception message, or possibly the exception class name, to determine if it's one of the relevant exceptions to convert and return as a message. It could help to prefix the exception message with an error code for easier and less-brittle string matching. Note that this fix requires manually stripping the ESLint helper text, which is also brittle, but workable. The code would be similar to Fix #2 but with string matching instead of relying on `err.messageForTest`. 2. Fix #2: Add a property to the relevant exceptions `err.messageForTest = '...';` at their source and then use this property to determine if it's one of the relevant exceptions to convert to a message and what the message should be. This is more robust than string matching, but the downside is we have to add a custom property to some exception objects, and it requires the source of these exceptions to have awareness of the rule tester, which is not ideal. Example code: @@ -206,7 +202,7 @@ Ideally, we can also draw attention to it with a paragraph and example usage in - A user testing that their rule schema catches invalid options may end up including the message from JSON Schema in their test case (e.g. `Value "bar" should be boolean.`). This could make it more difficult for ESLint to upgrade its version of ajv / JSON Schema in the future, as an upgraded version could tweak messages and necessitate updating any test cases that include the message. This is a relatively minor concern, as it's unlikely that messages will change often, and it's easy to update test cases if they do. Two factors that could mitigate this: - If [snapshot testing](https://github.com/eslint/eslint/issues/14936) is implemented in the future, it would become possible to fix test cases with a single command. - - We can encourage the use of `errorType` property (discussed in [Detailed Design](#detailed-design)) for the majority of cases that don't care about including the exact exception text. + - We can encourage the use of `error.name` property (discussed in [Detailed Design](#detailed-design)) for the majority of cases that don't care about including the exact exception text. ## Backwards Compatibility Analysis @@ -229,6 +225,8 @@ This new feature is a non-breaking change and has no impact unless a user choose ### Reuse `invalid` test cases +> f**Note:** During the RFC discussion, we decided against this alternative because fatal test cases are sufficiently-different from invalid test cases that they deserve to have their own separate array to avoid confusion. + Augment the error objects of invalid test cases to support a `fatal: true` property. This will indicate that the test case triggered an exception with the given message. Example with a schema validation error because a non-existent option is passed: @@ -257,7 +255,15 @@ New constraints on test cases with a fatal error: - Cannot have `output` in the top-level object (no violation/autofix can result from a fatal error) - Cannot have `suggestions` in the error object (suggestions are only for violations) -During the RFC discussion, we decided against this alternative because fatal test cases are sufficiently-different from invalid test cases that they deserve to have their own separate array to avoid confusion. +Pros of a separate `fatal` error vs. reusing the `invalid` array: + +- Avoid overloading existing invalid test cases to serve additional purposes which could complicate the usage, documentation, and implementation logic +- Avoid confusing third-party tooling that may be analyzing (e.g. linting in the case of [eslint-plugin-eslint-plugin](https://github.com/eslint-community/eslint-plugin-eslint-plugin)) or running invalid test cases with the assumption that they all report violations + +Cons of a separate `fatal` error vs. reusing the `invalid` array: + +- Not able to share as much code with invalid test cases +- Large change / addition to the API of rule tester, as opposed to just adding a single `error: true` property ## Open Questions From eebb1eba7ab50c27e80e14dde17c7e9fc0f3b505 Mon Sep 17 00:00:00 2001 From: Bryan Mishkin <698306+bmish@users.noreply.github.com> Date: Tue, 24 Jan 2023 19:39:00 -0500 Subject: [PATCH 5/9] simplify implementation section based on learnings from draft PR --- designs/2023-test-rule-errors/README.md | 86 +++++++++++-------------- 1 file changed, 39 insertions(+), 47 deletions(-) diff --git a/designs/2023-test-rule-errors/README.md b/designs/2023-test-rule-errors/README.md index 9fe32303..4becb3d5 100644 --- a/designs/2023-test-rule-errors/README.md +++ b/designs/2023-test-rule-errors/README.md @@ -91,6 +91,7 @@ Examples of how exceptions would be thrown with different exception classes that Differences between `fatal` test cases and `invalid` test cases: - `code` is not required (the code may be irrelevant when testing options) + - If neither `code` nor `name` are provided, we'll use a placeholder name `(Test Case #x` as the test case's `it` title) - There's a required `error` object instead of an `errors` array This feature can only be used for testing the following user-facing exceptions: @@ -104,26 +105,22 @@ This design is similar to the [API](https://github.com/ember-template-lint/ember ### Implementation -This will require changes to: +> Draft/WIP PR demonstrating a working implementation of the current design: [#16823](https://github.com/eslint/eslint/pull/16823) + +The primary changes will be to the rule tester: - lib/rule-tester/rule-tester.js - lib/rule-tester/flat-rule-tester.js - -And new tests in: - - tests/lib/rule-tester/rule-tester.js - tests/lib/rule-tester/flat-rule-tester.js First, we'll follow the pattern of the existing `valid` and `invalid` test case arrays to add support for a new `fatal` test case array in the rule tester. -Then, when we are executing a test case with a fatal error, we need some way to convert any ESLint-generated exception during schema validation and any rule-generated exception during rule execution into a standard test error object with a `LintMessage` with `fatal: true`. This will allow the message to be compared against the expected error message in the test case. +Then, when we are executing a test case with a fatal error, we need some way to convert any ESLint-generated exception during schema validation and any rule-generated exception during rule execution into an object that can be compared against the expected error message in the test case. -We could approach this in one of two ways: +In `runRuleForItem()`, when executing a fatal test case, we'll surround the `validate()` and `linter.verify()` calls with a try/catch block, and convert the relevant exceptions to an error object to be returned. -1. Approach #1: In `runRuleForItem()`, when executing a fatal test case, pass a new option to `validate()` and `linter.verify()` to instruct them to return a message for the relevant exceptions instead of throwing them. But `linter.verify()` is part of ESLint's public API and it's not clear this option would be useful for anyone else. So we're disqualifying this approach as of now. -2. Approach #2: In `runRuleForItem()`, when executing a fatal test case, surround the `validate()` and `linter.verify()` calls with a try/catch block, and convert the relevant exceptions to messages. - -Approach #2 is promising, but there are still a number of issues to solve with it: +However, there are a number of issues to solve with this: 1. Issue #1: We need some way to determine which exceptions to convert to messages, and which exceptions to let bubble up, as there are many irrelevant exceptions that can be thrown by these functions which users shouldn't be testing. 2. Issue #2: We need to omit the superfluous ESLint helper text from the error message returned, as ESLint helper text is subject to change, not part of the original exception from the schema validation or rule, and unnecessarily including these extra lines forces the user to always write out a tedious, multi-line string with them for the message in their test case (unless they use a regexp to match part of the message). Example of exceptions with the ESLint helper text: @@ -140,41 +137,38 @@ Approach #2 is promising, but there are still a number of issues to solve with i Rule: "no-foo" ``` -A few ways we could solve these issues with approach #2: - -1. Fix #1: Do string matching on the ESLint helper text in the exception message, or possibly the exception class name, to determine if it's one of the relevant exceptions to convert and return as a message. It could help to prefix the exception message with an error code for easier and less-brittle string matching. Note that this fix requires manually stripping the ESLint helper text, which is also brittle, but workable. The code would be similar to Fix #2 but with string matching instead of relying on `err.messageForTest`. - -2. Fix #2: Add a property to the relevant exceptions `err.messageForTest = '...';` at their source and then use this property to determine if it's one of the relevant exceptions to convert to a message and what the message should be. This is more robust than string matching, but the downside is we have to add a custom property to some exception objects, and it requires the source of these exceptions to have awareness of the rule tester, which is not ideal. Example code: - - ```js - // Inside the rule tester class. - function runRuleForItem(item) { - // ... - - // Only surround these calls with a try/catch if the current test case is for a fatal error. - try { - validate(...); - // or - messages = linter.verify(...); - } catch (err) { - if (err.messageForTest) { - // Return a message so this exception can be tested. - return { - messages: [{ - ruleId: ruleName, - fatal: true, - message: err.messageForTest, - }], - }; - } - - // Not one of the relevant exceptions for testing. - throw err; - } - } - ``` +To solve these issues, we'll add a property to the relevant exceptions `err.messageForTest = '...';` at their source and then use this property to distinguish testable exceptions and what the original exception text was. The downside is we have to add a custom property to some exception objects, and this requires the source of these exceptions to have awareness of the rule tester, which is not ideal. Example code: + +```js +// Inside the rule tester class. +function runRuleForItem(item) { + // ... + + // Only surround these calls with a try/catch if the current test case is for a fatal error. + try { + validate(...); + // or + messages = linter.verify(...); + } catch (err) { + if (err.messageForTest) { + // Return a message so this exception can be tested. + return { + messages: [{ + ruleId: ruleName, + fatal: true, + message: err.messageForTest, + name: err.name, + }], + }; + } + + // Not one of the relevant exceptions for testing. + throw err; + } +} +``` -I'm interested to gather feedback on these approaches and preferences about what the implementation should look like. +> Draft/WIP PR demonstrating a working implementation of the current design: [#16823](https://github.com/eslint/eslint/pull/16823) ## Documentation @@ -279,9 +273,7 @@ Cons of a separate `fatal` error vs. reusing the `invalid` array: --> 1. Have we come up with the ideal [implementation](#implementation)? -2. Are there other kinds of exceptions we want to allow to be tested? The current approach essentially defines an allowlist of exceptions that are testable so that users can't test irrelevant exceptions (and so that we don't unnecessarily expose exceptions that are subject to change in our API). Note that we can add more exceptions later if we determine a need. -3. It's unlikely but worth considering whether this feature would make it more difficult to support [snapshot testing](https://github.com/eslint/eslint/issues/14936). -4. The property `fatal: true` is used internally to represent parsing errors. Could overloading that cause any problem or confusion? Note that we don't want to allow testing syntax/parsing errors. Also note that parsing errors and the exceptions being tested are both fatal exceptions, so it could make sense to use the same property name, as long as it doesn't cause problems. `error`, `exception`, `throws` are alternative names which mean the same thing, but having multiple property names that mean the same thing could also be confusing. +2. Are there other kinds of exceptions we want to allow to be tested? The current approach essentially defines an allowlist of exceptions that are testable (invalid rule options, and custom exceptions thrown by a rule implementation) so that users can't test irrelevant exceptions (and so that we don't unnecessarily expose exceptions that are subject to change in our API). Note that we can add more exceptions later if we determine a need. ## Help Needed From e11432df11a326d9954d0b619ce67c3697b0a017 Mon Sep 17 00:00:00 2001 From: Bryan Mishkin <698306+bmish@users.noreply.github.com> Date: Tue, 7 Feb 2023 12:08:31 -0800 Subject: [PATCH 6/9] clarify name/message property types and constraints --- designs/2023-test-rule-errors/README.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/designs/2023-test-rule-errors/README.md b/designs/2023-test-rule-errors/README.md index 4becb3d5..dc08310e 100644 --- a/designs/2023-test-rule-errors/README.md +++ b/designs/2023-test-rule-errors/README.md @@ -80,7 +80,10 @@ Example with a schema validation error because a non-existent option is passed: To test a custom exception thrown by a rule, simply use the exception text as the message: `"message": "Some custom exception message from the rule."` and/or the name of the exception class as the `name`. -The `error` object in the test case uses the `name` and `message` properties directly from the original exception object, while grouping them nicely together in the same structure as the original exception. Similar to `message` and `messageId` in `invalid` test cases, only one of `message` and `name` is required. If the user doesn't care to test the full exception `message` text, the exception class `name` can be used instead. +- The `error` object groups the properties from the exception together nicely. +- Similar to `message` and `messageId` in `invalid` test cases, only one of `message` and `name` is required. In particular, if the user doesn't care to test the full exception `message` text, the exception class `name` can be used instead. +- `message` can be provided as a string or a regular expression (regexp), while `name` can only be provided as a string. +- `message` comes from `err.message`, while `name` comes from `err.constructor.name`. Examples of how exceptions would be thrown with different exception classes that could be distinguished by `name`: @@ -219,7 +222,7 @@ This new feature is a non-breaking change and has no impact unless a user choose ### Reuse `invalid` test cases -> f**Note:** During the RFC discussion, we decided against this alternative because fatal test cases are sufficiently-different from invalid test cases that they deserve to have their own separate array to avoid confusion. +> **Note:** During the RFC discussion, we decided against this alternative because fatal test cases are sufficiently-different from invalid test cases that they deserve to have their own separate array to avoid confusion. Augment the error objects of invalid test cases to support a `fatal: true` property. This will indicate that the test case triggered an exception with the given message. From ce4bc70c28e90f05d68c424d900a6f62cc0446f8 Mon Sep 17 00:00:00 2001 From: Bryan Mishkin <698306+bmish@users.noreply.github.com> Date: Wed, 8 Feb 2023 10:37:45 -0800 Subject: [PATCH 7/9] tweak name source --- designs/2023-test-rule-errors/README.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/designs/2023-test-rule-errors/README.md b/designs/2023-test-rule-errors/README.md index dc08310e..116fc881 100644 --- a/designs/2023-test-rule-errors/README.md +++ b/designs/2023-test-rule-errors/README.md @@ -83,7 +83,8 @@ To test a custom exception thrown by a rule, simply use the exception text as th - The `error` object groups the properties from the exception together nicely. - Similar to `message` and `messageId` in `invalid` test cases, only one of `message` and `name` is required. In particular, if the user doesn't care to test the full exception `message` text, the exception class `name` can be used instead. - `message` can be provided as a string or a regular expression (regexp), while `name` can only be provided as a string. -- `message` comes from `err.message`, while `name` comes from `err.constructor.name`. +- `message` comes from `err.message` +- `name` comes from `err.name` or `err.constructor.name` in case either has a custom value (besides just `Error`) Examples of how exceptions would be thrown with different exception classes that could be distinguished by `name`: @@ -160,7 +161,7 @@ function runRuleForItem(item) { ruleId: ruleName, fatal: true, message: err.messageForTest, - name: err.name, + name: err.name === 'Error' ? err.constructor.name : err.name, }], }; } From 5d0746b4c859c8951d5eab2b1b30d81e3d74a952 Mon Sep 17 00:00:00 2001 From: Bryan Mishkin <698306+bmish@users.noreply.github.com> Date: Wed, 15 Mar 2023 16:16:27 -0400 Subject: [PATCH 8/9] ajv text is subject to change --- designs/2023-test-rule-errors/README.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/designs/2023-test-rule-errors/README.md b/designs/2023-test-rule-errors/README.md index 116fc881..e0467d09 100644 --- a/designs/2023-test-rule-errors/README.md +++ b/designs/2023-test-rule-errors/README.md @@ -198,9 +198,10 @@ Ideally, we can also draw attention to it with a paragraph and example usage in implementing this RFC as possible. --> -- A user testing that their rule schema catches invalid options may end up including the message from JSON Schema in their test case (e.g. `Value "bar" should be boolean.`). This could make it more difficult for ESLint to upgrade its version of ajv / JSON Schema in the future, as an upgraded version could tweak messages and necessitate updating any test cases that include the message. This is a relatively minor concern, as it's unlikely that messages will change often, and it's easy to update test cases if they do. Two factors that could mitigate this: +- A user testing that their rule schema catches invalid options may end up including the message from JSON Schema in their test case (e.g. `Value "bar" should be boolean.`). This could make it more difficult for ESLint to upgrade its version of ajv / JSON Schema in the future, as an upgraded version could tweak messages and necessitate updating any test cases that include the message. This is a relatively minor concern, as it's unlikely that messages will change often, and it's easy to update test cases if they do. A few factors that could mitigate this: - If [snapshot testing](https://github.com/eslint/eslint/issues/14936) is implemented in the future, it would become possible to fix test cases with a single command. - We can encourage the use of `error.name` property (discussed in [Detailed Design](#detailed-design)) for the majority of cases that don't care about including the exact exception text. + - We can add a note in the documentation for this feature that JSON Schema text is subject to change (could theoretically happen during an ajv upgrade). ## Backwards Compatibility Analysis From ce405bc0755b22a79263255b735982a3f8a41875 Mon Sep 17 00:00:00 2001 From: Bryan Mishkin <698306+bmish@users.noreply.github.com> Date: Wed, 15 Mar 2023 16:21:13 -0400 Subject: [PATCH 9/9] code empty string --- designs/2023-test-rule-errors/README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/designs/2023-test-rule-errors/README.md b/designs/2023-test-rule-errors/README.md index e0467d09..1ea6d7fe 100644 --- a/designs/2023-test-rule-errors/README.md +++ b/designs/2023-test-rule-errors/README.md @@ -96,6 +96,7 @@ Differences between `fatal` test cases and `invalid` test cases: - `code` is not required (the code may be irrelevant when testing options) - If neither `code` nor `name` are provided, we'll use a placeholder name `(Test Case #x` as the test case's `it` title) + - If `code` is not provided, we'll pass an empty string for it to `linter.verify()` - There's a required `error` object instead of an `errors` array This feature can only be used for testing the following user-facing exceptions: