From 5e01e6b2caa7205ab4f5537ab33ce81feff4dad7 Mon Sep 17 00:00:00 2001 From: Macklin Underdown Date: Fri, 9 Feb 2018 10:01:16 -0500 Subject: [PATCH] feat(rule): add valid-describe rule (#57) Fixes #18 --- README.md | 1 + docs/rules/valid-describe.md | 53 +++++++ index.js | 2 + rules/__tests__/valid-describe.test.js | 195 +++++++++++++++++++++++++ rules/no-identical-title.js | 13 +- rules/util.js | 23 +++ rules/valid-describe.js | 63 ++++++++ rules/valid-expect-in-promise.js | 10 +- 8 files changed, 342 insertions(+), 18 deletions(-) create mode 100644 docs/rules/valid-describe.md create mode 100644 rules/__tests__/valid-describe.test.js create mode 100644 rules/valid-describe.js diff --git a/README.md b/README.md index cbbd8e232..1515c89a9 100644 --- a/README.md +++ b/README.md @@ -88,6 +88,7 @@ for more information about extending configuration files. | [prefer-to-be-null](docs/rules/prefer-to-be-null.md) | Suggest using `toBeNull()` | | ![fixable](https://img.shields.io/badge/-fixable-green.svg) | | [prefer-to-be-undefined](docs/rules/prefer-to-be-undefined.md) | Suggest using `toBeUndefined()` | | ![fixable](https://img.shields.io/badge/-fixable-green.svg) | | [prefer-expect-assertions](docs/rules/prefer-expect-assertions.md) | Suggest using `expect.assertions()` OR `expect.hasAssertions()` | | | +| [valid-describe](docs/rules/valid-describe.md) | Enforce valid `describe()` callback | | | | [valid-expect](docs/rules/valid-expect.md) | Enforce valid `expect()` usage | ![recommended](https://img.shields.io/badge/-recommended-lightgrey.svg) | | | [valid-expect-in-promise](docs/rules/valid-expect-in-promise.md) | Enforce having return statement when testing with promises | | | diff --git a/docs/rules/valid-describe.md b/docs/rules/valid-describe.md new file mode 100644 index 000000000..8bb06c71a --- /dev/null +++ b/docs/rules/valid-describe.md @@ -0,0 +1,53 @@ +# Enforce valid `describe()` callback (valid-describe) + +Using an improper `describe()` callback function can lead to unexpected test errors. + +## Rule Details + +This rule validates that the second parameter of a `describe()` function is a callback function. This callback function: + +* should not be [async](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function) +* should not contain any parameters +* should not contain any `return` statements + +The following `describe` function aliases are also validated: + +* `describe` +* `describe.only` +* `describe.skip` +* `fdescribe` +* `xdescribe` + +The following patterns are considered warnings: + +```js +// Async callback functions are not allowed +describe('myFunction()', async () => { + // ... +}); + +// Callback function parameters are not allowed +describe('myFunction()', done => { + // ... +}); + +// +describe('myFunction', () => { + // No return statements are allowed in block of a callback function + return Promise.resolve().then(() => { + it('breaks', () => { + throw new Error('Fail'); + }); + }); +}); +``` + +The following patterns are not considered warnings: + +```js +describe('myFunction()', () => { + it('returns a truthy value', () => { + expect(myFunction()).toBeTruthy(); + }); +}); +``` diff --git a/index.js b/index.js index 85853c5f7..cd30b2e3d 100644 --- a/index.js +++ b/index.js @@ -7,6 +7,7 @@ const noLargeSnapshots = require('./rules/no-large-snapshots'); const preferToBeNull = require('./rules/prefer-to-be-null'); const preferToBeUndefined = require('./rules/prefer-to-be-undefined'); const preferToHaveLength = require('./rules/prefer-to-have-length'); +const validDescribe = require('./rules/valid-describe'); const validExpect = require('./rules/valid-expect'); const preferExpectAssertions = require('./rules/prefer-expect-assertions'); const validExpectInPromise = require('./rules/valid-expect-in-promise'); @@ -63,6 +64,7 @@ module.exports = { 'prefer-to-be-null': preferToBeNull, 'prefer-to-be-undefined': preferToBeUndefined, 'prefer-to-have-length': preferToHaveLength, + 'valid-describe': validDescribe, 'valid-expect': validExpect, 'prefer-expect-assertions': preferExpectAssertions, 'valid-expect-in-promise': validExpectInPromise, diff --git a/rules/__tests__/valid-describe.test.js b/rules/__tests__/valid-describe.test.js new file mode 100644 index 000000000..b79921189 --- /dev/null +++ b/rules/__tests__/valid-describe.test.js @@ -0,0 +1,195 @@ +'use strict'; + +const RuleTester = require('eslint').RuleTester; +const rules = require('../..').rules; + +const ruleTester = new RuleTester({ + parserOptions: { + ecmaVersion: 8, + }, +}); + +ruleTester.run('valid-describe', rules['valid-describe'], { + valid: [ + 'describe("foo")', + 'describe("foo", function() {})', + 'describe("foo", () => {})', + 'xdescribe("foo", () => {})', + 'fdescribe("foo", () => {})', + 'describe.only("foo", () => {})', + 'describe.skip("foo", () => {})', + ` + describe('foo', () => { + it('bar', () => { + return Promise.resolve(42).then(value => { + expect(value).toBe(42) + }) + }) + }) + `, + ` + describe('foo', () => { + it('bar', async () => { + expect(await Promise.resolve(42)).toBe(42) + }) + }) + `, + ], + invalid: [ + { + code: 'describe("foo", async () => {})', + errors: [{ message: 'No async describe callback', line: 1, column: 17 }], + }, + { + code: 'describe("foo", async function () {})', + errors: [{ message: 'No async describe callback', line: 1, column: 17 }], + }, + { + code: 'xdescribe("foo", async function () {})', + errors: [{ message: 'No async describe callback', line: 1, column: 18 }], + }, + { + code: 'fdescribe("foo", async function () {})', + errors: [{ message: 'No async describe callback', line: 1, column: 18 }], + }, + { + code: 'describe.only("foo", async function () {})', + errors: [{ message: 'No async describe callback', line: 1, column: 22 }], + }, + { + code: 'describe.skip("foo", async function () {})', + errors: [{ message: 'No async describe callback', line: 1, column: 22 }], + }, + { + code: ` + describe('sample case', () => { + it('works', () => { + expect(true).toEqual(true); + }); + describe('async', async () => { + await new Promise(setImmediate); + it('breaks', () => { + throw new Error('Fail'); + }); + }); + });`, + errors: [{ message: 'No async describe callback', line: 6, column: 27 }], + }, + { + code: ` + describe('foo', function () { + return Promise.resolve().then(() => { + it('breaks', () => { + throw new Error('Fail') + }) + }) + }) + `, + errors: [ + { + message: 'Unexpected return statement in describe callback', + line: 3, + column: 9, + }, + ], + }, + { + code: ` + describe('foo', () => { + return Promise.resolve().then(() => { + it('breaks', () => { + throw new Error('Fail') + }) + }) + describe('nested', () => { + return Promise.resolve().then(() => { + it('breaks', () => { + throw new Error('Fail') + }) + }) + }) + }) + `, + errors: [ + { + message: 'Unexpected return statement in describe callback', + line: 3, + column: 9, + }, + { + message: 'Unexpected return statement in describe callback', + line: 9, + column: 11, + }, + ], + }, + { + code: ` + describe('foo', async () => { + await something() + it('does something') + describe('nested', () => { + return Promise.resolve().then(() => { + it('breaks', () => { + throw new Error('Fail') + }) + }) + }) + }) + `, + errors: [ + { + message: 'No async describe callback', + line: 2, + column: 23, + }, + { + message: 'Unexpected return statement in describe callback', + line: 6, + column: 11, + }, + ], + }, + { + code: 'describe("foo", done => {})', + errors: [ + { + message: 'Unexpected argument(s) in describe callback', + line: 1, + column: 17, + }, + ], + }, + { + code: 'describe("foo", function (done) {})', + errors: [ + { + message: 'Unexpected argument(s) in describe callback', + line: 1, + column: 27, + }, + ], + }, + { + code: 'describe("foo", function (one, two, three) {})', + errors: [ + { + message: 'Unexpected argument(s) in describe callback', + line: 1, + column: 27, + }, + ], + }, + { + code: 'describe("foo", async function (done) {})', + errors: [ + { message: 'No async describe callback', line: 1, column: 17 }, + { + message: 'Unexpected argument(s) in describe callback', + line: 1, + column: 33, + }, + ], + }, + ], +}); diff --git a/rules/no-identical-title.js b/rules/no-identical-title.js index 137b1813c..194ad4e6b 100644 --- a/rules/no-identical-title.js +++ b/rules/no-identical-title.js @@ -1,12 +1,6 @@ 'use strict'; -const describeAliases = Object.assign(Object.create(null), { - describe: true, - 'describe.only': true, - 'describe.skip': true, - fdescribe: true, - xdescribe: true, -}); +const isDescribe = require('./util').isDescribe; const testCaseNames = Object.assign(Object.create(null), { fit: true, @@ -27,11 +21,6 @@ const getNodeName = node => { return node.name; }; -const isDescribe = node => - node && - node.type === 'CallExpression' && - describeAliases[getNodeName(node.callee)]; - const isTestCase = node => node && node.type === 'CallExpression' && diff --git a/rules/util.js b/rules/util.js index 15dc85f0d..76a3a2ff5 100644 --- a/rules/util.js +++ b/rules/util.js @@ -80,6 +80,27 @@ const argument = node => node.parent.parent.arguments[0]; const argument2 = node => 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 getNodeName = node => { + if (node.type === 'MemberExpression') { + return node.object.name + '.' + node.property.name; + } + return node.name; +}; + +const isDescribe = node => + node.type === 'CallExpression' && describeAliases[getNodeName(node.callee)]; + +const isFunction = node => + node.type === 'FunctionExpression' || node.type === 'ArrowFunctionExpression'; + module.exports = { method: method, method2: method2, @@ -95,4 +116,6 @@ module.exports = { expectNotToEqualCase: expectNotToEqualCase, expectToBeUndefinedCase: expectToBeUndefinedCase, expectNotToBeUndefinedCase: expectNotToBeUndefinedCase, + isDescribe: isDescribe, + isFunction: isFunction, }; diff --git a/rules/valid-describe.js b/rules/valid-describe.js new file mode 100644 index 000000000..7d844441a --- /dev/null +++ b/rules/valid-describe.js @@ -0,0 +1,63 @@ +'use strict'; + +const isDescribe = require('./util').isDescribe; +const isFunction = require('./util').isFunction; + +const isAsync = node => node.async; + +const hasParams = node => node.params.length > 0; + +const paramsLocation = params => { + const first = params[0]; + const last = params[params.length - 1]; + return { + start: { + line: first.loc.start.line, + column: first.loc.start.column, + }, + end: { + line: last.loc.end.line, + column: last.loc.end.column, + }, + }; +}; + +module.exports = { + meta: { + docs: { + url: + 'https://github.com/jest-community/eslint-plugin-jest/blob/master/docs/rules/valid-describe.md', + }, + }, + create(context) { + return { + CallExpression(node) { + if (isDescribe(node)) { + const callbackFunction = node.arguments[1]; + if (callbackFunction && isFunction(callbackFunction)) { + if (isAsync(callbackFunction)) { + context.report({ + message: 'No async describe callback', + node: callbackFunction, + }); + } + if (hasParams(callbackFunction)) { + context.report({ + message: 'Unexpected argument(s) in describe callback', + loc: paramsLocation(callbackFunction.params), + }); + } + callbackFunction.body.body.forEach(node => { + if (node.type === 'ReturnStatement') { + context.report({ + message: 'Unexpected return statement in describe callback', + node: node, + }); + } + }); + } + } + }, + }; + }, +}; diff --git a/rules/valid-expect-in-promise.js b/rules/valid-expect-in-promise.js index 822400bd7..f0d50a7c0 100644 --- a/rules/valid-expect-in-promise.js +++ b/rules/valid-expect-in-promise.js @@ -1,5 +1,7 @@ 'use strict'; +const isFunction = require('./util').isFunction; + const reportMsg = 'Promise should be returned to test its fulfillment or rejection'; @@ -10,10 +12,6 @@ const isThenOrCatch = node => { ); }; -const isFunction = type => { - return type == 'FunctionExpression' || type == 'ArrowFunctionExpression'; -}; - const isExpectCallPresentInFunction = body => { if (body.type === 'BlockStatement') { return body.body.find(line => { @@ -79,7 +77,7 @@ const getFunctionBody = func => { const getTestFunction = node => { let parent = node.parent; while (parent) { - if (isFunction(parent.type) && isTestFunc(parent.parent)) { + if (isFunction(parent) && isTestFunc(parent.parent)) { return parent; } parent = parent.parent; @@ -102,7 +100,7 @@ const verifyExpectWithReturn = ( testFunctionBody ) => { promiseCallbacks.some(promiseCallback => { - if (promiseCallback && isFunction(promiseCallback.type)) { + if (promiseCallback && isFunction(promiseCallback)) { if ( isExpectCallPresentInFunction(promiseCallback.body) && !isParentThenOrPromiseReturned(node, testFunctionBody)