Change Request: Rule Tester: Enforce that a rule marked fixable or hasSuggestions has a test case that produces a relevant fixer

[bradzacher]

ESLint version

N/A

What problem do you want to solve?

Currently it's possible to declare a rule as both fixable and hasSuggestions without ever reporting either a fixer or a suggestion.

This is not great because often these flags are used for documentation purposes - which leads to bad documentation for users.

Additionally it's quite doable to create a suite of tests that never produces a fixer or a suggestion - this means that you can accidentally leave untested pathways in your rule. In the case of autofixers this is especially bad because you may not have validated that your code produces syntactically valid code!

What do you think is the correct solution?

It would be great if ESLint could do some post-run validation for a rule - for example:

• "if the rule is marked as fixable and no tests produced a fixer - error"
• "if the rule is marked as hasSuggestions and no tests produced suggestions - error"

One might suggest that this could be done via lint rules (eg eslint-plugin-eslint-plugin) - however it can be quite hard to statically analyse this given rules need not be contained within one file (so it's not possible to enforce the existence of a fixer on at least one context.report() call). Similarly tests may be assembled via generation in some way - meaning tests are dynamic and not possible to analyse. You might be able to catch some simpler cases with a lint rule.

Participation

• I am willing to submit a pull request for this change.

Additional comments

No response

[bmish]

I think it's a neat idea. While ESLint hasn't been prescriptive about requiring specific rule test cases thus far, the testing of fixability/suggestions is a good example of important test coverage that could be at least minimally enforced, while at the same time helping to ensure that the rule's metadata for those aspects is accurate.

In terms of the lint rules, both eslint-plugin/require-meta-fixable (through the catchNoFixerButFixableProperty option) and eslint-plugin/require-meta-has-suggestions have the ability to detect basic cases of incorrect fixable/hasSuggestions metadata (such as when these properties are enabled but when no fixer/suggestions are present), but as you mentioned, these features are somewhat limited by false positives since the lint rules can't always find/follow complex violation reporting code.

[nzakas]

I think this is a gray area between linting and testing -- this feels more lint-worthy to me than test-worthy, as in, it feels like this type of problem is more akin to a lint violation than a unit test failure.

I don't feel strongly either way, so would like to hear more opinions.

@eslint/eslint-team

[bmish]

Just to explore the design of such a feature some more, I see a few options:

1. A test failure, thus making this a breaking change.
2. A helpful warning in the test output.
3. Or, if we expected there to be a number of checks like this, it could get to the point where a user might want to configure them, and any checks considered too strict for most users could be off by default. I would be in favor of this option if we could conceive of a variety of possible checks for things that linting isn't great at catching. There could also be an optional strict option on RuleTester which enables all the additional checks.

[fasttime]

Currently, RuleTester doesn't complain if a rule has no invalid test cases. If this change is implemented straight as proposed, we will be ensuring that fixable rules actually provide a fix and rules with suggestions actually provide a suggestion, but we still won't care if a rule (non-fixable, without suggestions) reports anything or not.

[bradzacher]

but we still won't care if a rule (non-fixable, without suggestions) reports anything or not.

This isn't terrible - because there are helper rules like react/jsx-uses-react that don't report and just do work for a different rule.

The usecases for that sort of thing are pretty niche mind you. I think that it is literally limited to unused vars.

[fasttime]

The usecases for that sort of thing are pretty niche mind you. I think that it is literally limited to unused vars.

Yeah, there are a few use cases for rules that don't report problems, like when you just want the results of code path analysis. I wish there was a flag like fixable or hasSuggestions to mark rules that aren't expected to report problems.

[bradzacher]

I wish there was a flag like fixable or hasSuggestions to mark rules that aren't expected to report problems.

It could probably be a flag within the rule tester if we were to enforce at least one invalid case.

But also no reason there couldn't be a rarely used flag to do that. Hey might be worth a separate proposal?

[nzakas]

As a note: we really shouldn't be adding any further breaking changes to v9 at this point unless absolutely necessary. For this, I'd say any breaking change really needs to come in v10.

If folks want to explore the idea of rule tester options, we can definitely do that in a non-breaking way, just keep in mind that the RuleTester constructor has an argument that is the default config to use, so there isn't a natural place to add such options in the constructor.

[github-actions]

Oops! It looks like we lost track of this issue. What do we want to do here? This issue will auto-close in 7 days without an update.

[bmish]

Not stale, would be interested to see more exploration here.

[nzakas]

We need someone to drive this issue. As mentioned, the window for adding further breaking changes to v9 has closed, so that would need to be kept in mind.

[DMartens]

I would love to drive this issue as I have a custom wrapper for the rule tester for this and other cases.
Some of them are subjective but I think in general most checks would benefit the community.
Here is a short summary for the checks to show that there are many cases for linting for the tests:
Rule:

• minimum amount of valid / invalid test cases
• each message specified in meta.message is referenced as messageId in an error or suggestion
• messageIds for errors cannot be used for errors and vice versa
• defaultOptions: specify meta.defaultOptions iff the schema is non-empty
• check for unknown / non-matching meta properties

Rule options schema:

• disallow advanced JSON schema properties (e.g. $ref or not)
• require additionalProperties: false for objects
• base schema tests (allow passing no options, an empty object, ...)

Test cases:

• valid test cases cannot have a errors or output property (common copy & paste issue)
• shorthands such as using a string for valid test cases or a number for an error amount are not allowed

Error matcher:

• always use messageId and not message
• make sure the types of the properties match, as only truthy properties are checked (e.g. type: null is prohibited)
• message data: disallow specifying additional properties and not specifiying data if the message has no placeholders
• requiring test case (e.g. name / error (e.g. type) / suggestion (e.g. messageId) properties to be present
• "focused tests": test cases can only check for one messageId and have at most two errors

To make this not a breaking change I would propose to add the property ruleTester to the passed options for the constructor. This would allow people to opt-in to the rules in v9 and would allow developers to disable rules (such as the mentioned minimum amount of invalid test cases).

[bmish]

What does a "custom wrapper" entail?

It looks like there are some good ideas in that list. It may be worth creating a table indicating some more data-points for each:

• Is it linted by eslint-plugin-eslint-plugin already? If not, could it be linted, or is it not possible to lint?
• Is it subjective or suitable for everyone?

Given the number of possible checks and the subjectivity of some of them, I think it's important to identify which could be rolled out to everyone vs. which ones may need to be opt-in / configurable.

[DMartens]

I only mentioned the "custom wrapper" part to say those ideas are all easily implementable (e.g. via a function receiving the rule and the test cases which runs the test cases via the rule tester and then the custom lint rules).
The problem for most cases is that the cases are not easily statically lintable as:

• some rule tester test cases are not static objects (e.g. automatically created via Array#map, shared via ...spread or an helper which sets common test case / error properties)
• the eslint rule would need to have access to the rule meta and the test cases but they are in different files

For example eslint-plugin-eslint-plugin has a rule for using the defined meta.messages but bails if it cannot statically find reports. But when using the raw data from the rule tester, the data is easily available (the rule meta as well as the resulting messages).

Ultimately all the checks are subjective as none of them prevents an error at runtime and we do not know whether the rule or its tests are seen as "complete" (e.g. why does the rule tester throw an error if I just start writing tests).
As such I would not enable such rule by default and have all rules disabled by default (as is the case for eslint rules, you need to manually opt-in).

There are further questions we need to resolve:

• when to run the lint cases: only when all tests are passing? only after a test case / suite?
• how to the run the lint cases: create a test case per rule, create one test case for all rules?

[aladdin-add]

While the intent is good, it seems like it should be less of a tester and more of a test coverage check. a few concerns:

1. Having at least one doesn't mean it's completely covered by the tests - esp. for the rules having multi-fixers. :)

2. Not very common, but it's theoretically possible to execute tester.run() more than once, e.g.

ruleTester.run("my-rule", rule, {invalid: somecases});

ruleTester.run("my-rule", rule, {invalid: some-other-cases});

How should tester work if it's tested in somecases but not in the other? 🤔

[bradzacher]

Having at least one doesn't mean it's completely covered by the tests ¡

Honestly I think such a simple error is more than enough. The number of times I've seen someone add a fixer to an existing rule and forget to write tests for the change.
Or someone deletes some tests and didn't realise they deleted the last fixer test.

An error to remind people would be massive to help improve rule quality.

This is coming from my anecdotal experience working on internal eslint rules at two different companies. Both times I saw this exact problem that people would completely forget that test path. After a nudge at review time they then wrote very comprehensive tests.

People want to to good but they forget very easily given how much there is to think about.

it seems like it should be less of a tester and more of a test coverage check

That involves setting up code coverage and enforcing/reporting it from CI. A lot of projects don't have that setup. For example at work we don't have any code coverage metrics at all as it's seen as noise and low value.

Turning on code coverage for the internal lint rules is simply not an option.

Not very common, but it's theoretically possible to execute tester.run() more than once

Quick solution: add an option to rule tester like allowNoFixerTests or something to allow someone to opt out of the behaviour.

[DMartens]

it seems like it should be less of a tester and more of a test coverage check

I think the main motivation is to make sure the rule metadata is correct as other third-party tools rely on this (e.g. for documentation generation or only running fixable rules)

Not very common, but it's theoretically possible to execute tester.run() more than once

A possible alternative is to allow multiple test suites in the same call (this is something I wanted to properly propose in the future):

ruleTester.run('rule-id', rule, {
	tests: [{
		title: 'Edge Cases',
		valid: [],
		invalid: [],
	}, {
		title: 'Fixer',
		valid: [],
		invalid: [],
	}],
});