From 8f57efc5cde8bcc008a41c53ba9ddfd31bd5846a Mon Sep 17 00:00:00 2001 From: M4rk9696 Date: Thu, 11 Jul 2019 11:43:44 +0530 Subject: [PATCH 1/5] refactor: extract isHook to util --- src/rules/no-hooks.js | 10 +--------- src/rules/util.js | 13 +++++++++++++ 2 files changed, 14 insertions(+), 9 deletions(-) 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..9259b1ec5 100644 --- a/src/rules/util.js +++ b/src/rules/util.js @@ -99,6 +99,13 @@ const testCaseNames = Object.assign(Object.create(null), { xtest: true, }); +const testHookNames = Object.assign(Object.create(null), { + beforeAll: true, + beforeEach: true, + afterAll: true, + afterEach: true, +}); + const getNodeName = node => { function joinNames(a, b) { return a && b ? `${a}.${b}` : null; @@ -119,6 +126,11 @@ const getNodeName = node => { return null; }; +const isHook = node => + node && + node.type === 'CallExpression' && + testHookNames[getNodeName(node.callee)]; + const isTestCase = node => node && node.type === 'CallExpression' && @@ -224,6 +236,7 @@ module.exports = { getStringValue, isDescribe, isFunction, + isHook, isTemplateLiteral, isTestCase, isString, From cdb17a7416f0a2931c8c51ce11dbde3cbd7b17c7 Mon Sep 17 00:00:00 2001 From: M4rk9696 Date: Thu, 11 Jul 2019 19:49:01 +0530 Subject: [PATCH 2/5] feat: add rule no-duplicate-hook #231 --- README.md | 1 + docs/rules/no-duplicate-hooks.md | 75 +++++ src/__tests__/rules.test.js | 2 +- .../__tests__/no-duplicate-hooks.test.js | 311 ++++++++++++++++++ src/rules/no-duplicate-hooks.js | 48 +++ 5 files changed, 436 insertions(+), 1 deletion(-) 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 b370f1a50..6063691dd 100644 --- a/README.md +++ b/README.md @@ -158,6 +158,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..07c70d331 --- /dev/null +++ b/src/rules/__tests__/no-duplicate-hooks.test.js @@ -0,0 +1,311 @@ +'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'), + ], + + 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(); + } + }, + }; + }, +}; From 83cbdbb1728be3c43b51b39d3a00f2a01426dfa3 Mon Sep 17 00:00:00 2001 From: M4rk9696 Date: Sat, 13 Jul 2019 21:04:16 +0530 Subject: [PATCH 3/5] test(no-duplicate-hooks): add test convering all hooks in one describe --- src/rules/__tests__/no-duplicate-hooks.test.js | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/rules/__tests__/no-duplicate-hooks.test.js b/src/rules/__tests__/no-duplicate-hooks.test.js index 07c70d331..bb7fe76dd 100644 --- a/src/rules/__tests__/no-duplicate-hooks.test.js +++ b/src/rules/__tests__/no-duplicate-hooks.test.js @@ -27,6 +27,21 @@ ruleTester.run('basic describe block', rule, { ' some_fn();', '})', ].join('\n'), + [ + 'describe("foo", () => {', + ' beforeAll(() => {', + ' }),', + ' beforeEach(() => {', + ' }),', + ' afterEach(() => {', + ' }),', + ' afterAll(() => {', + ' }),', + ' test("bar", () => {', + ' some_fn();', + ' })', + '})', + ].join('\n'), ], invalid: [ From f685c2253207a76f6abaa25f86406f43ec755ec6 Mon Sep 17 00:00:00 2001 From: M4rk9696 Date: Sat, 13 Jul 2019 21:11:34 +0530 Subject: [PATCH 4/5] refactor: replace with Set in utils --- src/rules/no-focused-tests.js | 12 +++----- src/rules/util.js | 58 +++++++++++++++++------------------ 2 files changed, 34 insertions(+), 36 deletions(-) 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/util.js b/src/rules/util.js index 9259b1ec5..53f214a3d 100644 --- a/src/rules/util.js +++ b/src/rules/util.js @@ -79,32 +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 testHookNames = Object.assign(Object.create(null), { - beforeAll: true, - beforeEach: true, - afterAll: true, - afterEach: 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) { @@ -129,17 +129,17 @@ const getNodeName = node => { const isHook = node => node && node.type === 'CallExpression' && - testHookNames[getNodeName(node.callee)]; + 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 && From 4515ce1e00263f16c89727ef812e321177c1999b Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Mon, 15 Jul 2019 13:11:09 +0200 Subject: [PATCH 5/5] Update README.md --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 6063691dd..2b12e0a81 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 | | |