Merge pull request #226 from playwright-community/no-conditional-expect

feat: Add `no-conditional-expect` rule

Author: mskelton

README.md

@@ -123,6 +123,7 @@ command line option.\
 | ✔  |     |     | [max-nested-describe](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/max-nested-describe.md)                 | Enforces a maximum depth to nested describe calls                 |
 | ✔  | 🔧  |     | [missing-playwright-await](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/missing-playwright-await.md)       | Enforce Playwright APIs to be awaited                             |
 |     |     |     | [no-commented-out-test](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/no-commented-out-test.md)             | Disallow commented out tests                                      |
+| ✔  |     |     | [no-conditional-expect](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/no-conditional-expect.md)             | Disallow calling `expect` conditionally                           |
 | ✔  |     |     | [no-conditional-in-test](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/no-conditional-in-test.md)           | Disallow conditional logic in tests                               |
 | ✔  |     | 💡  | [no-element-handle](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/no-element-handle.md)                     | Disallow usage of element handles                                 |
 | ✔  |     |     | [no-eval](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/no-eval.md)                                         | Disallow usage of `page.$eval()` and `page.$$eval()`              |

docs/rules/no-conditional-expect.md

@@ -0,0 +1,135 @@
+# Disallow calling `expect` conditionally (`no-conditional-expect`)
+
+This rule prevents the use of `expect` in conditional blocks, such as `if`s &
+`catch`s.
+
+This includes using `expect` in callbacks to functions named `catch`, which are
+assumed to be promises.
+
+## Rule details
+
+Playwright only considers a test to have failed if it throws an error, meaning
+if calls to assertion functions like `expect` occur in conditional code such as
+a `catch` statement, tests can end up passing but not actually test anything.
+
+Additionally, conditionals tend to make tests more brittle and complex, as they
+increase the amount of mental thinking needed to understand what is actually
+being tested.
+
+While `expect.assertions` & `expect.hasAssertions` can help prevent tests from
+silently being skipped, when combined with conditionals they typically result in
+even more complexity being introduced.
+
+The following patterns are warnings:
+
+```js
+test('foo', () => {
+  doTest && expect(1).toBe(2);
+});
+
+test('bar', () => {
+  if (!skipTest) {
+    expect(1).toEqual(2);
+  }
+});
+
+test('baz', async () => {
+  try {
+    await foo();
+  } catch (err) {
+    expect(err).toMatchObject({ code: 'MODULE_NOT_FOUND' });
+  }
+});
+
+test('throws an error', async () => {
+  await foo().catch((error) => expect(error).toBeInstanceOf(error));
+});
+```
+
+The following patterns are not warnings:
+
+```js
+test('foo', () => {
+  expect(!value).toBe(false);
+});
+
+function getValue() {
+  if (process.env.FAIL) {
+    return 1;
+  }
+
+  return 2;
+}
+
+test('foo', () => {
+  expect(getValue()).toBe(2);
+});
+
+test('validates the request', () => {
+  try {
+    processRequest(request);
+  } catch {
+    // ignore errors
+  } finally {
+    expect(validRequest).toHaveBeenCalledWith(request);
+  }
+});
+
+test('throws an error', async () => {
+  await expect(foo).rejects.toThrow(Error);
+});
+```
+
+### How to catch a thrown error for testing without violating this rule
+
+A common situation that comes up with this rule is when wanting to test
+properties on a thrown error, as Playwright's `toThrow` matcher only checks the
+`message` property.
+
+Most people write something like this:
+
+```typescript
+test.describe('when the http request fails', () => {
+  test('includes the status code in the error', async () => {
+    try {
+      await makeRequest(url);
+    } catch (error) {
+      expect(error).toHaveProperty('statusCode', 404);
+    }
+  });
+});
+```
+
+As stated above, the problem with this is that if `makeRequest()` doesn't throw
+the test will still pass as if the `expect` had been called.
+
+While you can use `expect.assertions` & `expect.hasAssertions` for these
+situations, they only work with `expect`.
+
+A better way to handle this situation is to introduce a wrapper to handle the
+catching, and otherwise return a specific "no error thrown" error if nothing is
+thrown by the wrapped function:
+
+```typescript
+class NoErrorThrownError extends Error {}
+
+const getError = async <TError>(call: () => unknown): Promise<TError> => {
+  try {
+    await call();
+
+    throw new NoErrorThrownError();
+  } catch (error: unknown) {
+    return error as TError;
+  }
+};
+
+test.describe('when the http request fails', () => {
+  test('includes the status code in the error', async () => {
+    const error = await getError(async () => makeRequest(url));
+
+    // check that the returned error wasn't that no error was thrown
+    expect(error).not.toBeInstanceOf(NoErrorThrownError);
+    expect(error).toHaveProperty('statusCode', 404);
+  });
+});
+```

src/index.ts

