Prefer toThrow over try/catch/expect via jest/no-try-expect (#300)

CHANGELOG.md

@@ -11,6 +11,7 @@
 
 - New Rules:
   - `jest/no-commented-out-tests` disallows commented out tests.([275](https://github.com/Shopify/eslint-plugin-shopify/pull/275))
+  - `jest/no-try-expect` disallows `expect` calls in `catch` blocks ([300](https://github.com/Shopify/eslint-plugin-shopify/pull/300))
   - `node/prefer-promises/dns` and `node/prefer-promises/fs` These rules disallow the callback API in favor of promise API for the dns and fs modules. ([257](https://github.com/Shopify/eslint-plugin-shopify/pull/257))
   - `jest/no-mocks-import` This rule disallows manually importing from `__mocks__` ([246](https://github.com/Shopify/eslint-plugin-shopify/pull/246))
   - `react/state-in-constructor` Enforce state initialization to be in a class property. ([256](https://github.com/Shopify/eslint-plugin-shopify/pull/246))

docs/rules/jest/no-try-expect.md

@@ -0,0 +1,42 @@
+# Prevent catch assertions in tests. (jest/no-if)
+This rule prevents the use of `expect` inside `catch` blocks.
+
+## Rule Details
+
+Expectations inside a `catch` block can be silently skipped. While Jest provides an `expect.assertions(number)` helper, Shopify rarely uses this feature. Using `toThrow` concisely guarantees that an exception was thrown, and that its contents match expectations.
+
+Examples of **incorrect** code for this rule:
+
+```js
+it('foo', () => {
+  try {
+    foo(); // `foo` may be refactored to not throw exceptions, yet still appears to be tested here.
+  } catch (err) {
+    expect(err).toMatch(/foo error/);
+  }
+})
+
+it('bar', async () => {
+  try {
+    await foo();
+  } catch (err) {
+    expect(err).toMatch(/foo error/);
+  }
+})
+```
+
+Examples of **correct** code for this rule:
+
+```js
+it('foo', () => {
+  expect(() => foo()).toThrow(/foo error/);
+})
+
+it('bar', async () => {
+  await expect(fooPromise).rejects.toThrow(/foo error/);
+})
+```
+
+## When Not To Use It
+
+If you do not wish to prevent the use of catch expectations in tests, you can safely disable this rule.

index.js

@@ -5,6 +5,7 @@ module.exports = {
     'images-no-direct-imports': require('./lib/rules/images-no-direct-imports'),
     'jest/no-if': require('./lib/rules/jest/no-if'),
     'jest/no-snapshots': require('./lib/rules/jest/no-snapshots'),
+    'jest/no-try-expect': require('./lib/rules/jest/no-try-expect'),
     'jest/no-vague-titles': require('./lib/rules/jest/no-vague-titles'),
     'jsx-no-complex-expressions': require('./lib/rules/jsx-no-complex-expressions'),
     'jsx-no-hardcoded-content': require('./lib/rules/jsx-no-hardcoded-content'),

lib/config/jest.js

@@ -19,5 +19,7 @@ module.exports = {
     'shopify/jest/no-snapshots': 'error',
     // Prevent if statements in tests.
     'shopify/jest/no-if': 'error',
+    // Prevent assertions in catch blocks.
+    'shopify/jest/no-try-expect': 'error',
   }),
 };

lib/config/rules/shopify.js

@@ -9,6 +9,8 @@ module.exports = {
   'shopify/jest/no-if': 'off',
   // Disallow jest snapshots.
   'shopify/jest/no-snapshots': 'off',
+  // Prevent assertions in catch blocks.
+  'shopify/jest/no-try-expect': 'error',
   // Disallow vague words in test statements.
   'shopify/jest/no-vague-titles': 'off',
   // Disallow complex expressions embedded in in JSX.

lib/rules/jest/no-if.js

@@ -1,7 +1,5 @@
 const {docsUrl} = require('../../utilities');
-const {getTestMethodName} = require('../../utilities/jest');
-
-const TEST_FUNCTION_NAMES = ['it', 'xit', 'fit', 'test', 'xtest'];
+const {isTestDefinition} = require('../../utilities/jest');
 
 module.exports = {
   meta: {
@@ -72,16 +70,7 @@ module.exports = {
 };
 
 function ignore(node) {
-  return notTestFunction(node) || hasEmptyBody(node);
-}
-
-function notTestFunction(node) {
-  const method = getTestMethodName(node);
-  return !matchTestFunctionName(method);
-}
-
-function matchTestFunctionName(functionName) {
-  return TEST_FUNCTION_NAMES.includes(functionName);
+  return !isTestDefinition(node) || hasEmptyBody(node);
 }
 
 function hasEmptyBody(node) {

lib/rules/jest/no-try-expect.js

@@ -0,0 +1,56 @@
+const {docsUrl} = require('../../utilities');
+const {isTestDefinition} = require('../../utilities/jest');
+
+module.exports = {
+  meta: {
+    docs: {
+      description: 'Prefer using toThrow for exception tests.',
+      category: 'Best Practices',
+      recommended: false,
+      uri: docsUrl('jest/no-try-expect'),
+    },
+    messages: {
+      noTryExpect: [
+        'Tests should use Jest‘s exception helpers.',
+        'Use "expect(() => yourFunction()).toThrow()" for synchronous tests,',
+        'or "await expect(yourFunction()).rejects.toThrow()" for async tests',
+      ].join(' '),
+    },
+  },
+  create(context) {
+    let isTest = false;
+    let catchDepth = 0;
+
+    function isThrowExpectCall(node) {
+      return catchDepth > 0 && node.callee.name === 'expect';
+    }
+
+    return {
+      CallExpression(node) {
+        if (isTestDefinition(node)) {
+          isTest = true;
+        } else if (isTest && isThrowExpectCall(node)) {
+          context.report({
+            messageId: 'noTryExpect',
+            node,
+          });
+        }
+      },
+      CatchClause() {
+        if (isTest) {
+          ++catchDepth;
+        }
+      },
+      'CatchClause:exit': function() {
+        if (isTest) {
+          --catchDepth;
+        }
+      },
+      'CallExpression:exit': (node) => {
+        if (isTestDefinition(node)) {
+          isTest = false;
+        }
+      },
+    };
+  },
+};

lib/utilities/jest.js

@@ -9,6 +9,8 @@ const TEST_FUNCTION_NAMES = [
   'xdescribe',
 ];
 
+const TEST_DEFINITION_NAMES = ['it', 'xit', 'fit', 'test', 'xtest'];
+
 function getTestMethodName({callee}) {
   switch (callee.type) {
     case 'CallExpression':
@@ -24,7 +26,15 @@ function getTestMethodName({callee}) {
   }
 }
 
+function isTestDefinition(node) {
+  return (
+    node.type === 'CallExpression' &&
+    TEST_DEFINITION_NAMES.includes(getTestMethodName(node))
+  );
+}
+
 module.exports = {
   TEST_FUNCTION_NAMES,
   getTestMethodName,
+  isTestDefinition,
 };

tests/lib/rules/jest/no-try-expect.js

@@ -0,0 +1,73 @@
+const {RuleTester} = require('eslint');
+const rule = require('../../../../lib/rules/jest/no-try-expect');
+require('babel-eslint');
+
+const parser = 'babel-eslint';
+const ruleTester = new RuleTester();
+
+ruleTester.run('no-try-expect', rule, {
+  valid: [
+    {
+      code: `it('foo', () => {
+        expect('foo').toEqual('foo');
+      })`,
+      parser,
+    },
+    {
+      code: `
+        it('foo', () => {
+          expect('bar').toEqual('bar');
+        });
+        try {
+
+        } catch {
+          expect('foo').toEqual('foo');
+        }`,
+      parser,
+    },
+    {
+      code: `
+        it.skip('foo');
+        try {
+
+        } catch {
+          expect('foo').toEqual('foo');
+        }`,
+      parser,
+    },
+  ],
+  invalid: [
+    {
+      code: `it('foo', () => {
+        try {
+
+        } catch (err) {
+          expect(err).toMatch('Error');
+        }
+      })`,
+      parser,
+      errors: [
+        {
+          messageId: 'noTryExpect',
+        },
+      ],
+    },
+    {
+      code: `it('foo', async () => {
+        await wrapper('production', async () => {
+          try {
+
+          } catch (err) {
+            expect(err).toMatch('Error');
+          }
+        })
+      })`,
+      parser,
+      errors: [
+        {
+          messageId: 'noTryExpect',
+        },
+      ],
+    },
+  ],
+});