feat(valid-expect-in-promise): re-implement rule

[G-Rath]

This is an attempt at re-implementing the valid-expect-in-promise rule to hopefully make it less buggy - primarily this is focused around having it use isExpectCall to more accurately determine if a node is what we conventionally consider a usage of the Jest expect method, since the current method being used is far too lax, checking only if the given node is a CallExpression and then if its parent is a MemberExpression (e.g. it doesn't even attempt to check if it's named expect).

While the current implementation isn't super bad, it sort of approaches the problem from the wrong end of the stick (imo at least) by searching for promise method calls and then both moving up the AST to find if we're in a test call and down to find if there's an expect in the body of the promise method (using the aforementioned too lax check to do so).

I've gone somewhat the other way using a more standard approach that's common in our other rules: looking for the expect call, and marking info as we enter and exit CallExpressions (leveraging the way the AST is navigated) on if we're in something that looks like a call to a promise method.

---

While this is technically bug fixing, I've committed it as a feature for now because it's a significant re-write that I think will fix a number of bugs (including some we don't even know about).

Fixes #405
Fixes #219
Closes #406

[G-Rath]

I've included some new tests that fail with the current version of the rule due to the poor checking of expect - specifically:

it('passes', () => {
  Promise.resolve().then(() => {
    grabber.grabSomething();
  });
});

[G-Rath]

This should now fix #405 (& close #406 🎉 )

[G-Rath]

@SimenB I've still got to re-implement tracking assignment to variables, but might have to be tomorrow/next-week thing - the rest can be reviewed :)

[G-Rath]

@SimenB this is ready for review - fixes a bunch of bugs and edge-cases that we weren't testing for, and has a bunch of improvements too, including:

• sub-block support
• better error location reporting
• "runtime value" tracking (so re-assignments are taken into account)
• probably something else I've forgotten 🤷

I've tried to cover as many cases as I could, but wouldn't be surprised if there's a couple I've not thought of so let me know if you think of any that we should test :)

[SimenB]

a looooot of code here, but since the tests have mostly just been expanded this seems safe enough 🙂 Have you tried to benchmark this rule? It has quite a bit of code enow

[SimenB]

why the explicit type?

[G-Rath]

No strong reason - it is symmetrical with the CallExpression:exit (which requires it otherwise its an any) and ensures WebStorm isn't dumb (for some reason a few of its features sometimes don't resolve types as well if you omit this 🤷)

[G-Rath]

Have you tried to benchmark this rule?

Yup, ran it against the jest codebase - it's actually faster by a few seconds (at least for me):

❯ node_modules/.bin/eslint . --ext js,jsx,ts,tsx,md  --rule '{ "jest/valid-expect-in-promise": 2 }'
jest on  support-circular-references took 1m16s

❯ node_modules/.bin/eslint . --ext js,jsx,ts,tsx,md  --rule '{ "jest/valid-expect-in-promise2": 2 }'
jest on  support-circular-references took 1m13s

(ran it a couple of times - those numbers are about +/- 1 second, with the gap of a few seconds being constant)

It also doesn't flag a false positive 🎉

[G-Rath]

@SimenB I'm wondering if we should flag Promise.all, since it rejects as soon as any of the promises reject, so other promises with expects might not all resolve - but that might not be a big deal since you're still returning a promise that failed? 🤔

We should also be checking for allSettled, but (at least for now) not any or race I think

[SimenB]

seems reasonable to me

[G-Rath]

Following up on this: I decided to leave Promise.all in there for now because allSettled is Node 12+ and we've only just stopped supporting Node 10

[SimenB]

merge at will, lgtm 🙂

[G-Rath]

I realised we don't support direct promises in array expressions, e.g.

it('promises multiple people', () => {
  return Promise.allSettled([
    api.getPersonByName('bob').then(person => {
      expect(person).toHaveProperty('name', 'Bob');
    }),
    api.getPersonByName('alice').then(person => {
      expect(person).toHaveProperty('name', 'Alice');
    })
  ]);
});