@@ -3,6 +3,7 @@ import expectExpect from './rules/expect-expect';
 import maxNestedDescribe from './rules/max-nested-describe';
 import missingPlaywrightAwait from './rules/missing-playwright-await';
 import noCommentedOutTests from './rules/no-commented-out-tests';
+import noConditionalExpect from './rules/no-conditional-expect';
 import noConditionalInTest from './rules/no-conditional-in-test';
 import noElementHandle from './rules/no-element-handle';
 import noEval from './rules/no-eval';
@@ -43,6 +44,7 @@ const index = {
     'max-nested-describe': maxNestedDescribe,
     'missing-playwright-await': missingPlaywrightAwait,
     'no-commented-out-tests': noCommentedOutTests,
+    'no-conditional-expect': noConditionalExpect,
     'no-conditional-in-test': noConditionalInTest,
     'no-element-handle': noElementHandle,
     'no-eval': noEval,
@@ -84,6 +86,7 @@ const sharedConfig = {
     'playwright/expect-expect': 'warn',
     'playwright/max-nested-describe': 'warn',
     'playwright/missing-playwright-await': 'error',
+    'playwright/no-conditional-expect': 'warn',
     'playwright/no-conditional-in-test': 'warn',
     'playwright/no-element-handle': 'warn',
     'playwright/no-eval': 'warn',

src/rules/no-conditional-expect.ts

@@ -0,0 +1,113 @@
+import { Rule, Scope } from 'eslint';
+import * as ESTree from 'estree';
+import {
+  getExpectType,
+  getParent,
+  isPropertyAccessor,
+  isTestCall,
+} from '../utils/ast';
+import { KnownCallExpression } from '../utils/types';
+
+const isCatchCall = (
+  node: ESTree.CallExpression,
+): node is KnownCallExpression =>
+  node.callee.type === 'MemberExpression' &&
+  isPropertyAccessor(node.callee, 'catch');
+
+const getTestCallExpressionsFromDeclaredVariables = (
+  context: Rule.RuleContext,
+  declaredVariables: readonly Scope.Variable[],
+): ESTree.CallExpression[] => {
+  return declaredVariables.reduce<ESTree.CallExpression[]>(
+    (acc, { references }) => [
+      ...acc,
+      ...references
+        .map(({ identifier }) => getParent(identifier))
+        .filter(
+          (node): node is ESTree.CallExpression =>
+            node?.type === 'CallExpression' && isTestCall(context, node),
+        ),
+    ],
+    [],
+  );
+};
+
+export default {
+  create(context) {
+    let conditionalDepth = 0;
+    let inTestCase = false;
+    let inPromiseCatch = false;
+
+    const increaseConditionalDepth = () => inTestCase && conditionalDepth++;
+    const decreaseConditionalDepth = () => inTestCase && conditionalDepth--;
+
+    return {
+      CallExpression(node: ESTree.CallExpression) {
+        if (isTestCall(context, node)) {
+          inTestCase = true;
+        }
+
+        if (isCatchCall(node)) {
+          inPromiseCatch = true;
+        }
+
+        const expectType = getExpectType(context, node);
+        if (inTestCase && expectType && conditionalDepth > 0) {
+          context.report({
+            messageId: 'conditionalExpect',
+            node,
+          });
+        }
+
+        if (inPromiseCatch && expectType) {
+          context.report({
+            messageId: 'conditionalExpect',
+            node,
+          });
+        }
+      },
+      'CallExpression:exit'(node) {
+        if (isTestCall(context, node)) {
+          inTestCase = false;
+        }
+
+        if (isCatchCall(node)) {
+          inPromiseCatch = false;
+        }
+      },
+      CatchClause: increaseConditionalDepth,
+      'CatchClause:exit': decreaseConditionalDepth,
+      ConditionalExpression: increaseConditionalDepth,
+      'ConditionalExpression:exit': decreaseConditionalDepth,
+      FunctionDeclaration(node) {
+        const declaredVariables = context.sourceCode.getDeclaredVariables(node);
+        const testCallExpressions = getTestCallExpressionsFromDeclaredVariables(
+          context,
+          declaredVariables,
+        );
+
+        if (testCallExpressions.length > 0) {
+          inTestCase = true;
+        }
+      },
+      IfStatement: increaseConditionalDepth,
+      'IfStatement:exit': decreaseConditionalDepth,
+      LogicalExpression: increaseConditionalDepth,
+      'LogicalExpression:exit': decreaseConditionalDepth,
+      SwitchStatement: increaseConditionalDepth,
+      'SwitchStatement:exit': decreaseConditionalDepth,
+    };
+  },
+  meta: {
+    docs: {
+      category: 'Best Practices',
+      description: 'Disallow calling `expect` conditionally',
+      recommended: true,
+      url: 'https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/no-conditional-expect.md',
+    },
+    messages: {
+      conditionalExpect: 'Avoid calling `expect` conditionally`',
+    },
+    type: 'problem',
+  },
+} as Rule.RuleModule;