From 8ccb62eb53c123f51649c0510d29f2412e035090 Mon Sep 17 00:00:00 2001 From: Matt Seccafien Date: Sat, 6 Jul 2019 03:03:55 -0400 Subject: [PATCH] feat(rules): adds `no-if` rule Update no-if.md --- README.md | 2 + docs/rules/no-if.md | 53 +++++ src/__tests__/rules.test.js | 2 +- src/rules/__tests__/no-if.js | 372 +++++++++++++++++++++++++++++++++++ src/rules/no-if.js | 88 +++++++++ src/rules/util.js | 2 +- 6 files changed, 517 insertions(+), 2 deletions(-) create mode 100644 docs/rules/no-if.md create mode 100644 src/rules/__tests__/no-if.js create mode 100644 src/rules/no-if.js diff --git a/README.md b/README.md index 0e119c6fb..e4ac4b164 100644 --- a/README.md +++ b/README.md @@ -117,6 +117,7 @@ installations requiring long-term consistency. | [no-focused-tests][] | Disallow focused tests | ![recommended][] | | | [no-hooks][] | Disallow setup and teardown hooks | | | | [no-identical-title][] | Disallow identical titles | ![recommended][] | | +| [no-if][] | Disallow conditional logic | | | | [no-jasmine-globals][] | Disallow Jasmine globals | ![recommended][] | ![fixable-yellow][] | | [no-jest-import][] | Disallow importing `jest` | ![recommended][] | | | [no-mocks-import][] | Disallow manually importing from `__mocks__` | | | @@ -165,6 +166,7 @@ https://github.com/dangreenisrael/eslint-plugin-jest-formatting [no-focused-tests]: docs/rules/no-focused-tests.md [no-hooks]: docs/rules/no-hooks.md [no-identical-title]: docs/rules/no-identical-title.md +[no-if]: docs/rules/no-if.md [no-jasmine-globals]: docs/rules/no-jasmine-globals.md [no-jest-import]: docs/rules/no-jest-import.md [no-mocks-import]: docs/rules/no-mocks-import.md diff --git a/docs/rules/no-if.md b/docs/rules/no-if.md new file mode 100644 index 000000000..cdc2d3cb6 --- /dev/null +++ b/docs/rules/no-if.md @@ -0,0 +1,53 @@ +# Disallow conditional logic. (no-if) + +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 an if statement will usually be better served by a test devoted +to it. + +Conditionals are often used to satisfy the typescript type checker. In these +cases, using the non-null assertion operator (!) would be best. + +## Rule Details + +This rule prevents the use of if/ else statements and conditional (ternary) +operations in tests. + +The following patterns are considered warnings: + +```js +it('foo', () => { + if ('bar') { + // an if statement here is invalid + // you are probably testing too much + } +}); + +it('foo', () => { + const bar = foo ? 'bar' : null; +}); +``` + +These patterns would not be considered warnings: + +```js +it('foo', () => { + // only test the 'foo' case +}); + +it('bar', () => { + // test the 'bar' case separately +}); + +it('foo', () => { + function foo(bar) { + // nested functions are valid + return foo ? bar : null; + } +}); +``` + +## When Not To Use It + +If you do not wish to prevent the use of if statements in tests, you can safely +disable this rule. diff --git a/src/__tests__/rules.test.js b/src/__tests__/rules.test.js index be159332f..ef6a0fffd 100644 --- a/src/__tests__/rules.test.js +++ b/src/__tests__/rules.test.js @@ -3,7 +3,7 @@ import { resolve } from 'path'; import { rules } from '../'; const ruleNames = Object.keys(rules); -const numberOfRules = 33; +const numberOfRules = 34; describe('rules', () => { it('should have a corresponding doc for each rule', () => { diff --git a/src/rules/__tests__/no-if.js b/src/rules/__tests__/no-if.js new file mode 100644 index 000000000..680ce8c55 --- /dev/null +++ b/src/rules/__tests__/no-if.js @@ -0,0 +1,372 @@ +import { RuleTester } from 'eslint'; +import rule from '../no-if'; + +const ruleTester = new RuleTester({ + parserOptions: { + ecmaVersion: 6, + }, +}); + +ruleTester.run('no-if', rule, { + valid: [ + { + code: `if(foo) {}`, + }, + { + code: `it('foo', () => {})`, + }, + { + code: `foo('bar', () => { + if(baz) {} + })`, + }, + { + code: `describe('foo', () => { + if('bar') {} + })`, + }, + { + code: `describe.skip('foo', () => { + if('bar') {} + })`, + }, + { + code: `xdescribe('foo', () => { + if('bar') {} + })`, + }, + { + code: `fdescribe('foo', () => { + if('bar') {} + })`, + }, + { + code: `describe('foo', () => { + if('bar') {} + }) + if('baz') {} + `, + }, + { + code: `describe('foo', () => { + afterEach(() => { + if('bar') {} + }); + }) + `, + }, + { + code: `describe('foo', () => { + beforeEach(() => { + if('bar') {} + }); + }) + `, + }, + { + code: `const foo = bar ? foo : baz;`, + }, + { + code: ` + it('valid', () => { + const values = something.map((thing) => { + if (thing.isFoo) { + return thing.foo + } else { + return thing.bar; + } + }); + + expect(values).toStrictEqual(['foo']); + }); + `, + }, + { + code: ` + describe('valid', () => { + it('still valid', () => { + const values = something.map((thing) => { + if (thing.isFoo) { + return thing.foo + } else { + return thing.bar; + } + }); + + expect(values).toStrictEqual(['foo']); + }); + }); + `, + }, + { + code: ` + describe('valid', () => { + describe('still valid', () => { + it('really still valid', () => { + const values = something.map((thing) => { + if (thing.isFoo) { + return thing.foo + } else { + return thing.bar; + } + }); + + expect(values).toStrictEqual(['foo']); + }); + }); + }); + `, + }, + { + code: `it('foo', () => { + const foo = bar(() => qux ? qux() : false); + }); + `, + }, + { + code: `it('foo', () => { + const foo = bar => { + return foo ? bar : null; + }; + });`, + }, + { + code: `it('foo', () => { + const foo = function(bar) { + return foo ? bar : null; + }; + });`, + }, + { + code: `it('foo', () => { + const foo = function(bar) { + if (bar) { + return 1; + } else { + return 2; + } + }; + });`, + }, + { + code: `it('foo', () => { + function foo(bar) { + return foo ? bar : null; + }; + });`, + }, + { + code: `it('foo', () => { + function foo(bar) { + if (bar) { + return 1; + } else { + return 2; + } + }; + });`, + }, + ], + invalid: [ + { + code: `it('foo', () => { + if('bar') {} + })`, + errors: [ + { + messageId: 'noIf', + }, + ], + }, + { + code: `it.skip('foo', () => { + if('bar') {} + })`, + errors: [ + { + messageId: 'noIf', + }, + ], + }, + { + code: `it.only('foo', () => { + if('bar') {} + })`, + errors: [ + { + messageId: 'noIf', + }, + ], + }, + { + code: `xit('foo', () => { + if('bar') {} + })`, + errors: [ + { + messageId: 'noIf', + }, + ], + }, + { + code: `fit('foo', () => { + if('bar') {} + })`, + errors: [ + { + messageId: 'noIf', + }, + ], + }, + { + code: `test('foo', () => { + if('bar') {} + })`, + errors: [ + { + messageId: 'noIf', + }, + ], + }, + { + code: `test.skip('foo', () => { + if('bar') {} + })`, + errors: [ + { + messageId: 'noIf', + }, + ], + }, + { + code: `test.only('foo', () => { + if('bar') {} + })`, + errors: [ + { + messageId: 'noIf', + }, + ], + }, + { + code: `xtest('foo', () => { + if('bar') {} + })`, + errors: [ + { + messageId: 'noIf', + }, + ], + }, + { + code: `describe('foo', () => { + it('bar', () => { + if('bar') {} + }) + })`, + errors: [ + { + messageId: 'noIf', + }, + ], + }, + { + code: `describe('foo', () => { + it('bar', () => { + if('bar') {} + }) + it('baz', () => { + if('qux') {} + if('quux') {} + }) + })`, + errors: [ + { + messageId: 'noIf', + }, + { + messageId: 'noIf', + }, + { + messageId: 'noIf', + }, + ], + }, + { + code: `it('foo', () => { + callExpression() + if ('bar') {} + }) + `, + errors: [ + { + messageId: 'noIf', + }, + ], + }, + { + code: `it('foo', () => { + const foo = bar ? foo : baz; + }) + `, + errors: [ + { + messageId: 'noConditional', + }, + ], + }, + { + code: `it('foo', () => { + const foo = bar ? foo : baz; + }) + const foo = bar ? foo : baz; + `, + errors: [ + { + messageId: 'noConditional', + }, + ], + }, + { + code: `it('foo', () => { + const foo = bar ? foo : baz; + const anotherFoo = anotherBar ? anotherFoo : anotherBaz; + }) + `, + errors: [ + { + messageId: 'noConditional', + }, + { + messageId: 'noConditional', + }, + ], + }, + { + code: ` + describe('valid', () => { + describe('still valid', () => { + it('really still valid', () => { + const values = something.map((thing) => { + if (thing.isFoo) { + return thing.foo + } else { + return thing.bar; + } + }); + + if('invalid') { + expect(values).toStrictEqual(['foo']); + } + }); + }); + }); + `, + errors: [ + { + messageId: 'noIf', + }, + ], + }, + ], +}); diff --git a/src/rules/no-if.js b/src/rules/no-if.js new file mode 100644 index 000000000..7a08d80cb --- /dev/null +++ b/src/rules/no-if.js @@ -0,0 +1,88 @@ +import { getDocsUrl, testCaseNames, getNodeName, isTestCase } from './util'; + +const isTestArrowFunction = node => + node !== undefined && + node.type === 'ArrowFunctionExpression' && + node.parent.type === 'CallExpression' && + testCaseNames.has(getNodeName(node.parent.callee)); + +export default { + meta: { + docs: { + description: 'Disallow conditional logic', + category: 'Best Practices', + recommended: false, + uri: getDocsUrl('jest/no-if'), + }, + messages: { + noIf: [ + 'Tests should not contain if statements.', + 'This is usually an indication that you', + 'are attempting to test too much at once', + 'or not testing what you intend to.', + 'Consider breaking the if statement out', + 'into a separate test to resolve this error.', + ].join(' '), + noConditional: [ + 'Tests should not contain conditional statements.', + 'This is usually an indication that you', + 'are attempting to test too much at once', + 'or not testing what you intend to.', + 'Consider writing a separate test for', + 'each fork in the conditional statement.', + 'If your conditionals are required to', + 'satisfy the typescript type checker, consider', + 'using a non-null assertion operator (!) instead.', + ].join(' '), + }, + }, + + create(context) { + const stack = []; + + function validate(node) { + const lastElementInStack = stack[stack.length - 1]; + + if (stack.length === 0 || lastElementInStack === false) { + return; + } + + const messageId = + node.type === 'ConditionalExpression' ? 'noConditional' : 'noIf'; + + context.report({ + messageId, + node, + }); + } + + return { + CallExpression(node) { + stack.push(isTestCase(node)); + }, + FunctionExpression() { + stack.push(false); + }, + FunctionDeclaration() { + stack.push(false); + }, + ArrowFunctionExpression(node) { + stack.push(isTestArrowFunction(node)); + }, + IfStatement: validate, + ConditionalExpression: validate, + 'CallExpression:exit'() { + stack.pop(); + }, + 'FunctionExpression:exit'() { + stack.pop(); + }, + 'FunctionDeclaration:exit'() { + stack.pop(); + }, + 'ArrowFunctionExpression:exit'() { + stack.pop(); + }, + }; + }, +}; diff --git a/src/rules/util.js b/src/rules/util.js index 11aa18a34..4d1eaecd4 100644 --- a/src/rules/util.js +++ b/src/rules/util.js @@ -85,7 +85,7 @@ const describeAliases = new Set([ 'xdescribe', ]); -const testCaseNames = new Set([ +export const testCaseNames = new Set([ 'fit', 'it', 'it.only',