Annoyingly we can get really close without a lot of extra work, except that it then starts falling to bits with trying to bail out for cases like:

test('that we error on this destructuring', async () => {
  [promise] = something().then(value => {
    expect(value).toBe('red');
  });
});

For now I'm just going to move on because I want to ship this + the new major.

Here's the patch if anyone wants to pick it up later

Index: src/rules/__tests__/valid-expect-in-promise.test.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/rules/__tests__/valid-expect-in-promise.test.ts b/src/rules/__tests__/valid-expect-in-promise.test.ts
--- a/src/rules/__tests__/valid-expect-in-promise.test.ts	(revision c664be1eab66e69d8eed78bcb1404a98dd2ca57e)
+++ b/src/rules/__tests__/valid-expect-in-promise.test.ts	(date 1633806713585)
@@ -665,6 +665,20 @@
         return Promise.allSettled([onePromise, twoPromise]);
       });
     `,
+    {
+      code: dedent`
+        it('promises multiple people', () => {
+          return Promise.allSettled([
+            api.getPersonByName('bob').then(person => {
+              expect(person).toHaveProperty('name', 'Bob');
+            }),
+            api.getPersonByName('alice').then(person => {
+              expect(person).toHaveProperty('name', 'Alice');
+            })
+          ]);
+        });
+      `,
+    },
   ],
   invalid: [
     {
@@ -1353,6 +1367,161 @@
           endColumn: 5,
           line: 2,
           messageId: 'expectInFloatingPromise',
+        },
+      ],
+    },
+    {
+      code: dedent`
+        it('races multiple people', () => {
+          return Promise.race([
+            api.getPersonByName('bob').then(person => {
+              expect(person).toHaveProperty('name', 'Bob');
+            }),
+            api.getPersonByName('alice').then(person => {
+              expect(person).toHaveProperty('name', 'Alice');
+            })
+          ]);
+        });
+      `,
+      errors: [
+        {
+          line: 2,
+          column: 23,
+          messageId: 'expectInFloatingPromise',
+          endLine: 9,
+          endColumn: 4,
+        },
+        {
+          line: 2,
+          column: 23,
+          messageId: 'expectInFloatingPromise',
+          endLine: 9,
+          endColumn: 4,
+        },
+      ],
+    },
+    {
+      code: dedent`
+        it('promises any person', async () => {
+          await Promise.any([
+            api.getPersonByName('bob').then(person => {
+              expect(person).toHaveProperty('name', 'Bob');
+            }),
+            api.getPersonByName('alice').then(person => {
+              expect(person).toHaveProperty('name', 'Alice');
+            })
+          ]);
+        });
+      `,
+      errors: [
+        {
+          line: 2,
+          column: 21,
+          messageId: 'expectInFloatingPromise',
+          endLine: 9,
+          endColumn: 4,
+        },
+        {
+          line: 2,
+          column: 21,
+          messageId: 'expectInFloatingPromise',
+          endLine: 9,
+          endColumn: 4,
+        },
+      ],
+    },
+    {
+      code: dedent`
+        it('promises multiple people', () => {
+          Promise.allSettled([
+            api.getPersonByName('bob').then(person => {
+              expect(person).toHaveProperty('name', 'Bob');
+            }),
+            api.getPersonByName('alice').then(person => {
+              expect(person).toHaveProperty('name', 'Alice');
+            })
+          ]);
+        });
+      `,
+      errors: [
+        {
+          line: 2,
+          column: 22,
+          messageId: 'expectInFloatingPromise',
+          endLine: 9,
+          endColumn: 4,
+        },
+        {
+          line: 2,
+          column: 22,
+          messageId: 'expectInFloatingPromise',
+          endLine: 9,
+          endColumn: 4,
+        },
+      ],
+    },
+    {
+      code: dedent`
+        it('promises multiple people', async () => {
+          const promises = [
+            api.getPersonByName('bob').then(person => {
+              expect(person).toHaveProperty('name', 'Bob');
+            }),
+            api.getPersonByName('alice').then(person => {
+              expect(person).toHaveProperty('name', 'Alice');
+            })
+          ];
+
+          await promises;
+        });
+      `,
+      errors: [
+        {
+          line: 2,
+          column: 20,
+          messageId: 'expectInFloatingPromise',
+          endLine: 9,
+          endColumn: 4,
+        },
+        {
+          line: 2,
+          column: 20,
+          messageId: 'expectInFloatingPromise',
+          endLine: 9,
+          endColumn: 4,
+        },
+      ],
+    },
+    // we currently cannot track assignment of an array of promises
+    {
+      code: dedent`
+        it('promises multiple people', async () => {
+          const promises = [
+            api.getPersonByName('bob').then(person => {
+              expect(person).toHaveProperty('name', 'Bob');
+            }),
+            api.getPersonByName('alice').then(person => {
+              expect(person).toHaveProperty('name', 'Alice');
+            })
+          ];
+
+          return Promise.allSettled([]);
+        });
+      `,
+      errors: [
+        {
+          line: 2,
+          column: 20,
+          messageId: 'expectInFloatingPromise',
+          endLine: 9,
+          endColumn: 4,
+        },
+        {
+          line: 2,
+          column: 20,
+          messageId: 'expectInFloatingPromise',
+          endLine: 9,
+          endColumn: 4,
         },
       ],
     },
Index: src/rules/valid-expect-in-promise.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/rules/valid-expect-in-promise.ts b/src/rules/valid-expect-in-promise.ts
--- a/src/rules/valid-expect-in-promise.ts	(revision c664be1eab66e69d8eed78bcb1404a98dd2ca57e)
+++ b/src/rules/valid-expect-in-promise.ts	(date 1633806935035)
@@ -249,6 +249,48 @@
   return isValueAwaitedOrReturned(variable.id, body);
 };
 
+/**
+ * Checks if the given Array Expression is `await`ed or `return`ed by being
+ * passed as an argument to a `Promise.all` or `Promise.allSettled` call
+ */
+const arePromisesAwaitedOrReturned = (
+  node: TSESTree.ArrayExpression,
+): boolean => {
+  if (
+    node.parent?.type !== AST_NODE_TYPES.CallExpression ||
+    node.parent.arguments.length === 0
+  ) {
+    return false;
+  }
+
+  const nodeName = getNodeName(node.parent);
+
+  if (['Promise.all', 'Promise.allSettled'].includes(nodeName as string)) {
+    const [firstArg] = node.parent.arguments;
+
+    if (firstArg !== node) {
+      return false;
+    }
+
+    if (!node.parent.parent) {
+      return false;
+    }
+
+    if (node.parent.parent.type === AST_NODE_TYPES.ReturnStatement) {
+      return true;
+    }
+
+    if (
+      node.parent.parent.type === AST_NODE_TYPES.ExpressionStatement &&
+      node.parent.parent.parent?.type === AST_NODE_TYPES.AwaitExpression
+    ) {
+      return true;
+    }
+  }
+
+  return false;
+};
+
 export default createRule({
   name: __filename,
   meta: {
@@ -356,6 +398,14 @@
             ) {
               return;
             }
+
+            break;
+          }
+
+          case AST_NODE_TYPES.ArrayExpression: {
+            if (arePromisesAwaitedOrReturned(parent)) {
+              return;
+            }
 
             break;
           }

(seriously, I'm annoyed at how nicely this is working vs how ugly the edge cases are)

[github-actions]

🎉 This PR is included in version 24.6.0 🎉

The release is available on:

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

Your semantic-release bot 📦🚀

[github-actions]

🎉 This PR is included in version 25.0.0-next.6 🎉

The release is available on:

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

Your semantic-release bot 📦🚀