Skip to content
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

Update: RuleTester Improvements #25

Merged
merged 5 commits into from Feb 13, 2020
Merged
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
83 changes: 83 additions & 0 deletions designs/2019-rule-tester-improvements/design.md
@@ -0,0 +1,83 @@
- Start Date: 2019-06-07
- RFC PR: https://github.com/eslint/rfcs/pull/25
- 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)).<br>
mysticatea marked this conversation as resolved.
Show resolved Hide resolved
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.<br>
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)).<br>
`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).
mysticatea marked this conversation as resolved.
Show resolved Hide resolved

### 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.
mysticatea marked this conversation as resolved.
Show resolved Hide resolved

#### 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