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

Breaking: RuleTester Improvements (refs eslint/rfcs#25) #12955

Merged
merged 3 commits into from Mar 28, 2020
Merged

Conversation

mdjermanovic
Copy link
Member

Prerequisites checklist

  • I have read the contributing guidelines.
  • The team has reached consensus on the changes proposed in this pull request. If not, I understand that the evaluation process will begin with this pull request and won't be merged until the team has reached consensus.

What is the purpose of this pull request? (put an "X" next to an item)

[X] Add something to the core

refs eslint/rfcs#25

Additional validations in the RuleTester.

What changes did you make? (Give an overview)

This PR implements 1.-3. from eslint/rfcs#25

  1. AST nodes, tokens, and comments will throw on access to their .start and .end properties. Test that happens to trigger such code is supposed to fail.
  2. Test that doesn't have output property will fail if the rule has fixed the code.
  3. Test with an input that produces parsing error will always fail, regardless of any test options. It was already working like that, except for tests where errors is a number.

The remaining improvement from the rfc (4. fail on unknown error properties) is in PR #12096.

Is there anything you'd like reviewers to focus on?

  • The rfc mentions only node.start and node.end. This PR also includes token.start and token.end (and the same for comment tokens), is it okay?
  • Should the message for comments be "Use comment.range[0] instead of comment.start" (as it is implemented now), or "Use token.range[0] instead of token.start".
  • Two test cases from tests/lib/rules/spaced-comment.js are removed, since it's no longer possible to test parsing errors.
  • This PR deletes tests/lib/rules/_set-default-parser.js file. If I understand this correctly, that file is no longer needed.

@mdjermanovic mdjermanovic added core Relates to ESLint's core APIs and features accepted There is consensus among the team that this change meets the criteria for inclusion breaking This change is backwards-incompatible labels Feb 23, 2020
lib/rule-tester/rule-tester.js Outdated Show resolved Hide resolved
lib/rule-tester/rule-tester.js Outdated Show resolved Hide resolved
@mdjermanovic mdjermanovic added this to Implemented, pending review in v7.0.0 Mar 13, 2020
lib/rule-tester/rule-tester.js Outdated Show resolved Hide resolved
function defineStartEndAsErrorInTree(ast, visitorKeys) {
Traverser.traverse(ast, { visitorKeys, enter: defineStartEndAsError.bind(null, "node") });
ast.tokens.forEach(defineStartEndAsError.bind(null, "token"));
ast.comments.forEach(defineStartEndAsError.bind(null, "comment"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally think this should be token instead of comment for the sake of consistency.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, done!

@kaicataldo kaicataldo moved this from Implemented, pending review to Issues which have PR in v7.0.0 Mar 28, 2020
@kaicataldo
Copy link
Member

Thank you!

@kaicataldo kaicataldo merged commit 78c8cda into master Mar 28, 2020
v7.0.0 automation moved this from Issues which have PR to Done Mar 28, 2020
@kaicataldo kaicataldo deleted the rfc25-1to3 branch March 28, 2020 01:13
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Sep 25, 2020
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Sep 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion breaking This change is backwards-incompatible core Relates to ESLint's core APIs and features
Projects
No open projects
v7.0.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants