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..03c3fb6d0 100644 --- a/docs/rules/valid-expect-in-promise.md +++ b/docs/rules/valid-expect-in-promise.md @@ -1,31 +1,72 @@ -# 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 +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]); +}); ``` diff --git a/src/rules/__tests__/valid-expect-in-promise.test.ts b/src/rules/__tests__/valid-expect-in-promise.test.ts index 0a9b3deaa..74cd88fcf 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,61 @@ 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(() => { + 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) => { test() @@ -144,6 +202,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(() => { @@ -163,6 +241,15 @@ ruleTester.run('valid-expect-in-promise', rule, { 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 => { @@ -172,6 +259,48 @@ 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] = [ + something().then(value => { + expect(value).toBe('red'); + }) + ]; + }); + `, + 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 => { @@ -182,31 +311,49 @@ ruleTester.run('valid-expect-in-promise', rule, { ); `, dedent` - it('promise test', () => { - const somePromise = getThatPromise(); - somePromise.then((data) => { - expect(data).toEqual('foo'); + it('crawls for files based on patterns', () => { + const promise = nodeCrawl({}).then(data => { + expect(childProcess.spawn).lastCalledWith('find'); }); - expect(somePromise).toBeDefined(); - return somePromise; + return promise; }); `, dedent` - test('promise test', function () { - let somePromise = getThatPromise(); - somePromise.then((data) => { - expect(data).toEqual('foo'); + it('is a test', async () => { + const value = await somePromise().then(response => { + expect(response).toHaveProperty('data'); + + return response.data; }); - expect(somePromise).toBeDefined(); - return somePromise; + + expect(value).toBe('hello world'); }); `, dedent` - it('crawls for files based on patterns', () => { - const promise = nodeCrawl({}).then(data => { - expect(childProcess.spawn).lastCalledWith('find'); + 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; }); - return promise; }); `, dedent` @@ -254,8 +401,295 @@ ruleTester.run('valid-expect-in-promise', rule, { 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)); + }); + `, + dedent` + test('valid-expect-in-promise', async () => { + const text = await fetch('url') + .then(res => res.text()) + .then(text => text); + + 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) => { + 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; + }); + `, + 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'); + }); + + 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; + + { + somePromise = getPromise().then((data) => { + expect(data).toEqual('foo'); + }); + + { + await somePromise; + } + } + }); + `, + 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]); + }); + `, + 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); + }); + `, + 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: [ + { + 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: 'expectInFloatingPromise', + line: 8, + }, + ], + }, { code: dedent` it('it1', () => { @@ -264,7 +698,9 @@ ruleTester.run('valid-expect-in-promise', rule, { }); }); `, - errors: [{ column: 3, endColumn: 6, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 6, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -274,18 +710,22 @@ ruleTester.run('valid-expect-in-promise', rule, { }); }); `, - errors: [{ column: 3, endColumn: 6, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 6, messageId: 'expectInFloatingPromise' }, + ], + }, + { + code: ` + it('it1', () => { + somePromise['then'](() => { + expect(someThing).toEqual(true); + }); + }); + `, + errors: [ + { column: 10, endColumn: 13, messageId: 'expectInFloatingPromise' }, + ], }, - // { - // code: ` - // it('it1', () => { - // somePromise['then'](() => { - // expect(someThing).toEqual(true); - // }); - // }); - // `, - // errors: [{ column: 12, endColumn: 15, messageId: 'returnPromise' }], - // }, { code: dedent` it('it1', function() { @@ -294,7 +734,9 @@ ruleTester.run('valid-expect-in-promise', rule, { }); }); `, - errors: [{ column: 3, endColumn: 6, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 6, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -304,7 +746,9 @@ ruleTester.run('valid-expect-in-promise', rule, { }); }); `, - errors: [{ column: 3, endColumn: 6, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 6, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -314,7 +758,9 @@ ruleTester.run('valid-expect-in-promise', rule, { }) }) `, - errors: [{ column: 3, endColumn: 5, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -324,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` @@ -334,7 +782,9 @@ ruleTester.run('valid-expect-in-promise', rule, { }) }) `, - errors: [{ column: 3, endColumn: 5, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -346,7 +796,9 @@ ruleTester.run('valid-expect-in-promise', rule, { }) }) `, - errors: [{ column: 3, endColumn: 5, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -357,15 +809,35 @@ ruleTester.run('valid-expect-in-promise', rule, { }) }); `, - errors: [{ column: 3, endColumn: 5, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], }, { 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: 88, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 47, messageId: 'expectInFloatingPromise' }, + ], + }, + { + code: ` + it('test function', async () => { + Builder.getPromiseBuilder() + .get() + .build() + .then(data => expect(data).toEqual('Hi')); + }); + `, + errors: [ + { column: 11, endColumn: 55, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -376,7 +848,127 @@ ruleTester.run('valid-expect-in-promise', rule, { }) }); `, - errors: [{ column: 3, endColumn: 5, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], + }, + { + code: dedent` + it('is a test', () => { + somePromise + .then(() => {}) + .then(() => expect(someThing).toEqual(value)) + }); + `, + errors: [ + { column: 3, endColumn: 50, messageId: 'expectInFloatingPromise' }, + ], + }, + { + code: dedent` + it('is a test', () => { + somePromise + .then(() => expect(someThing).toEqual(value)) + .then(() => {}) + }); + `, + errors: [ + { column: 3, endColumn: 20, messageId: 'expectInFloatingPromise' }, + ], + }, + { + code: dedent` + it('is a test', () => { + somePromise.then(() => { + return value; + }) + .then(value => { + expect(someThing).toEqual(value); + }) + }); + `, + errors: [ + { column: 3, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], + }, + { + code: dedent` + it('is a test', () => { + somePromise.then(() => { + expect(someThing).toEqual(true); + }) + .then(() => { + console.log('this is silly'); + }) + }); + `, + errors: [ + { column: 3, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], + }, + { + code: dedent` + it('is a test', () => { + somePromise.then(() => { + // return value; + }) + .then(value => { + expect(someThing).toEqual(value); + }) + }); + `, + errors: [ + { column: 3, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], + }, + { + 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: 'expectInFloatingPromise' }, + ], + }, + { + 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: 'expectInFloatingPromise' }, + ], + }, + { + 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: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -387,7 +979,9 @@ ruleTester.run('valid-expect-in-promise', rule, { }) }); `, - errors: [{ column: 3, endColumn: 5, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -398,7 +992,9 @@ ruleTester.run('valid-expect-in-promise', rule, { }); }); `, - errors: [{ column: 9, endColumn: 5, messageId: 'returnPromise' }], + errors: [ + { column: 9, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -409,7 +1005,9 @@ ruleTester.run('valid-expect-in-promise', rule, { }) }); `, - errors: [{ column: 3, endColumn: 5, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], }, { code: dedent` @@ -420,7 +1018,343 @@ ruleTester.run('valid-expect-in-promise', rule, { }) }); `, - errors: [{ column: 3, endColumn: 5, messageId: 'returnPromise' }], + errors: [ + { column: 3, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], + }, + { + code: dedent` + test('later return', async () => { + const promise = something().then(value => { + expect(value).toBe('red'); + }); + + 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 1; + + 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([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 () => { + const promise = something().then(value => { + expect(value).toBe('red'); + }), x = 1; + }); + `, + errors: [ + { column: 9, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], + }, + { + code: dedent` + test('later return', async () => { + const x = 1, promise = something().then(value => { + expect(value).toBe('red'); + }); + }); + `, + errors: [ + { column: 16, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], + }, + { + 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: 'expectInFloatingPromise' }, + ], + }, + { + 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: 'expectInFloatingPromise' }, + ], + }, + { + code: dedent` + test('promise test', async function () { + let somePromise = getPromise().then((data) => { + expect(data).toEqual('foo'); + }); + + somePromise = null; + + await somePromise; + }); + `, + errors: [ + { column: 7, endColumn: 5, messageId: 'expectInFloatingPromise' }, + ], + }, + { + 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: 'expectInFloatingPromise', + }, + ], + }, + { + code: dedent` + test('promise test', async function () { + let somePromise = getPromise().then((data) => { + expect(data).toEqual('foo'); + }); + + ({ somePromise } = {}) + }); + `, + errors: [ + { + column: 7, + endColumn: 5, + line: 2, + messageId: 'expectInFloatingPromise', + }, + ], + }, + { + 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: 'expectInFloatingPromise', + }, + ], + }, + { + 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: 'expectInFloatingPromise', + }, + ], }, ], }); diff --git a/src/rules/valid-expect-in-promise.ts b/src/rules/valid-expect-in-promise.ts index 8466b2ccb..3bd562ab9 100644 --- a/src/rules/valid-expect-in-promise.ts +++ b/src/rules/valid-expect-in-promise.ts @@ -1,197 +1,378 @@ import { AST_NODE_TYPES, - TSESLint, TSESTree, } from '@typescript-eslint/experimental-utils'; import { - FunctionExpression, KnownCallExpression, createRule, getAccessorValue, - isExpectMember, + getNodeName, + isExpectCall, isFunction, + isIdentifier, isSupportedAccessor, isTestCaseCall, } from './utils'; -type MessageIds = 'returnPromise'; -type RuleContext = TSESLint.RuleContext; - type PromiseChainCallExpression = KnownCallExpression< 'then' | 'catch' | 'finally' >; 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)); - -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); - } - +): 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 isFullExpectCall(body); + return false; }; -const isFullExpectCall = (expression: TSESTree.Node) => - expression.type === AST_NODE_TYPES.CallExpression && - isExpectMember(expression.callee); +const findTopMostCallExpression = ( + node: TSESTree.CallExpression, +): TSESTree.CallExpression => { + let topMostCallExpression = node; + let { parent } = node; -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, - }); + while (parent) { + 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 topMostCallExpression; }; -const isPromiseReturnedLater = ( - node: TSESTree.Node, - testFunctionBody: TSESTree.Statement[], -) => { - let promiseName; +const isTestCaseCallWithCallbackArg = ( + node: TSESTree.CallExpression, +): boolean => { + if (!isTestCaseCall(node)) { + return false; + } + + const isJestEach = getNodeName(node).endsWith('.each'); 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 + isJestEach && + node.callee.type !== AST_NODE_TYPES.TaggedTemplateExpression ) { - promiseName = node.id.name; + // 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; +}; + +const isPromiseMethodThatUsesValue = ( + node: TSESTree.AwaitExpression | TSESTree.ReturnStatement, + identifier: TSESTree.Identifier, +): boolean => { + const { name } = identifier; + + if (node.argument === null) { + return false; } - const lastLineInTestFunc = testFunctionBody[testFunctionBody.length - 1]; + if ( + node.argument.type === AST_NODE_TYPES.CallExpression && + node.argument.arguments.length > 0 + ) { + const nodeName = getNodeName(node.argument); + + if (['Promise.all', 'Promise.allSettled'].includes(nodeName as string)) { + const [firstArg] = node.argument.arguments; + + if ( + firstArg.type === AST_NODE_TYPES.ArrayExpression && + firstArg.elements.some(nod => isIdentifier(nod, name)) + ) { + return true; + } + } + + if ( + ['Promise.resolve', 'Promise.reject'].includes(nodeName as string) && + node.argument.arguments.length === 1 + ) { + return isIdentifier(node.argument.arguments[0], name); + } + } return ( - lastLineInTestFunc.type === AST_NODE_TYPES.ReturnStatement && - lastLineInTestFunc.argument && - (('name' in lastLineInTestFunc.argument && - lastLineInTestFunc.argument.name === promiseName) || - !promiseName) + node.argument.type === AST_NODE_TYPES.Identifier && + isIdentifier(node.argument, name) ); }; -const findTestFunction = (node: TSESTree.Node | undefined) => { - while (node) { +/** + * 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) { + return isPromiseMethodThatUsesValue(node, identifier); + } + + if (node.type === AST_NODE_TYPES.ExpressionStatement) { + 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 + // 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; + } + } + if ( - isFunction(node) && - node.parent?.type === AST_NODE_TYPES.CallExpression && - isTestCaseCall(node.parent) + node.type === AST_NODE_TYPES.BlockStatement && + isValueAwaitedOrReturned(identifier, node.body) ) { - return node; + return true; } - - node = node.parent; } - return null; + return false; }; -const isParentThenOrPromiseReturned = ( +const findFirstBlockBodyUp = ( 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)) { - if ( - isExpectCallPresentInFunction(promiseCallback.body) && - node.parent.parent && - !isParentThenOrPromiseReturned(node.parent.parent, testFunctionBody) - ) { - reportReturnRequired(context, node.parent.parent); +): TSESTree.BlockStatement['body'] => { + let parent: TSESTree.Node['parent'] = node; - return true; - } + while (parent) { + if (parent.type === AST_NODE_TYPES.BlockStatement) { + return parent.body; } - return false; - }); + 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 isHavingAsyncCallBackParam = (testFunction: FunctionExpression) => - testFunction.params[0] && - testFunction.params[0].type === AST_NODE_TYPES.Identifier; +const isDirectlyWithinTestCaseCall = (node: TSESTree.Node): boolean => { + let parent: TSESTree.Node['parent'] = node; + + while (parent) { + if (isFunction(parent)) { + parent = parent.parent; -export default createRule({ + 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)) { + return true; + } + + return isValueAwaitedOrReturned(variable.id, body); +}; + +export default createRule({ name: __filename, 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: { - 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: [], }, 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) { - if ( - !isPromiseChainCall(node) || - (node.parent && node.parent.type === AST_NODE_TYPES.AwaitExpression) - ) { + 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; } - const testFunction = findTestFunction(node); - if (testFunction && !isHavingAsyncCallBackParam(testFunction)) { - const { body } = testFunction; + // 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); - if (body.type !== AST_NODE_TYPES.BlockStatement) { - return; + 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; } - 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; } + + if (!isPromiseChainCall(node)) { + 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; + } + + switch (parent.type) { + case AST_NODE_TYPES.VariableDeclarator: { + if (isVariableAwaitedOrReturned(parent)) { + return; + } + + break; + } + + 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; + } + + context.report({ + messageId: 'expectInFloatingPromise', + node: parent, + }); }, }; },