From aba53e4061f3b636ab0c0270e183c355c6f301e0 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Sun, 17 May 2020 19:26:52 +1200 Subject: [PATCH] feat: create `no-conditional-expect` rule --- README.md | 1 + docs/rules/no-conditional-expect.md | 70 +++ .../__snapshots__/rules.test.ts.snap | 1 + src/__tests__/rules.test.ts | 2 +- .../__tests__/no-conditional-expect.test.ts | 456 ++++++++++++++++++ src/rules/no-conditional-expect.ts | 71 +++ 6 files changed, 600 insertions(+), 1 deletion(-) create mode 100644 docs/rules/no-conditional-expect.md create mode 100644 src/rules/__tests__/no-conditional-expect.test.ts create mode 100644 src/rules/no-conditional-expect.ts diff --git a/README.md b/README.md index aa78b71f8..ed32c0834 100644 --- a/README.md +++ b/README.md @@ -134,6 +134,7 @@ installations requiring long-term consistency. | [lowercase-name](docs/rules/lowercase-name.md) | Enforce lowercase test names | | ![fixable][] | | [no-alias-methods](docs/rules/no-alias-methods.md) | Disallow alias methods | ![style][] | ![fixable][] | | [no-commented-out-tests](docs/rules/no-commented-out-tests.md) | Disallow commented out tests | ![recommended][] | | +| [no-conditional-expect](docs/rules/no-conditional-expect.md) | Prevent calling `expect` conditionally | | | | [no-deprecated-functions](docs/rules/no-deprecated-functions.md) | Disallow use of deprecated functions | | ![fixable][] | | [no-disabled-tests](docs/rules/no-disabled-tests.md) | Disallow disabled tests | ![recommended][] | | | [no-duplicate-hooks](docs/rules/no-duplicate-hooks.md) | Disallow duplicate setup and teardown hooks | | | diff --git a/docs/rules/no-conditional-expect.md b/docs/rules/no-conditional-expect.md new file mode 100644 index 000000000..e10b563ea --- /dev/null +++ b/docs/rules/no-conditional-expect.md @@ -0,0 +1,70 @@ +# Prevent calling `expect` conditionally (`no-conditional-expect`) + +This rule prevents the use of `expect` in conditional blocks, such as `if`s & +`catch`s. + +## Rule Details + +Jest considered a test to have failed if it throws an error, rather than on if +any particular function is called, meaning conditional calls to `expect` could +result in tests silently being skipped. + +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 +it('foo', () => { + doTest && expect(1).toBe(2); +}); + +it('bar', () => { + if (!skipTest) { + expect(1).toEqual(2); + } +}); + +it('baz', async () => { + try { + await foo(); + } catch (err) { + expect(err).toMatchObject({ code: 'MODULE_NOT_FOUND' }); + } +}); +``` + +The following patterns are not warnings: + +```js +it('foo', () => { + expect(!value).toBe(false); +}); + +function getValue() { + if (process.env.FAIL) { + return 1; + } + + return 2; +} + +it('foo', () => { + expect(getValue()).toBe(2); +}); + +it('validates the request', () => { + try { + processRequest(request); + } catch { + // ignore errors + } finally { + expect(validRequest).toHaveBeenCalledWith(request); + } +}); +``` diff --git a/src/__tests__/__snapshots__/rules.test.ts.snap b/src/__tests__/__snapshots__/rules.test.ts.snap index 23d76c1c8..bbae8229a 100644 --- a/src/__tests__/__snapshots__/rules.test.ts.snap +++ b/src/__tests__/__snapshots__/rules.test.ts.snap @@ -15,6 +15,7 @@ Object { "jest/lowercase-name": "error", "jest/no-alias-methods": "error", "jest/no-commented-out-tests": "error", + "jest/no-conditional-expect": "error", "jest/no-deprecated-functions": "error", "jest/no-disabled-tests": "error", "jest/no-duplicate-hooks": "error", diff --git a/src/__tests__/rules.test.ts b/src/__tests__/rules.test.ts index 98bdb5a1b..6c08c7a67 100644 --- a/src/__tests__/rules.test.ts +++ b/src/__tests__/rules.test.ts @@ -3,7 +3,7 @@ import { resolve } from 'path'; import plugin from '../'; const ruleNames = Object.keys(plugin.rules); -const numberOfRules = 42; +const numberOfRules = 43; describe('rules', () => { it('should have a corresponding doc for each rule', () => { diff --git a/src/rules/__tests__/no-conditional-expect.test.ts b/src/rules/__tests__/no-conditional-expect.test.ts new file mode 100644 index 000000000..1b294ea8e --- /dev/null +++ b/src/rules/__tests__/no-conditional-expect.test.ts @@ -0,0 +1,456 @@ +import { TSESLint } from '@typescript-eslint/experimental-utils'; +import resolveFrom from 'resolve-from'; +import rule from '../no-conditional-expect'; + +const ruleTester = new TSESLint.RuleTester({ + parser: resolveFrom(require.resolve('eslint'), 'espree'), + parserOptions: { + ecmaVersion: 2019, + }, +}); + +ruleTester.run('common tests', rule, { + valid: [ + ` + it('foo', () => { + expect(1).toBe(2); + }); + `, + ` + it('foo', () => { + expect(!true).toBe(false); + }); + `, + ], + invalid: [], +}); + +ruleTester.run('logical conditions', rule, { + valid: [ + ` + it('foo', () => { + process.env.FAIL && setNum(1); + + expect(num).toBe(2); + }); + `, + ` + function getValue() { + let num = 2; + + process.env.FAIL && setNum(1); + + return num; + } + + it('foo', () => { + expect(getValue()).toBe(2); + }); + `, + ` + it('foo', () => { + process.env.FAIL || setNum(1); + + expect(num).toBe(2); + }); + `, + ` + function getValue() { + let num = 2; + + process.env.FAIL || setNum(1); + + return num; + } + + it('foo', () => { + expect(getValue()).toBe(2); + }); + `, + ], + invalid: [ + { + code: ` + it('foo', () => { + something && expect(something).toHaveBeenCalled(); + }) + `, + errors: [{ messageId: 'conditionalExpect' }], + }, + { + code: ` + it('foo', () => { + a || b && expect(something).toHaveBeenCalled(); + }) + `, + errors: [{ messageId: 'conditionalExpect' }], + }, + { + code: ` + it('foo', () => { + (a || b) && expect(something).toHaveBeenCalled(); + }) + `, + errors: [{ messageId: 'conditionalExpect' }], + }, + { + code: ` + it('foo', () => { + a || (b && expect(something).toHaveBeenCalled()); + }) + `, + errors: [{ messageId: 'conditionalExpect' }], + }, + { + code: ` + it('foo', () => { + a && b && expect(something).toHaveBeenCalled(); + }) + `, + errors: [{ messageId: 'conditionalExpect' }], + }, + { + code: ` + it('foo', () => { + a && b || expect(something).toHaveBeenCalled(); + }) + `, + errors: [{ messageId: 'conditionalExpect' }], + }, + { + code: ` + it('foo', () => { + (a && b) || expect(something).toHaveBeenCalled(); + }) + `, + errors: [{ messageId: 'conditionalExpect' }], + }, + { + code: ` + function getValue() { + something && expect(something).toHaveBeenCalled(); + } + + it('foo', getValue); + `, + errors: [{ messageId: 'conditionalExpect' }], + }, + { + code: ` + it('foo', () => { + something || expect(something).toHaveBeenCalled(); + }) + `, + errors: [{ messageId: 'conditionalExpect' }], + }, + { + code: ` + function getValue() { + something || expect(something).toHaveBeenCalled(); + } + + it('foo', getValue); + `, + errors: [{ messageId: 'conditionalExpect' }], + }, + ], +}); + +ruleTester.run('conditional conditions', rule, { + valid: [ + ` + it('foo', () => { + const num = process.env.FAIL ? 1 : 2; + + expect(num).toBe(2); + }); + `, + ` + function getValue() { + return process.env.FAIL ? 1 : 2 + } + + it('foo', () => { + expect(getValue()).toBe(2); + }); + `, + ], + invalid: [ + { + code: ` + it('foo', () => { + something ? expect(something).toHaveBeenCalled() : noop(); + }) + `, + errors: [{ messageId: 'conditionalExpect' }], + }, + { + code: ` + function getValue() { + something ? expect(something).toHaveBeenCalled() : noop(); + } + + it('foo', getValue); + `, + errors: [{ messageId: 'conditionalExpect' }], + }, + { + code: ` + it('foo', () => { + something ? noop() : expect(something).toHaveBeenCalled(); + }) + `, + errors: [{ messageId: 'conditionalExpect' }], + }, + { + code: ` + function getValue() { + something ? noop() : expect(something).toHaveBeenCalled(); + } + + it('foo', getValue); + `, + errors: [{ messageId: 'conditionalExpect' }], + }, + ], +}); + +ruleTester.run('switch conditions', rule, { + valid: [ + ` + it('foo', () => { + let num; + + switch(process.env.FAIL) { + case true: + num = 1; + break; + case false: + num = 2; + break; + } + + expect(num).toBe(2); + }); + `, + ` + function getValue() { + switch(process.env.FAIL) { + case true: + return 1; + case false: + return 2; + } + } + + it('foo', () => { + expect(getValue()).toBe(2); + }); + `, + ], + invalid: [ + { + code: ` + it('foo', () => { + switch(something) { + case 'value': + break; + default: + expect(something).toHaveBeenCalled(); + } + }) + `, + errors: [{ messageId: 'conditionalExpect' }], + }, + { + code: ` + it('foo', () => { + switch(something) { + case 'value': + expect(something).toHaveBeenCalled(); + default: + break; + } + }) + `, + errors: [{ messageId: 'conditionalExpect' }], + }, + { + code: ` + function getValue() { + switch(something) { + case 'value': + break; + default: + expect(something).toHaveBeenCalled(); + } + } + + it('foo', getValue); + `, + errors: [{ messageId: 'conditionalExpect' }], + }, + ], +}); + +ruleTester.run('if conditions', rule, { + valid: [ + ` + it('foo', () => { + let num = 2; + + if(process.env.FAIL) { + num = 1; + } + + expect(num).toBe(2); + }); + `, + ` + function getValue() { + if(process.env.FAIL) { + return 1; + } + + return 2; + } + + it('foo', () => { + expect(getValue()).toBe(2); + }); + `, + ], + invalid: [ + { + code: ` + it('foo', () => { + if(doSomething) { + expect(something).toHaveBeenCalled(); + } + }) + `, + errors: [{ messageId: 'conditionalExpect' }], + }, + { + code: ` + it('foo', () => { + if(!doSomething) { + // do nothing + } else { + expect(something).toHaveBeenCalled(); + } + }) + `, + errors: [{ messageId: 'conditionalExpect' }], + }, + { + code: ` + function getValue() { + if(doSomething) { + expect(something).toHaveBeenCalled(); + } + } + + it('foo', getValue); + `, + errors: [{ messageId: 'conditionalExpect' }], + }, + { + code: ` + function getValue() { + if(!doSomething) { + // do nothing + } else { + expect(something).toHaveBeenCalled(); + } + } + + it('foo', getValue); + `, + errors: [{ messageId: 'conditionalExpect' }], + }, + ], +}); + +ruleTester.run('catch conditions', rule, { + valid: [ + ` + it('foo', () => { + try { + // do something + } catch { + // ignore errors + } finally { + expect(something).toHaveBeenCalled(); + } + }); + `, + ` + it('foo', () => { + try { + // do something + } catch { + // ignore errors + } + + expect(something).toHaveBeenCalled(); + }); + `, + ` + function getValue() { + try { + // do something + } catch { + // ignore errors + } finally { + expect(something).toHaveBeenCalled(); + } + } + + it('foo', getValue); + `, + ` + function getValue() { + try { + process.env.FAIL.toString(); + + return 1; + } catch { + return 2; + } + } + + it('foo', () => { + expect(getValue()).toBe(2); + }); + `, + ], + invalid: [ + { + code: ` + it('foo', () => { + try { + + } catch (err) { + expect(err).toMatch('Error'); + } + }) + `, + errors: [{ messageId: 'conditionalExpect' }], + }, + { + code: ` + function getValue() { + try { + // do something + } catch { + expect(something).toHaveBeenCalled(); + } + } + + it('foo', getValue); + `, + errors: [{ messageId: 'conditionalExpect' }], + }, + ], +}); diff --git a/src/rules/no-conditional-expect.ts b/src/rules/no-conditional-expect.ts new file mode 100644 index 000000000..511a20dc5 --- /dev/null +++ b/src/rules/no-conditional-expect.ts @@ -0,0 +1,71 @@ +import { TSESTree } from '@typescript-eslint/experimental-utils'; +import { + createRule, + getTestCallExpressionsFromDeclaredVariables, + isExpectCall, + isTestCase, +} from './utils'; + +export default createRule({ + name: __filename, + meta: { + docs: { + description: 'Prevent calling `expect` conditionally', + category: 'Best Practices', + recommended: false, + }, + messages: { + conditionalExpect: 'Avoid calling `expect` conditionally`', + }, + type: 'problem', + schema: [], + }, + defaultOptions: [], + create(context) { + let conditionalDepth = 0; + let inTestCase = false; + + const increaseConditionalDepth = () => inTestCase && conditionalDepth++; + const decreaseConditionalDepth = () => inTestCase && conditionalDepth--; + + return { + FunctionDeclaration(node) { + const declaredVariables = context.getDeclaredVariables(node); + const testCallExpressions = getTestCallExpressionsFromDeclaredVariables( + declaredVariables, + ); + + if (testCallExpressions.length > 0) { + inTestCase = true; + } + }, + CallExpression(node: TSESTree.CallExpression) { + if (isTestCase(node)) { + inTestCase = true; + } + + if (inTestCase && isExpectCall(node) && conditionalDepth > 0) { + context.report({ + messageId: 'conditionalExpect', + node, + }); + } + }, + 'CallExpression:exit'(node) { + if (isTestCase(node)) { + inTestCase = false; + } + }, + CatchClause: increaseConditionalDepth, + 'CatchClause:exit': decreaseConditionalDepth, + IfStatement: increaseConditionalDepth, + 'IfStatement:exit': decreaseConditionalDepth, + SwitchStatement: increaseConditionalDepth, + 'SwitchStatement:exit': decreaseConditionalDepth, + ConditionalExpression: increaseConditionalDepth, + 'ConditionalExpression:exit': decreaseConditionalDepth, + LogicalExpression: increaseConditionalDepth, + 'LogicalExpression:exit': decreaseConditionalDepth, + }; + }, +});