From 6ec2838523ee5d141dafaf5bd2e1790b3b4a4e68 Mon Sep 17 00:00:00 2001 From: Toru Nagashima Date: Fri, 7 Jun 2019 10:59:46 +0900 Subject: [PATCH 1/5] Update: RuleTester Improvements --- .../2019-rule-tester-improvements/design.md | 83 +++++++++++++++++++ 1 file changed, 83 insertions(+) create mode 100644 designs/2019-rule-tester-improvements/design.md diff --git a/designs/2019-rule-tester-improvements/design.md b/designs/2019-rule-tester-improvements/design.md new file mode 100644 index 00000000..7af43cb6 --- /dev/null +++ b/designs/2019-rule-tester-improvements/design.md @@ -0,0 +1,83 @@ +- Start Date: 2019-06-07 +- RFC PR: (leave this empty, to be filled in later) +- Authors: Toru Nagashima ([@mysticatea](https://github.com/mysticatea)) + +# `RuleTester` Improvements + +## Summary + +This RFC improves `RuleTester` to check more mistakes. + +## Motivation + +`RuleTester` overlooks some mistakes. + +- Using non-standard properties of AST ([typescript-eslint/typescript-eslint#405](https://github.com/typescript-eslint/typescript-eslint/issues/405)).
+ Especially, `node.start` and `node.end` exist in AST `espree` made, but it's not standardized and some custom parsers don't make those properties. But `node.loc` has `start`/`end` properties, so it's hard to detect `node.start`/`node.end` with static analysis. Therefore, `RuleTester` should detect those. +- Untested autofix.
+ If people forgot to write `output` property in test cases, `RuleTester` doesn't test autofix silently. +- `errors` property with a number (found in [eslint/eslint#11798](https://github.com/eslint/eslint/pull/11798)).
+ `errors` property with a number ignores syntax errors in test code. We overlooked the mistake of [tests/lib/rules/complexity.js#L84](https://github.com/eslint/eslint/blob/cb1922bdc07e58de0e55c13fd992dd8faf3292a4/tests/lib/rules/complexity.js#L84) due to this. The number `errors` property cannot check the reported error was the expected error. + +## Detailed Design + +1. Disallowing `node.start` and `node.end` +1. Ensuring to test autofix +1. Deprecating the `errors` property with a number + +### 1. Disallowing `node.start` and `node.end` + +`RuleTester` fails test cases if a rule implementation used `node.start` or `node.end` in the test case. + +#### Implementation + +- In `RuleTester`, it registers an internal custom parser that wraps `espree` or the parser of `item.parser` to `Linter` object. +- The internal custom parser fixes the AST that the original parser returned, as like [test-parser.js](https://github.com/eslint/eslint/blob/21f3131aa1636afa8e5c01053e0e870f968425b1/tools/internal-testers/test-parser.js). + +### 2. Ensuring to test autofix + +`RuleTester` fails test cases if a rule implementation fixed code but `output` property was not defined in the test case. + +#### Implementation + +- If `output` property didn't exist but the rule fixed the code, `RuleTester` fails the test case as "The rule fixed the code. Please add 'output' property." It's implemented around [lib/rule-tester/rule-tester.js#L594](https://github.com/eslint/eslint/blob/21f3131aa1636afa8e5c01053e0e870f968425b1/lib/rule-tester/rule-tester.js#L594). + +### 3. Deprecating the `errors` property with a number + +ESLint prints a deprecation warning if `errors` property was used with a number. + +#### Implementation + +- Adds a deprecation warning to [lib/rule-tester/rule-tester.js#L495](https://github.com/eslint/eslint/blob/21f3131aa1636afa8e5c01053e0e870f968425b1/lib/rule-tester/rule-tester.js#L495). It uses [`emitDeprecationWarning()`](https://github.com/eslint/eslint/blob/21f3131aa1636afa8e5c01053e0e870f968425b1/lib/shared/config-validator.js#L265) function for that. + +## Documentation + +[RuleTester](https://eslint.org/docs/developer-guide/nodejs-api#ruletester) should be updated. + +- `errors` ("number or array" → "array") +- `output` ("optional" → "required if the rule fixes code") + +## Drawbacks + +This change may enforce plugin owners to fix their tests. + +## Backwards Compatibility Analysis + +This is a breaking change that can break existing tests. + +But the breaking cases may indicate that the rule was not tested enough. + +## Alternatives + +- About "Disallowing `node.start` and `node.end`", we can standardize those properties. But it's a breaking change for custom parser owners. On the other hand, using `node.start` and `node.end` breaks the rule if users used custom parsers, so the impact of this disallowing is limited. + +## Open Questions + +## Frequently Asked Questions + +## Related Discussions + +- https://github.com/eslint/eslint/issues/8956 +- https://github.com/eslint/eslint/pull/8984 +- https://github.com/eslint/eslint/pull/11798 +- https://github.com/typescript-eslint/typescript-eslint/issues/405 From 8b0691a62aca20bc6e51e6c81618cd972fa95e71 Mon Sep 17 00:00:00 2001 From: Toru Nagashima Date: Fri, 7 Jun 2019 11:04:17 +0900 Subject: [PATCH 2/5] add PR link --- designs/2019-rule-tester-improvements/design.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/designs/2019-rule-tester-improvements/design.md b/designs/2019-rule-tester-improvements/design.md index 7af43cb6..2d2eab6d 100644 --- a/designs/2019-rule-tester-improvements/design.md +++ b/designs/2019-rule-tester-improvements/design.md @@ -1,5 +1,5 @@ - Start Date: 2019-06-07 -- RFC PR: (leave this empty, to be filled in later) +- RFC PR: https://github.com/eslint/rfcs/pull/25 - Authors: Toru Nagashima ([@mysticatea](https://github.com/mysticatea)) # `RuleTester` Improvements From f3a78ab7313255e961e0c315d1022fdc6ff43d68 Mon Sep 17 00:00:00 2001 From: Toru Nagashima Date: Thu, 25 Jul 2019 12:43:45 +0900 Subject: [PATCH 3/5] rename file --- designs/2019-rule-tester-improvements/{design.md => README.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename designs/2019-rule-tester-improvements/{design.md => README.md} (100%) diff --git a/designs/2019-rule-tester-improvements/design.md b/designs/2019-rule-tester-improvements/README.md similarity index 100% rename from designs/2019-rule-tester-improvements/design.md rename to designs/2019-rule-tester-improvements/README.md From 6569b476b27489f85f19da3197f9c941c449c9e5 Mon Sep 17 00:00:00 2001 From: Toru Nagashima Date: Thu, 25 Jul 2019 13:23:52 +0900 Subject: [PATCH 4/5] change `errors` strategy --- designs/2019-rule-tester-improvements/README.md | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/designs/2019-rule-tester-improvements/README.md b/designs/2019-rule-tester-improvements/README.md index 2d2eab6d..f3b7b725 100644 --- a/designs/2019-rule-tester-improvements/README.md +++ b/designs/2019-rule-tester-improvements/README.md @@ -23,7 +23,7 @@ This RFC improves `RuleTester` to check more mistakes. 1. Disallowing `node.start` and `node.end` 1. Ensuring to test autofix -1. Deprecating the `errors` property with a number +1. Changing the `errors` property of a number to fail on syntax errors ### 1. Disallowing `node.start` and `node.end` @@ -42,19 +42,18 @@ This RFC improves `RuleTester` to check more mistakes. - If `output` property didn't exist but the rule fixed the code, `RuleTester` fails the test case as "The rule fixed the code. Please add 'output' property." It's implemented around [lib/rule-tester/rule-tester.js#L594](https://github.com/eslint/eslint/blob/21f3131aa1636afa8e5c01053e0e870f968425b1/lib/rule-tester/rule-tester.js#L594). -### 3. Deprecating the `errors` property with a number +### 3. Changing the `errors` property of a number to fail on syntax errors -ESLint prints a deprecation warning if `errors` property was used with a number. +`RuleTester` fails test cases always if the `code` has a syntax error. #### Implementation -- Adds a deprecation warning to [lib/rule-tester/rule-tester.js#L495](https://github.com/eslint/eslint/blob/21f3131aa1636afa8e5c01053e0e870f968425b1/lib/rule-tester/rule-tester.js#L495). It uses [`emitDeprecationWarning()`](https://github.com/eslint/eslint/blob/21f3131aa1636afa8e5c01053e0e870f968425b1/lib/shared/config-validator.js#L265) function for that. +- Unwrap [lib/rule-tester/rule-tester.js#L414-L419](https://github.com/eslint/eslint/blob/02d7542cfd0c2e95c2222b1e9e38228f4c19df19/lib/rule-tester/rule-tester.js#L414-L419). ## Documentation [RuleTester](https://eslint.org/docs/developer-guide/nodejs-api#ruletester) should be updated. -- `errors` ("number or array" → "array") - `output` ("optional" → "required if the rule fixes code") ## Drawbacks @@ -71,10 +70,6 @@ But the breaking cases may indicate that the rule was not tested enough. - About "Disallowing `node.start` and `node.end`", we can standardize those properties. But it's a breaking change for custom parser owners. On the other hand, using `node.start` and `node.end` breaks the rule if users used custom parsers, so the impact of this disallowing is limited. -## Open Questions - -## Frequently Asked Questions - ## Related Discussions - https://github.com/eslint/eslint/issues/8956 From ea035245f370b6bd2d84dd0d62a2820b01642470 Mon Sep 17 00:00:00 2001 From: Toru Nagashima Date: Wed, 18 Sep 2019 07:29:16 +0900 Subject: [PATCH 5/5] add about eslint/eslint#12096 --- designs/2019-rule-tester-improvements/README.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/designs/2019-rule-tester-improvements/README.md b/designs/2019-rule-tester-improvements/README.md index f3b7b725..0522b08d 100644 --- a/designs/2019-rule-tester-improvements/README.md +++ b/designs/2019-rule-tester-improvements/README.md @@ -18,12 +18,15 @@ This RFC improves `RuleTester` to check more mistakes. If people forgot to write `output` property in test cases, `RuleTester` doesn't test autofix silently. - `errors` property with a number (found in [eslint/eslint#11798](https://github.com/eslint/eslint/pull/11798)).
`errors` property with a number ignores syntax errors in test code. We overlooked the mistake of [tests/lib/rules/complexity.js#L84](https://github.com/eslint/eslint/blob/cb1922bdc07e58de0e55c13fd992dd8faf3292a4/tests/lib/rules/complexity.js#L84) due to this. The number `errors` property cannot check the reported error was the expected error. +- Typo property names in `errors` property with objects.
+ [eslint/eslint#12096](https://github.com/eslint/eslint/pull/12096). ## Detailed Design 1. Disallowing `node.start` and `node.end` 1. Ensuring to test autofix 1. Changing the `errors` property of a number to fail on syntax errors +1. Changing the `errors` property of objects to fail on unknown properties ### 1. Disallowing `node.start` and `node.end` @@ -50,6 +53,14 @@ This RFC improves `RuleTester` to check more mistakes. - Unwrap [lib/rule-tester/rule-tester.js#L414-L419](https://github.com/eslint/eslint/blob/02d7542cfd0c2e95c2222b1e9e38228f4c19df19/lib/rule-tester/rule-tester.js#L414-L419). +### 4. Changing the `errors` property of objects to fail on unknown properties + +`RuleTester` fails test cases if any item of `errors` has unknown properties. + +#### Implementation + +- [eslint/eslint#12096](https://github.com/eslint/eslint/pull/12096) + ## Documentation [RuleTester](https://eslint.org/docs/developer-guide/nodejs-api#ruletester) should be updated. @@ -75,4 +86,5 @@ But the breaking cases may indicate that the rule was not tested enough. - https://github.com/eslint/eslint/issues/8956 - https://github.com/eslint/eslint/pull/8984 - https://github.com/eslint/eslint/pull/11798 +- https://github.com/eslint/eslint/pull/12096 - https://github.com/typescript-eslint/typescript-eslint/issues/405