From d5518503e7c5c3051e698bf1a53e9fa2b6d840dc Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Mon, 7 Feb 2022 09:36:04 +1300 Subject: [PATCH] feat: create `no-conditional-in-test` rule (#1027) --- README.md | 1 + docs/rules/no-conditional-in-test.md | 79 ++ .../__snapshots__/rules.test.ts.snap | 1 + src/__tests__/rules.test.ts | 2 +- .../__tests__/no-conditional-in-test.test.ts | 1026 +++++++++++++++++ src/rules/no-conditional-in-test.ts | 48 + 6 files changed, 1156 insertions(+), 1 deletion(-) create mode 100644 docs/rules/no-conditional-in-test.md create mode 100644 src/rules/__tests__/no-conditional-in-test.test.ts create mode 100644 src/rules/no-conditional-in-test.ts diff --git a/README.md b/README.md index 317290b12..55834f472 100644 --- a/README.md +++ b/README.md @@ -158,6 +158,7 @@ installations requiring long-term consistency. | [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 | ![recommended][] | | +| [no-conditional-in-test](docs/rules/no-conditional-in-test.md) | Disallow conditional logic in tests | | | | [no-deprecated-functions](docs/rules/no-deprecated-functions.md) | Disallow use of deprecated functions | ![recommended][] | ![fixable][] | | [no-disabled-tests](docs/rules/no-disabled-tests.md) | Disallow disabled tests | ![recommended][] | | | [no-done-callback](docs/rules/no-done-callback.md) | Avoid using a callback in asynchronous tests and hooks | ![recommended][] | ![suggest][] | diff --git a/docs/rules/no-conditional-in-test.md b/docs/rules/no-conditional-in-test.md new file mode 100644 index 000000000..6850cc47f --- /dev/null +++ b/docs/rules/no-conditional-in-test.md @@ -0,0 +1,79 @@ +# Disallow conditional logic in tests (`no-conditional-in-test`) + +Conditional logic in tests is usually an indication that a test is attempting to +cover too much, and not testing the logic it intends to. Each branch of code +executing within a conditional statement will usually be better served by a test +devoted to it. + +## Rule Details + +This rule reports on any use of a conditional statement such as `if`, `switch`, +and ternary expressions. + +Examples of **incorrect** code for this rule: + +```js +it('foo', () => { + if (true) { + doTheThing(); + } +}); + +it('bar', () => { + switch (mode) { + case 'none': + generateNone(); + case 'single': + generateOne(); + case 'multiple': + generateMany(); + } + + expect(fixtures.length).toBeGreaterThan(-1); +}); + +it('baz', async () => { + const promiseValue = () => { + return something instanceof Promise + ? something + : Promise.resolve(something); + }; + + await expect(promiseValue()).resolves.toBe(1); +}); +``` + +Examples of **correct** code for this rule: + +```js +describe('my tests', () => { + if (true) { + it('foo', () => { + doTheThing(); + }); + } +}); + +beforeEach(() => { + switch (mode) { + case 'none': + generateNone(); + case 'single': + generateOne(); + case 'multiple': + generateMany(); + } +}); + +it('bar', () => { + expect(fixtures.length).toBeGreaterThan(-1); +}); + +const promiseValue = something => { + return something instanceof Promise ? something : Promise.resolve(something); +}; + +it('baz', async () => { + await expect(promiseValue()).resolves.toBe(1); +}); +``` diff --git a/src/__tests__/__snapshots__/rules.test.ts.snap b/src/__tests__/__snapshots__/rules.test.ts.snap index 3adcd0db1..19305726b 100644 --- a/src/__tests__/__snapshots__/rules.test.ts.snap +++ b/src/__tests__/__snapshots__/rules.test.ts.snap @@ -16,6 +16,7 @@ Object { "jest/no-alias-methods": "error", "jest/no-commented-out-tests": "error", "jest/no-conditional-expect": "error", + "jest/no-conditional-in-test": "error", "jest/no-deprecated-functions": "error", "jest/no-disabled-tests": "error", "jest/no-done-callback": "error", diff --git a/src/__tests__/rules.test.ts b/src/__tests__/rules.test.ts index 6a8ff2df1..6703e7a13 100644 --- a/src/__tests__/rules.test.ts +++ b/src/__tests__/rules.test.ts @@ -2,7 +2,7 @@ import { existsSync } from 'fs'; import { resolve } from 'path'; import plugin from '../'; -const numberOfRules = 45; +const numberOfRules = 46; const ruleNames = Object.keys(plugin.rules); const deprecatedRules = Object.entries(plugin.rules) .filter(([, rule]) => rule.meta.deprecated) diff --git a/src/rules/__tests__/no-conditional-in-test.test.ts b/src/rules/__tests__/no-conditional-in-test.test.ts new file mode 100644 index 000000000..0afee2bb8 --- /dev/null +++ b/src/rules/__tests__/no-conditional-in-test.test.ts @@ -0,0 +1,1026 @@ +import { TSESLint } from '@typescript-eslint/utils'; +import dedent from 'dedent'; +import rule from '../no-conditional-in-test'; +import { espreeParser } from './test-utils'; + +const ruleTester = new TSESLint.RuleTester({ + parser: espreeParser, + parserOptions: { + ecmaVersion: 2015, + }, +}); + +ruleTester.run('conditional expressions', rule, { + valid: [ + 'const x = y ? 1 : 0', + dedent` + const foo = function (bar) { + return foo ? bar : null; + }; + + it('foo', () => { + foo(); + }); + `, + dedent` + const foo = function (bar) { + return foo ? bar : null; + }; + + it.each()('foo', function () { + foo(); + }); + `, + ], + invalid: [ + { + code: dedent` + it('foo', () => { + expect(bar ? foo : baz).toBe(boo); + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 10, + line: 2, + }, + ], + }, + { + code: dedent` + it('foo', function () { + const foo = function (bar) { + return foo ? bar : null; + }; + }); + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 12, + line: 3, + }, + ], + }, + { + code: dedent` + it('foo', () => { + const foo = bar ? foo : baz; + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 15, + line: 2, + }, + ], + }, + { + code: dedent` + it('foo', () => { + const foo = bar ? foo : baz; + }) + const foo = bar ? foo : baz; + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 15, + line: 2, + }, + ], + }, + { + code: dedent` + it('foo', () => { + const foo = bar ? foo : baz; + const anotherFoo = anotherBar ? anotherFoo : anotherBaz; + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 15, + line: 2, + }, + { + messageId: 'conditionalInTest', + column: 22, + line: 3, + }, + ], + }, + ], +}); + +ruleTester.run('switch statements', rule, { + valid: [ + `it('foo', () => {})`, + dedent` + switch (true) { + case true: {} + } + `, + dedent` + it('foo', () => {}); + function myTest() { + switch ('bar') { + } + } + `, + dedent` + foo('bar', () => { + switch(baz) {} + }) + `, + dedent` + describe('foo', () => { + switch('bar') {} + }) + `, + dedent` + describe.skip('foo', () => { + switch('bar') {} + }) + `, + dedent` + describe.skip.each()('foo', () => { + switch('bar') {} + }) + `, + dedent` + xdescribe('foo', () => { + switch('bar') {} + }) + `, + dedent` + fdescribe('foo', () => { + switch('bar') {} + }) + `, + dedent` + describe('foo', () => { + switch('bar') {} + }) + switch('bar') {} + `, + dedent` + describe('foo', () => { + afterEach(() => { + switch('bar') {} + }); + }); + `, + dedent` + const values = something.map(thing => { + switch (thing.isFoo) { + case true: + return thing.foo; + default: + return thing.bar; + } + }); + + it('valid', () => { + expect(values).toStrictEqual(['foo']); + }); + `, + dedent` + describe('valid', () => { + const values = something.map(thing => { + switch (thing.isFoo) { + case true: + return thing.foo; + default: + return thing.bar; + } + }); + it('still valid', () => { + expect(values).toStrictEqual(['foo']); + }); + }); + `, + ], + invalid: [ + { + code: dedent` + it('is invalid', () => { + const values = something.map(thing => { + switch (thing.isFoo) { + case true: + return thing.foo; + default: + return thing.bar; + } + }); + + expect(values).toStrictEqual(['foo']); + }); + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 5, + line: 3, + }, + ], + }, + { + code: dedent` + it('foo', () => { + switch (true) { + case true: {} + } + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 3, + line: 2, + }, + ], + }, + { + code: dedent` + it('foo', () => { + switch('bar') {} + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 3, + line: 2, + }, + ], + }, + { + code: dedent` + it.skip('foo', () => { + switch('bar') {} + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 3, + line: 2, + }, + ], + }, + { + code: dedent` + it.concurrent.skip('foo', () => { + switch('bar') {} + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 3, + line: 2, + }, + ], + }, + { + code: dedent` + it.only('foo', () => { + switch('bar') {} + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 3, + line: 2, + }, + ], + }, + { + code: dedent` + it.concurrent.only('foo', () => { + switch('bar') {} + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 3, + line: 2, + }, + ], + }, + { + code: dedent` + xit('foo', () => { + switch('bar') {} + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 3, + line: 2, + }, + ], + }, + { + code: dedent` + fit('foo', () => { + switch('bar') {} + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 3, + line: 2, + }, + ], + }, + { + code: dedent` + fit.concurrent('foo', () => { + switch('bar') {} + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 3, + line: 2, + }, + ], + }, + { + code: dedent` + test('foo', () => { + switch('bar') {} + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 3, + line: 2, + }, + ], + }, + { + code: dedent` + test.concurrent.skip('foo', () => { + switch('bar') {} + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 3, + line: 2, + }, + ], + }, + { + code: dedent` + test.concurrent.only('foo', () => { + switch('bar') {} + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 3, + line: 2, + }, + ], + }, + { + code: dedent` + xtest('foo', () => { + switch('bar') {} + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 3, + line: 2, + }, + ], + }, + { + code: dedent` + xtest('foo', function () { + switch('bar') {} + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 3, + line: 2, + }, + ], + }, + { + code: dedent` + describe('foo', () => { + it('bar', () => { + + switch('bar') {} + }) + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 5, + line: 4, + }, + ], + }, + { + code: dedent` + describe('foo', () => { + it('bar', () => { + switch('bar') {} + }) + it('baz', () => { + switch('qux') {} + switch('quux') {} + }) + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 5, + line: 3, + }, + { + messageId: 'conditionalInTest', + column: 5, + line: 6, + }, + { + messageId: 'conditionalInTest', + column: 5, + line: 7, + }, + ], + }, + { + code: dedent` + it('foo', () => { + callExpression() + switch ('bar') {} + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 3, + line: 3, + }, + ], + }, + { + code: dedent` + describe('valid', () => { + describe('still valid', () => { + it('is not valid', () => { + const values = something.map((thing) => { + switch (thing.isFoo) { + case true: + return thing.foo; + default: + return thing.bar; + } + }); + + switch('invalid') { + case true: + expect(values).toStrictEqual(['foo']); + } + }); + }); + }); + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 9, + line: 5, + }, + { + messageId: 'conditionalInTest', + column: 7, + line: 13, + }, + ], + }, + ], +}); + +ruleTester.run('if statements', rule, { + valid: [ + 'if (foo) {}', + "it('foo', () => {})", + 'it("foo", function () {})', + "it('foo', () => {}); function myTest() { if ('bar') {} }", + dedent` + foo('bar', () => { + if (baz) {} + }) + `, + dedent` + describe('foo', () => { + if ('bar') {} + }) + `, + dedent` + describe.skip('foo', () => { + if ('bar') {} + }) + `, + dedent` + xdescribe('foo', () => { + if ('bar') {} + }) + `, + dedent` + fdescribe('foo', () => { + if ('bar') {} + }) + `, + dedent` + describe('foo', () => { + if ('bar') {} + }) + if ('baz') {} + `, + dedent` + describe('foo', () => { + afterEach(() => { + if ('bar') {} + }); + }) + `, + dedent` + describe.each\`\`('foo', () => { + afterEach(() => { + if ('bar') {} + }); + }) + `, + dedent` + describe('foo', () => { + beforeEach(() => { + if ('bar') {} + }); + }) + `, + 'const foo = bar ? foo : baz;', + dedent` + const values = something.map((thing) => { + if (thing.isFoo) { + return thing.foo + } else { + return thing.bar; + } + }); + + describe('valid', () => { + it('still valid', () => { + expect(values).toStrictEqual(['foo']); + }); + }); + `, + dedent` + describe('valid', () => { + const values = something.map((thing) => { + if (thing.isFoo) { + return thing.foo + } else { + return thing.bar; + } + }); + + describe('still valid', () => { + it('really still valid', () => { + expect(values).toStrictEqual(['foo']); + }); + }); + }); + `, + ], + invalid: [ + { + code: dedent` + it('foo', () => { + const foo = function(bar) { + if (bar) { + return 1; + } else { + return 2; + } + }; + }); + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 5, + line: 3, + }, + ], + }, + { + code: dedent` + it('foo', () => { + function foo(bar) { + if (bar) { + return 1; + } else { + return 2; + } + }; + }); + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 5, + line: 3, + }, + ], + }, + { + code: dedent` + it('foo', () => { + if ('bar') {} + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 3, + line: 2, + }, + ], + }, + { + code: dedent` + it.skip('foo', () => { + if ('bar') {} + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 3, + line: 2, + }, + ], + }, + { + code: dedent` + it.skip('foo', function () { + if ('bar') {} + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 3, + line: 2, + }, + ], + }, + { + code: dedent` + it.concurrent.skip('foo', () => { + if ('bar') {} + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 3, + line: 2, + }, + ], + }, + { + code: dedent` + it.only('foo', () => { + if ('bar') {} + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 3, + line: 2, + }, + ], + }, + { + code: dedent` + it.concurrent.only('foo', () => { + if ('bar') {} + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 3, + line: 2, + }, + ], + }, + { + code: dedent` + xit('foo', () => { + if ('bar') {} + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 3, + line: 2, + }, + ], + }, + { + code: dedent` + fit('foo', () => { + if ('bar') {} + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 3, + line: 2, + }, + ], + }, + { + code: dedent` + fit.concurrent('foo', () => { + if ('bar') {} + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 3, + line: 2, + }, + ], + }, + { + code: dedent` + test('foo', () => { + if ('bar') {} + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 3, + line: 2, + }, + ], + }, + { + code: dedent` + test.concurrent.skip('foo', () => { + if ('bar') {} + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 3, + line: 2, + }, + ], + }, + { + code: dedent` + test.concurrent.only('foo', () => { + if ('bar') {} + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 3, + line: 2, + }, + ], + }, + { + code: dedent` + xtest('foo', () => { + if ('bar') {} + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 3, + line: 2, + }, + ], + }, + { + code: dedent` + describe('foo', () => { + it('bar', () => { + if ('bar') {} + }) + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 5, + line: 3, + }, + ], + }, + { + code: dedent` + describe('foo', () => { + it('bar', () => { + if ('bar') {} + }) + it('baz', () => { + if ('qux') {} + if ('quux') {} + }) + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 5, + line: 3, + }, + { + messageId: 'conditionalInTest', + column: 5, + line: 6, + }, + { + messageId: 'conditionalInTest', + column: 5, + line: 7, + }, + ], + }, + { + code: dedent` + it('foo', () => { + callExpression() + if ('bar') {} + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 3, + line: 3, + }, + ], + }, + { + code: dedent` + it.each\`\`('foo', () => { + callExpression() + if ('bar') {} + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 3, + line: 3, + }, + ], + }, + { + code: dedent` + it.each()('foo', () => { + callExpression() + if ('bar') {} + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 3, + line: 3, + }, + ], + }, + { + code: dedent` + it.only.each\`\`('foo', () => { + callExpression() + if ('bar') {} + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 3, + line: 3, + }, + ], + }, + { + code: dedent` + it.only.each()('foo', () => { + callExpression() + if ('bar') {} + }) + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 3, + line: 3, + }, + ], + }, + { + code: dedent` + describe('valid', () => { + describe('still valid', () => { + it('is invalid', () => { + const values = something.map((thing) => { + if (thing.isFoo) { + return thing.foo + } else { + return thing.bar; + } + }); + + if ('invalid') { + expect(values).toStrictEqual(['foo']); + } + }); + }); + }); + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 9, + line: 5, + }, + { + messageId: 'conditionalInTest', + column: 7, + line: 12, + }, + ], + }, + { + code: dedent` + test("shows error", () => { + if (1 === 2) { + expect(true).toBe(false); + } + }); + + test("does not show error", () => { + setTimeout(() => console.log("noop")); + if (1 === 2) { + expect(true).toBe(false); + } + }); + `, + errors: [ + { + messageId: 'conditionalInTest', + column: 3, + line: 2, + }, + { + messageId: 'conditionalInTest', + column: 3, + line: 9, + }, + ], + }, + ], +}); diff --git a/src/rules/no-conditional-in-test.ts b/src/rules/no-conditional-in-test.ts new file mode 100644 index 000000000..3fb06176e --- /dev/null +++ b/src/rules/no-conditional-in-test.ts @@ -0,0 +1,48 @@ +import { TSESTree } from '@typescript-eslint/utils'; +import { createRule, isTestCaseCall } from './utils'; + +export default createRule({ + name: __filename, + meta: { + docs: { + description: 'Disallow conditional logic in tests', + category: 'Best Practices', + recommended: false, + }, + messages: { + conditionalInTest: 'Avoid having conditionals in tests', + }, + type: 'problem', + schema: [], + }, + defaultOptions: [], + create(context) { + let inTestCase = false; + + const maybeReportConditional = (node: TSESTree.Node) => { + if (inTestCase) { + context.report({ + messageId: 'conditionalInTest', + node, + }); + } + }; + + return { + CallExpression(node: TSESTree.CallExpression) { + if (isTestCaseCall(node)) { + inTestCase = true; + } + }, + 'CallExpression:exit'(node) { + if (isTestCaseCall(node)) { + inTestCase = false; + } + }, + IfStatement: maybeReportConditional, + SwitchStatement: maybeReportConditional, + ConditionalExpression: maybeReportConditional, + LogicalExpression: maybeReportConditional, + }; + }, +});