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
feat: ability to test rule errors and invalid schemas #16823
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for docs-eslint canceled.
|
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update. |
WIP. The implementation proposal was approved recently. Don't close |
a33a598
to
fbd87fd
Compare
@@ -1340,6 +1340,7 @@ class Linter { | |||
providedOptions.physicalFilename | |||
); | |||
} catch (err) { | |||
err.messageForTest = err.message; // Original exception message without extra helper text for asserting in fatal test cases. |
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 not sure how to hit this codepath in the tests. This is in _verifyWithoutProcessors()
.
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 would assume creating a rule that throws an error would hit this?
This is ready for review. |
Marking as accepted, as this implements a merged RFC. |
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. Seems pretty straightforward. I just left a couple of cleanup suggestions but otherwise, nice work!
Would like @mdjermanovic to review before merging.
@@ -1340,6 +1340,7 @@ class Linter { | |||
providedOptions.physicalFilename | |||
); | |||
} catch (err) { | |||
err.messageForTest = err.message; // Original exception message without extra helper text for asserting in fatal test cases. |
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 would assume creating a rule that throws an error would hit this?
docs/src/integrate/nodejs-api.md
Outdated
@@ -836,6 +859,8 @@ In addition to the properties above, invalid test cases can also have the follow | |||
If a string is provided as an error instead of an object, the string is used to assert the `message` of the error. | |||
* `output` (string, required if the rule fixes code): Asserts the output that will be produced when using this rule for a single pass of autofixing (e.g. with the `--fix` command line flag). If this is `null`, asserts that none of the reported problems suggest autofixes. | |||
|
|||
Fatal test cases (which are optional) are the same as invalid test cases, except that `code` is optional (it may be irrelevant when testing rule options), and there's an `error` object instead of an `errors` array. The `error` object should include one or both of the `message` of the error and the error (exception) class `name`. Fatal test cases can be used to test custom errors thrown by the rule, or invalid rule options (in which case the error `name` will be `SchemaValidationError`, and the `message` will be come from JSON Schema -- note that strings from JSON Schema are subject to change with future upgrades). |
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.
"are the same as invalid test cases" would be confusing because fatal and invalid test cases do not share any specific properties (invalid test cases have errors
and output
; fatal test cases have error
).
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.
Reworded this paragraph to be more clear.
@@ -130,6 +153,7 @@ const RuleTesterParameters = [ | |||
"code", | |||
"filename", | |||
"options", | |||
"error", |
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.
It would be good to ensure that valid and invalid test cases don't have an error
property and that fatal test cases don't have errors
or output
.
(the same for flat-rule-tester)
@@ -688,6 +746,22 @@ class RuleTester { | |||
try { | |||
SourceCode.prototype.getComments = getCommentsDeprecation; | |||
messages = linter.verify(code, config, filename); |
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.
If code
wasn't specified in the test case, this will fail with TypeError: Cannot read properties of undefined (reading 'text')
.
(the same for flat-rule-tester)
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 think I've already tested this scenario with test cases prefixed with:
// omit `code`
Isn't that the scenario you're talking about?
@bmish can you circle back around to finish this up based on @mdjermanovic's feedback? |
Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com>
Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
* main: (101 commits) docs: Migrating `eslint-env` configuration comments (eslint#17390) Merge pull request from GHSA-qwh7-v8hg-w8rh feat: adds option for allowing empty object patterns as parameter (eslint#17365) fix: FlatESLint#getRulesMetaForResults shouldn't throw on unknown rules (eslint#17393) docs: fix Ignoring Files section in config migration guide (eslint#17392) docs: Update README feat: Improve config error messages (eslint#17385) fix: Update no-loop-func to not overlap with no-undef (eslint#17358) docs: Update README docs: Update README 8.45.0 Build: changelog update for 8.45.0 chore: package.json update for @eslint/js release docs: add playground links to correct and incorrect code blocks (eslint#17306) docs: Expand rule option schema docs (eslint#17198) docs: Config Migration Guide (eslint#17230) docs: Update README fix: Fix suggestion message in `no-useless-escape` (eslint#17339) docs: Update README chore: update eslint-config-eslint exports (eslint#17336) ...
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
@mdjermanovic this one is ready for you to look over again |
@@ -2232,6 +2232,268 @@ describe("RuleTester", () => { | |||
}); | |||
}); | |||
|
|||
describe("fatal test cases", () => { |
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.
Moved to draft since it looks like I need to add more test cases to ensure all these scenarios work correctly:
- empty schema, validation error when options are passed
- present schema that has one validation error
- present schema that has multiple validation errors
In #17475 we have changed the logic in I was thinking that the same change should be applied to fatal tests: |
@bmish what's the status on this? Looks like it hasn't been updated in a while. Are you looking to get this into v9? |
@nzakas this isn't a breaking change so I don't think it needs to be included in the initial version of ESLint v9. Will aim to get it into a later version. |
Sounds good. I'll take it off my radar for now. |
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[X] Add something to the core
[ ] Other, please explain:
What changes did you make? (Give an overview)
Implements this RFC:
Fixes #13434.
TODO:
flat-rule-tester.js
Is there anything you'd like reviewers to focus on?