From 28e5b618989f32ac0094780969076014a7d7c07e Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Sun, 26 Sep 2021 15:21:12 +1300 Subject: [PATCH 01/23] feat(valid-expect-in-promise): re-write implementation --- .../__tests__/valid-expect-in-promise.test.ts | 285 +++++++++++------- src/rules/valid-expect-in-promise.ts | 161 +++------- 2 files changed, 222 insertions(+), 224 deletions(-) diff --git a/src/rules/__tests__/valid-expect-in-promise.test.ts b/src/rules/__tests__/valid-expect-in-promise.test.ts index 0a9b3deaa..ee8ee273c 100644 --- a/src/rules/__tests__/valid-expect-in-promise.test.ts +++ b/src/rules/__tests__/valid-expect-in-promise.test.ts @@ -12,14 +12,60 @@ const ruleTester = new TSESLint.RuleTester({ ruleTester.run('valid-expect-in-promise', rule, { valid: [ + // todo: done callback + // dedent` + // it('it1', () => new Promise((done) => { + // test() + // .then(() => { + // expect(someThing).toEqual(true); + // done(); + // }); + // })); + // `, dedent` - it('it1', () => new Promise((done) => { - test() - .then(() => { - expect(someThing).toEqual(true); - done(); - }); - })); + it('passes', () => { + Promise.resolve().then(() => { + grabber.grabSomething(); + }); + }); + `, + dedent` + it('passes', async () => { + const grabbing = Promise.resolve().then(() => { + grabber.grabSomething(); + }); + + await grabbing; + + expect(grabber.grabbedItems).toHaveLength(1); + }); + `, + dedent` + const myFn = () => { + Promise.resolve().then(() => { + expect(true).toBe(false); + }); + }; + `, + dedent` + const myFn = () => { + Promise.resolve().then(() => { + subject.invokeMethod(); + }); + }; + `, + dedent` + const myFn = () => { + Promise.resolve().then(() => { + expect(true).toBe(false); + }); + }; + + it('it1', () => { + return somePromise.then(() => { + expect(someThing).toEqual(true); + }); + }); `, dedent` it('it1', () => new Promise((done) => { @@ -90,14 +136,15 @@ ruleTester.run('valid-expect-in-promise', rule, { }); }); `, - dedent` - it('it1', function () { - Promise.resolve().then(/*fulfillment*/ function () { - }, undefined, /*rejection*/ function () { - expect(someThing).toEqual(true) - }) - }); - `, + // todo: tighter "is promise" check + // dedent` + // it('it1', function () { + // Promise.resolve().then(/*fulfillment*/ function () { + // }, undefined, /*rejection*/ function () { + // expect(someThing).toEqual(true) + // }) + // }); + // `, dedent` it('it1', function () { return Promise.resolve().then(function () { @@ -113,27 +160,30 @@ ruleTester.run('valid-expect-in-promise', rule, { return somePromise.then() }); `, - dedent` - it('it1', async () => { - await Promise.resolve().then(function () { - expect(someThing).toEqual(true) - }); - }); - `, - dedent` - it('it1', async () => { - await somePromise.then(() => { - expect(someThing).toEqual(true) - }); - }); - `, - dedent` - it('it1', async () => { - await getSomeThing().getPromise().then(function () { - expect(someThing).toEqual(true) - }); - }); - `, + // todo: async + // dedent` + // it('it1', async () => { + // await Promise.resolve().then(function () { + // expect(someThing).toEqual(true) + // }); + // }); + // `, + // todo: async + // dedent` + // it('it1', async () => { + // await somePromise.then(() => { + // expect(someThing).toEqual(true) + // }); + // }); + // `, + // todo: async + // dedent` + // it('it1', async () => { + // await getSomeThing().getPromise().then(function () { + // expect(someThing).toEqual(true) + // }); + // }); + // `, dedent` it('it1', () => { return somePromise.then(() => { @@ -154,24 +204,26 @@ ruleTester.run('valid-expect-in-promise', rule, { }) }); `, - dedent` - test('later return', () => { - const promise = something().then(value => { - expect(value).toBe('red'); - }); - - return promise; - }); - `, - dedent` - test.only('later return', () => { - const promise = something().then(value => { - expect(value).toBe('red'); - }); - - return promise; - }); - `, + // todo: as variable + // dedent` + // test('later return', () => { + // const promise = something().then(value => { + // expect(value).toBe('red'); + // }); + // + // return promise; + // }); + // `, + // todo: as variable + // dedent` + // test.only('later return', () => { + // const promise = something().then(value => { + // expect(value).toBe('red'); + // }); + // + // return promise; + // }); + // `, dedent` it('shorthand arrow', () => something().then(value => { @@ -181,34 +233,37 @@ ruleTester.run('valid-expect-in-promise', rule, { }) ); `, - dedent` - it('promise test', () => { - const somePromise = getThatPromise(); - somePromise.then((data) => { - expect(data).toEqual('foo'); - }); - expect(somePromise).toBeDefined(); - return somePromise; - }); - `, - dedent` - test('promise test', function () { - let somePromise = getThatPromise(); - somePromise.then((data) => { - expect(data).toEqual('foo'); - }); - expect(somePromise).toBeDefined(); - return somePromise; - }); - `, - dedent` - it('crawls for files based on patterns', () => { - const promise = nodeCrawl({}).then(data => { - expect(childProcess.spawn).lastCalledWith('find'); - }); - return promise; - }); - `, + // todo: as variable + // dedent` + // it('promise test', () => { + // const somePromise = getThatPromise(); + // somePromise.then((data) => { + // expect(data).toEqual('foo'); + // }); + // expect(somePromise).toBeDefined(); + // return somePromise; + // }); + // `, + // todo: as variable + // dedent` + // test('promise test', function () { + // let somePromise = getThatPromise(); + // somePromise.then((data) => { + // expect(data).toEqual('foo'); + // }); + // expect(somePromise).toBeDefined(); + // return somePromise; + // }); + // `, + // todo: as variable + // dedent` + // it('crawls for files based on patterns', () => { + // const promise = nodeCrawl({}).then(data => { + // expect(childProcess.spawn).lastCalledWith('find'); + // }); + // return promise; + // }); + // `, dedent` it( 'test function', @@ -242,20 +297,40 @@ ruleTester.run('valid-expect-in-promise', rule, { })) `, 'it("it1", () => somePromise.then(() => expect(someThing).toEqual(true)))', - dedent` - it('promise test with done', (done) => { - const promise = getPromise(); - promise.then(() => expect(someThing).toEqual(true)); - }); - `, - dedent` - it('name of done param does not matter', (nameDoesNotMatter) => { - const promise = getPromise(); - promise.then(() => expect(someThing).toEqual(true)); - }); - `, + // todo: done callback + // dedent` + // it('promise test with done', (done) => { + // const promise = getPromise(); + // promise.then(() => expect(someThing).toEqual(true)); + // }); + // `, + // todo: done callback + // dedent` + // it('name of done param does not matter', (nameDoesNotMatter) => { + // const promise = getPromise(); + // promise.then(() => expect(someThing).toEqual(true)); + // }); + // `, ], invalid: [ + { + code: dedent` + const myFn = () => { + Promise.resolve().then(() => { + expect(true).toBe(false); + }); + }; + + it('it1', () => { + somePromise.then(() => { + expect(someThing).toEqual(true); + }); + }); + `, + errors: [ + { column: 3, endColumn: 6, messageId: 'returnPromise', line: 8 }, + ], + }, { code: dedent` it('it1', () => { @@ -276,16 +351,16 @@ ruleTester.run('valid-expect-in-promise', rule, { `, errors: [{ column: 3, endColumn: 6, messageId: 'returnPromise' }], }, - // { - // 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: 10, endColumn: 13, messageId: 'returnPromise' }], + }, { code: dedent` it('it1', function() { @@ -398,7 +473,7 @@ ruleTester.run('valid-expect-in-promise', rule, { }); }); `, - errors: [{ column: 9, endColumn: 5, messageId: 'returnPromise' }], + errors: [{ column: 3, endColumn: 6, messageId: 'returnPromise' }], }, { code: dedent` diff --git a/src/rules/valid-expect-in-promise.ts b/src/rules/valid-expect-in-promise.ts index 8466b2ccb..0f16425ee 100644 --- a/src/rules/valid-expect-in-promise.ts +++ b/src/rules/valid-expect-in-promise.ts @@ -4,12 +4,10 @@ import { TSESTree, } from '@typescript-eslint/experimental-utils'; import { - FunctionExpression, KnownCallExpression, createRule, getAccessorValue, - isExpectMember, - isFunction, + isExpectCall, isSupportedAccessor, isTestCaseCall, } from './utils'; @@ -29,27 +27,6 @@ const isPromiseChainCall = ( isSupportedAccessor(node.callee.property) && ['then', 'catch', 'finally'].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 false; - }); - } - - 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: { @@ -64,86 +41,26 @@ const reportReturnRequired = (context: RuleContext, node: TSESTree.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 findTestFunction = (node: TSESTree.Node | undefined) => { - while (node) { - if ( - isFunction(node) && - node.parent?.type === AST_NODE_TYPES.CallExpression && - isTestCaseCall(node.parent) - ) { - return node; - } - - node = node.parent; - } - - return null; -}; +const findTopOfBodyNode = ( + node: PromiseChainCallExpression, +): TSESTree.Statement | null => { + let { parent } = node; -const isParentThenOrPromiseReturned = ( - node: TSESTree.Node, - testFunctionBody: TSESTree.Statement[], -) => - node.type === AST_NODE_TYPES.ReturnStatement || - isPromiseReturnedLater(node, testFunctionBody); - -const verifyExpectWithReturn = ( - promiseCallbacks: Array, - node: PromiseChainCallExpression['callee'], - context: RuleContext, - testFunctionBody: TSESTree.Statement[], -) => { - promiseCallbacks.some(promiseCallback => { - if (promiseCallback && isFunction(promiseCallback)) { + while (parent) { + if (parent.parent && parent.parent.type === AST_NODE_TYPES.BlockStatement) { if ( - isExpectCallPresentInFunction(promiseCallback.body) && - node.parent.parent && - !isParentThenOrPromiseReturned(node.parent.parent, testFunctionBody) + parent.parent.parent?.parent?.type === AST_NODE_TYPES.CallExpression && + isTestCaseCall(parent.parent.parent?.parent) ) { - reportReturnRequired(context, node.parent.parent); - - return true; + return parent as TSESTree.Statement; } } - return false; - }); -}; + parent = parent.parent; + } -const isHavingAsyncCallBackParam = (testFunction: FunctionExpression) => - testFunction.params[0] && - testFunction.params[0].type === AST_NODE_TYPES.Identifier; + return null; +}; export default createRule({ name: __filename, @@ -162,35 +79,41 @@ export default createRule({ }, defaultOptions: [], create(context) { + let inPromiseChain = false; + let hasExpectCall = false; + return { CallExpression(node) { - if ( - !isPromiseChainCall(node) || - (node.parent && node.parent.type === AST_NODE_TYPES.AwaitExpression) - ) { + if (isPromiseChainCall(node)) { + inPromiseChain = true; + return; } - const testFunction = findTestFunction(node); - if (testFunction && !isHavingAsyncCallBackParam(testFunction)) { - const { body } = testFunction; + if (!inPromiseChain) { + return; + } - if (body.type !== AST_NODE_TYPES.BlockStatement) { - return; - } + if (isExpectCall(node)) { + hasExpectCall = true; - const testFunctionBody = body.body; - - // 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( - node.arguments.slice(0, 2), - node.callee, - context, - testFunctionBody, - ); + return; + } + }, + 'CallExpression:exit'(node: TSESTree.CallExpression) { + if (isPromiseChainCall(node)) { + inPromiseChain = false; + + if (hasExpectCall) { + const topNode = findTopOfBodyNode(node); + + if (!topNode) { + return; + } + if (topNode.type !== AST_NODE_TYPES.ReturnStatement) { + reportReturnRequired(context, topNode); + } + } } }, }; From f88a6e650bd3f2ac850795b42212095aafe40ac3 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Sun, 26 Sep 2021 16:30:27 +1300 Subject: [PATCH 02/23] fix(valid-expect-in-promise): check number of arguments being passed --- .../__tests__/valid-expect-in-promise.test.ts | 17 ++++++------ src/rules/valid-expect-in-promise.ts | 27 +++++++++++++++---- 2 files changed, 30 insertions(+), 14 deletions(-) diff --git a/src/rules/__tests__/valid-expect-in-promise.test.ts b/src/rules/__tests__/valid-expect-in-promise.test.ts index ee8ee273c..ea08e7a40 100644 --- a/src/rules/__tests__/valid-expect-in-promise.test.ts +++ b/src/rules/__tests__/valid-expect-in-promise.test.ts @@ -136,15 +136,14 @@ ruleTester.run('valid-expect-in-promise', rule, { }); }); `, - // todo: tighter "is promise" check - // dedent` - // it('it1', function () { - // Promise.resolve().then(/*fulfillment*/ function () { - // }, undefined, /*rejection*/ function () { - // expect(someThing).toEqual(true) - // }) - // }); - // `, + dedent` + it('it1', function () { + Promise.resolve().then(/*fulfillment*/ function () { + }, undefined, /*rejection*/ function () { + expect(someThing).toEqual(true) + }) + }); + `, dedent` it('it1', function () { return Promise.resolve().then(function () { diff --git a/src/rules/valid-expect-in-promise.ts b/src/rules/valid-expect-in-promise.ts index 0f16425ee..0fc320081 100644 --- a/src/rules/valid-expect-in-promise.ts +++ b/src/rules/valid-expect-in-promise.ts @@ -21,11 +21,28 @@ type PromiseChainCallExpression = KnownCallExpression< const isPromiseChainCall = ( node: TSESTree.Node, -): node is PromiseChainCallExpression => - node.type === AST_NODE_TYPES.CallExpression && - node.callee.type === AST_NODE_TYPES.MemberExpression && - isSupportedAccessor(node.callee.property) && - ['then', 'catch', 'finally'].includes(getAccessorValue(node.callee.property)); +): node is PromiseChainCallExpression => { + if ( + node.type === AST_NODE_TYPES.CallExpression && + node.callee.type === AST_NODE_TYPES.MemberExpression && + isSupportedAccessor(node.callee.property) + ) { + // promise methods should have at least 1 argument + if (node.arguments.length === 0) { + return false; + } + + switch (getAccessorValue(node.callee.property)) { + case 'then': + return node.arguments.length < 3; + case 'catch': + case 'finally': + return node.arguments.length < 2; + } + } + + return false; +}; const reportReturnRequired = (context: RuleContext, node: TSESTree.Node) => { context.report({ From 4f7357f4ee2f144de4a053ea4a2301482eb359e4 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Sun, 26 Sep 2021 16:54:41 +1300 Subject: [PATCH 03/23] fix(valid-expect-in-promise): bailout if `done` callback is present --- .../__tests__/valid-expect-in-promise.test.ts | 57 +++++++++++-------- src/rules/valid-expect-in-promise.ts | 52 +++++++++++++++++ 2 files changed, 85 insertions(+), 24 deletions(-) diff --git a/src/rules/__tests__/valid-expect-in-promise.test.ts b/src/rules/__tests__/valid-expect-in-promise.test.ts index ea08e7a40..f4be3a060 100644 --- a/src/rules/__tests__/valid-expect-in-promise.test.ts +++ b/src/rules/__tests__/valid-expect-in-promise.test.ts @@ -12,16 +12,15 @@ const ruleTester = new TSESLint.RuleTester({ ruleTester.run('valid-expect-in-promise', rule, { valid: [ - // todo: done callback - // dedent` - // it('it1', () => new Promise((done) => { - // test() - // .then(() => { - // expect(someThing).toEqual(true); - // done(); - // }); - // })); - // `, + dedent` + it('it1', () => new Promise((done) => { + test() + .then(() => { + expect(someThing).toEqual(true); + done(); + }); + })); + `, dedent` it('passes', () => { Promise.resolve().then(() => { @@ -296,20 +295,30 @@ ruleTester.run('valid-expect-in-promise', rule, { })) `, 'it("it1", () => somePromise.then(() => expect(someThing).toEqual(true)))', - // todo: done callback - // dedent` - // it('promise test with done', (done) => { - // const promise = getPromise(); - // promise.then(() => expect(someThing).toEqual(true)); - // }); - // `, - // todo: done callback - // dedent` - // it('name of done param does not matter', (nameDoesNotMatter) => { - // const promise = getPromise(); - // promise.then(() => expect(someThing).toEqual(true)); - // }); - // `, + dedent` + it('promise test with done', (done) => { + const promise = getPromise(); + promise.then(() => expect(someThing).toEqual(true)); + }); + `, + dedent` + it('name of done param does not matter', (nameDoesNotMatter) => { + const promise = getPromise(); + promise.then(() => expect(someThing).toEqual(true)); + }); + `, + dedent` + it.each([])('name of done param does not matter', (nameDoesNotMatter) => { + const promise = getPromise(); + promise.then(() => expect(someThing).toEqual(true)); + }); + `, + dedent` + it.each\`\`('name of done param does not matter', ({}, nameDoesNotMatter) => { + const promise = getPromise(); + promise.then(() => expect(someThing).toEqual(true)); + }); + `, ], invalid: [ { diff --git a/src/rules/valid-expect-in-promise.ts b/src/rules/valid-expect-in-promise.ts index 0fc320081..470c5ef10 100644 --- a/src/rules/valid-expect-in-promise.ts +++ b/src/rules/valid-expect-in-promise.ts @@ -7,7 +7,9 @@ import { KnownCallExpression, createRule, getAccessorValue, + getNodeName, isExpectCall, + isFunction, isSupportedAccessor, isTestCaseCall, } from './utils'; @@ -79,6 +81,41 @@ const findTopOfBodyNode = ( return null; }; +const isTestCaseCallWithCallbackArg = ( + node: TSESTree.CallExpression, +): boolean => { + if (!isTestCaseCall(node)) { + return false; + } + + const isJestEach = getNodeName(node).endsWith('.each'); + + if ( + isJestEach && + node.callee.type !== AST_NODE_TYPES.TaggedTemplateExpression + ) { + // isJestEach but not a TaggedTemplateExpression, so this must be + // the `jest.each([])()` syntax which this rule doesn't support due + // to its complexity (see jest-community/eslint-plugin-jest#710) + // so we return true to trigger bailout + return true; + } + + if (isJestEach || node.arguments.length >= 2) { + const [, callback] = node.arguments; + + const callbackArgIndex = Number(isJestEach); + + return ( + callback && + isFunction(callback) && + callback.params.length === 1 + callbackArgIndex + ); + } + + return false; +}; + export default createRule({ name: __filename, meta: { @@ -96,11 +133,18 @@ export default createRule({ }, defaultOptions: [], create(context) { + let inTestCaseWithDoneCallback = false; let inPromiseChain = false; let hasExpectCall = false; return { CallExpression(node) { + if (isTestCaseCallWithCallbackArg(node)) { + inTestCaseWithDoneCallback = true; + + return; + } + if (isPromiseChainCall(node)) { inPromiseChain = true; @@ -118,6 +162,14 @@ export default createRule({ } }, 'CallExpression:exit'(node: TSESTree.CallExpression) { + if (inTestCaseWithDoneCallback) { + if (isTestCaseCall(node)) { + inTestCaseWithDoneCallback = false; + } + + return; + } + if (isPromiseChainCall(node)) { inPromiseChain = false; From 5d1d41938cf795ab74296567d9fb55bae5c92b9b Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Sun, 26 Sep 2021 17:08:50 +1300 Subject: [PATCH 04/23] fix(valid-expect-in-promise): allow awaited promises --- .../__tests__/valid-expect-in-promise.test.ts | 72 ++++++++++++------- src/rules/valid-expect-in-promise.ts | 12 ++-- 2 files changed, 54 insertions(+), 30 deletions(-) diff --git a/src/rules/__tests__/valid-expect-in-promise.test.ts b/src/rules/__tests__/valid-expect-in-promise.test.ts index f4be3a060..2a550bd35 100644 --- a/src/rules/__tests__/valid-expect-in-promise.test.ts +++ b/src/rules/__tests__/valid-expect-in-promise.test.ts @@ -158,30 +158,27 @@ ruleTester.run('valid-expect-in-promise', rule, { return somePromise.then() }); `, - // todo: async - // dedent` - // it('it1', async () => { - // await Promise.resolve().then(function () { - // expect(someThing).toEqual(true) - // }); - // }); - // `, - // todo: async - // dedent` - // it('it1', async () => { - // await somePromise.then(() => { - // expect(someThing).toEqual(true) - // }); - // }); - // `, - // todo: async - // dedent` - // it('it1', async () => { - // await getSomeThing().getPromise().then(function () { - // expect(someThing).toEqual(true) - // }); - // }); - // `, + dedent` + it('it1', async () => { + await Promise.resolve().then(function () { + expect(someThing).toEqual(true) + }); + }); + `, + dedent` + it('it1', async () => { + await somePromise.then(() => { + expect(someThing).toEqual(true) + }); + }); + `, + dedent` + it('it1', async () => { + await getSomeThing().getPromise().then(function () { + expect(someThing).toEqual(true) + }); + }); + `, dedent` it('it1', () => { return somePromise.then(() => { @@ -319,6 +316,15 @@ ruleTester.run('valid-expect-in-promise', rule, { promise.then(() => expect(someThing).toEqual(true)); }); `, + dedent` + test('valid-expect-in-promise', async () => { + const text = await fetch('url') + .then(res => res.text()) + .then(text => text); + + expect(text).toBe('text'); + }); + `, ], invalid: [ { @@ -445,10 +451,24 @@ ruleTester.run('valid-expect-in-promise', rule, { { code: dedent` it('test function', () => { - Builder.getPromiseBuilder().get().build().then((data) => expect(data).toEqual('Hi')); + Builder.getPromiseBuilder() + .get() + .build() + .then(data => expect(data).toEqual('Hi')); + }); + `, + errors: [{ column: 3, endColumn: 47, messageId: 'returnPromise' }], + }, + { + code: ` + it('test function', async () => { + Builder.getPromiseBuilder() + .get() + .build() + .then(data => expect(data).toEqual('Hi')); }); `, - errors: [{ column: 3, endColumn: 88, messageId: 'returnPromise' }], + errors: [{ column: 11, endColumn: 55, messageId: 'returnPromise' }], }, { code: dedent` diff --git a/src/rules/valid-expect-in-promise.ts b/src/rules/valid-expect-in-promise.ts index 470c5ef10..b1c4a4b51 100644 --- a/src/rules/valid-expect-in-promise.ts +++ b/src/rules/valid-expect-in-promise.ts @@ -176,12 +176,16 @@ export default createRule({ if (hasExpectCall) { const topNode = findTopOfBodyNode(node); - if (!topNode) { + if ( + !topNode || + topNode.type === AST_NODE_TYPES.ReturnStatement || + (topNode.type === AST_NODE_TYPES.ExpressionStatement && + topNode.expression.type === AST_NODE_TYPES.AwaitExpression) + ) { return; } - if (topNode.type !== AST_NODE_TYPES.ReturnStatement) { - reportReturnRequired(context, topNode); - } + + reportReturnRequired(context, topNode); } } }, From c914556721508280830f208cacaaff122db82b35 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Sun, 3 Oct 2021 09:09:21 +1300 Subject: [PATCH 05/23] fix(valid-expect-in-promise): handle multi-chained promises properly Method call chains are represented in AST in a nested fashion, meaning it's not enough to just separately track if we're both in a promise chain call and then if we've found an `expect` call, because when we exit another CallExpression in the same chain it'll look like that has an `expect` call. Instead, we need to track our depth as we enter CallExpression nodes so that when we exit those nodes we can check if we encountered an `expect` call at that same depth. --- .../__tests__/valid-expect-in-promise.test.ts | 122 ++++++++++++++++++ src/rules/valid-expect-in-promise.ts | 13 +- 2 files changed, 127 insertions(+), 8 deletions(-) diff --git a/src/rules/__tests__/valid-expect-in-promise.test.ts b/src/rules/__tests__/valid-expect-in-promise.test.ts index 2a550bd35..5dbb0c7fd 100644 --- a/src/rules/__tests__/valid-expect-in-promise.test.ts +++ b/src/rules/__tests__/valid-expect-in-promise.test.ts @@ -189,6 +189,26 @@ ruleTester.run('valid-expect-in-promise', rule, { }) }); `, + dedent` + it('it1', () => { + return somePromise.then(() => { + return value; + }) + .then(value => { + expect(someThing).toEqual(value); + }) + }); + `, + dedent` + it('it1', () => { + return somePromise.then(() => { + expect(someThing).toEqual(true); + }) + .then(() => { + console.log('this is silly'); + }) + }); + `, dedent` it('it1', () => { return somePromise.then(() => { @@ -481,6 +501,108 @@ ruleTester.run('valid-expect-in-promise', rule, { `, errors: [{ column: 3, endColumn: 5, messageId: 'returnPromise' }], }, + { + code: dedent` + it('is a test', () => { + somePromise + .then(() => {}) + .then(() => expect(someThing).toEqual(value)) + }); + `, + errors: [{ column: 3, endColumn: 50, messageId: 'returnPromise' }], + }, + { + code: dedent` + it('is a test', () => { + somePromise + .then(() => expect(someThing).toEqual(value)) + .then(() => {}) + }); + `, + errors: [{ column: 3, endColumn: 20, messageId: 'returnPromise' }], + }, + { + code: dedent` + it('is a test', () => { + somePromise.then(() => { + return value; + }) + .then(value => { + expect(someThing).toEqual(value); + }) + }); + `, + errors: [{ column: 3, endColumn: 5, messageId: 'returnPromise' }], + }, + { + code: dedent` + it('is a test', () => { + somePromise.then(() => { + expect(someThing).toEqual(true); + }) + .then(() => { + console.log('this is silly'); + }) + }); + `, + errors: [{ column: 3, endColumn: 5, messageId: 'returnPromise' }], + }, + { + code: dedent` + it('is a test', () => { + somePromise.then(() => { + // return value; + }) + .then(value => { + expect(someThing).toEqual(value); + }) + }); + `, + errors: [{ column: 3, endColumn: 5, messageId: 'returnPromise' }], + }, + { + code: dedent` + it('is a test', () => { + somePromise.then(() => { + return value; + }) + .then(value => { + expect(someThing).toEqual(value); + }) + + return anotherPromise.then(() => expect(x).toBe(y)); + }); + `, + errors: [{ column: 3, endColumn: 5, messageId: 'returnPromise' }], + }, + { + code: dedent` + it('is a test', () => { + somePromise + .then(() => 1) + .then(x => x + 1) + .catch(() => -1) + .then(v => expect(v).toBe(2)); + + return anotherPromise.then(() => expect(x).toBe(y)); + }); + `, + errors: [{ column: 3, endColumn: 35, messageId: 'returnPromise' }], + }, + { + code: dedent` + it('is a test', () => { + somePromise + .then(() => 1) + .then(v => expect(v).toBe(2)) + .then(x => x + 1) + .catch(() => -1); + + return anotherPromise.then(() => expect(x).toBe(y)); + }); + `, + errors: [{ column: 3, endColumn: 22, messageId: 'returnPromise' }], + }, { code: dedent` it('it1', () => { diff --git a/src/rules/valid-expect-in-promise.ts b/src/rules/valid-expect-in-promise.ts index b1c4a4b51..7d5affe9c 100644 --- a/src/rules/valid-expect-in-promise.ts +++ b/src/rules/valid-expect-in-promise.ts @@ -134,8 +134,7 @@ export default createRule({ defaultOptions: [], create(context) { let inTestCaseWithDoneCallback = false; - let inPromiseChain = false; - let hasExpectCall = false; + const chains: boolean[] = []; return { CallExpression(node) { @@ -146,17 +145,17 @@ export default createRule({ } if (isPromiseChainCall(node)) { - inPromiseChain = true; + chains.unshift(false); return; } - if (!inPromiseChain) { + if (chains.length === 0) { return; } if (isExpectCall(node)) { - hasExpectCall = true; + chains[0] = true; return; } @@ -171,9 +170,7 @@ export default createRule({ } if (isPromiseChainCall(node)) { - inPromiseChain = false; - - if (hasExpectCall) { + if (chains.shift()) { const topNode = findTopOfBodyNode(node); if ( From 2973335558a96f1f62d03a0766a6424ba3b1792d Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Sun, 3 Oct 2021 10:30:11 +1300 Subject: [PATCH 06/23] fix(valid-expect-in-promise): handle promises assigned to variables --- .../__tests__/valid-expect-in-promise.test.ts | 133 +++++++++++------- src/rules/valid-expect-in-promise.ts | 70 ++++++++- 2 files changed, 146 insertions(+), 57 deletions(-) diff --git a/src/rules/__tests__/valid-expect-in-promise.test.ts b/src/rules/__tests__/valid-expect-in-promise.test.ts index 5dbb0c7fd..7d4c636b7 100644 --- a/src/rules/__tests__/valid-expect-in-promise.test.ts +++ b/src/rules/__tests__/valid-expect-in-promise.test.ts @@ -219,26 +219,42 @@ ruleTester.run('valid-expect-in-promise', rule, { }) }); `, - // todo: as variable - // dedent` - // test('later return', () => { - // const promise = something().then(value => { - // expect(value).toBe('red'); - // }); - // - // return promise; - // }); - // `, - // todo: as variable - // dedent` - // test.only('later return', () => { - // const promise = something().then(value => { - // expect(value).toBe('red'); - // }); - // - // return promise; - // }); - // `, + dedent` + test('later return', () => { + const promise = something().then(value => { + expect(value).toBe('red'); + }); + + return promise; + }); + `, + dedent` + test('later return', async () => { + const promise = something().then(value => { + expect(value).toBe('red'); + }); + + await promise; + }); + `, + dedent` + test.only('later return', () => { + const promise = something().then(value => { + expect(value).toBe('red'); + }); + + return promise; + }); + `, + dedent` + test('that we bailout if destructuring is used', () => { + const [promise] = [ + something().then(value => { + expect(value).toBe('red'); + }) + ]; + }); + `, dedent` it('shorthand arrow', () => something().then(value => { @@ -248,37 +264,14 @@ ruleTester.run('valid-expect-in-promise', rule, { }) ); `, - // todo: as variable - // dedent` - // it('promise test', () => { - // const somePromise = getThatPromise(); - // somePromise.then((data) => { - // expect(data).toEqual('foo'); - // }); - // expect(somePromise).toBeDefined(); - // return somePromise; - // }); - // `, - // todo: as variable - // dedent` - // test('promise test', function () { - // let somePromise = getThatPromise(); - // somePromise.then((data) => { - // expect(data).toEqual('foo'); - // }); - // expect(somePromise).toBeDefined(); - // return somePromise; - // }); - // `, - // todo: as variable - // dedent` - // it('crawls for files based on patterns', () => { - // const promise = nodeCrawl({}).then(data => { - // expect(childProcess.spawn).lastCalledWith('find'); - // }); - // return promise; - // }); - // `, + dedent` + it('crawls for files based on patterns', () => { + const promise = nodeCrawl({}).then(data => { + expect(childProcess.spawn).lastCalledWith('find'); + }); + return promise; + }); + `, dedent` it( 'test function', @@ -647,5 +640,43 @@ ruleTester.run('valid-expect-in-promise', rule, { `, errors: [{ column: 3, endColumn: 5, messageId: 'returnPromise' }], }, + { + code: dedent` + test('later return', async () => { + const promise = something().then(value => { + expect(value).toBe('red'); + }); + + promise; + }); + `, + errors: [{ column: 3, endColumn: 6, messageId: 'returnPromise' }], + }, + { + code: dedent` + it('promise test', () => { + const somePromise = getThatPromise(); + somePromise.then((data) => { + expect(data).toEqual('foo'); + }); + expect(somePromise).toBeDefined(); + return somePromise; + }); + `, + errors: [{ column: 3, endColumn: 6, messageId: 'returnPromise' }], + }, + { + code: dedent` + test('promise test', function () { + let somePromise = getThatPromise(); + somePromise.then((data) => { + expect(data).toEqual('foo'); + }); + expect(somePromise).toBeDefined(); + return somePromise; + }); + `, + errors: [{ column: 3, endColumn: 6, messageId: 'returnPromise' }], + }, ], }); diff --git a/src/rules/valid-expect-in-promise.ts b/src/rules/valid-expect-in-promise.ts index 7d5affe9c..5d96618c1 100644 --- a/src/rules/valid-expect-in-promise.ts +++ b/src/rules/valid-expect-in-promise.ts @@ -10,6 +10,7 @@ import { getNodeName, isExpectCall, isFunction, + isIdentifier, isSupportedAccessor, isTestCaseCall, } from './utils'; @@ -60,9 +61,13 @@ const reportReturnRequired = (context: RuleContext, node: TSESTree.Node) => { }); }; -const findTopOfBodyNode = ( +type StartingStatementNode = TSESTree.Statement & { + parent: TSESTree.BlockStatement; +}; + +const findStartingStatementInTestBody = ( node: PromiseChainCallExpression, -): TSESTree.Statement | null => { +): StartingStatementNode | null => { let { parent } = node; while (parent) { @@ -71,7 +76,7 @@ const findTopOfBodyNode = ( parent.parent.parent?.parent?.type === AST_NODE_TYPES.CallExpression && isTestCaseCall(parent.parent.parent?.parent) ) { - return parent as TSESTree.Statement; + return parent as StartingStatementNode; } } @@ -116,6 +121,49 @@ const isTestCaseCallWithCallbackArg = ( return false; }; +const getVariableName = ( + variable: TSESTree.VariableDeclarator, +): string | null => { + if (isIdentifier(variable.id)) { + return variable.id.name; + } + + return null; +}; + +const isVariableAwaitedOrReturned = ( + variables: TSESTree.VariableDeclaration & { parent: TSESTree.BlockStatement }, +): boolean => { + const { body } = variables.parent; + const [variable] = variables.declarations; + const name = getVariableName(variable); + + // null means that the variable is destructured, which is pretty much impossible + // for us to track, so we return true to bailout gracefully + if (name === null) { + return true; + } + + for (const node of body) { + if ( + node.type === AST_NODE_TYPES.ReturnStatement && + node.argument?.type === AST_NODE_TYPES.Identifier + ) { + return isIdentifier(node.argument, name); + } + + if ( + node.type === AST_NODE_TYPES.ExpressionStatement && + node.expression.type === AST_NODE_TYPES.AwaitExpression && + node.expression.argument?.type === AST_NODE_TYPES.Identifier + ) { + return isIdentifier(node.expression.argument, name); + } + } + + return false; +}; + export default createRule({ name: __filename, meta: { @@ -137,7 +185,7 @@ export default createRule({ const chains: boolean[] = []; return { - CallExpression(node) { + CallExpression(node: TSESTree.CallExpression) { if (isTestCaseCallWithCallbackArg(node)) { inTestCaseWithDoneCallback = true; @@ -171,10 +219,20 @@ export default createRule({ if (isPromiseChainCall(node)) { if (chains.shift()) { - const topNode = findTopOfBodyNode(node); + const topNode = findStartingStatementInTestBody(node); + + if (!topNode) { + return; + } + + if ( + topNode.type === AST_NODE_TYPES.VariableDeclaration && + isVariableAwaitedOrReturned(topNode) + ) { + return; + } if ( - !topNode || topNode.type === AST_NODE_TYPES.ReturnStatement || (topNode.type === AST_NODE_TYPES.ExpressionStatement && topNode.expression.type === AST_NODE_TYPES.AwaitExpression) From c5d0ffc2a36c9a59566e878ed9e54e65a07c3fd9 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Sun, 3 Oct 2021 10:33:42 +1300 Subject: [PATCH 07/23] refactor(valid-expect-in-promise): adjust conditions to dedent code --- src/rules/valid-expect-in-promise.ts | 50 ++++++++++++++-------------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/src/rules/valid-expect-in-promise.ts b/src/rules/valid-expect-in-promise.ts index 5d96618c1..8100ee803 100644 --- a/src/rules/valid-expect-in-promise.ts +++ b/src/rules/valid-expect-in-promise.ts @@ -217,32 +217,32 @@ export default createRule({ return; } - if (isPromiseChainCall(node)) { - if (chains.shift()) { - const topNode = findStartingStatementInTestBody(node); - - if (!topNode) { - return; - } - - if ( - topNode.type === AST_NODE_TYPES.VariableDeclaration && - isVariableAwaitedOrReturned(topNode) - ) { - return; - } - - if ( - topNode.type === AST_NODE_TYPES.ReturnStatement || - (topNode.type === AST_NODE_TYPES.ExpressionStatement && - topNode.expression.type === AST_NODE_TYPES.AwaitExpression) - ) { - return; - } - - reportReturnRequired(context, topNode); - } + if (!isPromiseChainCall(node) || !chains.shift()) { + return; + } + + const topNode = findStartingStatementInTestBody(node); + + if (!topNode) { + return; } + + if ( + topNode.type === AST_NODE_TYPES.VariableDeclaration && + isVariableAwaitedOrReturned(topNode) + ) { + return; + } + + if ( + topNode.type === AST_NODE_TYPES.ReturnStatement || + (topNode.type === AST_NODE_TYPES.ExpressionStatement && + topNode.expression.type === AST_NODE_TYPES.AwaitExpression) + ) { + return; + } + + reportReturnRequired(context, topNode); }, }; }, From 48a62c2392a6671b1f844ff492dd523837863451 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Sun, 3 Oct 2021 10:36:22 +1300 Subject: [PATCH 08/23] fix(valid-expect-in-promise): allow variables assigned awaited promises --- .../__tests__/valid-expect-in-promise.test.ts | 38 +++++++++++++++++++ src/rules/valid-expect-in-promise.ts | 4 ++ 2 files changed, 42 insertions(+) diff --git a/src/rules/__tests__/valid-expect-in-promise.test.ts b/src/rules/__tests__/valid-expect-in-promise.test.ts index 7d4c636b7..28143a180 100644 --- a/src/rules/__tests__/valid-expect-in-promise.test.ts +++ b/src/rules/__tests__/valid-expect-in-promise.test.ts @@ -272,6 +272,44 @@ ruleTester.run('valid-expect-in-promise', rule, { return promise; }); `, + dedent` + it('is a test', async () => { + const value = await somePromise().then(response => { + expect(response).toHaveProperty('data'); + + return response.data; + }); + + expect(value).toBe('hello world'); + }); + `, + dedent` + it('is a test', async () => { + return await somePromise().then(response => { + expect(response).toHaveProperty('data'); + + return response.data; + }); + }); + `, + dedent` + it('is a test', async () => { + return somePromise().then(response => { + expect(response).toHaveProperty('data'); + + return response.data; + }); + }); + `, + dedent` + it('is a test', async () => { + await somePromise().then(response => { + expect(response).toHaveProperty('data'); + + return response.data; + }); + }); + `, dedent` it( 'test function', diff --git a/src/rules/valid-expect-in-promise.ts b/src/rules/valid-expect-in-promise.ts index 8100ee803..f1e7fa049 100644 --- a/src/rules/valid-expect-in-promise.ts +++ b/src/rules/valid-expect-in-promise.ts @@ -138,6 +138,10 @@ const isVariableAwaitedOrReturned = ( const [variable] = variables.declarations; const name = getVariableName(variable); + if (variable.init?.type === AST_NODE_TYPES.AwaitExpression) { + return true; + } + // null means that the variable is destructured, which is pretty much impossible // for us to track, so we return true to bailout gracefully if (name === null) { From 21750258924fb5ecf185b371a24ceb5326fa1e3a Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Sun, 3 Oct 2021 11:29:56 +1300 Subject: [PATCH 09/23] feat(valid-expect-in-promise): track promise vars across reassignments --- .../__tests__/valid-expect-in-promise.test.ts | 87 +++++++++++++++++ src/rules/valid-expect-in-promise.ts | 94 ++++++++++++------- 2 files changed, 146 insertions(+), 35 deletions(-) diff --git a/src/rules/__tests__/valid-expect-in-promise.test.ts b/src/rules/__tests__/valid-expect-in-promise.test.ts index 28143a180..5f57792b0 100644 --- a/src/rules/__tests__/valid-expect-in-promise.test.ts +++ b/src/rules/__tests__/valid-expect-in-promise.test.ts @@ -376,6 +376,47 @@ ruleTester.run('valid-expect-in-promise', rule, { expect(text).toBe('text'); }); `, + dedent` + test('promise test', async function () { + let somePromise = getPromise().then((data) => { + expect(data).toEqual('foo'); + }); + + await somePromise; + + somePromise = getPromise().then((data) => { + expect(data).toEqual('foo'); + }); + + await somePromise; + }); + `, + dedent` + test('promise test', async function () { + let somePromise = getPromise().then((data) => { + expect(data).toEqual('foo'); + }); + + await somePromise; + + somePromise = getPromise().then((data) => { + expect(data).toEqual('foo'); + }); + + return somePromise; + }); + `, + dedent` + test('promise test', async function () { + let somePromise = getPromise().then((data) => { + expect(data).toEqual('foo'); + }); + + {} + + await somePromise; + }); + `, ], invalid: [ { @@ -716,5 +757,51 @@ ruleTester.run('valid-expect-in-promise', rule, { `, errors: [{ column: 3, endColumn: 6, messageId: 'returnPromise' }], }, + { + code: dedent` + test('promise test', async function () { + let somePromise = getPromise().then((data) => { + expect(data).toEqual('foo'); + }); + + somePromise = null; + + await somePromise; + }); + `, + errors: [{ column: 3, endColumn: 6, messageId: 'returnPromise' }], + }, + { + code: dedent` + test('promise test', async function () { + let somePromise = getPromise().then((data) => { + expect(data).toEqual('foo'); + }); + + somePromise = getPromise().then((data) => { + expect(data).toEqual('foo'); + }); + + await somePromise; + }); + `, + errors: [ + { column: 3, endColumn: 6, line: 2, messageId: 'returnPromise' }, + ], + }, + { + code: dedent` + test('promise test', async function () { + let somePromise = getPromise().then((data) => { + expect(data).toEqual('foo'); + }); + + ({ somePromise } = {}) + }); + `, + errors: [ + { column: 3, endColumn: 6, line: 2, messageId: 'returnPromise' }, + ], + }, ], }); diff --git a/src/rules/valid-expect-in-promise.ts b/src/rules/valid-expect-in-promise.ts index f1e7fa049..c2b519b53 100644 --- a/src/rules/valid-expect-in-promise.ts +++ b/src/rules/valid-expect-in-promise.ts @@ -121,14 +121,47 @@ const isTestCaseCallWithCallbackArg = ( return false; }; -const getVariableName = ( - variable: TSESTree.VariableDeclarator, -): string | null => { - if (isIdentifier(variable.id)) { - return variable.id.name; +/** + * Attempts to determine if the runtime value represented by the given `identifier` + * is `await`ed or `return`ed within the given `body` of statements + */ +const isValueAwaitedOrReturned = ( + identifier: TSESTree.Identifier, + body: TSESTree.Statement[], +): boolean => { + const { name } = identifier; + + for (const node of body) { + // skip all nodes that are before this identifier, because they'd probably + // be affecting a different runtime value (e.g. due to reassignment) + if (node.range[0] <= identifier.range[0]) { + continue; + } + + if ( + node.type === AST_NODE_TYPES.ReturnStatement && + node.argument?.type === AST_NODE_TYPES.Identifier + ) { + return isIdentifier(node.argument, name); + } + + if (node.type === AST_NODE_TYPES.ExpressionStatement) { + if ( + node.expression.type === AST_NODE_TYPES.AwaitExpression && + node.expression.argument?.type === AST_NODE_TYPES.Identifier + ) { + return isIdentifier(node.expression.argument, name); + } + + // (re)assignment changes the runtime value, so if we've not found an + // await or return already we act as if we've reached the end of the body + if (node.expression.type === AST_NODE_TYPES.AssignmentExpression) { + break; + } + } } - return null; + return false; }; const isVariableAwaitedOrReturned = ( @@ -136,36 +169,18 @@ const isVariableAwaitedOrReturned = ( ): boolean => { const { body } = variables.parent; const [variable] = variables.declarations; - const name = getVariableName(variable); if (variable.init?.type === AST_NODE_TYPES.AwaitExpression) { return true; } - // null means that the variable is destructured, which is pretty much impossible - // for us to track, so we return true to bailout gracefully - if (name === null) { + // it's pretty much impossible for us to track destructuring assignments, + // so we return true to bailout gracefully + if (!isIdentifier(variable.id)) { return true; } - for (const node of body) { - if ( - node.type === AST_NODE_TYPES.ReturnStatement && - node.argument?.type === AST_NODE_TYPES.Identifier - ) { - return isIdentifier(node.argument, name); - } - - if ( - node.type === AST_NODE_TYPES.ExpressionStatement && - node.expression.type === AST_NODE_TYPES.AwaitExpression && - node.expression.argument?.type === AST_NODE_TYPES.Identifier - ) { - return isIdentifier(node.expression.argument, name); - } - } - - return false; + return isValueAwaitedOrReturned(variable.id, body); }; export default createRule({ @@ -227,7 +242,7 @@ export default createRule({ const topNode = findStartingStatementInTestBody(node); - if (!topNode) { + if (!topNode || topNode.type === AST_NODE_TYPES.ReturnStatement) { return; } @@ -238,12 +253,21 @@ export default createRule({ return; } - if ( - topNode.type === AST_NODE_TYPES.ReturnStatement || - (topNode.type === AST_NODE_TYPES.ExpressionStatement && - topNode.expression.type === AST_NODE_TYPES.AwaitExpression) - ) { - return; + if (topNode.type === AST_NODE_TYPES.ExpressionStatement) { + if (topNode.expression.type === AST_NODE_TYPES.AwaitExpression) { + return; + } + + if ( + topNode.expression.type === AST_NODE_TYPES.AssignmentExpression && + topNode.expression.left.type === AST_NODE_TYPES.Identifier && + isValueAwaitedOrReturned( + topNode.expression.left, + topNode.parent.body, + ) + ) { + return; + } } reportReturnRequired(context, topNode); From f5c00209be115cd474316fd4f03324bc259fb467 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Sun, 3 Oct 2021 13:49:31 +1300 Subject: [PATCH 10/23] feat(valid-expect-in-promise): support blocks and multi-variable assigns --- .../__tests__/valid-expect-in-promise.test.ts | 190 +++++++++++++++++- src/rules/valid-expect-in-promise.ts | 135 +++++++++---- 2 files changed, 278 insertions(+), 47 deletions(-) diff --git a/src/rules/__tests__/valid-expect-in-promise.test.ts b/src/rules/__tests__/valid-expect-in-promise.test.ts index 5f57792b0..01cd16418 100644 --- a/src/rules/__tests__/valid-expect-in-promise.test.ts +++ b/src/rules/__tests__/valid-expect-in-promise.test.ts @@ -12,6 +12,9 @@ const ruleTester = new TSESLint.RuleTester({ ruleTester.run('valid-expect-in-promise', rule, { valid: [ + "test('something', () => Promise.resolve().then(() => expect(1).toBe(2)));", + 'Promise.resolve().then(() => expect(1).toBe(2))', + 'const x = Promise.resolve().then(() => expect(1).toBe(2))', dedent` it('it1', () => new Promise((done) => { test() @@ -21,6 +24,16 @@ ruleTester.run('valid-expect-in-promise', rule, { }); })); `, + dedent` + it('it1', () => { + return new Promise(done => { + test().then(() => { + expect(someThing).toEqual(true); + done(); + }); + }); + }); + `, dedent` it('passes', () => { Promise.resolve().then(() => { @@ -246,6 +259,20 @@ ruleTester.run('valid-expect-in-promise', rule, { return promise; }); `, + dedent` + test('that we bailout if destructuring is used', () => { + const [promise] = something().then(value => { + expect(value).toBe('red'); + }); + }); + `, + dedent` + test('that we bailout if destructuring is used', async () => { + const [promise] = await something().then(value => { + expect(value).toBe('red'); + }); + }); + `, dedent` test('that we bailout if destructuring is used', () => { const [promise] = [ @@ -255,6 +282,25 @@ ruleTester.run('valid-expect-in-promise', rule, { ]; }); `, + dedent` + test('that we bailout if destructuring is used', () => { + const {promise} = { + promise: something().then(value => { + expect(value).toBe('red'); + }) + }; + }); + `, + dedent` + test('that we bailout in complex cases', () => { + promiseSomething({ + timeout: 500, + promise: something().then(value => { + expect(value).toBe('red'); + }) + }); + }); + `, dedent` it('shorthand arrow', () => something().then(value => { @@ -376,6 +422,24 @@ ruleTester.run('valid-expect-in-promise', rule, { expect(text).toBe('text'); }); `, + dedent` + test('promise test', async function () { + let somePromise = getPromise().then((data) => { + expect(data).toEqual('foo'); + }), x = 1; + + await somePromise; + }); + `, + dedent` + test('promise test', async function () { + let x = 1, somePromise = getPromise().then((data) => { + expect(data).toEqual('foo'); + }); + + await somePromise; + }); + `, dedent` test('promise test', async function () { let somePromise = getPromise().then((data) => { @@ -417,6 +481,70 @@ ruleTester.run('valid-expect-in-promise', rule, { await somePromise; }); `, + dedent` + test('promise test', async function () { + const somePromise = getPromise().then((data) => { + expect(data).toEqual('foo'); + }); + + { + await somePromise; + } + }); + `, + dedent` + test('promise test', async function () { + let somePromise = getPromise().then((data) => { + expect(data).toEqual('foo'); + }); + + { + await somePromise; + + somePromise = getPromise().then((data) => { + expect(data).toEqual('foo'); + }); + + await somePromise; + } + }); + `, + dedent` + test('promise test', async function () { + let somePromise = getPromise().then((data) => { + expect(data).toEqual('foo'); + }); + + await somePromise; + + { + somePromise = getPromise().then((data) => { + expect(data).toEqual('foo'); + }); + + await somePromise; + } + }); + `, + dedent` + test('promise test', async function () { + let somePromise = getPromise().then((data) => { + expect(data).toEqual('foo'); + }); + + await somePromise; + + { + somePromise = getPromise().then((data) => { + expect(data).toEqual('foo'); + }); + + { + await somePromise; + } + } + }); + `, ], invalid: [ { @@ -695,7 +823,7 @@ ruleTester.run('valid-expect-in-promise', rule, { }); }); `, - errors: [{ column: 3, endColumn: 6, messageId: 'returnPromise' }], + errors: [{ column: 9, endColumn: 5, messageId: 'returnPromise' }], }, { code: dedent` @@ -729,7 +857,27 @@ ruleTester.run('valid-expect-in-promise', rule, { promise; }); `, - errors: [{ column: 3, endColumn: 6, messageId: 'returnPromise' }], + errors: [{ column: 9, endColumn: 5, messageId: 'returnPromise' }], + }, + { + code: dedent` + test('later return', async () => { + const promise = something().then(value => { + expect(value).toBe('red'); + }), x = 1; + }); + `, + errors: [{ column: 9, endColumn: 5, messageId: 'returnPromise' }], + }, + { + code: dedent` + test('later return', async () => { + const x = 1, promise = something().then(value => { + expect(value).toBe('red'); + }); + }); + `, + errors: [{ column: 16, endColumn: 5, messageId: 'returnPromise' }], }, { code: dedent` @@ -769,7 +917,7 @@ ruleTester.run('valid-expect-in-promise', rule, { await somePromise; }); `, - errors: [{ column: 3, endColumn: 6, messageId: 'returnPromise' }], + errors: [{ column: 7, endColumn: 5, messageId: 'returnPromise' }], }, { code: dedent` @@ -786,7 +934,7 @@ ruleTester.run('valid-expect-in-promise', rule, { }); `, errors: [ - { column: 3, endColumn: 6, line: 2, messageId: 'returnPromise' }, + { column: 7, endColumn: 5, line: 2, messageId: 'returnPromise' }, ], }, { @@ -800,7 +948,39 @@ ruleTester.run('valid-expect-in-promise', rule, { }); `, errors: [ - { column: 3, endColumn: 6, line: 2, messageId: 'returnPromise' }, + { column: 7, endColumn: 5, line: 2, messageId: 'returnPromise' }, + ], + }, + { + code: dedent` + test('promise test', async function () { + let somePromise = getPromise().then((data) => { + expect(data).toEqual('foo'); + }); + + { + somePromise = getPromise().then((data) => { + expect(data).toEqual('foo'); + }); + + await somePromise; + } + }); + `, + errors: [ + { column: 7, endColumn: 5, line: 2, messageId: 'returnPromise' }, + ], + }, + { + code: dedent` + test('that we error on this destructuring', async () => { + [promise] = something().then(value => { + expect(value).toBe('red'); + }); + }); + `, + errors: [ + { column: 3, endColumn: 5, line: 2, messageId: 'returnPromise' }, ], }, ], diff --git a/src/rules/valid-expect-in-promise.ts b/src/rules/valid-expect-in-promise.ts index c2b519b53..d3f3e3e1f 100644 --- a/src/rules/valid-expect-in-promise.ts +++ b/src/rules/valid-expect-in-promise.ts @@ -61,29 +61,29 @@ const reportReturnRequired = (context: RuleContext, node: TSESTree.Node) => { }); }; -type StartingStatementNode = TSESTree.Statement & { - parent: TSESTree.BlockStatement; -}; - -const findStartingStatementInTestBody = ( - node: PromiseChainCallExpression, -): StartingStatementNode | null => { +const findTopMostCallExpression = ( + node: TSESTree.CallExpression, +): TSESTree.CallExpression => { + let topMostCallExpression = node; let { parent } = node; while (parent) { - if (parent.parent && parent.parent.type === AST_NODE_TYPES.BlockStatement) { - if ( - parent.parent.parent?.parent?.type === AST_NODE_TYPES.CallExpression && - isTestCaseCall(parent.parent.parent?.parent) - ) { - return parent as StartingStatementNode; - } + if (parent.type === AST_NODE_TYPES.CallExpression) { + topMostCallExpression = parent; + + parent = parent.parent; + + continue; + } + + if (parent.type !== AST_NODE_TYPES.MemberExpression) { + break; } parent = parent.parent; } - return null; + return topMostCallExpression; }; const isTestCaseCallWithCallbackArg = ( @@ -159,21 +159,60 @@ const isValueAwaitedOrReturned = ( break; } } + + if ( + node.type === AST_NODE_TYPES.BlockStatement && + isValueAwaitedOrReturned(identifier, node.body) + ) { + return true; + } } return false; }; -const isVariableAwaitedOrReturned = ( - variables: TSESTree.VariableDeclaration & { parent: TSESTree.BlockStatement }, -): boolean => { - const { body } = variables.parent; - const [variable] = variables.declarations; +const findFirstBlockBodyUp = ( + node: TSESTree.Node, +): TSESTree.BlockStatement['body'] => { + let parent: TSESTree.Node['parent'] = node; - if (variable.init?.type === AST_NODE_TYPES.AwaitExpression) { - return true; + while (parent) { + if (parent.type === AST_NODE_TYPES.BlockStatement) { + return parent.body; + } + + parent = parent.parent; + } + + /* istanbul ignore next */ + throw new Error( + `Could not find BlockStatement - please file a github issue at https://github.com/jest-community/eslint-plugin-jest`, + ); +}; + +const isDirectlyWithinTestCaseCall = (node: TSESTree.Node): boolean => { + let parent: TSESTree.Node['parent'] = node; + + while (parent) { + if (isFunction(parent)) { + parent = parent.parent; + + return !!( + parent?.type === AST_NODE_TYPES.CallExpression && isTestCaseCall(parent) + ); + } + + parent = parent.parent; } + return false; +}; + +const isVariableAwaitedOrReturned = ( + variable: TSESTree.VariableDeclarator, +): boolean => { + const body = findFirstBlockBodyUp(variable); + // it's pretty much impossible for us to track destructuring assignments, // so we return true to bailout gracefully if (!isIdentifier(variable.id)) { @@ -240,37 +279,49 @@ export default createRule({ return; } - const topNode = findStartingStatementInTestBody(node); - - if (!topNode || topNode.type === AST_NODE_TYPES.ReturnStatement) { - return; - } + const { parent } = findTopMostCallExpression(node); if ( - topNode.type === AST_NODE_TYPES.VariableDeclaration && - isVariableAwaitedOrReturned(topNode) + !parent || + !isDirectlyWithinTestCaseCall(parent) || + parent.type === AST_NODE_TYPES.ArrowFunctionExpression ) { return; } - if (topNode.type === AST_NODE_TYPES.ExpressionStatement) { - if (topNode.expression.type === AST_NODE_TYPES.AwaitExpression) { - return; + switch (parent?.type) { + case AST_NODE_TYPES.VariableDeclarator: { + if (isVariableAwaitedOrReturned(parent)) { + return; + } + + break; } - if ( - topNode.expression.type === AST_NODE_TYPES.AssignmentExpression && - topNode.expression.left.type === AST_NODE_TYPES.Identifier && - isValueAwaitedOrReturned( - topNode.expression.left, - topNode.parent.body, - ) - ) { - return; + case AST_NODE_TYPES.AssignmentExpression: { + if ( + parent.left.type === AST_NODE_TYPES.Identifier && + isValueAwaitedOrReturned( + parent.left, + findFirstBlockBodyUp(parent), + ) + ) { + return; + } + + break; } + + case AST_NODE_TYPES.ExpressionStatement: + break; + + case AST_NODE_TYPES.ReturnStatement: + case AST_NODE_TYPES.AwaitExpression: + default: + return; } - reportReturnRequired(context, topNode); + reportReturnRequired(context, parent); }, }; }, From 099b27a55fb3596c609b4d5a6dcb90d8534a5d07 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Sun, 3 Oct 2021 14:14:18 +1300 Subject: [PATCH 11/23] feat(valid-expect-in-promise): support re-assignment promise chaining --- .../__tests__/valid-expect-in-promise.test.ts | 42 +++++++++++++++++++ src/rules/valid-expect-in-promise.ts | 10 +++++ 2 files changed, 52 insertions(+) diff --git a/src/rules/__tests__/valid-expect-in-promise.test.ts b/src/rules/__tests__/valid-expect-in-promise.test.ts index 01cd16418..505f471cb 100644 --- a/src/rules/__tests__/valid-expect-in-promise.test.ts +++ b/src/rules/__tests__/valid-expect-in-promise.test.ts @@ -532,6 +532,48 @@ ruleTester.run('valid-expect-in-promise', rule, { expect(data).toEqual('foo'); }); + somePromise = somePromise.then((data) => { + expect(data).toEqual('foo'); + }); + + await somePromise; + }); + `, + dedent` + test('promise test', async function () { + let somePromise = getPromise().then((data) => { + expect(data).toEqual('foo'); + }); + + somePromise = somePromise + .then((data) => data) + .then((data) => data) + .then((data) => { + expect(data).toEqual('foo'); + }); + + await somePromise; + }); + `, + dedent` + test('promise test', async function () { + let somePromise = getPromise().then((data) => { + expect(data).toEqual('foo'); + }); + + somePromise = somePromise + .then((data) => data) + .then((data) => data) + + await somePromise; + }); + `, + dedent` + test('promise test', async function () { + let somePromise = getPromise().then((data) => { + expect(data).toEqual('foo'); + }); + await somePromise; { diff --git a/src/rules/valid-expect-in-promise.ts b/src/rules/valid-expect-in-promise.ts index d3f3e3e1f..582467599 100644 --- a/src/rules/valid-expect-in-promise.ts +++ b/src/rules/valid-expect-in-promise.ts @@ -156,6 +156,16 @@ const isValueAwaitedOrReturned = ( // (re)assignment changes the runtime value, so if we've not found an // await or return already we act as if we've reached the end of the body if (node.expression.type === AST_NODE_TYPES.AssignmentExpression) { + // unless we're assigning to the same identifier, in which case + // we might be chaining off the existing promise value + if ( + isIdentifier(node.expression.left, name) && + getNodeName(node.expression.right)?.startsWith(`${name}.`) && + isPromiseChainCall(node.expression.right) + ) { + continue; + } + break; } } From e10900d39c940817113d5c590e4bff899d208662 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Sun, 3 Oct 2021 14:19:03 +1300 Subject: [PATCH 12/23] refactor(valid-expect-in-promise): remove unneeded condition --- src/rules/valid-expect-in-promise.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/rules/valid-expect-in-promise.ts b/src/rules/valid-expect-in-promise.ts index 582467599..141def79f 100644 --- a/src/rules/valid-expect-in-promise.ts +++ b/src/rules/valid-expect-in-promise.ts @@ -291,11 +291,7 @@ export default createRule({ const { parent } = findTopMostCallExpression(node); - if ( - !parent || - !isDirectlyWithinTestCaseCall(parent) || - parent.type === AST_NODE_TYPES.ArrowFunctionExpression - ) { + if (!parent || !isDirectlyWithinTestCaseCall(parent)) { return; } From d966f01e78775a0ed20837750cafc93b9f938bf3 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Sun, 3 Oct 2021 14:21:49 +1300 Subject: [PATCH 13/23] refactor(valid-expect-in-promise): inline reporting function We only call it in one place, and so it lets us also remove some minor types that can be otherwise inferred --- src/rules/valid-expect-in-promise.ts | 25 +++++-------------------- 1 file changed, 5 insertions(+), 20 deletions(-) diff --git a/src/rules/valid-expect-in-promise.ts b/src/rules/valid-expect-in-promise.ts index 141def79f..443199a06 100644 --- a/src/rules/valid-expect-in-promise.ts +++ b/src/rules/valid-expect-in-promise.ts @@ -1,6 +1,5 @@ import { AST_NODE_TYPES, - TSESLint, TSESTree, } from '@typescript-eslint/experimental-utils'; import { @@ -15,9 +14,6 @@ import { isTestCaseCall, } from './utils'; -type MessageIds = 'returnPromise'; -type RuleContext = TSESLint.RuleContext; - type PromiseChainCallExpression = KnownCallExpression< 'then' | 'catch' | 'finally' >; @@ -47,20 +43,6 @@ const isPromiseChainCall = ( return false; }; -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 findTopMostCallExpression = ( node: TSESTree.CallExpression, ): TSESTree.CallExpression => { @@ -232,7 +214,7 @@ const isVariableAwaitedOrReturned = ( return isValueAwaitedOrReturned(variable.id, body); }; -export default createRule({ +export default createRule({ name: __filename, meta: { docs: { @@ -327,7 +309,10 @@ export default createRule({ return; } - reportReturnRequired(context, parent); + context.report({ + messageId: 'returnPromise', + node: parent, + }); }, }; }, From d9fa51646e865cf02a18972cd1680e57e5ca13f8 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Sun, 3 Oct 2021 14:23:07 +1300 Subject: [PATCH 14/23] fix(valid-expect-in-promise): rewrite rule message to be clearer --- .../__tests__/valid-expect-in-promise.test.ts | 159 ++++++++++++++---- src/rules/valid-expect-in-promise.ts | 6 +- 2 files changed, 126 insertions(+), 39 deletions(-) diff --git a/src/rules/__tests__/valid-expect-in-promise.test.ts b/src/rules/__tests__/valid-expect-in-promise.test.ts index 505f471cb..2dfa3c360 100644 --- a/src/rules/__tests__/valid-expect-in-promise.test.ts +++ b/src/rules/__tests__/valid-expect-in-promise.test.ts @@ -604,7 +604,12 @@ ruleTester.run('valid-expect-in-promise', rule, { }); `, errors: [ - { column: 3, endColumn: 6, messageId: 'returnPromise', line: 8 }, + { + column: 3, + endColumn: 6, + messageId: 'expectInFloatingPromise', + line: 8, + }, ], }, { @@ -615,7 +620,9 @@ ruleTester.run('valid-expect-in-promise', rule, { }); }); `, - errors: [{ column: 3, endColumn: 6, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 6, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -625,7 +632,9 @@ ruleTester.run('valid-expect-in-promise', rule, { }); }); `, - errors: [{ column: 3, endColumn: 6, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 6, messageId: 'expectInFloatingPromise' }, + ], }, { code: ` @@ -635,7 +644,9 @@ ruleTester.run('valid-expect-in-promise', rule, { }); }); `, - errors: [{ column: 10, endColumn: 13, messageId: 'returnPromise' }], + errors: [ + { column: 10, endColumn: 13, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -645,7 +656,9 @@ ruleTester.run('valid-expect-in-promise', rule, { }); }); `, - errors: [{ column: 3, endColumn: 6, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 6, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -655,7 +668,9 @@ ruleTester.run('valid-expect-in-promise', rule, { }); }); `, - errors: [{ column: 3, endColumn: 6, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 6, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -665,7 +680,9 @@ ruleTester.run('valid-expect-in-promise', rule, { }) }) `, - errors: [{ column: 3, endColumn: 5, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -675,7 +692,9 @@ ruleTester.run('valid-expect-in-promise', rule, { }) }) `, - errors: [{ column: 3, endColumn: 5, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -685,7 +704,9 @@ ruleTester.run('valid-expect-in-promise', rule, { }) }) `, - errors: [{ column: 3, endColumn: 5, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -697,7 +718,9 @@ ruleTester.run('valid-expect-in-promise', rule, { }) }) `, - errors: [{ column: 3, endColumn: 5, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -708,7 +731,9 @@ ruleTester.run('valid-expect-in-promise', rule, { }) }); `, - errors: [{ column: 3, endColumn: 5, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -719,7 +744,9 @@ ruleTester.run('valid-expect-in-promise', rule, { .then(data => expect(data).toEqual('Hi')); }); `, - errors: [{ column: 3, endColumn: 47, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 47, messageId: 'expectInFloatingPromise' }, + ], }, { code: ` @@ -730,7 +757,9 @@ ruleTester.run('valid-expect-in-promise', rule, { .then(data => expect(data).toEqual('Hi')); }); `, - errors: [{ column: 11, endColumn: 55, messageId: 'returnPromise' }], + errors: [ + { column: 11, endColumn: 55, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -741,7 +770,9 @@ ruleTester.run('valid-expect-in-promise', rule, { }) }); `, - errors: [{ column: 3, endColumn: 5, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -751,7 +782,9 @@ ruleTester.run('valid-expect-in-promise', rule, { .then(() => expect(someThing).toEqual(value)) }); `, - errors: [{ column: 3, endColumn: 50, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 50, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -761,7 +794,9 @@ ruleTester.run('valid-expect-in-promise', rule, { .then(() => {}) }); `, - errors: [{ column: 3, endColumn: 20, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 20, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -774,7 +809,9 @@ ruleTester.run('valid-expect-in-promise', rule, { }) }); `, - errors: [{ column: 3, endColumn: 5, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -787,7 +824,9 @@ ruleTester.run('valid-expect-in-promise', rule, { }) }); `, - errors: [{ column: 3, endColumn: 5, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -800,7 +839,9 @@ ruleTester.run('valid-expect-in-promise', rule, { }) }); `, - errors: [{ column: 3, endColumn: 5, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -815,7 +856,9 @@ ruleTester.run('valid-expect-in-promise', rule, { return anotherPromise.then(() => expect(x).toBe(y)); }); `, - errors: [{ column: 3, endColumn: 5, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -829,7 +872,9 @@ ruleTester.run('valid-expect-in-promise', rule, { return anotherPromise.then(() => expect(x).toBe(y)); }); `, - errors: [{ column: 3, endColumn: 35, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 35, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -843,7 +888,9 @@ ruleTester.run('valid-expect-in-promise', rule, { return anotherPromise.then(() => expect(x).toBe(y)); }); `, - errors: [{ column: 3, endColumn: 22, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 22, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -854,7 +901,9 @@ ruleTester.run('valid-expect-in-promise', rule, { }) }); `, - errors: [{ column: 3, endColumn: 5, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -865,7 +914,9 @@ ruleTester.run('valid-expect-in-promise', rule, { }); }); `, - errors: [{ column: 9, endColumn: 5, messageId: 'returnPromise' }], + errors: [ + { column: 9, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -876,7 +927,9 @@ ruleTester.run('valid-expect-in-promise', rule, { }) }); `, - errors: [{ column: 3, endColumn: 5, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -887,7 +940,9 @@ ruleTester.run('valid-expect-in-promise', rule, { }) }); `, - errors: [{ column: 3, endColumn: 5, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -899,7 +954,9 @@ ruleTester.run('valid-expect-in-promise', rule, { promise; }); `, - errors: [{ column: 9, endColumn: 5, messageId: 'returnPromise' }], + errors: [ + { column: 9, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -909,7 +966,9 @@ ruleTester.run('valid-expect-in-promise', rule, { }), x = 1; }); `, - errors: [{ column: 9, endColumn: 5, messageId: 'returnPromise' }], + errors: [ + { column: 9, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -919,7 +978,9 @@ ruleTester.run('valid-expect-in-promise', rule, { }); }); `, - errors: [{ column: 16, endColumn: 5, messageId: 'returnPromise' }], + errors: [ + { column: 16, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -932,7 +993,9 @@ ruleTester.run('valid-expect-in-promise', rule, { return somePromise; }); `, - errors: [{ column: 3, endColumn: 6, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 6, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -945,7 +1008,9 @@ ruleTester.run('valid-expect-in-promise', rule, { return somePromise; }); `, - errors: [{ column: 3, endColumn: 6, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 6, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -959,7 +1024,9 @@ ruleTester.run('valid-expect-in-promise', rule, { await somePromise; }); `, - errors: [{ column: 7, endColumn: 5, messageId: 'returnPromise' }], + errors: [ + { column: 7, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -976,7 +1043,12 @@ ruleTester.run('valid-expect-in-promise', rule, { }); `, errors: [ - { column: 7, endColumn: 5, line: 2, messageId: 'returnPromise' }, + { + column: 7, + endColumn: 5, + line: 2, + messageId: 'expectInFloatingPromise', + }, ], }, { @@ -990,7 +1062,12 @@ ruleTester.run('valid-expect-in-promise', rule, { }); `, errors: [ - { column: 7, endColumn: 5, line: 2, messageId: 'returnPromise' }, + { + column: 7, + endColumn: 5, + line: 2, + messageId: 'expectInFloatingPromise', + }, ], }, { @@ -1010,7 +1087,12 @@ ruleTester.run('valid-expect-in-promise', rule, { }); `, errors: [ - { column: 7, endColumn: 5, line: 2, messageId: 'returnPromise' }, + { + column: 7, + endColumn: 5, + line: 2, + messageId: 'expectInFloatingPromise', + }, ], }, { @@ -1022,7 +1104,12 @@ ruleTester.run('valid-expect-in-promise', rule, { }); `, errors: [ - { column: 3, endColumn: 5, line: 2, messageId: 'returnPromise' }, + { + column: 3, + endColumn: 5, + line: 2, + messageId: 'expectInFloatingPromise', + }, ], }, ], diff --git a/src/rules/valid-expect-in-promise.ts b/src/rules/valid-expect-in-promise.ts index 443199a06..2c83b9019 100644 --- a/src/rules/valid-expect-in-promise.ts +++ b/src/rules/valid-expect-in-promise.ts @@ -223,8 +223,8 @@ export default createRule({ recommended: 'error', }, messages: { - returnPromise: - 'Promise should be returned to test its fulfillment or rejection', + expectInFloatingPromise: + "This promise should either be returned or awaited to ensure the expects in it's chain are called", }, type: 'suggestion', schema: [], @@ -310,7 +310,7 @@ export default createRule({ } context.report({ - messageId: 'returnPromise', + messageId: 'expectInFloatingPromise', node: parent, }); }, From 5142136df4cbcb89e42eef2130e38d6874e63d71 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Sun, 3 Oct 2021 14:27:02 +1300 Subject: [PATCH 15/23] fix(valid-expect-in-promise): rewrite rule description to be clearer --- README.md | 2 +- docs/rules/valid-expect-in-promise.md | 2 +- src/rules/valid-expect-in-promise.ts | 3 ++- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 8ec411f84..e23e98b16 100644 --- a/README.md +++ b/README.md @@ -193,7 +193,7 @@ installations requiring long-term consistency. | [require-top-level-describe](docs/rules/require-top-level-describe.md) | Require test cases and hooks to be inside a `describe` block | | | | [valid-describe](docs/rules/valid-describe.md) | Enforce valid `describe()` callback | ![recommended][] | | | [valid-expect](docs/rules/valid-expect.md) | Enforce valid `expect()` usage | ![recommended][] | | -| [valid-expect-in-promise](docs/rules/valid-expect-in-promise.md) | Enforce having return statement when testing with promises | ![recommended][] | | +| [valid-expect-in-promise](docs/rules/valid-expect-in-promise.md) | Ensure promises that have expectations in their chain are valid | ![recommended][] | | | [valid-title](docs/rules/valid-title.md) | Enforce valid titles | ![recommended][] | ![fixable][] | diff --git a/docs/rules/valid-expect-in-promise.md b/docs/rules/valid-expect-in-promise.md index e4f108adc..09090d531 100644 --- a/docs/rules/valid-expect-in-promise.md +++ b/docs/rules/valid-expect-in-promise.md @@ -1,4 +1,4 @@ -# Enforce having return statement when testing with promises (`valid-expect-in-promise`) +# Ensure promises that have expectations in their chain are valid (`valid-expect-in-promise`) Ensure to return promise when having assertions in `then` or `catch` block of promise diff --git a/src/rules/valid-expect-in-promise.ts b/src/rules/valid-expect-in-promise.ts index 2c83b9019..385db410d 100644 --- a/src/rules/valid-expect-in-promise.ts +++ b/src/rules/valid-expect-in-promise.ts @@ -219,7 +219,8 @@ export default createRule({ meta: { docs: { category: 'Best Practices', - description: 'Enforce having return statement when testing with promises', + description: + 'Ensure promises that have expectations in their chain are valid', recommended: 'error', }, messages: { From 11b8469c8996cfdbe049b619a364dd67f4b59f1d Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Sun, 3 Oct 2021 15:13:05 +1300 Subject: [PATCH 16/23] fix(valid-expect-in-promise): ignore unreachable code after return --- .../__tests__/valid-expect-in-promise.test.ts | 32 +++++++++++++++++++ src/rules/valid-expect-in-promise.ts | 14 +++++--- 2 files changed, 41 insertions(+), 5 deletions(-) diff --git a/src/rules/__tests__/valid-expect-in-promise.test.ts b/src/rules/__tests__/valid-expect-in-promise.test.ts index 2dfa3c360..932ebae08 100644 --- a/src/rules/__tests__/valid-expect-in-promise.test.ts +++ b/src/rules/__tests__/valid-expect-in-promise.test.ts @@ -958,6 +958,38 @@ ruleTester.run('valid-expect-in-promise', rule, { { column: 9, endColumn: 5, messageId: 'expectInFloatingPromise' }, ], }, + { + code: dedent` + test('later return', async () => { + const promise = something().then(value => { + expect(value).toBe('red'); + }); + + return; + + await promise; + }); + `, + errors: [ + { column: 9, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], + }, + { + code: dedent` + test('later return', async () => { + const promise = something().then(value => { + expect(value).toBe('red'); + }); + + return 1; + + await promise; + }); + `, + errors: [ + { column: 9, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], + }, { code: dedent` test('later return', async () => { diff --git a/src/rules/valid-expect-in-promise.ts b/src/rules/valid-expect-in-promise.ts index 385db410d..e78cb6d46 100644 --- a/src/rules/valid-expect-in-promise.ts +++ b/src/rules/valid-expect-in-promise.ts @@ -120,11 +120,15 @@ const isValueAwaitedOrReturned = ( continue; } - if ( - node.type === AST_NODE_TYPES.ReturnStatement && - node.argument?.type === AST_NODE_TYPES.Identifier - ) { - return isIdentifier(node.argument, name); + if (node.type === AST_NODE_TYPES.ReturnStatement) { + if (node.argument === null) { + return false; + } + + return ( + node.argument.type === AST_NODE_TYPES.Identifier && + isIdentifier(node.argument, name) + ); } if (node.type === AST_NODE_TYPES.ExpressionStatement) { From c49f70424b318097d61645f0869f43e8f1482a82 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Sun, 3 Oct 2021 15:27:45 +1300 Subject: [PATCH 17/23] fix(valid-expect-in-promise): support `Promise.all` --- .../__tests__/valid-expect-in-promise.test.ts | 152 ++++++++++++++++++ src/rules/valid-expect-in-promise.ts | 47 ++++-- 2 files changed, 186 insertions(+), 13 deletions(-) diff --git a/src/rules/__tests__/valid-expect-in-promise.test.ts b/src/rules/__tests__/valid-expect-in-promise.test.ts index 932ebae08..549f7c468 100644 --- a/src/rules/__tests__/valid-expect-in-promise.test.ts +++ b/src/rules/__tests__/valid-expect-in-promise.test.ts @@ -587,6 +587,24 @@ ruleTester.run('valid-expect-in-promise', rule, { } }); `, + dedent` + test('promise test', async function () { + const somePromise = getPromise().then((data) => { + expect(data).toEqual('foo'); + }); + + await Promise.all([somePromise]); + }); + `, + dedent` + test('promise test', async function () { + const somePromise = getPromise().then((data) => { + expect(data).toEqual('foo'); + }); + + return Promise.all([somePromise]); + }); + `, ], invalid: [ { @@ -990,6 +1008,140 @@ ruleTester.run('valid-expect-in-promise', rule, { { column: 9, endColumn: 5, messageId: 'expectInFloatingPromise' }, ], }, + { + code: dedent` + test('later return', async () => { + const promise = something().then(value => { + expect(value).toBe('red'); + }); + + return []; + + await promise; + }); + `, + errors: [ + { column: 9, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], + }, + { + code: dedent` + test('later return', async () => { + const promise = something().then(value => { + expect(value).toBe('red'); + }); + + return Promise.all([anotherPromise]); + + await promise; + }); + `, + errors: [ + { column: 9, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], + }, + { + code: dedent` + test('later return', async () => { + const promise = something().then(value => { + expect(value).toBe('red'); + }); + + return {}; + + await promise; + }); + `, + errors: [ + { column: 9, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], + }, + { + code: dedent` + test('later return', async () => { + const promise = something().then(value => { + expect(value).toBe('red'); + }); + + return Promise.all([]); + + await promise; + }); + `, + errors: [ + { column: 9, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], + }, + { + code: dedent` + test('later return', async () => { + const promise = something().then(value => { + expect(value).toBe('red'); + }); + + await 1; + }); + `, + errors: [ + { column: 9, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], + }, + { + code: dedent` + test('later return', async () => { + const promise = something().then(value => { + expect(value).toBe('red'); + }); + + await []; + }); + `, + errors: [ + { column: 9, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], + }, + { + code: dedent` + test('later return', async () => { + const promise = something().then(value => { + expect(value).toBe('red'); + }); + + await Promise.all([anotherPromise]); + }); + `, + errors: [ + { column: 9, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], + }, + { + code: dedent` + test('later return', async () => { + const promise = something().then(value => { + expect(value).toBe('red'); + }); + + await {}; + }); + `, + errors: [ + { column: 9, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], + }, + { + code: dedent` + test('later return', async () => { + const promise = something().then(value => { + expect(value).toBe('red'); + }); + + await Promise.all([]); + }); + `, + errors: [ + { column: 9, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], + }, { code: dedent` test('later return', async () => { diff --git a/src/rules/valid-expect-in-promise.ts b/src/rules/valid-expect-in-promise.ts index e78cb6d46..3ea541b92 100644 --- a/src/rules/valid-expect-in-promise.ts +++ b/src/rules/valid-expect-in-promise.ts @@ -103,6 +103,37 @@ const isTestCaseCallWithCallbackArg = ( return false; }; +const isPromiseMethodThatUsesValue = ( + node: TSESTree.AwaitExpression | TSESTree.ReturnStatement, + identifier: TSESTree.Identifier, +): boolean => { + const { name } = identifier; + + if (node.argument === null) { + return false; + } + + if ( + node.argument.type === AST_NODE_TYPES.CallExpression && + node.argument.arguments.length > 0 && + getNodeName(node.argument) === 'Promise.all' + ) { + const [firstArg] = node.argument.arguments; + + if ( + firstArg.type === AST_NODE_TYPES.ArrayExpression && + firstArg.elements.some(nod => isIdentifier(nod, name)) + ) { + return true; + } + } + + return ( + node.argument.type === AST_NODE_TYPES.Identifier && + isIdentifier(node.argument, name) + ); +}; + /** * Attempts to determine if the runtime value represented by the given `identifier` * is `await`ed or `return`ed within the given `body` of statements @@ -121,22 +152,12 @@ const isValueAwaitedOrReturned = ( } if (node.type === AST_NODE_TYPES.ReturnStatement) { - if (node.argument === null) { - return false; - } - - return ( - node.argument.type === AST_NODE_TYPES.Identifier && - isIdentifier(node.argument, name) - ); + return isPromiseMethodThatUsesValue(node, identifier); } if (node.type === AST_NODE_TYPES.ExpressionStatement) { - if ( - node.expression.type === AST_NODE_TYPES.AwaitExpression && - node.expression.argument?.type === AST_NODE_TYPES.Identifier - ) { - return isIdentifier(node.expression.argument, name); + if (node.expression.type === AST_NODE_TYPES.AwaitExpression) { + return isPromiseMethodThatUsesValue(node.expression, identifier); } // (re)assignment changes the runtime value, so if we've not found an From 3491dd713214be9caddc13463515d111f1b96f76 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Sun, 3 Oct 2021 15:40:38 +1300 Subject: [PATCH 18/23] fix(valid-expect-in-promise): support `resolve` & `reject` methods --- .../__tests__/valid-expect-in-promise.test.ts | 36 +++++++++++++++++++ src/rules/valid-expect-in-promise.ts | 22 ++++++++---- 2 files changed, 52 insertions(+), 6 deletions(-) diff --git a/src/rules/__tests__/valid-expect-in-promise.test.ts b/src/rules/__tests__/valid-expect-in-promise.test.ts index 549f7c468..3ae64efa5 100644 --- a/src/rules/__tests__/valid-expect-in-promise.test.ts +++ b/src/rules/__tests__/valid-expect-in-promise.test.ts @@ -605,6 +605,42 @@ ruleTester.run('valid-expect-in-promise', rule, { return Promise.all([somePromise]); }); `, + dedent` + test('promise test', async function () { + const somePromise = getPromise().then((data) => { + expect(data).toEqual('foo'); + }); + + return Promise.resolve(somePromise); + }); + `, + dedent` + test('promise test', async function () { + const somePromise = getPromise().then((data) => { + expect(data).toEqual('foo'); + }); + + return Promise.reject(somePromise); + }); + `, + dedent` + test('promise test', async function () { + const somePromise = getPromise().then((data) => { + expect(data).toEqual('foo'); + }); + + await Promise.resolve(somePromise); + }); + `, + dedent` + test('promise test', async function () { + const somePromise = getPromise().then((data) => { + expect(data).toEqual('foo'); + }); + + await Promise.reject(somePromise); + }); + `, ], invalid: [ { diff --git a/src/rules/valid-expect-in-promise.ts b/src/rules/valid-expect-in-promise.ts index 3ea541b92..fa0f345a4 100644 --- a/src/rules/valid-expect-in-promise.ts +++ b/src/rules/valid-expect-in-promise.ts @@ -115,16 +115,26 @@ const isPromiseMethodThatUsesValue = ( if ( node.argument.type === AST_NODE_TYPES.CallExpression && - node.argument.arguments.length > 0 && - getNodeName(node.argument) === 'Promise.all' + node.argument.arguments.length > 0 ) { - const [firstArg] = node.argument.arguments; + const nodeName = getNodeName(node.argument); + + if (nodeName === 'Promise.all') { + const [firstArg] = node.argument.arguments; + + if ( + firstArg.type === AST_NODE_TYPES.ArrayExpression && + firstArg.elements.some(nod => isIdentifier(nod, name)) + ) { + return true; + } + } if ( - firstArg.type === AST_NODE_TYPES.ArrayExpression && - firstArg.elements.some(nod => isIdentifier(nod, name)) + ['Promise.resolve', 'Promise.reject'].includes(nodeName as string) && + node.argument.arguments.length === 1 ) { - return true; + return isIdentifier(node.argument.arguments[0], name); } } From 72bc5d6c919c81682113ed159e44a29c8c4bd8cb Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Mon, 4 Oct 2021 16:21:22 +1300 Subject: [PATCH 19/23] refactor(valid-expect-in-promise): remove unneeded optional chain --- src/rules/valid-expect-in-promise.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rules/valid-expect-in-promise.ts b/src/rules/valid-expect-in-promise.ts index fa0f345a4..643fa7b2b 100644 --- a/src/rules/valid-expect-in-promise.ts +++ b/src/rules/valid-expect-in-promise.ts @@ -313,7 +313,7 @@ export default createRule({ return; } - switch (parent?.type) { + switch (parent.type) { case AST_NODE_TYPES.VariableDeclarator: { if (isVariableAwaitedOrReturned(parent)) { return; From 0b46e0f4fa2f303496f9d753a013aa75a130eca3 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Sat, 9 Oct 2021 07:29:58 +1300 Subject: [PATCH 20/23] refactor(valid-expect-in-promise): adjust conditions slightly --- src/rules/valid-expect-in-promise.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/rules/valid-expect-in-promise.ts b/src/rules/valid-expect-in-promise.ts index 643fa7b2b..6c49cd7b5 100644 --- a/src/rules/valid-expect-in-promise.ts +++ b/src/rules/valid-expect-in-promise.ts @@ -284,14 +284,8 @@ export default createRule({ return; } - if (chains.length === 0) { - return; - } - - if (isExpectCall(node)) { + if (chains.length > 0 && isExpectCall(node)) { chains[0] = true; - - return; } }, 'CallExpression:exit'(node: TSESTree.CallExpression) { @@ -303,7 +297,13 @@ export default createRule({ return; } - if (!isPromiseChainCall(node) || !chains.shift()) { + if (!isPromiseChainCall(node)) { + return; + } + + const hasExpectCall = chains.shift(); + + if (!hasExpectCall) { return; } From 5e5de478b0235884a558066f53d55deb8fe36bcf Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Sat, 9 Oct 2021 07:26:57 +1300 Subject: [PATCH 21/23] chore(valid-expect-in-promise): add a bunch of comments --- src/rules/valid-expect-in-promise.ts | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/rules/valid-expect-in-promise.ts b/src/rules/valid-expect-in-promise.ts index 6c49cd7b5..934c6630e 100644 --- a/src/rules/valid-expect-in-promise.ts +++ b/src/rules/valid-expect-in-promise.ts @@ -268,27 +268,43 @@ export default createRule({ defaultOptions: [], create(context) { let inTestCaseWithDoneCallback = false; + // an array of booleans representing each promise chain we enter, with the + // boolean value representing if we think a given chain contains an expect + // in it's body. + // + // since we only care about the inner-most chain, we represent the state in + // reverse with the inner-most being the first item, as that makes it + // slightly less code to assign to by not needing to know the length const chains: boolean[] = []; return { CallExpression(node: TSESTree.CallExpression) { + // there are too many ways that the done argument could be used with + // promises that contain expect that would make the promise safe for us if (isTestCaseCallWithCallbackArg(node)) { inTestCaseWithDoneCallback = true; return; } + // if this call expression is a promise chain, add it to the stack with + // value of "false", as we assume there are no expect calls initially if (isPromiseChainCall(node)) { chains.unshift(false); return; } + // if we're within a promise chain, and this call expression looks like + // an expect call, mark the deepest chain as having an expect call if (chains.length > 0 && isExpectCall(node)) { chains[0] = true; } }, 'CallExpression:exit'(node: TSESTree.CallExpression) { + // there are too many ways that the "done" argument could be used to + // make promises containing expects safe in a test for us to be able to + // accurately check, so we just bail out completely if it's present if (inTestCaseWithDoneCallback) { if (isTestCaseCall(node)) { inTestCaseWithDoneCallback = false; @@ -301,14 +317,22 @@ export default createRule({ return; } + // since we're exiting this call expression (which is a promise chain) + // we remove it from the stack of chains, since we're unwinding const hasExpectCall = chains.shift(); + // if the promise chain we're exiting doesn't contain an expect, + // then we don't need to check it for anything if (!hasExpectCall) { return; } const { parent } = findTopMostCallExpression(node); + // if we don't have a parent (which is technically impossible at runtime) + // or our parent is not directly within the test case, we stop checking + // because we're most likely in the body of a function being defined + // within the test, which we can't track if (!parent || !isDirectlyWithinTestCaseCall(parent)) { return; } From c664be1eab66e69d8eed78bcb1404a98dd2ca57e Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Sat, 9 Oct 2021 08:22:38 +1300 Subject: [PATCH 22/23] feat(valid-expect-in-promise): support `Promise.allSettled` --- .../__tests__/valid-expect-in-promise.test.ts | 24 +++++++++++++++++++ src/rules/valid-expect-in-promise.ts | 2 +- 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/src/rules/__tests__/valid-expect-in-promise.test.ts b/src/rules/__tests__/valid-expect-in-promise.test.ts index 3ae64efa5..74cd88fcf 100644 --- a/src/rules/__tests__/valid-expect-in-promise.test.ts +++ b/src/rules/__tests__/valid-expect-in-promise.test.ts @@ -641,6 +641,30 @@ ruleTester.run('valid-expect-in-promise', rule, { await Promise.reject(somePromise); }); `, + dedent` + test('later return', async () => { + const onePromise = something().then(value => { + console.log(value); + }); + const twoPromise = something().then(value => { + expect(value).toBe('red'); + }); + + return Promise.all([onePromise, twoPromise]); + }); + `, + dedent` + test('later return', async () => { + const onePromise = something().then(value => { + console.log(value); + }); + const twoPromise = something().then(value => { + expect(value).toBe('red'); + }); + + return Promise.allSettled([onePromise, twoPromise]); + }); + `, ], invalid: [ { diff --git a/src/rules/valid-expect-in-promise.ts b/src/rules/valid-expect-in-promise.ts index 934c6630e..3bd562ab9 100644 --- a/src/rules/valid-expect-in-promise.ts +++ b/src/rules/valid-expect-in-promise.ts @@ -119,7 +119,7 @@ const isPromiseMethodThatUsesValue = ( ) { const nodeName = getNodeName(node.argument); - if (nodeName === 'Promise.all') { + if (['Promise.all', 'Promise.allSettled'].includes(nodeName as string)) { const [firstArg] = node.argument.arguments; if ( From 8f9ccf5ae68df816ea850c29ec8a7f560dc4388f Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Sun, 10 Oct 2021 08:20:58 +1300 Subject: [PATCH 23/23] docs(valid-expect-in-promise): add more examples and reword --- docs/rules/valid-expect-in-promise.md | 67 +++++++++++++++++++++------ 1 file changed, 54 insertions(+), 13 deletions(-) diff --git a/docs/rules/valid-expect-in-promise.md b/docs/rules/valid-expect-in-promise.md index 09090d531..03c3fb6d0 100644 --- a/docs/rules/valid-expect-in-promise.md +++ b/docs/rules/valid-expect-in-promise.md @@ -1,31 +1,72 @@ # Ensure promises that have expectations in their chain are valid (`valid-expect-in-promise`) -Ensure to return promise when having assertions in `then` or `catch` block of -promise +Ensure promises that include expectations are returned or awaited. ## Rule details -This rule looks for tests that have assertions in `then` and `catch` methods on -promises that are not returned by the test. +This rule flags any promises within the body of a test that include expectations +that have either not been returned or awaited. -### Default configuration - -The following pattern is considered warning: +The following patterns is considered warning: ```js -it('promise test', () => { - somePromise.then(data => { - expect(data).toEqual('foo'); +it('promises a person', () => { + api.getPersonByName('bob').then(person => { + expect(person).toHaveProperty('name', 'Bob'); + }); +}); + +it('promises a counted person', () => { + const promise = api.getPersonByName('bob').then(person => { + expect(person).toHaveProperty('name', 'Bob'); + }); + + promise.then(() => { + expect(analytics.gottenPeopleCount).toBe(1); }); }); + +it('promises multiple people', () => { + const firstPromise = api.getPersonByName('bob').then(person => { + expect(person).toHaveProperty('name', 'Bob'); + }); + const secondPromise = api.getPersonByName('alice').then(person => { + expect(person).toHaveProperty('name', 'Alice'); + }); + + return Promise.any([firstPromise, secondPromise]); +}); ``` The following pattern is not warning: ```js -it('promise test', () => { - return somePromise.then(data => { - expect(data).toEqual('foo'); +it('promises a person', async () => { + await api.getPersonByName('bob').then(person => { + expect(person).toHaveProperty('name', 'Bob'); }); }); + +it('promises a counted person', () => { + let promise = api.getPersonByName('bob').then(person => { + expect(person).toHaveProperty('name', 'Bob'); + }); + + promise = promise.then(() => { + expect(analytics.gottenPeopleCount).toBe(1); + }); + + return promise; +}); + +it('promises multiple people', () => { + const firstPromise = api.getPersonByName('bob').then(person => { + expect(person).toHaveProperty('name', 'Bob'); + }); + const secondPromise = api.getPersonByName('alice').then(person => { + expect(person).toHaveProperty('name', 'Alice'); + }); + + return Promise.allSettled([firstPromise, secondPromise]); +}); ```