no-test-callback autofix can produce invalid code

[bodinsamuel]

Hello team,

With the release of v23 our team had issues with the new recommended rules, and in particular no-test-callback.

test('my test', async (done) => {
  await myAsyncTask();
  expect(true).toBe(false);
  done();
});

This code is marked as Illegal usage of test callback which is a fair warning; I would say the done usage is more useless than illegal (but just a matter of wording here).

This code will be transformed to:

test('my test', async () => {await new Promise(done => {
  await myAsyncTask();
  expect(true).toBe(false);
  done();
})});

Which is:

• not very pretty
• invalid
  • as await now lives in non async method
  • if you put the async keyword you can encounter an other linter error no-async-promise-executor

---

This is not a bug nor an urgent matter, but I would be in favor of letting the user change his/her code instead of modifying the behaviour of the test without the user paying attention.

Have a great day,

[G-Rath]

I'm not really sure what we can do about this aside from putting async on the executor to make sure valid code is generated.

We'd have to check the entire body for the use of await, which is a pretty tough ask, and very error prone.

@SimenB any thoughts on how we could tackle this?

[SimenB]

If the function is an async function, the generated handler should also be async. If this later triggers no-async-promise-executor that's something the user should fix manually. Mixing done callbacks and promises is very error prone.

We could also bail on async functions using a callback and just yell at you (not apply the fix)? Probably a safer approach, which might make more sense

not very pretty

My thinking was that people also run prettier, so the prettiness would fix itself within the same eslint --fix run without the user seeing the ugly code. Not sure we can do much about the fix looking ugly though beyond removing the fix entirely, which'd make me sad.

[bodinsamuel]

We could also bail on async functions using a callback and just yell at you (not apply the fix)?

Fine to me ☺️

My thinking was that people also run prettier

true, in my setup I need an additional cmd + s but not really a blocker ^^

[SimenB]

FWIW we should yell at this in Jest itself at runtime - you should either use the callback or return a promise, never both

EDIT: Opened up jestjs/jest#9129 for it

[github-actions]

🎉 This issue has been resolved in version 23.0.3 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[bodinsamuel]

amazing, thanks 🙌

[G-Rath]

On an aside, this could be a good place to use the new eslint suggestions API:

It's very similar to fixes mechanism that already exists in ESLint, but will provide ability to create multiple suggestions per rule, suggestions will not be automatically applied by using --fix flag, and will require user interaction.

I've not looked into, but since it requires user interaction, it might mean you can have your cake and eat it too?

[SimenB]

Oooh, exciting! Didn't know about that. Seems the pr with the implementation landed today

[G-Rath]

Yeah, that's how I found out about it - probably want to wait at least a few months before we consider actually shipping something that uses it (unless that won't break? I'm not sure how backwards combat works w/ eslint plugins - now that I type this, I'm assuming it'll just be a property prior version of eslint would just ignore, so maybe we can start shipping 🤔).

If it works how I'd expect by the description, it should allow for a lot more auto-fixes a la codemod.

[SimenB]

Yeah, I'd hope we can ship this without waiting. Might need to wait for typescript-eslint to add types for it though (typescript-eslint/typescript-eslint#1249)

[SimenB]

@G-Rath support released, if you wanna play with it: https://github.com/typescript-eslint/typescript-eslint/releases/tag/v2.9.0