From dd4bbaf5ea03704f6c04e8eac4ff49d01b4a58df Mon Sep 17 00:00:00 2001 From: Mark Date: Mon, 15 Jul 2019 16:41:49 +0530 Subject: [PATCH] feat(rules): no-duplicate-hooks (#298) Fixes #231 --- README.md | 2 + docs/rules/no-duplicate-hooks.md | 75 ++++ src/__tests__/rules.test.js | 2 +- .../__tests__/no-duplicate-hooks.test.js | 326 ++++++++++++++++++ src/rules/no-duplicate-hooks.js | 48 +++ src/rules/no-focused-tests.js | 12 +- src/rules/no-hooks.js | 10 +- src/rules/util.js | 55 +-- 8 files changed, 492 insertions(+), 38 deletions(-) create mode 100644 docs/rules/no-duplicate-hooks.md create mode 100644 src/rules/__tests__/no-duplicate-hooks.test.js create mode 100644 src/rules/no-duplicate-hooks.js diff --git a/README.md b/README.md index c9378304e..e7d30449a 100644 --- a/README.md +++ b/README.md @@ -112,6 +112,7 @@ installations requiring long-term consistency. | [no-alias-methods][] | Disallow alias methods | ![recommended][] | ![fixable-green][] | | [no-disabled-tests][] | Disallow disabled tests | ![recommended][] | | | [no-commented-out-tests][] | Disallow commented out tests | | | +| [no-duplicate-hooks][] | Disallow duplicate hooks withing a `describe` block | | | | [no-empty-title][] | Disallow empty titles | | | | [no-focused-tests][] | Disallow focused tests | ![recommended][] | | | [no-hooks][] | Disallow setup and teardown hooks | | | @@ -158,6 +159,7 @@ https://github.com/dangreenisrael/eslint-plugin-jest-formatting [lowercase-name]: docs/rules/lowercase-name.md [no-alias-methods]: docs/rules/no-alias-methods.md [no-disabled-tests]: docs/rules/no-disabled-tests.md +[no-duplicate-hooks]: docs/rules/no-duplicate-hooks.md [no-commented-out-tests]: docs/rules/no-commented-out-tests.md [no-empty-title]: docs/rules/no-empty-title.md [no-focused-tests]: docs/rules/no-focused-tests.md diff --git a/docs/rules/no-duplicate-hooks.md b/docs/rules/no-duplicate-hooks.md new file mode 100644 index 000000000..c11388e12 --- /dev/null +++ b/docs/rules/no-duplicate-hooks.md @@ -0,0 +1,75 @@ +# Disallow duplicate setup and teardown hooks (no-duplicate-hooks) + +A describe block should not contain duplicate hooks. + +## Rule Details + +Examples of **incorrect** code for this rule + +```js +/* eslint jest/no-duplicate-hooks: "error" */ + +describe('foo', () => { + beforeEach(() => { + // some setup + }); + beforeEach(() => { + // some setup + }); + test('foo_test', () => { + // some test + }); +}); + +// Nested describe scenario +describe('foo', () => { + beforeEach(() => { + // some setup + }); + test('foo_test', () => { + // some test + }); + describe('bar', () => { + test('bar_test', () => { + afterAll(() => { + // some teardown + }); + afterAll(() => { + // some teardown + }); + }); + }); +}); +``` + +Examples of **correct** code for this rule + +```js +/* eslint jest/no-duplicate-hooks: "error" */ + +describe('foo', () => { + beforeEach(() => { + // some setup + }); + test('foo_test', () => { + // some test + }); +}); + +// Nested describe scenario +describe('foo', () => { + beforeEach(() => { + // some setup + }); + test('foo_test', () => { + // some test + }); + describe('bar', () => { + test('bar_test', () => { + beforeEach(() => { + // some setup + }); + }); + }); +}); +``` 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-duplicate-hooks.test.js b/src/rules/__tests__/no-duplicate-hooks.test.js new file mode 100644 index 000000000..bb7fe76dd --- /dev/null +++ b/src/rules/__tests__/no-duplicate-hooks.test.js @@ -0,0 +1,326 @@ +'use strict'; + +const { RuleTester } = require('eslint'); +const rule = require('../no-duplicate-hooks'); + +const ruleTester = new RuleTester({ + parserOptions: { + ecmaVersion: 6, + }, +}); + +ruleTester.run('basic describe block', rule, { + valid: [ + [ + 'describe("foo", () => {', + ' beforeEach(() => {', + ' }),', + ' test("bar", () => {', + ' some_fn();', + ' })', + '})', + ].join('\n'), + [ + 'beforeEach(() => {', + '}),', + 'test("bar", () => {', + ' some_fn();', + '})', + ].join('\n'), + [ + 'describe("foo", () => {', + ' beforeAll(() => {', + ' }),', + ' beforeEach(() => {', + ' }),', + ' afterEach(() => {', + ' }),', + ' afterAll(() => {', + ' }),', + ' test("bar", () => {', + ' some_fn();', + ' })', + '})', + ].join('\n'), + ], + + invalid: [ + { + code: [ + 'describe("foo", () => {', + ' beforeEach(() => {', + ' }),', + ' beforeEach(() => {', + ' }),', + ' test("bar", () => {', + ' some_fn();', + ' })', + '})', + ].join('\n'), + errors: [ + { + messageId: 'noDuplicateHook', + data: { hook: 'beforeEach' }, + column: 3, + line: 4, + }, + ], + }, + { + code: [ + 'describe.skip("foo", () => {', + ' beforeEach(() => {', + ' }),', + ' beforeAll(() => {', + ' }),', + ' beforeAll(() => {', + ' }),', + ' test("bar", () => {', + ' some_fn();', + ' })', + '})', + ].join('\n'), + errors: [ + { + messageId: 'noDuplicateHook', + data: { hook: 'beforeAll' }, + column: 3, + line: 6, + }, + ], + }, + { + code: [ + 'describe.skip("foo", () => {', + ' afterEach(() => {', + ' }),', + ' afterEach(() => {', + ' }),', + ' test("bar", () => {', + ' some_fn();', + ' })', + '})', + ].join('\n'), + errors: [ + { + messageId: 'noDuplicateHook', + data: { hook: 'afterEach' }, + column: 3, + line: 4, + }, + ], + }, + { + code: [ + 'describe.skip("foo", () => {', + ' afterAll(() => {', + ' }),', + ' afterAll(() => {', + ' }),', + ' test("bar", () => {', + ' some_fn();', + ' })', + '})', + ].join('\n'), + errors: [ + { + messageId: 'noDuplicateHook', + data: { hook: 'afterAll' }, + column: 3, + line: 4, + }, + ], + }, + { + code: [ + 'afterAll(() => {', + '}),', + 'afterAll(() => {', + '}),', + 'test("bar", () => {', + ' some_fn();', + '})', + ].join('\n'), + errors: [ + { + messageId: 'noDuplicateHook', + data: { hook: 'afterAll' }, + column: 1, + line: 3, + }, + ], + }, + { + code: [ + 'describe("foo", () => {', + ' beforeEach(() => {', + ' }),', + ' beforeEach(() => {', + ' }),', + ' beforeEach(() => {', + ' }),', + ' test("bar", () => {', + ' some_fn();', + ' })', + '})', + ].join('\n'), + errors: [ + { + messageId: 'noDuplicateHook', + data: { hook: 'beforeEach' }, + column: 3, + line: 4, + }, + { + messageId: 'noDuplicateHook', + data: { hook: 'beforeEach' }, + column: 3, + line: 6, + }, + ], + }, + { + code: [ + 'describe.skip("foo", () => {', + ' afterAll(() => {', + ' }),', + ' afterAll(() => {', + ' }),', + ' beforeAll(() => {', + ' }),', + ' beforeAll(() => {', + ' }),', + ' test("bar", () => {', + ' some_fn();', + ' })', + '})', + ].join('\n'), + errors: [ + { + messageId: 'noDuplicateHook', + data: { hook: 'afterAll' }, + column: 3, + line: 4, + }, + { + messageId: 'noDuplicateHook', + data: { hook: 'beforeAll' }, + column: 3, + line: 8, + }, + ], + }, + ], +}); + +ruleTester.run('multiple describe blocks', rule, { + valid: [ + [ + 'describe.skip("foo", () => {', + ' beforeEach(() => {', + ' }),', + ' beforeAll(() => {', + ' }),', + ' test("bar", () => {', + ' some_fn();', + ' })', + '})', + 'describe("foo", () => {', + ' beforeEach(() => {', + ' }),', + ' beforeAll(() => {', + ' }),', + ' test("bar", () => {', + ' some_fn();', + ' })', + '})', + ].join('\n'), + ], + + invalid: [ + { + code: [ + 'describe.skip("foo", () => {', + ' beforeEach(() => {', + ' }),', + ' beforeAll(() => {', + ' }),', + ' test("bar", () => {', + ' some_fn();', + ' })', + '})', + 'describe("foo", () => {', + ' beforeEach(() => {', + ' }),', + ' beforeEach(() => {', + ' }),', + ' beforeAll(() => {', + ' }),', + ' test("bar", () => {', + ' some_fn();', + ' })', + '})', + ].join('\n'), + errors: [ + { + messageId: 'noDuplicateHook', + data: { hook: 'beforeEach' }, + column: 3, + line: 13, + }, + ], + }, + ], +}); + +ruleTester.run('nested describe blocks', rule, { + valid: [ + [ + 'describe("foo", () => {', + ' beforeEach(() => {', + ' }),', + ' test("bar", () => {', + ' some_fn();', + ' })', + ' describe("inner_foo", () => {', + ' beforeEach(() => {', + ' })', + ' test("inner bar", () => {', + ' some_fn();', + ' })', + ' })', + '})', + ].join('\n'), + ], + + invalid: [ + { + code: [ + 'describe("foo", () => {', + ' beforeAll(() => {', + ' }),', + ' test("bar", () => {', + ' some_fn();', + ' })', + ' describe("inner_foo", () => {', + ' beforeEach(() => {', + ' })', + ' beforeEach(() => {', + ' })', + ' test("inner bar", () => {', + ' some_fn();', + ' })', + ' })', + '})', + ].join('\n'), + errors: [ + { + messageId: 'noDuplicateHook', + data: { hook: 'beforeEach' }, + column: 5, + line: 10, + }, + ], + }, + ], +}); diff --git a/src/rules/no-duplicate-hooks.js b/src/rules/no-duplicate-hooks.js new file mode 100644 index 000000000..2bdf04007 --- /dev/null +++ b/src/rules/no-duplicate-hooks.js @@ -0,0 +1,48 @@ +'use strict'; + +const { getDocsUrl, isDescribe, isHook } = require('./util'); + +const newHookContext = () => ({ + beforeAll: 0, + beforeEach: 0, + afterAll: 0, + afterEach: 0, +}); + +module.exports = { + meta: { + docs: { + url: getDocsUrl(__filename), + }, + messages: { + noDuplicateHook: 'Duplicate {{hook}} in describe block', + }, + }, + create(context) { + const hookContexts = [newHookContext()]; + return { + CallExpression(node) { + if (isDescribe(node)) { + hookContexts.push(newHookContext()); + } + + if (isHook(node)) { + const currentLayer = hookContexts[hookContexts.length - 1]; + currentLayer[node.callee.name] += 1; + if (currentLayer[node.callee.name] > 1) { + context.report({ + messageId: 'noDuplicateHook', + data: { hook: node.callee.name }, + node, + }); + } + } + }, + 'CallExpression:exit'(node) { + if (isDescribe(node)) { + hookContexts.pop(); + } + }, + }; + }, +}; diff --git a/src/rules/no-focused-tests.js b/src/rules/no-focused-tests.js index cd41a093d..c3fadf6f7 100644 --- a/src/rules/no-focused-tests.js +++ b/src/rules/no-focused-tests.js @@ -2,16 +2,14 @@ const { getDocsUrl } = require('./util'); -const testFunctions = Object.assign(Object.create(null), { - describe: true, - it: true, - test: true, -}); +const testFunctions = new Set(['describe', 'it', 'test']); -const matchesTestFunction = object => object && testFunctions[object.name]; +const matchesTestFunction = object => object && testFunctions.has(object.name); const isCallToFocusedTestFunction = object => - object && object.name[0] === 'f' && testFunctions[object.name.substring(1)]; + object && + object.name[0] === 'f' && + testFunctions.has(object.name.substring(1)); const isPropertyNamedOnly = property => property && (property.name === 'only' || property.value === 'only'); diff --git a/src/rules/no-hooks.js b/src/rules/no-hooks.js index 06a0c2214..f73f7af86 100644 --- a/src/rules/no-hooks.js +++ b/src/rules/no-hooks.js @@ -1,6 +1,6 @@ 'use strict'; -const { getDocsUrl } = require('./util'); +const { getDocsUrl, isHook } = require('./util'); module.exports = { meta: { @@ -24,13 +24,6 @@ module.exports = { }, ], create(context) { - const testHookNames = Object.assign(Object.create(null), { - beforeAll: true, - beforeEach: true, - afterAll: true, - afterEach: true, - }); - const whitelistedHookNames = ( context.options[0] || { allow: [] } ).allow.reduce((hashMap, value) => { @@ -38,7 +31,6 @@ module.exports = { return hashMap; }, Object.create(null)); - const isHook = node => testHookNames[node.callee.name]; const isWhitelisted = node => whitelistedHookNames[node.callee.name]; return { diff --git a/src/rules/util.js b/src/rules/util.js index b067abfd5..53f214a3d 100644 --- a/src/rules/util.js +++ b/src/rules/util.js @@ -79,25 +79,32 @@ const argument = node => const argument2 = node => node.parent.parent.parent.arguments && node.parent.parent.parent.arguments[0]; -const describeAliases = Object.assign(Object.create(null), { - describe: true, - 'describe.only': true, - 'describe.skip': true, - fdescribe: true, - xdescribe: true, -}); - -const testCaseNames = Object.assign(Object.create(null), { - fit: true, - it: true, - 'it.only': true, - 'it.skip': true, - test: true, - 'test.only': true, - 'test.skip': true, - xit: true, - xtest: true, -}); +const describeAliases = new Set([ + 'describe', + 'describe.only', + 'describe.skip', + 'fdescribe', + 'xdescribe', +]); + +const testCaseNames = new Set([ + 'fit', + 'it', + 'it.only', + 'it.skip', + 'test', + 'test.only', + 'test.skip', + 'xit', + 'xtest', +]); + +const testHookNames = new Set([ + 'beforeAll', + 'beforeEach', + 'afterAll', + 'afterEach', +]); const getNodeName = node => { function joinNames(a, b) { @@ -119,15 +126,20 @@ const getNodeName = node => { return null; }; +const isHook = node => + node && + node.type === 'CallExpression' && + testHookNames.has(getNodeName(node.callee)); + const isTestCase = node => node && node.type === 'CallExpression' && - testCaseNames[getNodeName(node.callee)]; + testCaseNames.has(getNodeName(node.callee)); const isDescribe = node => node && node.type === 'CallExpression' && - describeAliases[getNodeName(node.callee)]; + describeAliases.has(getNodeName(node.callee)); const isFunction = node => node && @@ -224,6 +236,7 @@ module.exports = { getStringValue, isDescribe, isFunction, + isHook, isTemplateLiteral, isTestCase, isString,