Autofix rules

[sindresorhus]

Would be useful to support autofixing for some of the rules. Not all are possible.

http://eslint.org/docs/developer-guide/working-with-rules#contextreport

• [ ] no-skip-test (just remove the skip modifier)
• [x] no-only-test (just remove the only modifier)
• [ ] no-identical-title (append #1, #2, etc, to the title)
• [ ] no-skip-assert (just remove the skip modifier)

Can't think of any way to reliably fix the other rules, but input welcome!

[jfmengels]

:+1: Great idea.

Not sure about no-identical-title though, as they won't really make things more readable in the source code (only when matching AVA's output to a test). Don't forget that the rule can also be triggered because there is no title.

[sindresorhus]

Not sure about no-identical-title though, as they won't really make things more readable in the source code (only when matching AVA's output to a test).

Appending a #n does make it clear it's testing the same thing just split up for readability, but I don't feel strongly about it, and is a pretty edge-case anyways.

[jamestalmage]

W/regard to the no-skip-* rules, I think auto fixing those is problematic. It will likely make the tests fail. If I am running xo --fix to clear up some whitespace / semicolon mistakes - I probably don't want you messing with my tests/assertions.

I don't think we should auto-fix anything that modifies code behavior (unless maybe the user sets some other CLI option or environment variable? FIX_AVA=true xo --fix.

One exception to the above. I think removing only is fine. Users should never check in .only tests, but .skip is acceptable in a number of situations (i.e. somebody has submitted a PR with a failing test but not the fix itself).

[sindresorhus]

But without it will make XO fail. So it will still fail.

[jamestalmage]

Does --fix only fix errors, or warnings too?

[sindresorhus]

Warnings too.

[florianb]

@sindresorhus: i would like to give it a try if it is still relevant.

[sindresorhus]

@florianb Sure. Go ahead. Seems like we couldn't agree on all of these yet, but you could start with autofixing for no-only-test :) There might also be other rules not mentioned here that could potentially have autofixing. Feel free to look into that too.

[florianb]

Great - i am sure a PR will stimulate discussions.. 😄

[florianb]

Hey everybody, i am stuck testing the fixResult.output since error.actual does not get printed. If i am not wrong this needs to be fixed in https://github.com/jfmengels/eslint-ava-rule-tester - i would be very happy to fix that if a contributor could check my assumption and approve that a PR for that would be welcome.

Thank you very much! 💝

[sindresorhus]

@florianb Just apply the fix mention in https://github.com/jfmengels/eslint-ava-rule-tester/issues/5 locally to your node_modules and you should be able to debug it for now.

[florianb]

@sindresorhus i guess this fix is not applicable since eslint forbids the modification of the AST. While it it would (probably) be possible to rename .only into something different, i can't remove it without causing the following exception:

Rule should not modify AST.

Actual:
[object Object]

Expected:
[object Object]

assertASTDidntChange (node_modules/eslint/lib/testers/rule-tester.js:406:24)
...

I opened a corresponding SO-question without helpful responses. I guess i could start a bounty to raise attention but i can't think of any solution overcoming this barrier.

I am really sorry that i just got stuck again. 😞

[mightyiam]

Keep it up, Florian. The light is ahead of the curve.

On Fri, Mar 31, 2017, 12:53 Florian Breisch [email protected] wrote:

@sindresorhus https://github.com/sindresorhus i guess this fix is not applicable since eslint forbids the modification of the AST. While it it would (probably) be possible to rename .only into something different, i can't remove it without causing the following exception:

Rule should not modify AST.

Actual: [object Object]

Expected: [object Object]

assertASTDidntChange (node_modules/eslint/lib/testers/rule-tester.js:406:24) ...

I opened a corresponding SO-question https://stackoverflow.com/questions/43043746/how-to-modify-the-javascript-ast-in-an-eslint-rule-fix without helpful responses. I guess i could start a bounty to raise attention but i can't think of any solution overcoming this barrier.

I am really sorry that i just got stuck again. 😞

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/avajs/eslint-plugin-ava/issues/21#issuecomment-290670396, or mute the thread https://github.com/notifications/unsubscribe-auth/AAmyx8zhN7eb-uHslydazj9H6hF7W9umks5rrM0lgaJpZM4HkYpp .

[sindresorhus]

