From eb31a54467d00d713dc1120f7b5bc4ffc04a26a3 Mon Sep 17 00:00:00 2001 From: tushardhole Date: Wed, 17 Jan 2018 19:54:24 +0530 Subject: [PATCH] feat: implement valid-expect-in-promise rule (#42) * feat: implement valid-expect-in-promise rule introduce a lint rule to report error when testing promises. If a expectation is added inside a then or catch block of promise then as per jest docs, it is mandatory to return that promise for succesfull execution of that expectation. This rule will report all such expectations where promise is not returned. * fix: do not add the rule to recommended * fix: do not validate await expressions for valid_expect_in_promise * fix: scenario with expect in nested then/catch * docs: updating docs for valid-expect-in-promise rule * test: add failing test case * fix: scenario where promise is returned later * fix: adding more scenarios where promise is returned later * test: adding more tests * fix: adding scenarios for non test functions * fix: refactor to avoid multiple execution of getTestFuncBody * fix: scenario with arrow-short-hand-fn with implicit return statement * fix: scenario with multiline function in then block * fix: scenario with short hand arrow function in then block * fix: do not validate tests with done async param * fix: scenario where expect in then is preceded by return * fix: duplicate warning for same promise * fix: better naming * test: better formatting --- README.md | 1 + docs/rules/valid-expect-in-promise.md | 33 ++ index.js | 2 + .../__tests__/valid_expect_in_promise.test.js | 391 ++++++++++++++++++ rules/valid_expect_in_promise.js | 159 +++++++ 5 files changed, 586 insertions(+) create mode 100644 docs/rules/valid-expect-in-promise.md create mode 100644 rules/__tests__/valid_expect_in_promise.test.js create mode 100644 rules/valid_expect_in_promise.js diff --git a/README.md b/README.md index 5502f65a1..cbbd8e232 100644 --- a/README.md +++ b/README.md @@ -89,6 +89,7 @@ for more information about extending configuration files. | [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-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 | | | ## Credit diff --git a/docs/rules/valid-expect-in-promise.md b/docs/rules/valid-expect-in-promise.md new file mode 100644 index 000000000..8de200ca5 --- /dev/null +++ b/docs/rules/valid-expect-in-promise.md @@ -0,0 +1,33 @@ +# Enforce having return statement when testing with promises (valid-expect-in-promise) + +Ensure to return promise when having assertions in `then` or `catch` block of +promise + +## Rule details + +This rule triggers a warning if, + +* test is having assertions in `then` or `catch` block of a promise +* and that promise is not returned from the test + +### Default configuration + +The following pattern is considered warning: + +```js +it('promise test', () => { + somePromise.then(data => { + expect(data).toEqual('foo'); + }); +}); +``` + +The following pattern is not warning: + +```js +it('promise test', () => { + return somePromise.then(data => { + expect(data).toEqual('foo'); + }); +}); +``` diff --git a/index.js b/index.js index f8802796c..d3eac65e4 100644 --- a/index.js +++ b/index.js @@ -9,6 +9,7 @@ const preferToBeUndefined = require('./rules/prefer_to_be_undefined'); const preferToHaveLength = require('./rules/prefer_to_have_length'); const validExpect = require('./rules/valid_expect'); const preferExpectAssertions = require('./rules/prefer_expect_assertions'); +const validExpectInPromise = require('./rules/valid_expect_in_promise'); const snapshotProcessor = require('./processors/snapshot-processor'); @@ -64,5 +65,6 @@ module.exports = { 'prefer-to-have-length': preferToHaveLength, 'valid-expect': validExpect, 'prefer-expect-assertions': preferExpectAssertions, + 'valid-expect-in-promise': validExpectInPromise, }, }; diff --git a/rules/__tests__/valid_expect_in_promise.test.js b/rules/__tests__/valid_expect_in_promise.test.js new file mode 100644 index 000000000..9dd55c19f --- /dev/null +++ b/rules/__tests__/valid_expect_in_promise.test.js @@ -0,0 +1,391 @@ +'use strict'; + +const RuleTester = require('eslint').RuleTester; +const rules = require('../..').rules; + +const ruleTester = new RuleTester({ + parserOptions: { + ecmaVersion: 8, + }, +}); + +const expectedMsg = + 'Promise should be returned to test its fulfillment or rejection'; + +ruleTester.run('valid-expect-in-promise', rules['valid-expect-in-promise'], { + invalid: [ + { + code: ` + it('it1', () => { + somePromise.then(() => { + expect(someThing).toEqual(true); + }); + }); + `, + errors: [ + { + column: 12, + endColumn: 15, + message: expectedMsg, + }, + ], + }, + { + code: ` + it('it1', function() { + getSomeThing().getPromise().then(function() { + expect(someThing).toEqual(true); + }); + }); + `, + errors: [ + { + column: 13, + endColumn: 16, + message: expectedMsg, + }, + ], + }, + { + code: ` + it('it1', function() { + Promise.resolve().then(function() { + expect(someThing).toEqual(true); + }); + }); + `, + errors: [ + { + column: 13, + endColumn: 16, + message: expectedMsg, + }, + ], + }, + { + code: ` + it('it1', function() { + somePromise.catch(function() { + expect(someThing).toEqual(true) + }) + } + ) + `, + errors: [ + { + column: 13, + endColumn: 15, + message: expectedMsg, + }, + ], + }, + { + code: ` + it('it1', function() { + somePromise.then(function() { + expect(someThing).toEqual(true) + }) + } + ) + `, + errors: [ + { + column: 13, + endColumn: 15, + message: expectedMsg, + }, + ], + }, + { + code: ` + it('it1', function () { + Promise.resolve().then(/*fulfillment*/ function () { + expect(someThing).toEqual(true); + }, /*rejection*/ function () { + expect(someThing).toEqual(true); + }) + }) + `, + errors: [ + { + column: 11, + endColumn: 13, + message: expectedMsg, + }, + ], + }, + { + code: ` + it('it1', function () { + Promise.resolve().then(/*fulfillment*/ function () { + }, /*rejection*/ function () { + expect(someThing).toEqual(true) + }) + }); + `, + errors: [ + { + column: 11, + endColumn: 13, + message: expectedMsg, + }, + ], + }, + { + code: ` + it('test function', () => { + Builder.getPromiseBuilder().get().build().then((data) => expect(data).toEqual('Hi')); + }); + `, + errors: [ + { + column: 11, + endColumn: 96, + message: expectedMsg, + }, + ], + }, + { + code: ` + it('it1', () => { + somePromise.then(() => { + doSomeOperation(); + expect(someThing).toEqual(true); + }) + }); + `, + errors: [ + { + column: 13, + endColumn: 15, + message: expectedMsg, + }, + ], + }, + { + code: ` + test('invalid return', () => { + const promise = something().then(value => { + return expect(value).toBe('red'); + }); + }); + `, + errors: [ + { + column: 18, + endColumn: 14, + message: expectedMsg, + }, + ], + }, + ], + + valid: [ + ` + it('it1', () => { + return somePromise.then(() => { + expect(someThing).toEqual(true); + }); + }); + `, + + ` + it('it1', function() { + return somePromise.catch(function() { + expect(someThing).toEqual(true); + }); + }); + `, + + ` + it('it1', function() { + return somePromise.then(function() { + doSomeThingButNotExpect(); + }); + }); + `, + + ` + it('it1', function() { + return getSomeThing().getPromise().then(function() { + expect(someThing).toEqual(true); + }); + }); + `, + + ` + it('it1', function() { + return Promise.resolve().then(function() { + expect(someThing).toEqual(true); + }); + }); + `, + + ` + it('it1', function () { + return Promise.resolve().then(function () { + /*fulfillment*/ + expect(someThing).toEqual(true); + }, function () { + /*rejection*/ + expect(someThing).toEqual(true); + }); + }); + `, + + ` + it('it1', function () { + return Promise.resolve().then(function () { + /*fulfillment*/ + }, function () { + /*rejection*/ + expect(someThing).toEqual(true); + }); + }); + `, + + ` + it('it1', function () { + return somePromise.then() + }); + `, + + ` + it('it1', async () => { + await Promise.resolve().then(function () { + expect(someThing).toEqual(true) + }); + }); + `, + + ` + it('it1', async () => { + await somePromise.then(() => { + expect(someThing).toEqual(true) + }); + }); + `, + + ` + it('it1', async () => { + await getSomeThing().getPromise().then(function () { + expect(someThing).toEqual(true) + }); + }); + `, + + ` + it('it1', () => { + return somePromise.then(() => { + expect(someThing).toEqual(true); + }) + .then(() => { + expect(someThing).toEqual(true); + }) + }); + `, + + ` + it('it1', () => { + return somePromise.then(() => { + expect(someThing).toEqual(true); + }) + .catch(() => { + expect(someThing).toEqual(false); + }) + }); + `, + + ` + test('later return', () => { + const promise = something().then(value => { + expect(value).toBe('red'); + }); + + return promise; + }); + `, + + ` + it('shorthand arrow', () => + something().then(value => { + expect(() => { + value(); + }).toThrow(); + })); + `, + + ` + it('promise test', () => { + const somePromise = getThatPromise(); + somePromise.then((data) => { + expect(data).toEqual('foo'); + }); + expect(somePromise).toBeDefined(); + return somePromise; + }); + `, + + ` + test('promise test', function () { + let somePromise = getThatPromise(); + somePromise.then((data) => { + expect(data).toEqual('foo'); + }); + expect(somePromise).toBeDefined(); + return somePromise; + }); + `, + + ` + it('crawls for files based on patterns', () => { + const promise = nodeCrawl({}).then(data => { + expect(childProcess.spawn).lastCalledWith('find'); + }); + return promise; + }); + `, + + ` + it('test function', + () => { + return Builder.getPromiseBuilder().get().build() + .then((data) => { + expect(data).toEqual('Hi'); + }); + }); + `, + + ` + notATestFunction('not a test function', + () => { + Builder.getPromiseBuilder().get().build() + .then((data) => { + expect(data).toEqual('Hi'); + }); + }); + `, + + ` + it("it1", () => somePromise.then(() => { + expect(someThing).toEqual(true) + })) + `, + + ` it("it1", () => somePromise.then(() => expect(someThing).toEqual(true)))`, + + ` + it('promise test with done', (done) => { + const promise = getPromise(); + promise.then(() => expect(someThing).toEqual(true)); + }); + `, + + ` + it('name of done param does not matter', (nameDoesNotMatter) => { + const promise = getPromise(); + promise.then(() => expect(someThing).toEqual(true)); + }); + `, + ], +}); diff --git a/rules/valid_expect_in_promise.js b/rules/valid_expect_in_promise.js new file mode 100644 index 000000000..0d6f0242c --- /dev/null +++ b/rules/valid_expect_in_promise.js @@ -0,0 +1,159 @@ +'use strict'; + +const reportMsg = + 'Promise should be returned to test its fulfillment or rejection'; + +const isThenOrCatch = node => { + return ( + node.property && + (node.property.name == 'then' || node.property.name == 'catch') + ); +}; + +const isFunction = type => { + return type == 'FunctionExpression' || type == 'ArrowFunctionExpression'; +}; + +const isExpectCallPresentInFunction = body => { + if (body.type === 'BlockStatement') { + return body.body.find(line => { + if (line.type === 'ExpressionStatement') + return isExpectCall(line.expression); + if (line.type === 'ReturnStatement') return isExpectCall(line.argument); + }); + } else { + return isExpectCall(body); + } +}; + +const isExpectCall = expression => { + return ( + expression && + expression.type == 'CallExpression' && + expression.callee.type === 'MemberExpression' && + expression.callee.object.type === 'CallExpression' && + expression.callee.object.callee.name === 'expect' + ); +}; + +const reportReturnRequired = (context, node) => { + context.report({ + loc: { + end: { + column: node.parent.parent.loc.end.column, + line: node.parent.parent.loc.end.line, + }, + start: node.parent.parent.loc.start, + }, + message: reportMsg, + node, + }); +}; + +const isPromiseReturnedLater = (node, testFunctionBody) => { + let promiseName; + if (node.parent.parent.type === 'ExpressionStatement') { + promiseName = node.parent.parent.expression.callee.object.name; + } else if (node.parent.parent.type === 'VariableDeclarator') { + promiseName = node.parent.parent.id.name; + } + const lastLineInTestFunc = testFunctionBody[testFunctionBody.length - 1]; + return ( + lastLineInTestFunc.type === 'ReturnStatement' && + lastLineInTestFunc.argument.name === promiseName + ); +}; + +const isTestFunc = node => { + return ( + node.type === 'CallExpression' && + (node.callee.name === 'it' || node.callee.name === 'test') + ); +}; + +const getFunctionBody = func => { + if (func.body.type === 'BlockStatement') return func.body.body; + return func.body; //arrow-short-hand-fn +}; + +const getTestFunction = node => { + let parent = node.parent; + while (parent) { + if (isFunction(parent.type) && isTestFunc(parent.parent)) { + return parent; + } + parent = parent.parent; + } +}; + +const isParentThenOrPromiseReturned = (node, testFunctionBody) => { + return ( + testFunctionBody.type == 'CallExpression' || + node.parent.parent.type == 'ReturnStatement' || + isPromiseReturnedLater(node, testFunctionBody) || + isThenOrCatch(node.parent.parent) + ); +}; + +const verifyExpectWithReturn = ( + promiseCallbacks, + node, + context, + testFunctionBody +) => { + promiseCallbacks.some(promiseCallback => { + if (promiseCallback && isFunction(promiseCallback.type)) { + if ( + isExpectCallPresentInFunction(promiseCallback.body) && + !isParentThenOrPromiseReturned(node, testFunctionBody) + ) { + reportReturnRequired(context, node); + return true; + } + } + }); +}; + +const isAwaitExpression = node => { + return node.parent.parent && node.parent.parent.type == 'AwaitExpression'; +}; + +const isHavingAsyncCallBackParam = testFunction => { + try { + return testFunction.params[0].type === 'Identifier'; + } catch (e) { + return false; + } +}; + +module.exports = context => { + return { + MemberExpression(node) { + if ( + node.type == 'MemberExpression' && + isThenOrCatch(node) && + node.parent.type == 'CallExpression' && + !isAwaitExpression(node) + ) { + const testFunction = getTestFunction(node); + if (testFunction && !isHavingAsyncCallBackParam(testFunction)) { + const testFunctionBody = getFunctionBody(testFunction); + const parent = node.parent; + const fulfillmentCallback = parent.arguments[0]; + const rejectionCallback = parent.arguments[1]; + + // then block can have two args, fulfillment & rejection + // then block can have one args, fulfillment + // catch block can have one args, rejection + // ref: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise + verifyExpectWithReturn( + [fulfillmentCallback, rejectionCallback], + node, + context, + testFunctionBody + ); + } + } + }, + }; +};