feat: add assertion options to RuleTester

[ST-DDT]

Summary

Add options that control which assertions are required for each RuleTester test case.

Related Issues

The idea was initially sparked by this comment: vuejs/eslint-plugin-vue#2773 (comment)

• feat(no-deprecated-slot-attribute): regex ignore patterns vuejs/eslint-plugin-vue#2773 (comment)

It might be helpful to include more detailed error information, such as line, column, endLine, and endColumn...
[...]
Let’s make the new cases more detailed first. 😊

The first steps have been taken in: eslint/eslint#19904

• feat: output full actual location in rule tester if different eslint#19904

This lead to the issue that this RFC is based on: eslint/eslint#19921

• Change Request: Add options to rule-tester requiring certain assertions eslint#19921

[michaelfaith]

I think it's a great idea to drive this kind of consistency, but I do wonder if this is the best place to set that kind of expectation. It seems like something that might be more appropriate as an eslint rule in eslint-plugin-eslint-plugin, which has rules governing conventions of authoring eslint plugins: https://github.com/eslint-community/eslint-plugin-eslint-plugin Something like require-test-assertions that's configurable with the specific assertions the project wants to enforce.

[nzakas]

Thanks for putting this together. I left some comments inline.

[ST-DDT]

I think I covered all comments.

[ST-DDT]

All addressed

[ST-DDT]

All addressed

[ST-DDT]

All pending comments addressed.

[ST-DDT]

All pending comments addressed.

[mdjermanovic]

LGTM, thanks!

[fasttime]

I left a suggestion, but otherwise LGTM.

[DMartens]

I think there should be an extra assertion to disallow using a number for errors (maybe named disallowErrorShorthand).
As a result the boolean variant would be useless as the rule tester checks this by default for error objects.
Further this allows an easier migration, as the plugin maintainers can gradually enable the rules by either:

• First disallowing the error shorthand and then requiring the properties
• First adding the required location or message properties and then handling the error shorthands

[ST-DDT]

Just for clarification, so instead of

errors: 5

You have to use

A) errors: ['foo',...]
or B) errors: [{...}]

[DMartens]

This is dependent on the other assertions.

• none: allows both
• requireMessage: "message" would allow both as the message provided
• requireMessage: "messageId" would only allow B) as the message is provided
• requireLocation: true would only allow B) as a location is required

[ST-DDT]

IMO this is entirely/mostly covered by adding requireMessage.
What do the others think?

[fasttime]

@DMartens Are you suggesting that requireMessage and requireLocation alone should not disallow numeric values for errors, and instead there should be a separate option to do so?

[ST-DDT]

So if disallowShorthand is false/undefined, you can just use the error count even if requireMessage is enabled?

I dont like that. Because then it behaves like requireMessage/location isnt set in the first place.

[DMartens]

We can handle the option as true by default (when not specified) for the other options but I think the possibility to differentiate should be there.

[fasttime]

The requirement for errors to include either a message or messageId was discussed in #84. While this makes sense in many scenarios, it can be bypassed by specifying errors as a number (accepting all the implied limitations of doing so). That discussion also affirmed that using a numeric value for errors remains a valid option.

Given that ESLint has supported this behavior by default, I tend to agree with @DMartens that it makes sense to maintain the possibility of specifying a number for errors, even if requireMessage will enforce one of message or messageId when errors is an array. I'm open to revisiting the scope of this RFC if the team can reach an agreement.

[nzakas]

I'm not sure I agree that we should make a change here. To me, if someone wants to use errors: number, then they wouldn't opt-in to these assertions in the first place.

I think requireMessage: true explicitly affirms that someone does not want to allow errors: number.

[DMartens]

I think this is solved with the mentioned additional assertion disallowErrorShorthand, which defaults to true.
This keeps the current proposed behavior but allows users to opt-out.
This behaviour could also be implemented with requireMessage: false but I think disallowErrorShorthand should be preferred as requireLoc (and maybe requireData) is also affected.

[DMartens]

Another helpful check would be requireData which requires the data error property when a messageId is used and the referenced message has placeholders ({{ placeholder }}).

[ST-DDT]

Would this imply requireMessage: 'messageId'?

[ST-DDT]

I also considered requiring the data block (if needed) into requireMessage: 'messageId'.
What do you+the others think?

[DMartens]

This would not imply requireMessage: 'messageId' as the message already has the placeholders filled in. So there is no need for data.

[ST-DDT]

I'm not sure whether I understand the last comment.

IMO requireData results in ({data: *})[]

AFAICT The current code only checks for data in the messageId block. Which makes sense to me, as data is used to fill in the placeholders in its message.

Most messageId checks are very broad as most rules only have one or two messages. So also checking for data is kind of relevant. Which is why I would implicitly attach it to the requireMessageId option.

[DMartens]

This option (requireData) should only be active when the error has a messageId as data is useless when message is specified (you already provide the message with every placeholder filled in).
The validator only has to check whether data is specified as there is already an error when data is specified and placeholders are missing.
Coupling this with the requireMessage: "messageId" would be unintuitive as that reads as only the "messageId" is required.

[fasttime]

I agree a requireData assertion could be helpful. As this would be a separate option, it could be also discussed in a follow-up RFC.

[nzakas]

Agreed, the scope of this RFC is already well-defined and I don't think we should be expanding at this point in time. @DMartens feel free to open an issue to start the discussion around a requireData assertion option.

[nzakas]

I'm happy with the current state of this RFC.

[mdjermanovic]

@fasttime can you check if your review comments have been addressed?

[fasttime]

I don't have any concerns with the proposed solution. @DMartens, I understand you see room for improvement and would prefer some changes. Do you think the RFC is acceptable in its current form nonetheless?

[DMartens]

Yes, I see both of the suggestions to be non-blocking:

• disallowErrorShorthand: This suggestion was preventative of what I thought users may want and users can then raise the issue
• requireData: I will raise an issue for this after this has landed

[mdjermanovic]

Moving to final commenting.

[fasttime]

No further comments here, so merging.

[ST-DDT]

Should I create a PR for the implementation now?

[fasttime]

Should I create a PR for the implementation now?

@ST-DDT yes, please! Issue eslint/eslint#19921 is already assigned to you.