From 6204b311e849b51a0e4705015575139f590ae7a4 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Sun, 10 Oct 2021 22:08:22 +1300 Subject: [PATCH] feat: create `require-hook` rule (#929) --- README.md | 1 + docs/rules/require-hook.md | 149 +++++++++++ .../__snapshots__/rules.test.ts.snap | 1 + src/__tests__/rules.test.ts | 2 +- src/rules/__tests__/require-hook.test.ts | 252 ++++++++++++++++++ src/rules/require-hook.ts | 83 ++++++ 6 files changed, 487 insertions(+), 1 deletion(-) create mode 100644 docs/rules/require-hook.md create mode 100644 src/rules/__tests__/require-hook.test.ts create mode 100644 src/rules/require-hook.ts diff --git a/README.md b/README.md index e23e98b16..c20413cd1 100644 --- a/README.md +++ b/README.md @@ -189,6 +189,7 @@ installations requiring long-term consistency. | [prefer-to-contain](docs/rules/prefer-to-contain.md) | Suggest using `toContain()` | ![style][] | ![fixable][] | | [prefer-to-have-length](docs/rules/prefer-to-have-length.md) | Suggest using `toHaveLength()` | ![style][] | ![fixable][] | | [prefer-todo](docs/rules/prefer-todo.md) | Suggest using `test.todo` | | ![fixable][] | +| [require-hook](docs/rules/require-hook.md) | Require setup and teardown code to be within a hook | | | | [require-to-throw-message](docs/rules/require-to-throw-message.md) | Require a message for `toThrow()` | | | | [require-top-level-describe](docs/rules/require-top-level-describe.md) | Require test cases and hooks to be inside a `describe` block | | | | [valid-describe](docs/rules/valid-describe.md) | Enforce valid `describe()` callback | ![recommended][] | | diff --git a/docs/rules/require-hook.md b/docs/rules/require-hook.md new file mode 100644 index 000000000..473749317 --- /dev/null +++ b/docs/rules/require-hook.md @@ -0,0 +1,149 @@ +# Require setup and teardown code to be within a hook (`require-hook`) + +Often while writing tests you have some setup work that needs to happen before +tests run, and you have some finishing work that needs to happen after tests +run. Jest provides helper functions to handle this. + +It's common when writing tests to need to perform setup work that needs to +happen before tests run, and finishing work after tests run. + +Because Jest executes all `describe` handlers in a test file _before_ it +executes any of the actual tests, it's important to ensure setup and teardown +work is done inside `before*` and `after*` handlers respectively, rather than +inside the `describe` blocks. + +## Rule details + +This rule flags any expression that is either at the toplevel of a test file or +directly within the body of a `describe`, _except_ for the following: + +- `import` statements +- `const` variables +- `let` _declarations_ +- Types +- Calls to the standard Jest globals + +This rule flags any function calls within test files that are directly within +the body of a `describe`, and suggests wrapping them in one of the four +lifecycle hooks. + +Here is a slightly contrived test file showcasing some common cases that would +be flagged: + +```js +import { database, isCity } from '../database'; +import { Logger } from '../../../src/Logger'; +import { loadCities } from '../api'; + +jest.mock('../api'); + +const initializeCityDatabase = () => { + database.addCity('Vienna'); + database.addCity('San Juan'); + database.addCity('Wellington'); +}; + +const clearCityDatabase = () => { + database.clear(); +}; + +initializeCityDatabase(); + +test('that persists cities', () => { + expect(database.cities.length).toHaveLength(3); +}); + +test('city database has Vienna', () => { + expect(isCity('Vienna')).toBeTruthy(); +}); + +test('city database has San Juan', () => { + expect(isCity('San Juan')).toBeTruthy(); +}); + +describe('when loading cities from the api', () => { + let consoleWarnSpy = jest.spyOn(console, 'warn'); + + loadCities.mockResolvedValue(['Wellington', 'London']); + + it('does not duplicate cities', async () => { + await database.loadCities(); + + expect(database.cities).toHaveLength(4); + }); + + it('logs any duplicates', async () => { + await database.loadCities(); + + expect(consoleWarnSpy).toHaveBeenCalledWith( + 'Ignored duplicate cities: Wellington', + ); + }); +}); + +clearCityDatabase(); +``` + +Here is the same slightly contrived test file showcasing the same common cases +but in ways that would be **not** flagged: + +```js +import { database, isCity } from '../database'; +import { Logger } from '../../../src/Logger'; +import { loadCities } from '../api'; + +jest.mock('../api'); + +const initializeCityDatabase = () => { + database.addCity('Vienna'); + database.addCity('San Juan'); + database.addCity('Wellington'); +}; + +const clearCityDatabase = () => { + database.clear(); +}; + +beforeEach(() => { + initializeCityDatabase(); +}); + +test('that persists cities', () => { + expect(database.cities.length).toHaveLength(3); +}); + +test('city database has Vienna', () => { + expect(isCity('Vienna')).toBeTruthy(); +}); + +test('city database has San Juan', () => { + expect(isCity('San Juan')).toBeTruthy(); +}); + +describe('when loading cities from the api', () => { + let consoleWarnSpy; + + beforeEach(() => { + consoleWarnSpy = jest.spyOn(console, 'warn'); + loadCities.mockResolvedValue(['Wellington', 'London']); + }); + + it('does not duplicate cities', async () => { + await database.loadCities(); + + expect(database.cities).toHaveLength(4); + }); + + it('logs any duplicates', async () => { + await database.loadCities(); + + expect(consoleWarnSpy).toHaveBeenCalledWith( + 'Ignored duplicate cities: Wellington', + ); + }); +}); + +afterEach(() => { + clearCityDatabase(); +}); +``` diff --git a/src/__tests__/__snapshots__/rules.test.ts.snap b/src/__tests__/__snapshots__/rules.test.ts.snap index 43896620b..4cfae100c 100644 --- a/src/__tests__/__snapshots__/rules.test.ts.snap +++ b/src/__tests__/__snapshots__/rules.test.ts.snap @@ -47,6 +47,7 @@ Object { "jest/prefer-to-contain": "error", "jest/prefer-to-have-length": "error", "jest/prefer-todo": "error", + "jest/require-hook": "error", "jest/require-to-throw-message": "error", "jest/require-top-level-describe": "error", "jest/unbound-method": "error", diff --git a/src/__tests__/rules.test.ts b/src/__tests__/rules.test.ts index 484887325..826bfb533 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 = 48; +const numberOfRules = 49; const ruleNames = Object.keys(plugin.rules); const deprecatedRules = Object.entries(plugin.rules) .filter(([, rule]) => rule.meta.deprecated) diff --git a/src/rules/__tests__/require-hook.test.ts b/src/rules/__tests__/require-hook.test.ts new file mode 100644 index 000000000..351600207 --- /dev/null +++ b/src/rules/__tests__/require-hook.test.ts @@ -0,0 +1,252 @@ +import { TSESLint } from '@typescript-eslint/experimental-utils'; +import dedent from 'dedent'; +import resolveFrom from 'resolve-from'; +import rule from '../require-hook'; + +const ruleTester = new TSESLint.RuleTester({ + parser: resolveFrom(require.resolve('eslint'), 'espree'), + parserOptions: { + ecmaVersion: 2017, + }, +}); + +ruleTester.run('require-hook', rule, { + valid: [ + 'describe()', + 'describe("just a title")', + dedent` + describe('a test', () => + test('something', () => { + expect(true).toBe(true); + })); + `, + dedent` + test('it', () => { + // + }); + `, + dedent` + const { myFn } = require('../functions'); + + test('myFn', () => { + expect(myFn()).toBe(1); + }); + `, + dedent` + const { myFn } = require('../functions'); + + describe('myFn', () => { + it('returns one', () => { + expect(myFn()).toBe(1); + }); + }); + `, + dedent` + describe('some tests', () => { + it('is true', () => { + expect(true).toBe(true); + }); + }); + `, + dedent` + describe('some tests', () => { + it('is true', () => { + expect(true).toBe(true); + }); + + describe('more tests', () => { + it('is false', () => { + expect(true).toBe(false); + }); + }); + }); + `, + dedent` + describe('some tests', () => { + let consoleLogSpy; + + beforeEach(() => { + consoleLogSpy = jest.spyOn(console, 'log'); + }); + + it('prints a message', () => { + printMessage('hello world'); + + expect(consoleLogSpy).toHaveBeenCalledWith('hello world'); + }); + }); + `, + dedent` + describe('some tests', () => { + beforeEach(() => { + setup(); + }); + }); + `, + dedent` + beforeEach(() => { + initializeCityDatabase(); + }); + + afterEach(() => { + clearCityDatabase(); + }); + + test('city database has Vienna', () => { + expect(isCity('Vienna')).toBeTruthy(); + }); + + test('city database has San Juan', () => { + expect(isCity('San Juan')).toBeTruthy(); + }); + `, + dedent` + describe('cities', () => { + beforeEach(() => { + initializeCityDatabase(); + }); + + test('city database has Vienna', () => { + expect(isCity('Vienna')).toBeTruthy(); + }); + + test('city database has San Juan', () => { + expect(isCity('San Juan')).toBeTruthy(); + }); + + afterEach(() => { + clearCityDatabase(); + }); + }); + `, + ], + invalid: [ + { + code: 'setup();', + errors: [ + { + messageId: 'useHook', + line: 1, + column: 1, + }, + ], + }, + { + code: dedent` + describe('some tests', () => { + setup(); + }); + `, + errors: [ + { + messageId: 'useHook', + line: 2, + column: 3, + }, + ], + }, + { + code: dedent` + describe('some tests', () => { + setup(); + + it('is true', () => { + expect(true).toBe(true); + }); + + describe('more tests', () => { + setup(); + + it('is false', () => { + expect(true).toBe(false); + }); + }); + }); + `, + errors: [ + { + messageId: 'useHook', + line: 2, + column: 3, + }, + { + messageId: 'useHook', + line: 9, + column: 5, + }, + ], + }, + { + code: dedent` + import { database, isCity } from '../database'; + import { loadCities } from '../api'; + + jest.mock('../api'); + + const initializeCityDatabase = () => { + database.addCity('Vienna'); + database.addCity('San Juan'); + database.addCity('Wellington'); + }; + + const clearCityDatabase = () => { + database.clear(); + }; + + initializeCityDatabase(); + + test('that persists cities', () => { + expect(database.cities.length).toHaveLength(3); + }); + + test('city database has Vienna', () => { + expect(isCity('Vienna')).toBeTruthy(); + }); + + test('city database has San Juan', () => { + expect(isCity('San Juan')).toBeTruthy(); + }); + + describe('when loading cities from the api', () => { + let consoleWarnSpy = jest.spyOn(console, 'warn'); + + loadCities.mockResolvedValue(['Wellington', 'London']); + + it('does not duplicate cities', async () => { + await database.loadCities(); + + expect(database.cities).toHaveLength(4); + }); + + it('logs any duplicates', async () => { + await database.loadCities(); + + expect(consoleWarnSpy).toHaveBeenCalledWith( + 'Ignored duplicate cities: Wellington', + ); + }); + }); + + clearCityDatabase(); + `, + parserOptions: { sourceType: 'module' }, + errors: [ + { + messageId: 'useHook', + line: 16, + column: 1, + }, + { + messageId: 'useHook', + line: 33, + column: 3, + }, + { + messageId: 'useHook', + line: 50, + column: 1, + }, + ], + }, + ], +}); diff --git a/src/rules/require-hook.ts b/src/rules/require-hook.ts new file mode 100644 index 000000000..cb8dc06cf --- /dev/null +++ b/src/rules/require-hook.ts @@ -0,0 +1,83 @@ +import { + AST_NODE_TYPES, + TSESTree, +} from '@typescript-eslint/experimental-utils'; +import { + createRule, + getNodeName, + isDescribeCall, + isFunction, + isHook, + isTestCaseCall, +} from './utils'; + +const isJestFnCall = (node: TSESTree.CallExpression): boolean => { + if (isDescribeCall(node) || isTestCaseCall(node) || isHook(node)) { + return true; + } + + return !!getNodeName(node)?.startsWith('jest.'); +}; + +const shouldBeInHook = (node: TSESTree.Node): boolean => { + switch (node.type) { + case AST_NODE_TYPES.ExpressionStatement: + return shouldBeInHook(node.expression); + case AST_NODE_TYPES.CallExpression: + return !isJestFnCall(node); + + default: + return false; + } +}; + +export default createRule({ + name: __filename, + meta: { + docs: { + category: 'Best Practices', + description: 'Require setup and teardown code to be within a hook', + recommended: false, + }, + messages: { + useHook: 'This should be done within a hook', + }, + type: 'suggestion', + schema: [], + }, + defaultOptions: [], + create(context) { + const checkBlockBody = (body: TSESTree.BlockStatement['body']) => { + for (const statement of body) { + if (shouldBeInHook(statement)) { + context.report({ + node: statement, + messageId: 'useHook', + }); + } + } + }; + + return { + Program(program) { + checkBlockBody(program.body); + }, + CallExpression(node) { + if (!isDescribeCall(node) || node.arguments.length < 2) { + return; + } + + const [, testFn] = node.arguments; + + if ( + !isFunction(testFn) || + testFn.body.type !== AST_NODE_TYPES.BlockStatement + ) { + return; + } + + checkBlockBody(testFn.body.body); + }, + }; + }, +});