Rule tester output not applying all fixes

[armano2]

Tell us about your environment

• ESLint Version: 5.10.x
• Node Version: 6-11

While working with rules RuleTester is not applying all fixes, but CLI does.

In RuleTester we have something like this, it means that we are accessing SourceCodeFixer dirrectly instead of having similar logic to verifyAndFix from Linter

const fixResult = SourceCodeFixer.applyFixes(item.code, messages);
assert.strictEqual(fixResult.output, item.output, "Output is incorrect.");

Fixed output of tests is "processed" once instead of 'less than 10 if there are still fixes to apply'

---

• Linter:verifyAndFix - https://github.com/eslint/eslint/blob/master/lib/linter.js#L1010
• RuleTester:testInvalidTemplate - https://github.com/eslint/eslint/blob/master/lib/testers/rule-tester.js#L567

[platinumazure]

Hi @armano2, thanks for the issue.

This is by design. We want rule tests to focus on just what changes they are making, rather than worrying about how other rules are configured or what happens after multiple passes.

Are you experiencing a particular problem as a result of this approach? If so, maybe we could consider an enhancement here.

[armano2]

I'm working on new rule for typescript in eslint-plugin-typescript and i'd like to check if there is no collisions while converting nested arrays to []

let a: ({ foo: Array<Array<Bar> | Array<any>> })[] = []

should return

let a: ({ foo: (Bar[] | any[])[] })[] = []

i do understand why it's not working (ranges......) and needs multiple executions

---

for now i'm just going to write it with snapshots and API execution... but it will be really nice if this will work out of box :>

[eslint-deprecated[bot]]

Unfortunately, it looks like there wasn't enough interest from the team or community to implement this change. While we wish we'd be able to accommodate everyone's requests, we do need to prioritize. We've found that issues failing to reach accepted status after 21 days tend to never be accepted, and as such, we close those issues. This doesn't mean the idea isn't interesting or useful, just that it's not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

[futpib]

I'm hitting this too, in https://github.com/sindresorhus/eslint-plugin-unicorn/blob/ec879b78083d8ed81f0132bb3d557760a81e8c4f/test/prevent-abbreviations.js#L763-L800

An option to enable complete application of fixes would've been nice

[g-plane]

I think we can keep fixing one report by default, but we can provide an option (maybe called applyAllFixes) to allow plugin developer to apply all fixes, which won't bring breaking change.

[platinumazure]

Just to make sure I'm understanding: RuleTester only runs one rule at a time, and also does one "pass" of linting and fixing at a time. The request here is to support multiple passes with a new option, but still only one rule at a time. Is that correct?

If so, I'm not sure I understand the value. If the same rule has to run multiple times to get the output, why not just get to that output in the first pass? (To me, this is distinct from the case where you need to get to a specific output using multiple rules, such as combining a syntax-altering rule with one that figures out the whitespace style in the new syntax.)

I'm not specifically opposed to this feature, but I'm not understanding what value it would necessarily add for single rule runs.

[futpib]

In my case the rule produces all the needed fixes in one pass, but the RuleTester seems to apply only the first one. Is this a separate issue?

[g-plane]

I agree with @futpib . For the RuleTester, the behavior should be either reporting only one problem then apply one fix, or reporting all fixable problems then allow to fix them all one time.

[platinumazure]

@futpib Weird- I would expect that RuleTester allows you to output and validate multiple errors, and that all of them would be fixed and the new source code compared against the output value. That does sound like a potential bug.

The only thing I can think of is if your fixes collide in terms of range. Then only some might be applied. I know that we've also had issues in the past where ranges that are consecutive but not overlapping would not be fixed. Could this be a possibility in your case?

[futpib]

I tried boiling it down to a small example https://github.com/futpib/eslint-rule-tester-issue-11187

Here is the log with the source code, the two fixes the rule returned, the actual and expected output:

getSource: 
                                let a, b;
                                console.log(a, b);

fixes: [ { range: [ 9, 10 ], text: 'alpha' },
  { range: [ 31, 32 ], text: 'alpha' } ]
fixes: [ { range: [ 12, 13 ], text: 'beta' },
  { range: [ 34, 35 ], text: 'beta' } ]
actual: 
                                let alpha, b;
                                console.log(alpha, b);

expected: 
                                let alpha, beta;
                                console.log(alpha, beta);

You can see that only the fixes from the first reported problem are applied.

