From 574eaed9fafcdc4ed5624451f792c8951eb49f0a Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Sun, 28 Aug 2022 21:44:43 +1200 Subject: [PATCH] feat: create `prefer-each` rule (#1222) --- README.md | 1 + docs/rules/prefer-each.md | 54 +++ .../__snapshots__/rules.test.ts.snap | 1 + src/__tests__/rules.test.ts | 2 +- src/rules/__tests__/prefer-each.test.ts | 310 ++++++++++++++++++ src/rules/prefer-each.ts | 88 +++++ src/rules/utils/parseJestFnCall.ts | 8 +- 7 files changed, 462 insertions(+), 2 deletions(-) create mode 100644 docs/rules/prefer-each.md create mode 100644 src/rules/__tests__/prefer-each.test.ts create mode 100644 src/rules/prefer-each.ts diff --git a/README.md b/README.md index 42a578a3c..0d12ec50e 100644 --- a/README.md +++ b/README.md @@ -226,6 +226,7 @@ installations requiring long-term consistency. | [no-test-return-statement](docs/rules/no-test-return-statement.md) | Disallow explicitly returning from tests | | | | [prefer-called-with](docs/rules/prefer-called-with.md) | Suggest using `toBeCalledWith()` or `toHaveBeenCalledWith()` | | | | [prefer-comparison-matcher](docs/rules/prefer-comparison-matcher.md) | Suggest using the built-in comparison matchers | | ![fixable][] | +| [prefer-each](docs/rules/prefer-each.md) | Prefer using `.each` rather than manual loops | | | | [prefer-equality-matcher](docs/rules/prefer-equality-matcher.md) | Suggest using the built-in equality matchers | | ![suggest][] | | [prefer-expect-assertions](docs/rules/prefer-expect-assertions.md) | Suggest using `expect.assertions()` OR `expect.hasAssertions()` | | ![suggest][] | | [prefer-expect-resolves](docs/rules/prefer-expect-resolves.md) | Prefer `await expect(...).resolves` over `expect(await ...)` syntax | | ![fixable][] | diff --git a/docs/rules/prefer-each.md b/docs/rules/prefer-each.md new file mode 100644 index 000000000..cb995ba00 --- /dev/null +++ b/docs/rules/prefer-each.md @@ -0,0 +1,54 @@ +# Prefer using `.each` rather than manual loops (`prefer-each`) + +Reports where you might be able to use `.each` instead of native loops. + +## Rule details + +This rule triggers a warning if you use test case functions like `describe`, +`test`, and `it`, in a native loop - generally you should be able to use `.each` +instead which gives better output and makes it easier to run specific cases. + +Examples of **incorrect** code for this rule: + +```js +for (const number of getNumbers()) { + it('is greater than five', function () { + expect(number).toBeGreaterThan(5); + }); +} + +for (const [input, expected] of data) { + beforeEach(() => setupSomething(input)); + + test(`results in ${expected}`, () => { + expect(doSomething()).toBe(expected); + }); +} +``` + +Examples of **correct** code for this rule: + +```js +it.each(getNumbers())( + 'only returns numbers that are greater than seven', + number => { + expect(number).toBeGreaterThan(7); + }, +); + +describe.each(data)('when input is %s', ([input, expected]) => { + beforeEach(() => setupSomething(input)); + + test(`results in ${expected}`, () => { + expect(doSomething()).toBe(expected); + }); +}); + +// we don't warn on loops _in_ test functions because those typically involve +// complex setup that is better done in the test function itself +it('returns numbers that are greater than five', () => { + for (const number of getNumbers()) { + expect(number).toBeGreaterThan(5); + } +}); +``` diff --git a/src/__tests__/__snapshots__/rules.test.ts.snap b/src/__tests__/__snapshots__/rules.test.ts.snap index ab02a0366..9ce2dc5d4 100644 --- a/src/__tests__/__snapshots__/rules.test.ts.snap +++ b/src/__tests__/__snapshots__/rules.test.ts.snap @@ -37,6 +37,7 @@ Object { "jest/no-test-return-statement": "error", "jest/prefer-called-with": "error", "jest/prefer-comparison-matcher": "error", + "jest/prefer-each": "error", "jest/prefer-equality-matcher": "error", "jest/prefer-expect-assertions": "error", "jest/prefer-expect-resolves": "error", diff --git a/src/__tests__/rules.test.ts b/src/__tests__/rules.test.ts index a8c66cc18..86fdf0a38 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 = 50; +const numberOfRules = 51; const ruleNames = Object.keys(plugin.rules); const deprecatedRules = Object.entries(plugin.rules) .filter(([, rule]) => rule.meta.deprecated) diff --git a/src/rules/__tests__/prefer-each.test.ts b/src/rules/__tests__/prefer-each.test.ts new file mode 100644 index 000000000..d001ccb0c --- /dev/null +++ b/src/rules/__tests__/prefer-each.test.ts @@ -0,0 +1,310 @@ +import { TSESLint } from '@typescript-eslint/utils'; +import dedent from 'dedent'; +import rule from '../prefer-each'; +import { espreeParser } from './test-utils'; + +const ruleTester = new TSESLint.RuleTester({ + parser: espreeParser, + parserOptions: { + ecmaVersion: 2015, + }, +}); + +ruleTester.run('prefer-each', rule, { + valid: [ + 'it("is true", () => { expect(true).toBe(false) });', + dedent` + it.each(getNumbers())("only returns numbers that are greater than seven", number => { + expect(number).toBeGreaterThan(7); + }); + `, + // while these cases could be done with .each, it's reasonable to have more + // complex cases that would not look good in .each, so we consider this valid + dedent` + it("returns numbers that are greater than five", function () { + for (const number of getNumbers()) { + expect(number).toBeGreaterThan(5); + } + }); + `, + dedent` + it("returns things that are less than ten", function () { + for (const thing in things) { + expect(thing).toBeLessThan(10); + } + }); + `, + dedent` + it("only returns numbers that are greater than seven", function () { + const numbers = getNumbers(); + + for (let i = 0; i < numbers.length; i++) { + expect(numbers[i]).toBeGreaterThan(7); + } + }); + `, + ], + invalid: [ + { + code: dedent` + for (const [input, expected] of data) { + it(\`results in $\{expected}\`, () => { + expect(fn(input)).toBe(expected) + }); + } + `, + errors: [ + { + data: { fn: 'it' }, + messageId: 'preferEach', + }, + ], + }, + { + code: dedent` + for (const [input, expected] of data) { + describe(\`when the input is $\{input}\`, () => { + it(\`results in $\{expected}\`, () => { + expect(fn(input)).toBe(expected) + }); + }); + } + `, + errors: [ + { + data: { fn: 'describe' }, + messageId: 'preferEach', + }, + ], + }, + { + code: dedent` + for (const [input, expected] of data) { + describe(\`when the input is $\{input}\`, () => { + it(\`results in $\{expected}\`, () => { + expect(fn(input)).toBe(expected) + }); + }); + } + + for (const [input, expected] of data) { + it.skip(\`results in $\{expected}\`, () => { + expect(fn(input)).toBe(expected) + }); + } + `, + errors: [ + { + data: { fn: 'describe' }, + messageId: 'preferEach', + }, + { + data: { fn: 'it' }, + messageId: 'preferEach', + }, + ], + }, + { + code: dedent` + for (const [input, expected] of data) { + it.skip(\`results in $\{expected}\`, () => { + expect(fn(input)).toBe(expected) + }); + } + `, + errors: [ + { + data: { fn: 'it' }, + messageId: 'preferEach', + }, + ], + }, + { + code: dedent` + it('is true', () => { + expect(true).toBe(false); + }); + + for (const [input, expected] of data) { + it.skip(\`results in $\{expected}\`, () => { + expect(fn(input)).toBe(expected) + }); + } + `, + errors: [ + { + data: { fn: 'it' }, + messageId: 'preferEach', + }, + ], + }, + { + code: dedent` + for (const [input, expected] of data) { + it.skip(\`results in $\{expected}\`, () => { + expect(fn(input)).toBe(expected) + }); + } + + it('is true', () => { + expect(true).toBe(false); + }); + `, + errors: [ + { + data: { fn: 'it' }, + messageId: 'preferEach', + }, + ], + }, + { + code: dedent` + it('is true', () => { + expect(true).toBe(false); + }); + + for (const [input, expected] of data) { + it.skip(\`results in $\{expected}\`, () => { + expect(fn(input)).toBe(expected) + }); + } + + it('is true', () => { + expect(true).toBe(false); + }); + `, + errors: [ + { + data: { fn: 'it' }, + messageId: 'preferEach', + }, + ], + }, + { + code: dedent` + for (const [input, expected] of data) { + it(\`results in $\{expected}\`, () => { + expect(fn(input)).toBe(expected) + }); + + it(\`results in $\{expected}\`, () => { + expect(fn(input)).toBe(expected) + }); + } + `, + errors: [ + { + data: { fn: 'describe' }, + messageId: 'preferEach', + }, + ], + }, + { + code: dedent` + for (const [input, expected] of data) { + it(\`results in $\{expected}\`, () => { + expect(fn(input)).toBe(expected) + }); + } + + for (const [input, expected] of data) { + it(\`results in $\{expected}\`, () => { + expect(fn(input)).toBe(expected) + }); + } + `, + errors: [ + { + data: { fn: 'it' }, + messageId: 'preferEach', + }, + { + data: { fn: 'it' }, + messageId: 'preferEach', + }, + ], + }, + { + code: dedent` + for (const [input, expected] of data) { + it(\`results in $\{expected}\`, () => { + expect(fn(input)).toBe(expected) + }); + } + + for (const [input, expected] of data) { + test(\`results in $\{expected}\`, () => { + expect(fn(input)).toBe(expected) + }); + } + `, + errors: [ + { + data: { fn: 'it' }, + messageId: 'preferEach', + }, + { + data: { fn: 'it' }, + messageId: 'preferEach', + }, + ], + }, + { + code: dedent` + for (const [input, expected] of data) { + beforeEach(() => setupSomething(input)); + + test(\`results in $\{expected}\`, () => { + expect(doSomething()).toBe(expected) + }); + } + `, + errors: [ + { + data: { fn: 'describe' }, + messageId: 'preferEach', + }, + ], + }, + { + code: dedent` + for (const [input, expected] of data) { + it("only returns numbers that are greater than seven", function () { + const numbers = getNumbers(input); + + for (let i = 0; i < numbers.length; i++) { + expect(numbers[i]).toBeGreaterThan(7); + } + }); + } + `, + errors: [ + { + data: { fn: 'it' }, + messageId: 'preferEach', + }, + ], + }, + { + code: dedent` + for (const [input, expected] of data) { + beforeEach(() => setupSomething(input)); + + it("only returns numbers that are greater than seven", function () { + const numbers = getNumbers(); + + for (let i = 0; i < numbers.length; i++) { + expect(numbers[i]).toBeGreaterThan(7); + } + }); + } + `, + errors: [ + { + data: { fn: 'describe' }, + messageId: 'preferEach', + }, + ], + }, + ], +}); diff --git a/src/rules/prefer-each.ts b/src/rules/prefer-each.ts new file mode 100644 index 000000000..975869e1a --- /dev/null +++ b/src/rules/prefer-each.ts @@ -0,0 +1,88 @@ +import { TSESTree } from '@typescript-eslint/utils'; +import { JestFnType, createRule, parseJestFnCall } from './utils'; + +export default createRule({ + name: __filename, + meta: { + docs: { + category: 'Best Practices', + description: 'Prefer using `.each` rather than manual loops', + recommended: false, + }, + messages: { + preferEach: 'prefer using `{{ fn }}.each` rather than a manual loop', + }, + type: 'suggestion', + schema: [], + }, + defaultOptions: [], + create(context) { + const jestFnCalls: JestFnType[] = []; + let inTestCaseCall = false; + + const recommendFn = () => { + if (jestFnCalls.length === 1 && jestFnCalls[0] === 'test') { + return 'it'; + } + + return 'describe'; + }; + + const enterForLoop = () => { + if (jestFnCalls.length === 0 || inTestCaseCall) { + return; + } + + jestFnCalls.length = 0; + }; + const exitForLoop = ( + node: + | TSESTree.ForInStatement + | TSESTree.ForOfStatement + | TSESTree.ForStatement, + ) => { + if (jestFnCalls.length === 0 || inTestCaseCall) { + return; + } + + context.report({ + node, + messageId: 'preferEach', + data: { fn: recommendFn() }, + }); + + jestFnCalls.length = 0; + }; + + return { + ForStatement: enterForLoop, + 'ForStatement:exit': exitForLoop, + ForInStatement: enterForLoop, + 'ForInStatement:exit': exitForLoop, + ForOfStatement: enterForLoop, + 'ForOfStatement:exit': exitForLoop, + CallExpression(node) { + const { type: jestFnCallType } = parseJestFnCall(node, context) ?? {}; + + if ( + jestFnCallType === 'hook' || + jestFnCallType === 'describe' || + jestFnCallType === 'test' + ) { + jestFnCalls.push(jestFnCallType); + } + + if (jestFnCallType === 'test') { + inTestCaseCall = true; + } + }, + 'CallExpression:exit'(node) { + const { type: jestFnCallType } = parseJestFnCall(node, context) ?? {}; + + if (jestFnCallType === 'test') { + inTestCaseCall = false; + } + }, + }; + }, +}); diff --git a/src/rules/utils/parseJestFnCall.ts b/src/rules/utils/parseJestFnCall.ts index ea1e628bc..0c75211ab 100644 --- a/src/rules/utils/parseJestFnCall.ts +++ b/src/rules/utils/parseJestFnCall.ts @@ -50,7 +50,13 @@ export interface ResolvedJestFnWithNode extends ResolvedJestFn { node: AccessorNode; } -type JestFnType = 'hook' | 'describe' | 'test' | 'expect' | 'jest' | 'unknown'; +export type JestFnType = + | 'hook' + | 'describe' + | 'test' + | 'expect' + | 'jest' + | 'unknown'; const determineJestFnType = (name: string): JestFnType => { if (name === 'expect') {