feat!: Stricter rule test validation

[bmish]

Summary

We will add additional assertions to the rule test runner to address several potential points of poor test coverage, especially related to rule suggestions.

Assertions added to invalid test case error objects:

• Must contain suggestions if the test case produces suggestions
• Must contain the violation message (message or messageId, but not both)

Assertions added to invalid test case suggestion objects:

• Must contain the suggestion code output
• Must contain the suggestion message (messageId or desc, but not both)
• The suggestion code output must differ from the test case code

Assertions added to all test cases:

• Any autofix output must differ from the test case code
• Cannot have identical test cases
• The optional filename property must be a string
• The optional only property must be a boolean

Demonstrations

• Partial implementation in: https://github.com/bmish/eslint/tree/RFC-84

Related Issues

Fixes eslint/eslint#15104.

[nzakas]

I like the direction. I think all of the suggested changes make sense and will help rule authors.

[nzakas]

I like it!

[bmish]

I added two more very minor breaking changes to this to ensure we can't have tests passing when properties have invalid types (see the RFC body for the full details):

• The optional filename property must be a string
• The optional only property must be a boolean

This was inspired in part by eslint/eslint#13917.

[mdjermanovic]

We had problems with using JSON.stringify() in an attempt to clone options (eslint/eslint#13034), here are relevant issues::

• Config clone before validating kills Object data (7.3.0) eslint#13427
• Config json cloning is a BREAKING CHANGE! eslint#13447

In short, there are rules that accept option values such as regular expressions, functions, etc.

For example, this tests case and this test case for unicorn/filename-case will be seen as duplicates when compared by JSON.stringify(), because the only difference is in regular expressions, but they're both stringified to same "{}".

[bmish]

Good point. We might be able to convert regexps to strings before serializing? Otherwise, for any non-serializable properties like functions, I would suggest we skip these test cases during the duplicate test case check. Open to better ways to compare them, but I'm not too worried about detecting duplicates with these edge cases.

[mdjermanovic]

We might be able to convert regexps to strings before serializing?

That's doable, but then a test case might become a duplicate of a test case that has a string value in the same place. If we decide to skip test cases with other non-serializable values, then I think it's fine to just skip test cases with regexps as well.

I would suggest we skip these test cases during the duplicate test case check.

Sounds good to me. Detecting duplicate test cases is a nice-to-have feature, it's fine if it isn't able to catch all edge cases. Using non-serializable values in configurations is not something we intended to allow, anyway.

An alternative to JSON serialization could be to use a library like fast-deep-equal (we're already using it in RuleTester), but that would be less performant as we'd have to compare each test case with every other.

[bmish]

Great points. Since rule options can contain many kinds of non-serializable properties, and it could be tricky to detect whether non-serializable properties are present, I updated the algorithm to just ignore any test cases with options.

[mdjermanovic]

Question: should we enforce this for invalid test cases that don't have error objects? (and thus also enforce those test cases to specify errors as arrays of error objects if there are suggestions)

Currently allowed "invalid" test case formats without error objects are:

{
    code: "some code",
    // ...
    errors: 2 // asserts only that there are two errors
},
{
    code: "some code",
    // ...
    errors: ["message text", /message regex/] // asserts that there are two errors, and asserts their messages
}

[bmish]

@mdjermanovic thanks for calling this out. I think we should enforce this even for test cases that don't have error objects. Why? To be consistent with test cases that produce autofixes. Test cases that produce autofixes cannot be written with the above shorthands because the autofix is required to be tested.

[mdjermanovic]

Test cases that produce autofixes cannot be written with the above shorthands because the autofix is required to be tested.

output is a top-level property so it can be used in combination with errors: <number> and other shorthands for errors. We have such tests in the ESLint codebase:

https://github.com/eslint/eslint/blob/18f5e05bce10503186989d81ca484abb185a2c9d/tests/lib/rules/comma-dangle.js#L1796-L1824

Those are mostly tests where we run multiple rules to check whether the combined autofix breaks the code, so output is the main concern, and errors would have to specify errors for fixture rules so we left out the details.

[bmish]

Ah, my bad, thank you for the correction. So my recommendation is still that when suggestions are produced, we require them to be tested, meaning the error shorthand can't be used then. Does that sound good?

[mdjermanovic]

I think it makes sense for errors: ["message text", /message regex/], as these are just shorthands for errors: [{ message: "message text" }, { message: /message regex/ }].

But I'm not sure about errors: 2. That looks like an opt-out from testing any details about the errors.

[nzakas]

The errors: 2 was the first option because we didn’t really know the best way to test things at the start. I think this is probably a terrible way to test rules now. :)

[bmish]

The errors: <number> and (new) suggestions: <number> shorthands generally aren't the best way to write tests but I'm fine to allow these both in all situations. Updated.

[nzakas]

@bmish There is some updated feedback here. Can you please take a look?

[bmish]

@fisker I just want to capture your comment here:

eslint/eslint#15104 (comment)

👎 for this.

In some rule, I only add one test for the suggestions, maybe because it's simple and good enough, then focus other tests on the auto-fix or something else, eg "message", "report loaction", why I need assert suggestions in all those?

The tester should not force anything, test one thing in one case is good practice!

I do think this is a fair point. However, you could make the same point about this existing requirement to test autofixes added in ESLint v7:

AssertionError [ERR_ASSERTION]: The rule fixed the code. Please add 'output' property.

My new requirement about testing suggestions is just an extension of the existing requirement about testing autofixes. The question is, should we have both requirements to be consistent and to ensure maximum test coverage, have only one because we consider one more important than the other, or have neither? I lean towards enforcing maximum test coverage, but open to discussing this.

[nzakas]

I think the damage done by forgetting to test a suggestion is greater than the annoyance of needing to copy-paste suggestions into multiple test cases. We want to ensure the highest quality for rules so I’m 👍 for enforcing suggestion testing by default.

I do wonder, though, if maybe there might be a future opportunity to allow devs to select which of the automatic tests and validations they might want to opt out of. I’m not convinced it’s needed now, just something to think about.

[mdjermanovic]

Maybe we could allow suggestions: <number> as an opt-out from testing details of the suggestions. We already allow errors: <number>.

suggestions: 1 would assert that there's 1 suggestion.

The absence of suggestions property would assert that there are no suggestions, as proposed in this RFC.

[nzakas]

I think that’s a fine compromise.

[bmish]

I like the idea of suggestions: <number> as a compromise. And I'm fine to allow both the errors: <number> and suggestions: <number> shorthands in all situations. Updated.

[fisker]

How to make sure message is generated by messageId and data(not calling content.report() with message property)?

How to make sure the template string is replaced correctly?

[mdjermanovic]

You can use messageId and data in test cases: https://eslint.org/docs/developer-guide/nodejs-api#testing-errors-with-messageid

How to make sure message is generated by messageId and data(not calling content.report() with message property)?

If you specify messageId in the test case, it asserts that context.report() was called with a messageId, and checks if that messageId is the same as the messageId specified in the test case.

How to make sure the template string is replaced correctly?

If you also specify data in the test case, RuleTester will get the meta message (by messageId specified in the test case) from rule.meta.messages, and replace placeholders with data from the test case to calculate the expected message. Then, it asserts that the expected message is the same as the reported message (which was calculated from messageId and data passed to context.report()).

Assertions that use messageId and data from the test case are here: https://github.com/eslint/eslint/blob/18f5e05bce10503186989d81ca484abb185a2c9d/lib/rule-tester/rule-tester.js#L814-L843

[fisker]

I know how to do this test for current version. But this line forbid test both messageId and message.I'll have to do this test separately.

[fisker]

Why should we forbid assert {message, messageId, data} together? So ONE test can make sure the message is generated by messageId and data, the template is also replaced successfully.

[nzakas]

This question seems tangential to the RFC. Can we open a separate issue to discuss?

[fisker]

Not really interested. I barely use the builtin tester.

[nzakas]

Okay, then I’ll mark this discussion as resolved.

[bmish]

Sorry to reopen this, but I would like to ask some clarifications.

There's an existing rule tester assertion today Error should not specify both 'message' and a 'messageId'. The only thing I'm changing here is that you have to specify the message in SOME form, as previously it was possibly to omit any mention of the message.

So @fisker, is it just this existing assertion you are unhappy with?

Do let me know if you have a problem with new behavior in the RFC so we can consider your use cases.

[fisker]

I haven't used the built-in tester for a while, the first time I want test both messageId + data and message was we first use suggestion api, I thought data was shared between error and suggestions at first, but found the message template was not replaced correctly.

See sindresorhus/eslint-plugin-unicorn#635

Since then, I always add an extra test to test the actual message/desc https://github.com/sindresorhus/eslint-plugin-unicorn/blob/c501243e7f9fb012b09a69ffc4ab66fb3376bb7d/test/no-array-callback-reference.mjs#L286

I guess I was unhappy with current version? I'm not sure.

[nzakas]

I closed this because it’s not related to the RFC specifically and shouldn’t distract from the larger discussion.

Please move the discussion to an issue if you’d like to continue it.

[mdjermanovic]

There are other possibly non-serializable properties that can appear in test cases, as config properties.

Currently, I think it's settings and parserOptions (depending on the parser) as possibly unserializable. With the flat config, we'll have settings, languageOptions (parser can be a parser object with parse / parseForESlint function) and plugins as possibly unserializable properties.

[bmish]

Good call out, I'm happy to limit this to only test cases which only contain known-serializable properties. Updated.

[btmills]

It looks like all of the open discussions have satisfactory answers. I think this provides a good direction for rule authors that will catch tests that weren't asserting quite what they were supposed to prove while allowing reasonable opt-outs for those who don't want to make extensive changes. Thanks, @bmish!

[nzakas]

@mdjermanovic are you ok with moving to final commenting?

[mdjermanovic]

Looks good to me! Moving to final commenting.