storybookjs / eslint-plugin-storybook

🎗Official ESLint plugin for Storybook
MIT License
238 stars 42 forks source link

await-interactions: false positive and bad fix if await result is passed to expect in one line #136

Open jrencz opened 11 months ago

jrencz commented 11 months ago

Describe the bug Rule reports problem on following code:

const promise = new Promise<{foo: string}>((resolve) => {
  setTimeout(() => {
    resolve({foo: 'bar'});
  }, 500);
});

expect((await promise).foo).toBe('bar');

if it's written this way:

const promise = new Promise<{foo: string}>((resolve) => {
  setTimeout(() => {
    resolve({foo: 'bar'});
  }, 500);
});

const {foo} = await promise;
expect(foo).toBe('bar');

rule doesn't complain.

Fix is proposed to prepend expect((await promise).foo).toBe('bar') which is incorrect as it doesn't need that in the first place.

That's true Jest expect can be awaited, but then

expect(promise).toBe(resolveValue)

should become

await expect(promise).resolves.toBe(resolveValue)

and fixer just prepends the await keyword

Additional context

jrencz commented 11 months ago

After a brief look into the rule code I can narrow the cause of what I see as a problem down to this section:

https://github.com/storybookjs/eslint-plugin-storybook/blob/611b70eae394a491d3284f060c0566d74e6bc545/lib/rules/await-interactions.ts#L101-L109

There's a design decision to enforce all expect calls to be awaited, but it's not even covered fully (e.g. it won't trigger the rule if it's expect(…).not.toBe(…) where the pattern of CallExpression>MemberExpression>CallExpression, which this chunk above detects is not preserved)

I went through the history of changes, of the rule, and I see that expect initially was one of FUNCTIONS_TO_BE_AWAITED but then some refinements came, but I'm not sure why was it considered as "it needs to be awaited" at all - it's not at all obvious it should