Skip to content

Commit

Permalink
simplify implementation section based on learnings from draft PR
Browse files Browse the repository at this point in the history
  • Loading branch information
bmish committed Jan 25, 2023
1 parent 8359bad commit 5554e9d
Showing 1 changed file with 38 additions and 44 deletions.
82 changes: 38 additions & 44 deletions designs/2023-test-rule-errors/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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:
Expand All @@ -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

Expand Down

0 comments on commit 5554e9d

Please sign in to comment.