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: Test with an unknown error property should fail in RuleTester #12096

Merged
merged 2 commits into from Feb 28, 2020
Merged

Breaking: Test with an unknown error property should fail in RuleTester #12096

merged 2 commits into from Feb 28, 2020

Conversation

mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Aug 14, 2019

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

[X] Add something to the core

What changes did you make? (Give an overview)

Additional check in the RuleTester - an invalid test will fail if any of its error objects contains a key that isn't expected.

Since none of the error params is mandatory, test case might be silently useless or missing to check an important detail (e.g. node type or location) just because of a small typo.

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

This might be a breaking change at the moment, but at least it found 90 tests with typos such as messsage, messgeId, messagesId, or using nodeType instead of type.

If accepted, I'll add test cases for this change, of course.

@eslint-deprecated eslint-deprecated bot added the triage label Aug 14, 2019
Copy link
Member

@kaicataldo kaicataldo left a comment

I'm in support of surfacing these kinds of errors, but since the RuleTester is part out of ESLint's public API, I do have concerns with releasing this in a non-semver-major release. This would require some discussion (and most likely a formal proposal using our RFC process), but I wonder if we could add this behavior behind a configuration option passed to the RuleTester constructor (with the idea of making it the default behavior in a future semver-major release)?

@mdjermanovic
Copy link
Member Author

@mdjermanovic mdjermanovic commented Aug 18, 2019

Maybe to read the option from user's package.json?

@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Aug 18, 2019

@mdjermanovic Do you mind expanding on that?

@mdjermanovic mdjermanovic added the breaking label Aug 23, 2019
@mdjermanovic
Copy link
Member Author

@mdjermanovic mdjermanovic commented Aug 23, 2019

Just a thought to avoid modifying all test files, for an option that will be deprecated in the major version.

Perhaps better to implement this in the major version, like how it is planned with eslint/rfcs#25.

mysticatea added a commit to eslint/rfcs that referenced this issue Sep 17, 2019
@mysticatea mysticatea added this to Memorandum in v7.0.0 Sep 17, 2019
@platinumazure platinumazure added core evaluating and removed triage labels Sep 29, 2019
@platinumazure
Copy link
Member

@platinumazure platinumazure commented Sep 29, 2019

For what it's worth, I think this should also be semver-major. That way we don't need to worry about options that will be removed later.

@mysticatea mysticatea moved this from Memorandum to Needs discussion in v7.0.0 Nov 7, 2019
@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Dec 14, 2019

Should this maybe go through the RFC process?

@mysticatea
Copy link
Member

@mysticatea mysticatea commented Dec 14, 2019

Actually, RFC25 contains this.

@nzakas
Copy link
Member

@nzakas nzakas commented Feb 11, 2020

There hasn’t been any activity on this for two months. Are we still planning on doing this?

@mdjermanovic
Copy link
Member Author

@mdjermanovic mdjermanovic commented Feb 12, 2020

There hasn’t been any activity on this for two months. Are we still planning on doing this?

I think this is waiting for RFC25 which is still in process.

This PR also doesn't cover Suggestions, since that feature has been added in the meantime.

@mdjermanovic mdjermanovic added the do not merge label Feb 12, 2020
@nzakas nzakas moved this from Needs discussion to Accepted, ready to implement in v7.0.0 Feb 13, 2020
@mysticatea mysticatea moved this from Accepted, ready to implement to Implemented, pending review in v7.0.0 Feb 13, 2020
@btmills btmills added accepted and removed evaluating labels Feb 13, 2020
@btmills
Copy link
Member

@btmills btmills commented Feb 13, 2020

In today's TSC meeting, we accepted and merged RFC25 as a semver-major change for v7.0.0, so this change is also accepted as it implements part of the RFC!

@mdjermanovic
Copy link
Member Author

@mdjermanovic mdjermanovic commented Feb 14, 2020

A question: do we want to check properties in Suggestion objects, too? They're within error objects.

If so, this may be blocked by #12635 (or vice versa).

@mdjermanovic mdjermanovic changed the title Update: Check for unknown error object keys in RuleTester Breaking: Test with an unknown error property should fail in RuleTester Feb 22, 2020
@mdjermanovic
Copy link
Member Author

@mdjermanovic mdjermanovic commented Feb 22, 2020

Rebased and added "suggestions" property name, so this works now.

Notes about this validation:

  • It doesn't check properties with Symbol keys.
  • It doesn't check non-enumerable properties.
  • It doesn't check suggestion objects.

Questions:

  • Should it check non-enumerable properties in error objects?
  • Should it check property names in suggestion objects?

Additionally, this PR also fixes behavior with null elements (RuleTester used to crash on null). After this change, it will show the "Error should be a string, object, or RegExp..." message. Off-topic: that message prints the linting output, I guess it should show typeof error instead.

@mdjermanovic mdjermanovic removed the do not merge label Feb 22, 2020
@nzakas
Copy link
Member

@nzakas nzakas commented Feb 25, 2020

Are there nonenumerable properties we care about? If not, then I think it’s fine to skip.

I’d like to see the same checks in suggestions — I can’t think of a reason why we’d want validation everywhere but suggestions.

@mdjermanovic
Copy link
Member Author

@mdjermanovic mdjermanovic commented Feb 25, 2020

Are there nonenumerable properties we care about? If not, then I think it’s fine to skip.

Rule tester in general doesn't distinguish between enumerable and non-enumerable properties, it just reads values from properties with expected names. From that point of view, it might make sense to check all properties including non-enumerable ones.

On the other hand, allowing unknown non-enumerable properties can be a way to allow custom properties in error objects (some concerns about this were raised in the RFC), so I think it's okay to skip them in this validation, as it is implemented in this PR.

I’d like to see the same checks in suggestions — I can’t think of a reason why we’d want validation everywhere but suggestions.

There is no real reason, just had to doublecheck because suggestions were not explicitly mentioned in the RFC.

The latest commit adds validation of properties on suggestion objects as well, so this PR should be ready for review now.

nzakas
nzakas approved these changes Feb 26, 2020
Copy link
Member

@nzakas nzakas left a comment

LGTM. Thanks!

@kaicataldo kaicataldo merged commit 4af06fc into eslint:master Feb 28, 2020
11 checks passed
v7.0.0 automation moved this from Implemented, pending review to Done Feb 28, 2020
@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Feb 28, 2020

Thanks for working on this!

montmanu pushed a commit to montmanu/eslint that referenced this issue Mar 4, 2020
…er (eslint#12096)

* Breaking: Test with an unknown error property should fail in RuleTester

* Check suggestion properties
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Aug 28, 2020
@eslint-deprecated eslint-deprecated bot added the archived due to age label Aug 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted archived due to age breaking core
Projects
No open projects
v7.0.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants