feat: add error assertion options #20247
Conversation
✅ Deploy Preview for docs-eslint canceled.
|
|
It may conflict with #20135 |
mdjermanovic
left a comment
There was a problem hiding this comment.
Can you also update the RuleTester documentation?
https://eslint.org/docs/latest/integrate/nodejs-api#ruletester
|
And types: Lines 1927 to 1991 in 2584187 |
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
…DDT/eslint into feat/error-assertion-options
|
I think 5ec4b83 should be reverted because |
mdjermanovic
left a comment
There was a problem hiding this comment.
Looks good overall. Just a few small suggestion, then LGTM.
|
After this gets merged, we should probably re-sort/re-group the rule-tester unit tests to align them with the methods in the source file. e.g.
Because I found it pretty hard to find the correct location for the new tests in that 6200 lines of code file. Do you accept those kind of PRs? |
| if (requireLocation) { | ||
| const missingKeys = locationProperties.filter( | ||
| key => | ||
| !hasOwnProperty(error, key) && | ||
| hasOwnProperty(message, key), | ||
| ); | ||
| assert.ok( | ||
| missingKeys.length === 0, | ||
| `Error is missing expected location properties: ${missingKeys.join(", ")}`, | ||
| ); |
There was a problem hiding this comment.
Shouldn't we move this check to assertErrorsProperty for early feedback?
There was a problem hiding this comment.
Because it requires the test execution to determine the actually required fields.
There was a problem hiding this comment.
This wouldn't be possible as properties that don't exist on the actual error message are not mandatory, so we need to run the rule first.
There was a problem hiding this comment.
It would be possible to check only line and column, as those should always be present on the actual error message, but I think splitting this into two checks would be an unnecessary complication, so it's fine to leave this as already implemented in this PR (check all location properties together here).
| }); | ||
| }); | ||
|
|
||
| describe("messageId", () => { |
There was a problem hiding this comment.
Can we add a test case where we have placeholders in error message when requireMessage : "messageId"?
| hasOwnProperty(error, "message"), | ||
| `errors[${number}] should specify 'message' (and not 'messageId') when 'assertionOptions.requireMessage' is 'message'.`, | ||
| ); | ||
| } else if (requireMessage === "messageId") { |
There was a problem hiding this comment.
ideally if the value is messageId we should ensure that all the placeholder values are also passed.
There was a problem hiding this comment.
In eslint/rfcs#137 this was discussed, but it was excluded from the first iteration.
Search for requireData.
There was a problem hiding this comment.
When data property is passed, RuleTester already checks if all needed placeholder values are passed in it. Requiring data property to be passed is excluded from the first iteration, as @ST-DDT noted.
Co-authored-by: Nitin Kumar <snitin315@gmail.com>
mdjermanovic
left a comment
There was a problem hiding this comment.
LGTM, thanks! Leaving open for a second review.
lumirlumir
left a comment
There was a problem hiding this comment.
LGTM, thanks!
Would like one more check from the team before merging.
|
Merging since there are already two approvals. |
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:
Implements #19921, eslint/rfcs/pull/137
What changes did you make? (Give an overview)
Adds new
assertionOptionstoRuleTester.runto allow for a consistent base level of test assertions.Is there anything you'd like reviewers to focus on?