Change Request: Rule Tester: improve testing of multi-pass fixes

[bradzacher]

ESLint version

N/A

What problem do you want to solve?

Currently the rule tester does exactly one fix pass over the code. It's worth noting that this works differently to Linter.verifyAndFix which does up to 10 fix passes on the code.

This difference is confusing for plugin developers because it's possible and in some cases easy to write a test case for a rule where two reports have overlapping fixes.

This causes an issue because the output reported by the rule tester does not match the output you would get if you were to fix the code from the CLI. The confusing part is that this behaviour is opaque from the perspective of a consumer of the tester - leading to many people not knowing that their test case isn't actually what they expect - i.e. developers will often just correct the output to what the test asks for without realising what it means.

Note: It is documented on the rule tester's docs:

Asserts the output that will be produced when using this rule for a single pass of autofixing (e.g. with the --fix command line flag)

However this is kind of vague - eg what does it mean to be a single pass? how does that compare to normal linting from the CLI? etc. Additionally this sentence is fairly buried. The journey to get there is:

1. Custom Rules docs (https://eslint.org/docs/latest/extend/custom-rules)
2. Scroll to the almost bottom to "Rule Unit Tests" (https://eslint.org/docs/latest/extend/custom-rules#rule-unit-tests)
3. Click the link taking you to the Rule Tester API doc (https://eslint.org/docs/latest/integrate/nodejs-api#ruletester)
4. Scroll to the output property.
5. You've found the sentence

TBH I doubt most newbies go past step (1) when setting up a new plugin!

---

As an example for @typescript-eslint we have an extension for space-infix-ops which applies the rule's logic to type ops. One of our tests looks like this:

    {
      code: `
        type Test=|string|(((() => void)))|string;
      `,
      output: `
        type Test = |string | (((() => void))) | string;
      `,
      errors: [
        {
          messageId: 'missingSpace',
          column: 18,
          line: 2,
        },
        {
          messageId: 'missingSpace',
          column: 19,
          line: 2,
        },
        {
          messageId: 'missingSpace',
          column: 26,
          line: 2,
        },
        {
          messageId: 'missingSpace',
          column: 43,
          line: 2,
        },
      ],
    },

To the untrained eye - it's hard to tell that the output isn't actually correct. If you were to run the same code through the CLI it would instead fix to type Test = | string | (((() => void))) | string; - Note the extra space after the first |. (playground)

---

In one particular case within @typescript-eslint, we actually wanted to test the multi-pass fix behaviour so that we could validate that our fixers would "layer" correctly and be applied correctly for some more complex (but common) fix cases. However there's currently no way to do this with the rule tester, and so we had to resort to building our own test on top of Linter:

https://github.com/typescript-eslint/typescript-eslint/blob/01556f50376b87eff21c42d121b627e3a6f003f0/packages/eslint-plugin/tests/rules/array-type.test.ts#L1924-L2159

What do you think is the correct solution?

I can think of a few solutions (not mutually exclusive):

1. Clearly document this limitation with an explanation of what it means and how it compares to CLI linting.
2. Allow rules to opt-in to multi-pass fixing for specific test cases
   • One idea is a simple boolean flag that just turns on multi-pass fixing for that specific case - eg multipassFixing: true.
   • Another might be that a rule can specify an array for output which declares the code after each fix pass - allowing a test to declare (a) how many passes should occur, (b) how the fixes should look, and (c) the end state of the code
3. Error by default if a test case produces code that requires multiple passes to fix.
   • This would serve as a hint to either split the case up or enable the above flag (if added).
   • It would prevent test cases that aren't properly testing their code paths.

Participation

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

Additional comments

Example user issue: https://github.com/typescript-eslint/typescript-eslint/issues/8251

[nzakas]

This is an interesting case. On the one hand, the fact that ESLint does multiple passes is an implementation detail, it's not something people should rely on nor seek to replicate. There is no guarantee that all fixes will be applied in any situation (especially true if it takes more than 11 passes to get all the fixes in), so in a sense, the test is accurately reflecting reality: when there are two fixes on the same line there is a nonzero chance that both fixes won't be applied.

We can't really start doing multiple passes for every test case in RuleTester because that would, at a minimum, double the amount of time it takes for tests to run when most of the time the extra pass wouldn't be needed.

I suppose we could let people opt-in on a specific case, but then again, I go back to my first paragraph here.

So I'm :-1: on adding this functionality into RuleTester, but open to hearing other opinions from @eslint/eslint-team.

[github-actions[bot]]

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.

[fasttime]

If we aren't going to make any changes to RuleTester, I agree it would be good to clarify the fact that the expected output of single test cases does not need to match the fixed code produced by the linter.

[Rec0iL99]

ping @eslint/eslint-team

[nzakas]

Agree with @fasttime. Marking as accepted for a docs update.

[bradzacher]

Could we do a breaking change here to "alert" on a test if it has unapplied fixes after the first pass?

That would help users catch and understand tests that should be split up, or at least make them acknowledge that the test intentionally has dangling fixes.

Perhaps an extension to the current API could allow an array form of output that allows users to say "please run the fixer exhaustively as I want to test this case with overlapping fixes".

[nzakas]

At this point, the most I'm willing to do is up the docs. If people are confused about the behavior of RuleTester, their next stop should be the docs.

To repeat, multi-pass fixing is an implementation detail and not something that anyone should rely on or be able to turn off and on.