The fixes are not overlapping. ESlint errors on overlapping fixes, doesn't it?

[platinumazure]

@futpib ESLint wouldn't error-- it just wouldn't apply the fixes.

Curious to see what happens if you spread the variables out further (as I said earlier, our "overlap" detection might be strict)-- any chance you could try something like let a, validVariable, b and see what happens in that case?

[futpib]

@platinumazure It's the same, b is not renamed.

Log

``` getSource: let a, validVariable, b; console.log(a, validVariable, b); fixes: [ { range: [ 9, 10 ], text: 'alpha' }, { range: [ 46, 47 ], text: 'alpha' } ] fixes: [ { range: [ 27, 28 ], text: 'beta' }, { range: [ 64, 65 ], text: 'beta' } ] actual: let alpha, validVariable, b; console.log(alpha, validVariable, b); expected: let alpha, validVariable, beta; console.log(alpha, validVariable, beta); ```

[platinumazure]

@futpib Thanks for testing that.

Why do you have two fixes arrays? What does the actual rule output look like? (Try new (require("eslint").CLIEngine({ fix: false, /* other opts */ })).executeOnText() and look at the results output.)

[futpib]

@platinumazure Here is the result of executeOnText.

executeOnText

```json { "results": [ { "filePath": "", "messages": [ { "ruleId": "rule", "severity": 2, "message": "foo", "line": 2, "column": 9, "nodeType": "Identifier", "endLine": 2, "endColumn": 10, "fix": { "range": [ 9, 32 ], "text": "alpha, b;\n\t\t\t\tconsole.log(alpha" } }, { "ruleId": "rule", "severity": 2, "message": "foo", "line": 2, "column": 12, "nodeType": "Identifier", "endLine": 2, "endColumn": 13, "fix": { "range": [ 12, 35 ], "text": "beta;\n\t\t\t\tconsole.log(a, beta" } } ], "errorCount": 2, "warningCount": 0, "fixableErrorCount": 2, "fixableWarningCount": 0, "source": "\n\t\t\t\tlet a, b;\n\t\t\t\tconsole.log(a, b);\n\t\t\t" } ], "errorCount": 2, "warningCount": 0, "fixableErrorCount": 2, "fixableWarningCount": 0, "usedDeprecatedRules": [] } ```

The fixes here are indeed overlapping, but they each seem to be a result of a merge of the rule's fixer.replaceText calls. The rule issued two more granular replacements for each a identifier, while the fix here is spanning multiple lines.

I've also updated the demo repo to include this CLIEngine run.

[mysticatea]

Hi. This is by design.

CLIEngine has limitation to loop autofix to avoid infinite looping, so auto-fixing may stop anytime. Some runtimes such as VSCode don't loop auto-fixing. And it can apply the fix of each error individually with GUI.

Therefore, please test each fix to ensure the expected code. Additional test cases which have the output of the previous fix as that input would make clear what happens in whole.

@futpib it looks the expected result.

[platinumazure]

@futpib In this case, as @mysticatea noted, you do have a range overlap (as a result of issuing multiple fixes). The solution in your case would be to call context.report once per identifier change (so report 4 errors, not 2). That will ensure there is no overlap. Thanks!

[mysticatea]

The solution in your case would be to call context.report once per identifier change (so report 4 errors, not 2).

No. 😇 The fix of each error may be applied individually, so if it separated errors per identifier change, it may break code; an identifier was renamed but others were not renamed (it makes ReferenceErrors).

[futpib]

Why does eslint merge multiple fixes into one? It seems that keeping the original representation (array of {text, range} objects) for each problem would not require autofix looping, while also allowing multiple problem fixes to be applied separately or simultaneously.

[mysticatea]

The discussion was on #7348 (issue) and #8101 (PR).

The main reason is two:

• Avoiding breaking change: The change of the result object is a breaking change. Merging fix objects avoided the breaking. For example, VSCode's error bulbs would get broken if we make the fix property an array.
• Ensuring to not break code: ESLint applies the autofix of many rules at the same time. I'm not sure if the comparison of sparse ranges between different rules ensures to not break code.

Also, a strict comparison of sparse ranges may have a performance impact.

[kaicataldo]

@g-plane Are you still championing this? It looks like this is working as intended.

[g-plane]

If it works as intended, there may be nothing should be changed.

[nzakas]

Working as intended, so closing.