@florianb Can you submit a WIP PR so we can take a look and maybe help?

[florianb]

@sindresorhus - of course. Wait a minute.. 😄

[florianb]

@sindresorhus - created #173

[florianb]

Okaydo - heading on to implement the no-skip-test-fix.

[florianb]

🎺 I'm heading over to implement the fixer for no-skip-assert.

In thought of a fix for no-identical-title i'd propose adding with ${superhero()}-powers instead of numbers.

[sindresorhus]

In thought of a fix for no-identical-title i'd propose adding with ${superhero()}-powers instead of numbers.

Haha. Sounds funny in theory, but I think people will not appreciate our humor.

[jfmengels]

:trumpet: I'm heading over to implement the fixer for no-skip-assert.

Cool !

As for no-identical-title, joke aside, I am against having a fix for it. The title should IMO be a description of the expected behavior and conditions for that test case, and all of them grouped together should form a specification of the tested function(ality). It should be fixed by the developer so that the test has a useful title.

That said, these tests could have be autofixed:

• no-async-without-await
• no-duplicate-modifiers
• no-todo-implementation (debatable)
• no-unknown-modifiers (debatable)

[sindresorhus]

As for no-identical-title, joke aside, I am against having a fix for it. The title should IMO be a description of the expected behavior and conditions for that test case, and all of them grouped together should form a specification of the tested function(ality). It should be fixed by the developer so that the test has a useful title.

I agree.

[florianb]

Sure - so just to check if i got you all right 😅:

• [x] No superpowers for identical titles
• [x] No fix for identical titles
• [x] The former list of three fixes this issue aimed for magically expanded to provide also fixes for:
• [x] no-async-without-await
• [x] no-duplicate-modifiers
• [ ] no-todo-implementation
• [x] no-unknown-modifiers

[sindresorhus]

Correct

[sindresorhus]

But I don't think we should do a fixer for no-todo-implementation. What if the user accidentally wrote a test body and then we just remove it. Better for them to just fix it manually.

[jfmengels]

For no-todo-implementation, I thought we could remove the todo modifier. Yeah we definitely don't want to remove the test body.

On Fri, Dec 8, 2017, 19:41 Sindre Sorhus [email protected] wrote:

But I don't think we should do no-todo-implementation. What if the user accidentally wrote a test body and then we just remove it. Better for them to just fix it manually.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/avajs/eslint-plugin-ava/issues/21#issuecomment-350339727, or mute the thread https://github.com/notifications/unsubscribe-auth/ADsK5Ftfe3rfgROJPe33AMJuAK7Cf1ajks5s-YLlgaJpZM4HkYpp .

[sindresorhus]

Oh ok, makes more sense. I still don't think it should have a fixer though.

[florianb]

Well - then thanks for extending the scope.. 😆

I'll ignore the no-todo-implementation for now.

[mmkal]

@sindresorhus piggy-backing on this issue: IMO no-only-test should not have an autofixer. See https://github.com/eslint/eslint/issues/7549#issuecomment-383002000 for why and https://github.com/eslint/eslint/issues/7549#issuecomment-383010918 for an explicit note that this kind of fix is not recommended by the eslint team.

It can be actively harmful to auto-fix no-only-test (and no-skip-test) - here's a scenario the current behaviour encourages:

• I write a test which is failing, and I want to debug it
• I add test.only in the code as a quick way to run just this test and no others in the file
• vscode instantly deletes what I wrote(!) when I press save
• I write it again, and this time suppress the lint warning
• I debug and fix the test
• I am forgetful. Everything looks normal so I check the code with test.only and the suppression in
• Tests pass - but only one of them is running
• This test gap goes unnoticed in master for months

By comparison, here's what happens with jest/no-disabled-tests

• I write a test which is failing, and I want to debug it
• I add test.only in the code as a quick way to run just this test and no others in the file
• vscode shows me a red squiggly. I'm fine with that. Only my test runs; I debug and fix it.
• Since I can see the red squiggly, I'm much less likely to forget about the .only, so probably I remove it before checking in. But even if I don't, CI will catch it and tell me about a lint error.
• All tests keep running in master

[sindresorhus]

@mmkal Yes, we plan to drop the auto-fixer for those rules. See: https://github.com/avajs/eslint-plugin-ava/issues/281#issuecomment-587137623