fix(no-conditional-expect): Fix false positive with asymmetric matchers (#304)

* Expect parsing

* Get it working

* Fix up

* Tests

* Re-org

Author: mskelton

src/rules/no-conditional-expect.test.ts

@@ -1,22 +1,36 @@
-import dedent from 'dedent'
 import rule from '../../src/rules/no-conditional-expect'
-import { runRuleTester } from '../utils/rule-tester'
+import { javascript, runRuleTester } from '../utils/rule-tester'
 
 const messageId = 'conditionalExpect'
 
 runRuleTester('common tests', rule, {
   invalid: [],
   valid: [
-    `
+    javascript`
       test('foo', () => {
         expect(1).toBe(2);
       });
     `,
-    `
+    javascript`
       test('foo', () => {
         expect(!true).toBe(false);
       });
     `,
+    {
+      code: javascript`
+        test('foo', () => {
+          const expected = arr.map((x) => {
+            if (typeof x === 'string') {
+              return expect.arrayContaining([x])
+            } else {
+              return b
+            }
+          })
+
+          expect([1, 2, 3]).toEqual(expected);
+        });
+      `,
+    },
   ],
 })
 
@@ -154,7 +168,7 @@ runRuleTester('logical conditions', rule, {
     `,
     // Global aliases
     {
-      code: dedent`
+      code: javascript`
         it('foo', () => {
           process.env.FAIL && setNum(1);
 
@@ -331,7 +345,6 @@ runRuleTester('catch conditions', rule, {
       code: `
         test('foo', () => {
           try {
-  
           } catch (err) {
             expect(err).toMatch('Error');
           }
@@ -397,7 +410,7 @@ runRuleTester('catch conditions', rule, {
 runRuleTester('promises', rule, {
   invalid: [
     {
-      code: dedent`
+      code: javascript`
         test('works', async () => {
           await Promise.resolve()
             .then(() => { throw new Error('oh noes!'); })
@@ -407,7 +420,7 @@ runRuleTester('promises', rule, {
       errors: [{ messageId }],
     },
     {
-      code: dedent`
+      code: javascript`
         test('works', async () => {
           await Promise.resolve()
             .then(() => { throw new Error('oh noes!'); })
@@ -421,7 +434,7 @@ runRuleTester('promises', rule, {
       errors: [{ messageId }],
     },
     {
-      code: dedent`
+      code: javascript`
         test('works', async () => {
           await Promise.resolve()
             .catch(error => expect(error).toBeInstanceOf(Error))
@@ -432,7 +445,7 @@ runRuleTester('promises', rule, {
       errors: [{ messageId }],
     },
     {
-      code: dedent`
+      code: javascript`
         test('works', async () => {
           await Promise.resolve()
             .catch(error => expect(error).toBeInstanceOf(Error))
@@ -444,7 +457,7 @@ runRuleTester('promises', rule, {
       errors: [{ messageId }],
     },
     {
-      code: dedent`
+      code: javascript`
         test('works', async () => {
           await somePromise
             .then(() => { throw new Error('oh noes!'); })
@@ -454,7 +467,7 @@ runRuleTester('promises', rule, {
       errors: [{ messageId }],
     },
     {
-      code: dedent`
+      code: javascript`
         test('works', async () => {
           await somePromise.catch(error => expect(error).toBeInstanceOf(Error));
         });

src/rules/valid-expect.test.ts

@@ -215,6 +215,11 @@ runRuleTester('valid-expect', rule, {
   valid: [
     { code: 'expectPayButtonToBeEnabled()' },
     { code: 'expect("something").toBe("else")' },
+    { code: 'expect.anything()' },
+    { code: 'expect.arrayContaining()' },
+    { code: 'expect.not.arrayContaining()' },
+    { code: 'expect.objectContaining(expected)' },
+    { code: 'expect.not.objectContaining(expected)' },
     { code: 'expect("something").toBe(expect.anything())' },
     { code: 'expect("something").toEqual({ foo: expect.anything() })' },
     { code: 'expect("something").toBe(expect.arrayContaining([1, 2, 3]))' },

src/utils/parseFnCall.test.ts

@@ -260,36 +260,6 @@ runRuleTester('expect', rule, {
       code: dedent`
         import { expect } from '@playwright/test';
 
-        expect.assertions();
-      `,
-      errors: [
-        {
-          column: 1,
-          data: expectedParsedFnCallResultData({
-            args: [],
-            group: 'expect',
-            head: {
-              local: 'expect',
-              node: 'expect',
-              original: null,
-            },
-            matcher: 'assertions',
-            matcherArgs: [],
-            matcherName: 'assertions',
-            members: ['assertions'],
-            modifiers: [],
-            name: 'expect',
-            type: 'expect',
-          }),
-          line: 3,
-          messageId: 'details',
-        },
-      ],
-    },
-    {
-      code: dedent`
-        import { expect } from '@playwright/test';
-
         expect(x).toBe(y);
       `,
       errors: [
@@ -380,58 +350,6 @@ runRuleTester('expect', rule, {
       code: dedent`
         import { expect } from '@playwright/test';
 
-        expect.anything();
-        expect.not.arrayContaining();
-      `,
-      errors: [
-        {
-          column: 1,
-          data: expectedParsedFnCallResultData({
-            args: [],
-            group: 'expect',
-            head: {
-              local: 'expect',
-              node: 'expect',
-              original: null,
-            },
-            matcher: 'anything',
-            matcherArgs: [],
-            matcherName: 'anything',
-            members: ['anything'],
-            modifiers: [],
-            name: 'expect',
-            type: 'expect',
-          }),
-          line: 3,
-          messageId: 'details',
-        },
-        {
-          column: 1,
-          data: expectedParsedFnCallResultData({
-            args: [],
-            group: 'expect',
-            head: {
-              local: 'expect',
-              node: 'expect',
-              original: null,
-            },
-            matcher: 'arrayContaining',
-            matcherArgs: [],
-            matcherName: 'arrayContaining',
-            members: ['not', 'arrayContaining'],
-            modifiers: ['not'],
-            name: 'expect',
-            type: 'expect',
-          }),
-          line: 4,
-          messageId: 'details',
-        },
-      ],
-    },
-    {
-      code: dedent`
-        import { expect } from '@playwright/test';
-
         expect;
         expect(x);
         expect(x).toBe;
@@ -1187,5 +1105,18 @@ runTSRuleTester('typescript', rule, {
     },
     "it('is not a  function', () => {});",
     'dedent()',
+    'expect.assertions()',
+    'expect.anything()',
+    'expect.arrayContaining()',
+    'expect.objectContaining(expected)',
+    'expect.not.objectContaining(expected)',
+    {
+      code: dedent`
+        import { expect } from '@playwright/test';
+
+        expect.assertions();
+        expect.anything();
+      `,
+    },
   ],
 })

src/utils/parseFnCall.ts

@@ -94,21 +94,51 @@ export const isSupportedAccessor = (
 ): node is AccessorNode =>
   isIdentifier(node, value) || isStringNode(node, value)
 
-function getNodeChain(node: ESTree.Node): AccessorNode[] | null {
-  if (isSupportedAccessor(node)) {
-    return [node]
+class Chain {
+  #nodes: AccessorNode[] | null = null
+  #leaves: WeakSet<AccessorNode> = new WeakSet()
+
+  constructor(node: ESTree.Node) {
+    this.#nodes = this.#buildChain(node)
+  }
+
+  isLeaf(node: AccessorNode): boolean {
+    return this.#leaves.has(node)
   }
 
-  switch (node.type) {
-    case 'TaggedTemplateExpression':
-      return getNodeChain(node.tag)
-    case 'MemberExpression':
-      return joinChains(getNodeChain(node.object), getNodeChain(node.property))
-    case 'CallExpression':
-      return getNodeChain(node.callee)
+  get nodes() {
+    return this.#nodes
   }
 
-  return null
+  #buildChain(node: ESTree.Node, insideCall = false): AccessorNode[] | null {
+    if (isSupportedAccessor(node)) {
+      // If we are inside a call expression, then the current node is a leaf,
+      // that is, the end of the sub-chain. For example, in
+      // `expect.soft(x).not.toBe()`, `soft` and `toBe` are leaves.
+      if (insideCall) {
+        this.#leaves.add(node)
+      }
+
+      return [node]
+    }
+
+    switch (node.type) {
+      case 'TaggedTemplateExpression':
+        return this.#buildChain(node.tag)
+
+      case 'MemberExpression':
+        return joinChains(
+          this.#buildChain(node.object),
+          this.#buildChain(node.property, insideCall),
+        )
+
+      case 'CallExpression':
+        return this.#buildChain(node.callee, true)
+
+      default:
+        return null
+    }
+  }
 }
 
 const resolvePossibleAliasedGlobal = (
@@ -166,12 +196,13 @@ function determinePlaywrightFnGroup(name: string): FnGroup {
 export const modifiers = new Set(['not', 'resolves', 'rejects'])
 
 const findModifiersAndMatcher = (
+  chain: Chain,
   members: KnownMemberExpressionProperty[],
-): ModifiersAndMatcher | string => {
+  stage: ExpectParseStage,
+): ModifiersAndMatcher | string | null => {
   const modifiers: KnownMemberExpressionProperty[] = []
 
   for (const member of members) {
-    // Otherwise, it should be a modifier
     const name = getStringValue(member)
 
     if (name === 'soft' || name === 'poll') {
@@ -187,6 +218,12 @@ const findModifiersAndMatcher = (
         return 'modifier-unknown'
       }
     } else if (name !== 'not') {
+      // If we're in the "modifiers" stage and we find an unknown modifier,
+      // then it's actually an asymmetric matcher which we don't care about.
+      if (stage === 'modifiers') {
+        return null
+      }
+
       // Check if the member is being called, which means it is the matcher
       // (and also the end of the entire "expect" call chain).
       if (
@@ -205,6 +242,13 @@ const findModifiersAndMatcher = (
       return 'modifier-unknown'
     }
 
+    // When we find a leaf node, we're done with the modifiers and are moving
+    // on to the matchers.
+    if (chain.isLeaf(member)) {
+      stage = 'matchers'
+    }
+
+    // Add the modifier to the list of modifiers
     modifiers.push(member)
   }
 
@@ -263,10 +307,22 @@ export interface ParsedExpectFnCall
 
 export type ParsedFnCall = ParsedGeneralFnCall | ParsedExpectFnCall
 
+type ExpectParseStage = 'matchers' | 'modifiers'
+
 const parseExpectCall = (
+  chain: Chain,
   call: Omit<ParsedFnCall, 'group' | 'type'>,
-): ParsedExpectFnCall | string => {
-  const modifiersAndMatcher = findModifiersAndMatcher(call.members)
+  stage: ExpectParseStage,
+): ParsedExpectFnCall | string | null => {
+  const modifiersAndMatcher = findModifiersAndMatcher(
+    chain,
+    call.members,
+    stage,
+  )
+
+  if (!modifiersAndMatcher) {
+    return null
+  }
 
   if (typeof modifiersAndMatcher === 'string') {
     return modifiersAndMatcher
@@ -316,13 +372,10 @@ function parse(
   context: Rule.RuleContext,
   node: ESTree.CallExpression,
 ): ParsedFnCall | string | null {
-  const chain = getNodeChain(node)
-
-  if (!chain?.length) {
-    return null
-  }
+  const chain = new Chain(node)
+  if (!chain.nodes?.length) return null
 
-  const [first, ...rest] = chain
+  const [first, ...rest] = chain.nodes
   const resolved = resolveToPlaywrightFn(context, first)
   if (!resolved) return null
 
@@ -355,14 +408,20 @@ function parse(
   const group = determinePlaywrightFnGroup(name)
 
   if (group === 'expect') {
+    let stage: ExpectParseStage = chain.isLeaf(parsedFnCall.head.node)
+      ? 'matchers'
+      : 'modifiers'
+
     // If using `test.expect` style, the `rest` array will start with `expect`
     // and we need to remove it to ensure the chain accurately represents the
     // `expect` call chain.
     if (isIdentifier(rest[0], 'expect')) {
+      stage = chain.isLeaf(rest[0]) ? 'matchers' : 'modifiers'
       parsedFnCall.members.shift()
     }
 
-    const result = parseExpectCall(parsedFnCall)
+    const result = parseExpectCall(chain, parsedFnCall, stage)
+    if (!result) return null
 
     // If the `expect` call chain is not valid, only report on the topmost node
     // since all members in the chain are likely to get flagged for some reason
@@ -384,8 +443,8 @@ function parse(
 
   // Check that every link in the chain except the last is a member expression
   if (
-    chain
-      .slice(0, chain.length - 1)
+    chain.nodes
+      .slice(0, chain.nodes.length - 1)
       .some((n) => getParent(n)?.type !== 'MemberExpression')
   ) {
     return null

src/utils/rule-tester.ts

@@ -1,3 +1,4 @@
+import dedent from 'dedent'
 import { RuleTester } from 'eslint'
 import { describe, it } from 'vitest'
 
@@ -38,3 +39,5 @@ export function runTSRuleTester(...args: Parameters<RuleTester['run']>) {
 }
 
 export const test = (input: string) => `test('test', async () => { ${input} })`
+
+export const javascript = dedent