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

[mdjermanovic]

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.

[kaicataldo]

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]

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

[kaicataldo]

@mdjermanovic Do you mind expanding on that?

[mdjermanovic]

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.

[platinumazure]

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.

[kaicataldo]

Should this maybe go through the RFC process?

[mysticatea]

Actually, RFC25 contains this.

[nzakas]

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

[mdjermanovic]

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.

[btmills]

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]

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]

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.

[nzakas]

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]

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]

LGTM. Thanks!

[kaicataldo]

Thanks for working on this!