diff --git a/src/rules/__tests__/valid-expect-in-promise.test.js b/src/rules/__tests__/valid-expect-in-promise.test.ts similarity index 95% rename from src/rules/__tests__/valid-expect-in-promise.test.js rename to src/rules/__tests__/valid-expect-in-promise.test.ts index 77ab6dca6..9808d6582 100644 --- a/src/rules/__tests__/valid-expect-in-promise.test.js +++ b/src/rules/__tests__/valid-expect-in-promise.test.ts @@ -1,121 +1,13 @@ -import { RuleTester } from 'eslint'; +import { TSESLint } from '@typescript-eslint/experimental-utils'; import rule from '../valid-expect-in-promise'; -const ruleTester = new RuleTester({ +const ruleTester = new TSESLint.RuleTester({ parserOptions: { ecmaVersion: 8, }, }); ruleTester.run('valid-expect-in-promise', rule, { - invalid: [ - { - code: ` - it('it1', () => { - somePromise.then(() => { - expect(someThing).toEqual(true); - }); - }); - `, - errors: [{ column: 12, endColumn: 15, messageId: 'returnPromise' }], - }, - { - code: ` - it('it1', function() { - getSomeThing().getPromise().then(function() { - expect(someThing).toEqual(true); - }); - }); - `, - errors: [{ column: 13, endColumn: 16, messageId: 'returnPromise' }], - }, - { - code: ` - it('it1', function() { - Promise.resolve().then(function() { - expect(someThing).toEqual(true); - }); - }); - `, - errors: [{ column: 13, endColumn: 16, messageId: 'returnPromise' }], - }, - { - code: ` - it('it1', function() { - somePromise.catch(function() { - expect(someThing).toEqual(true) - }) - } - ) - `, - errors: [{ column: 13, endColumn: 15, messageId: 'returnPromise' }], - }, - { - code: ` - it('it1', function() { - somePromise.then(function() { - expect(someThing).toEqual(true) - }) - } - ) - `, - errors: [{ column: 13, endColumn: 15, messageId: 'returnPromise' }], - }, - { - code: ` - it('it1', function () { - Promise.resolve().then(/*fulfillment*/ function () { - expect(someThing).toEqual(true); - }, /*rejection*/ function () { - expect(someThing).toEqual(true); - }) - }) - `, - errors: [{ column: 11, endColumn: 13, messageId: 'returnPromise' }], - }, - { - code: ` - it('it1', function () { - Promise.resolve().then(/*fulfillment*/ function () { - }, /*rejection*/ function () { - expect(someThing).toEqual(true) - }) - }); - `, - errors: [{ column: 11, endColumn: 13, messageId: 'returnPromise' }], - }, - { - code: ` - it('test function', () => { - Builder.getPromiseBuilder().get().build().then((data) => expect(data).toEqual('Hi')); - }); - `, - errors: [{ column: 11, endColumn: 96, messageId: 'returnPromise' }], - }, - { - code: ` - it('it1', () => { - somePromise.then(() => { - doSomeOperation(); - expect(someThing).toEqual(true); - }) - }); - `, - errors: [{ column: 13, endColumn: 15, messageId: 'returnPromise' }], - }, - { - code: ` - test('invalid return', () => { - const promise = something().then(value => { - const foo = "foo"; - return expect(value).toBe('red'); - }); - }); - `, - errors: [{ column: 18, endColumn: 14, messageId: 'returnPromise' }], - }, - ], - valid: [ ` it('it1', () => new Promise((done) => { @@ -133,7 +25,6 @@ ruleTester.run('valid-expect-in-promise', rule, { }); }); `, - ` it('it1', function() { return somePromise.catch(function() { @@ -141,7 +32,6 @@ ruleTester.run('valid-expect-in-promise', rule, { }); }); `, - ` it('it1', function() { return somePromise.then(function() { @@ -149,7 +39,6 @@ ruleTester.run('valid-expect-in-promise', rule, { }); }); `, - ` it('it1', function() { return getSomeThing().getPromise().then(function() { @@ -157,7 +46,6 @@ ruleTester.run('valid-expect-in-promise', rule, { }); }); `, - ` it('it1', function() { return Promise.resolve().then(function() { @@ -165,7 +53,6 @@ ruleTester.run('valid-expect-in-promise', rule, { }); }); `, - ` it('it1', function () { return Promise.resolve().then(function () { @@ -177,7 +64,6 @@ ruleTester.run('valid-expect-in-promise', rule, { }); }); `, - ` it('it1', function () { return Promise.resolve().then(function () { @@ -188,13 +74,11 @@ ruleTester.run('valid-expect-in-promise', rule, { }); }); `, - ` it('it1', function () { return somePromise.then() }); `, - ` it('it1', async () => { await Promise.resolve().then(function () { @@ -202,7 +86,6 @@ ruleTester.run('valid-expect-in-promise', rule, { }); }); `, - ` it('it1', async () => { await somePromise.then(() => { @@ -210,7 +93,6 @@ ruleTester.run('valid-expect-in-promise', rule, { }); }); `, - ` it('it1', async () => { await getSomeThing().getPromise().then(function () { @@ -218,7 +100,6 @@ ruleTester.run('valid-expect-in-promise', rule, { }); }); `, - ` it('it1', () => { return somePromise.then(() => { @@ -229,7 +110,6 @@ ruleTester.run('valid-expect-in-promise', rule, { }) }); `, - ` it('it1', () => { return somePromise.then(() => { @@ -240,7 +120,6 @@ ruleTester.run('valid-expect-in-promise', rule, { }) }); `, - ` test('later return', () => { const promise = something().then(value => { @@ -250,7 +129,6 @@ ruleTester.run('valid-expect-in-promise', rule, { return promise; }); `, - ` it('shorthand arrow', () => something().then(value => { @@ -259,7 +137,6 @@ ruleTester.run('valid-expect-in-promise', rule, { }).toThrow(); })); `, - ` it('promise test', () => { const somePromise = getThatPromise(); @@ -270,7 +147,6 @@ ruleTester.run('valid-expect-in-promise', rule, { return somePromise; }); `, - ` test('promise test', function () { let somePromise = getThatPromise(); @@ -281,7 +157,6 @@ ruleTester.run('valid-expect-in-promise', rule, { return somePromise; }); `, - ` it('crawls for files based on patterns', () => { const promise = nodeCrawl({}).then(data => { @@ -290,7 +165,6 @@ ruleTester.run('valid-expect-in-promise', rule, { return promise; }); `, - ` it('test function', () => { @@ -300,7 +174,6 @@ ruleTester.run('valid-expect-in-promise', rule, { }); }); `, - ` notATestFunction('not a test function', () => { @@ -310,13 +183,11 @@ ruleTester.run('valid-expect-in-promise', rule, { }); }); `, - ` it("it1", () => somePromise.then(() => { expect(someThing).toEqual(true) })) `, - ` it("it1", () => somePromise.then(() => expect(someThing).toEqual(true)))`, ` @@ -325,7 +196,6 @@ ruleTester.run('valid-expect-in-promise', rule, { promise.then(() => expect(someThing).toEqual(true)); }); `, - ` it('name of done param does not matter', (nameDoesNotMatter) => { const promise = getPromise(); @@ -333,4 +203,121 @@ ruleTester.run('valid-expect-in-promise', rule, { }); `, ], + invalid: [ + { + code: ` + it('it1', () => { + somePromise.then(() => { + expect(someThing).toEqual(true); + }); + }); + `, + errors: [{ column: 12, endColumn: 15, messageId: 'returnPromise' }], + }, + // { + // code: ` + // it('it1', () => { + // somePromise['then'](() => { + // expect(someThing).toEqual(true); + // }); + // }); + // `, + // errors: [{ column: 12, endColumn: 15, messageId: 'returnPromise' }], + // }, + { + code: ` + it('it1', function() { + getSomeThing().getPromise().then(function() { + expect(someThing).toEqual(true); + }); + }); + `, + errors: [{ column: 13, endColumn: 16, messageId: 'returnPromise' }], + }, + { + code: ` + it('it1', function() { + Promise.resolve().then(function() { + expect(someThing).toEqual(true); + }); + }); + `, + errors: [{ column: 13, endColumn: 16, messageId: 'returnPromise' }], + }, + { + code: ` + it('it1', function() { + somePromise.catch(function() { + expect(someThing).toEqual(true) + }) + } + ) + `, + errors: [{ column: 13, endColumn: 15, messageId: 'returnPromise' }], + }, + { + code: ` + it('it1', function() { + somePromise.then(function() { + expect(someThing).toEqual(true) + }) + } + ) + `, + errors: [{ column: 13, endColumn: 15, messageId: 'returnPromise' }], + }, + { + code: ` + it('it1', function () { + Promise.resolve().then(/*fulfillment*/ function () { + expect(someThing).toEqual(true); + }, /*rejection*/ function () { + expect(someThing).toEqual(true); + }) + }) + `, + errors: [{ column: 11, endColumn: 13, messageId: 'returnPromise' }], + }, + { + code: ` + it('it1', function () { + Promise.resolve().then(/*fulfillment*/ function () { + }, /*rejection*/ function () { + expect(someThing).toEqual(true) + }) + }); + `, + errors: [{ column: 11, endColumn: 13, messageId: 'returnPromise' }], + }, + { + code: ` + it('test function', () => { + Builder.getPromiseBuilder().get().build().then((data) => expect(data).toEqual('Hi')); + }); + `, + errors: [{ column: 11, endColumn: 96, messageId: 'returnPromise' }], + }, + { + code: ` + it('it1', () => { + somePromise.then(() => { + doSomeOperation(); + expect(someThing).toEqual(true); + }) + }); + `, + errors: [{ column: 13, endColumn: 15, messageId: 'returnPromise' }], + }, + { + code: ` + test('invalid return', () => { + const promise = something().then(value => { + const foo = "foo"; + return expect(value).toBe('red'); + }); + }); + `, + errors: [{ column: 18, endColumn: 14, messageId: 'returnPromise' }], + }, + ], }); diff --git a/src/rules/tsUtils.ts b/src/rules/tsUtils.ts index f2b09e032..fe0d4afd0 100644 --- a/src/rules/tsUtils.ts +++ b/src/rules/tsUtils.ts @@ -116,6 +116,47 @@ interface KnownMemberExpression property: AccessorNode; } +/** + * Represents a `CallExpression` with a "known" `property` accessor. + * + * i.e `KnownCallExpression<'includes'>` represents `.includes()`. + */ +export interface KnownCallExpression + extends TSESTree.CallExpression { + callee: CalledKnownMemberExpression; +} + +/** + * Represents a `MemberExpression` with a "known" `property`, that is called. + * + * This is `KnownCallExpression` from the perspective of the `MemberExpression` node. + */ +export interface CalledKnownMemberExpression + extends KnownMemberExpression { + parent: KnownCallExpression; +} + +/** + * Represents a `CallExpression` with a single argument. + */ +export interface CallExpressionWithSingleArgument< + Argument extends TSESTree.Expression = TSESTree.Expression +> extends TSESTree.CallExpression { + arguments: [Argument]; +} + +/** + * Guards that the given `call` has only one `argument`. + * + * @param {CallExpression} call + * + * @return {call is CallExpressionWithSingleArgument} + */ +export const hasOnlyOneArgument = ( + call: TSESTree.CallExpression, +): call is CallExpressionWithSingleArgument => + call.arguments && call.arguments.length === 1; + /** * An `Identifier` with a known `name` value - i.e `expect`. */ @@ -171,34 +212,6 @@ export const isSupportedAccessor = ( ): node is AccessorNode => isIdentifier(node, value) || isStringNode(node, value); -/** - * Represents a `CallExpression` with a single argument. - */ -export interface CallExpressionWithSingleArgument< - Argument extends TSESTree.Expression = TSESTree.Expression -> extends TSESTree.CallExpression { - arguments: [Argument]; -} - -/** - * Guards that the given `call` has only one `argument`. - * - * @param {CallExpression} call - * - * @return {call is CallExpressionWithSingleArgument} - */ -export const hasOnlyOneArgument = ( - call: TSESTree.CallExpression, -): call is CallExpressionWithSingleArgument => - call.arguments && call.arguments.length === 1; - -/** - * An `Identifier` with a known `name` value - i.e `expect`. - */ -interface KnownIdentifier extends TSESTree.Identifier { - name: Name; -} - /** * Gets the value of the given `AccessorNode`, * account for the different node types. diff --git a/src/rules/util.js b/src/rules/util.js index 7c4b8bfb2..60ff20a2a 100644 --- a/src/rules/util.js +++ b/src/rules/util.js @@ -97,11 +97,6 @@ export const argument = node => export const argument2 = node => node.parent.parent.parent.arguments && node.parent.parent.parent.arguments[0]; -export const isFunction = node => - node && - (node.type === 'FunctionExpression' || - node.type === 'ArrowFunctionExpression'); - /** * Generates the URL to documentation for the given rule name. It uses the * package version to build the link to a tagged version of the diff --git a/src/rules/valid-expect-in-promise.js b/src/rules/valid-expect-in-promise.js deleted file mode 100644 index f13977093..000000000 --- a/src/rules/valid-expect-in-promise.js +++ /dev/null @@ -1,165 +0,0 @@ -import { getDocsUrl, isFunction } from './util'; - -const isThenOrCatch = node => { - return ( - node.property && - (node.property.name === 'then' || node.property.name === 'catch') - ); -}; - -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); - }); - } - 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, - }, - messageId: 'returnPromise', - 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; - while (parent) { - if (isFunction(parent) && isTestFunc(parent.parent)) { - return parent; - } - parent = parent.parent; - } -}; - -const isParentThenOrPromiseReturned = (node, testFunctionBody) => { - return ( - testFunctionBody.type === 'CallExpression' || - testFunctionBody.type === 'NewExpression' || - 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)) { - 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; - } -}; - -export default { - meta: { - docs: { - url: getDocsUrl(__filename), - }, - messages: { - returnPromise: - 'Promise should be returned to test its fulfillment or rejection', - }, - schema: [], - }, - create(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 [ - fulfillmentCallback, - rejectionCallback, - ] = node.parent.arguments; - - // 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, - ); - } - } - }, - }; - }, -}; diff --git a/src/rules/valid-expect-in-promise.ts b/src/rules/valid-expect-in-promise.ts new file mode 100644 index 000000000..3e97eb196 --- /dev/null +++ b/src/rules/valid-expect-in-promise.ts @@ -0,0 +1,214 @@ +import { + AST_NODE_TYPES, + TSESLint, + TSESTree, +} from '@typescript-eslint/experimental-utils'; +import { + CalledKnownMemberExpression, + FunctionExpression, + KnownCallExpression, + TestCaseName, + createRule, + getAccessorValue, + isExpectMember, + isFunction, + isSupportedAccessor, +} from './tsUtils'; + +type MessageIds = 'returnPromise'; +type RuleContext = TSESLint.RuleContext; + +type ThenOrCatchCallExpression = KnownCallExpression<'then' | 'catch'>; + +const isThenOrCatchCall = ( + node: TSESTree.Node, +): node is ThenOrCatchCallExpression => + node.type === AST_NODE_TYPES.CallExpression && + node.callee.type === AST_NODE_TYPES.MemberExpression && + isSupportedAccessor(node.callee.property) && + ['then', 'catch'].includes(getAccessorValue(node.callee.property)); + +const isExpectCallPresentInFunction = (body: TSESTree.Node) => { + if (body.type === AST_NODE_TYPES.BlockStatement) { + return body.body.find(line => { + if (line.type === AST_NODE_TYPES.ExpressionStatement) { + return isFullExpectCall(line.expression); + } + if (line.type === AST_NODE_TYPES.ReturnStatement && line.argument) { + return isFullExpectCall(line.argument); + } + }); + } + + return isFullExpectCall(body); +}; + +const isFullExpectCall = (expression: TSESTree.Node) => + expression.type === AST_NODE_TYPES.CallExpression && + isExpectMember(expression.callee); + +const reportReturnRequired = (context: RuleContext, node: TSESTree.Node) => { + context.report({ + loc: { + end: { + column: node.loc.end.column, + line: node.loc.end.line, + }, + start: node.loc.start, + }, + messageId: 'returnPromise', + node, + }); +}; + +const isPromiseReturnedLater = ( + node: TSESTree.Node, + testFunctionBody: TSESTree.Statement[], +) => { + let promiseName; + if ( + node.type === AST_NODE_TYPES.ExpressionStatement && + node.expression.type === AST_NODE_TYPES.CallExpression && + node.expression.callee.type === AST_NODE_TYPES.MemberExpression && + isSupportedAccessor(node.expression.callee.object) + ) { + promiseName = getAccessorValue(node.expression.callee.object); + } else if ( + node.type === AST_NODE_TYPES.VariableDeclarator && + node.id.type === AST_NODE_TYPES.Identifier + ) { + promiseName = node.id.name; + } + + const lastLineInTestFunc = testFunctionBody[testFunctionBody.length - 1]; + + return ( + lastLineInTestFunc.type === AST_NODE_TYPES.ReturnStatement && + lastLineInTestFunc.argument && + (('name' in lastLineInTestFunc.argument && + lastLineInTestFunc.argument.name === promiseName) || + !promiseName) + ); +}; + +const isTestFunc = (node: TSESTree.Node) => + node.type === AST_NODE_TYPES.CallExpression && + isSupportedAccessor(node.callee) && + ([TestCaseName.it, TestCaseName.test] as string[]).includes( + getAccessorValue(node.callee), + ); + +const findTestFunction = (node: TSESTree.Node | undefined) => { + while (node) { + if (isFunction(node) && node.parent && isTestFunc(node.parent)) { + return node; + } + + node = node.parent; + } + + return null; +}; + +const isParentThenOrPromiseReturned = ( + node: TSESTree.Node, + testFunctionBody: TSESTree.Statement[], +) => + node.type === AST_NODE_TYPES.ReturnStatement || + isPromiseReturnedLater(node, testFunctionBody); + +// prettier-ignore +// WEB-40105 +type PromiseCallbacks = [ + TSESTree.Expression | undefined, + TSESTree.Expression | undefined +] + +const verifyExpectWithReturn = ( + promiseCallbacks: PromiseCallbacks, + node: CalledKnownMemberExpression<'then' | 'catch'>, + context: RuleContext, + testFunctionBody: TSESTree.Statement[], +) => { + promiseCallbacks.some(promiseCallback => { + if ( + promiseCallback && + isFunction(promiseCallback) && + promiseCallback.body + ) { + if ( + isExpectCallPresentInFunction(promiseCallback.body) && + node.parent && + node.parent.parent && + !isParentThenOrPromiseReturned(node.parent.parent, testFunctionBody) + ) { + reportReturnRequired(context, node.parent.parent); + + return true; + } + } + }); +}; + +const isHavingAsyncCallBackParam = (testFunction: FunctionExpression) => + testFunction.params[0] && + testFunction.params[0].type === AST_NODE_TYPES.Identifier; + +export default createRule({ + name: __filename, + meta: { + docs: { + category: 'Best Practices', + description: 'Enforce having return statement when testing with promises', + recommended: 'error', + }, + messages: { + returnPromise: + 'Promise should be returned to test its fulfillment or rejection', + }, + type: 'suggestion', + schema: [], + }, + defaultOptions: [], + create(context) { + return { + CallExpression(node) { + if ( + !isThenOrCatchCall(node) || + (node.parent && node.parent.type === AST_NODE_TYPES.AwaitExpression) + ) { + return; + } + const testFunction = findTestFunction(node); + if (testFunction && !isHavingAsyncCallBackParam(testFunction)) { + const { body } = testFunction; + + /* istanbul ignore if */ + if (!body) { + throw new Error( + `Unexpected null when attempting to fix ${context.getFilename()} - please file a github issue at https://github.com/jest-community/eslint-plugin-jest`, + ); + } + + if (body.type !== AST_NODE_TYPES.BlockStatement) { + return; + } + + const testFunctionBody = body.body; + const [fulfillmentCallback, rejectionCallback] = node.arguments; + + // 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.callee, + context, + testFunctionBody, + ); + } + }, + }; + }, +});