-
Notifications
You must be signed in to change notification settings - Fork 248
feat(rule): disallow unnecessary async function wrapper for single await call
#1863
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
c497904
c7e1d1e
b23a00b
36e4235
0bf81a6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| # Disallow unnecessary async function wrapper for expected promises (`no-async-wrapper-for-expected-promise`) | ||
|
|
||
| 🔧 This rule is automatically fixable by the | ||
| [`--fix` CLI option](https://eslint.org/docs/latest/user-guide/command-line-interface#--fix). | ||
|
|
||
| <!-- end auto-generated rule header --> | ||
|
|
||
| `Jest` can handle fulfilled/rejected promisified function call normally but | ||
| occassionally, engineers wrap said function in another `async` function that is | ||
| excessively verbose and make the tests harder to read. | ||
|
|
||
| ## Rule details | ||
|
|
||
| This rule triggers a warning if a single `await` function call is wrapped by an | ||
| unnecessary `async` function. | ||
|
|
||
| Examples of **incorrect** code for this rule | ||
|
|
||
| ```js | ||
| it('wrong1', async () => { | ||
| await expect(async () => { | ||
| await doSomethingAsync(); | ||
| }).rejects.toThrow(); | ||
| }); | ||
|
|
||
| it('wrong2', async () => { | ||
| await expect(async function () { | ||
| await doSomethingAsync(); | ||
| }).rejects.toThrow(); | ||
| }); | ||
| ``` | ||
|
|
||
| Examples of **correct** code for this rule | ||
|
|
||
| ```js | ||
| it('right1', async () => { | ||
| await expect(doSomethingAsync()).rejects.toThrow(); | ||
| }); | ||
| ``` | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,149 @@ | ||
| import dedent from 'dedent'; | ||
| import rule from '../no-async-wrapper-for-expected-promise'; | ||
| import { FlatCompatRuleTester as RuleTester, espreeParser } from './test-utils'; | ||
|
|
||
| const ruleTester = new RuleTester({ | ||
| parser: espreeParser, | ||
| parserOptions: { | ||
| ecmaVersion: 2017, | ||
| }, | ||
| }); | ||
|
|
||
| ruleTester.run('no-async-wrapper-for-expected-promise', rule, { | ||
| valid: [ | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it would be good to have some tests that:
|
||
| 'expect.hasAssertions()', | ||
| dedent` | ||
| it('pass', async () => { | ||
| expect(); | ||
| }) | ||
| `, | ||
| dedent` | ||
| it('pass', async () => { | ||
| await expect(doSomethingAsync()).rejects.toThrow(); | ||
| }) | ||
| `, | ||
| dedent` | ||
| it('pass', async () => { | ||
| await expect(doSomethingAsync(1, 2)).resolves.toBe(1); | ||
| }) | ||
| `, | ||
| dedent` | ||
| it('pass', async () => { | ||
| await expect(async () => { | ||
| await doSomethingAsync(); | ||
| await doSomethingTwiceAsync(1, 2); | ||
| }).rejects.toThrow(); | ||
| }) | ||
| `, | ||
| { | ||
| code: dedent` | ||
| import { expect as pleaseExpect } from '@jest/globals'; | ||
| it('pass', async () => { | ||
| await pleaseExpect(doSomethingAsync()).rejects.toThrow(); | ||
| }) | ||
| `, | ||
| parserOptions: { sourceType: 'module' }, | ||
| }, | ||
| dedent` | ||
| it('pass', async () => { | ||
| await expect(async () => { | ||
| doSomethingSync(); | ||
| }).rejects.toThrow(); | ||
| }) | ||
| `, | ||
| dedent` | ||
| it('pass', async () => { | ||
| await expect(async () => { | ||
| const a = 1; | ||
| await doSomethingSync(a); | ||
| }).rejects.toThrow(); | ||
| }) | ||
| `, | ||
| ], | ||
| invalid: [ | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it'd be good if we could have a test with say a |
||
| { | ||
| code: dedent` | ||
| it('should be fix', async () => { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: these would be more correct as "should be fixed" 😅 |
||
| await expect(async () => { | ||
| await doSomethingAsync(); | ||
| }).rejects.toThrow(); | ||
| }) | ||
| `, | ||
| output: dedent` | ||
| it('should be fix', async () => { | ||
| await expect(doSomethingAsync()).rejects.toThrow(); | ||
| }) | ||
| `, | ||
| errors: [ | ||
| { | ||
| endColumn: 4, | ||
| column: 16, | ||
| messageId: 'noAsyncWrapperForExpectedPromise', | ||
| }, | ||
| ], | ||
| }, | ||
| { | ||
| code: dedent` | ||
| it('should be fix', async () => { | ||
| await expect(async function () { | ||
| await doSomethingAsync(); | ||
| }).rejects.toThrow(); | ||
| }) | ||
| `, | ||
| output: dedent` | ||
| it('should be fix', async () => { | ||
| await expect(doSomethingAsync()).rejects.toThrow(); | ||
| }) | ||
| `, | ||
| errors: [ | ||
| { | ||
| endColumn: 4, | ||
| column: 16, | ||
| messageId: 'noAsyncWrapperForExpectedPromise', | ||
| }, | ||
| ], | ||
| }, | ||
| { | ||
| code: dedent` | ||
| it('should be fix', async () => { | ||
| await expect(async () => { | ||
| await doSomethingAsync(1, 2); | ||
| }).rejects.toThrow(); | ||
| }) | ||
| `, | ||
| output: dedent` | ||
| it('should be fix', async () => { | ||
| await expect(doSomethingAsync(1, 2)).rejects.toThrow(); | ||
| }) | ||
| `, | ||
| errors: [ | ||
| { | ||
| endColumn: 4, | ||
| column: 16, | ||
| messageId: 'noAsyncWrapperForExpectedPromise', | ||
| }, | ||
| ], | ||
| }, | ||
| { | ||
| code: dedent` | ||
| it('should be fix', async () => { | ||
| await expect(async function () { | ||
| await doSomethingAsync(1, 2); | ||
| }).rejects.toThrow(); | ||
| }) | ||
| `, | ||
| output: dedent` | ||
| it('should be fix', async () => { | ||
| await expect(doSomethingAsync(1, 2)).rejects.toThrow(); | ||
| }) | ||
| `, | ||
| errors: [ | ||
| { | ||
| endColumn: 4, | ||
| column: 16, | ||
| messageId: 'noAsyncWrapperForExpectedPromise', | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| import { AST_NODE_TYPES, type TSESTree } from '@typescript-eslint/utils'; | ||
| import { createRule, isFunction, parseJestFnCall } from './utils'; | ||
|
|
||
| export default createRule({ | ||
| name: __filename, | ||
| meta: { | ||
| docs: { | ||
| description: | ||
| 'Disallow unnecessary async function wrapper for expected promises', | ||
| }, | ||
| fixable: 'code', | ||
| messages: { | ||
| noAsyncWrapperForExpectedPromise: 'Unnecessary async function wrapper', | ||
| }, | ||
| schema: [], | ||
| type: 'suggestion', | ||
| }, | ||
| defaultOptions: [], | ||
| create(context) { | ||
| return { | ||
| CallExpression(node: TSESTree.CallExpression) { | ||
| const jestFnCall = parseJestFnCall(node, context); | ||
|
|
||
| if (jestFnCall?.type !== 'expect') { | ||
| return; | ||
| } | ||
|
|
||
| const { parent } = jestFnCall.head.node; | ||
|
|
||
| if (parent?.type !== AST_NODE_TYPES.CallExpression) { | ||
| return; | ||
| } | ||
|
|
||
| const [awaitNode] = parent.arguments; | ||
|
|
||
| if ( | ||
| !awaitNode || | ||
| !isFunction(awaitNode) || | ||
| !awaitNode?.async || | ||
| awaitNode.body.type !== AST_NODE_TYPES.BlockStatement || | ||
| awaitNode.body.body.length !== 1 | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| const [callback] = awaitNode.body.body; | ||
|
|
||
| if ( | ||
| callback.type === AST_NODE_TYPES.ExpressionStatement && | ||
| callback.expression.type === AST_NODE_TYPES.AwaitExpression && | ||
| callback.expression.argument.type === AST_NODE_TYPES.CallExpression | ||
| ) { | ||
| const innerAsyncFuncCall = callback.expression.argument; | ||
|
|
||
| context.report({ | ||
| node: awaitNode, | ||
| messageId: 'noAsyncWrapperForExpectedPromise', | ||
| fix(fixer) { | ||
| const { sourceCode } = context; | ||
|
|
||
| return [ | ||
| fixer.replaceTextRange( | ||
| [awaitNode.range[0], awaitNode.range[1]], | ||
| sourceCode.getText(innerAsyncFuncCall), | ||
| ), | ||
| ]; | ||
| }, | ||
| }); | ||
| } | ||
| }, | ||
| }; | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to say something like "This rule triggers a warning if
expectis passed anasyncfunction that has a singleawaitcall", since that is what "unnecessary" means in this context