diff --git a/README.md b/README.md index b370f1a50..9aaa416a4 100644 --- a/README.md +++ b/README.md @@ -116,6 +116,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__` | | | @@ -163,6 +164,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..dd7825c61 --- /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 in to satisfy the typescript type checker when using +a non-null assertion operator (!) would work. + +## 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 defda6016..47a359833 100644 --- a/src/__tests__/rules.test.js +++ b/src/__tests__/rules.test.js @@ -5,7 +5,7 @@ const path = require('path'); const { rules } = require('../'); const ruleNames = Object.keys(rules); -const numberOfRules = 32; +const numberOfRules = 33; 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..01c5c0f35 --- /dev/null +++ b/src/rules/__tests__/no-if.js @@ -0,0 +1,374 @@ +'use strict'; + +const { RuleTester } = require('eslint'); +const rule = require('../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..ee9e46356 --- /dev/null +++ b/src/rules/no-if.js @@ -0,0 +1,93 @@ +'use strict'; + +const { + getDocsUrl, + testCaseNames, + getNodeName, + isTestCase, +} = require('./util'); + +const isTestArrowFunction = node => + node && + node.type === 'ArrowFunctionExpression' && + node.parent.type === 'CallExpression' && + testCaseNames[getNodeName(node.parent.callee)]; + +module.exports = { + 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) { + if (!stack[stack.length - 1]) { + 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 b067abfd5..784662dc3 100644 --- a/src/rules/util.js +++ b/src/rules/util.js @@ -231,4 +231,5 @@ module.exports = { getDocsUrl, scopeHasLocalReference, composeFixers, + testCaseNames, };