Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
131 changes: 66 additions & 65 deletions README.md

Large diffs are not rendered by default.

39 changes: 39 additions & 0 deletions docs/rules/no-async-wrapper-for-expected-promise.md
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
Copy link
Collaborator

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 expect is passed an async function that has a single await call", since that is what "unnecessary" means in this context

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();
});
```
2 changes: 2 additions & 0 deletions src/__tests__/__snapshots__/rules.test.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ exports[`rules should export configs that refer to actual rules 1`] = `
"jest/max-expects": "error",
"jest/max-nested-describe": "error",
"jest/no-alias-methods": "error",
"jest/no-async-wrapper-for-expected-promise": "error",
"jest/no-commented-out-tests": "error",
"jest/no-conditional-expect": "error",
"jest/no-conditional-in-test": "error",
Expand Down Expand Up @@ -108,6 +109,7 @@ exports[`rules should export configs that refer to actual rules 1`] = `
"jest/max-expects": "error",
"jest/max-nested-describe": "error",
"jest/no-alias-methods": "error",
"jest/no-async-wrapper-for-expected-promise": "error",
"jest/no-commented-out-tests": "error",
"jest/no-conditional-expect": "error",
"jest/no-conditional-in-test": "error",
Expand Down
2 changes: 1 addition & 1 deletion src/__tests__/rules.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { existsSync } from 'fs';
import { resolve } from 'path';
import plugin from '../';

const numberOfRules = 64;
const numberOfRules = 65;
const ruleNames = Object.keys(plugin.rules);
const deprecatedRules = Object.entries(plugin.rules)
.filter(([, rule]) => rule.meta.deprecated)
Expand Down
141 changes: 141 additions & 0 deletions src/rules/__tests__/no-async-wrapper-for-expected-promise.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
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: [
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be good to have some tests that:

  • pass a non-async function to expect
  • pass a reference to another function which is async
  • uses await within the expect call (e.g. expect(await x()))
  • uses of other matchers, to show that we don't actually care if rejects or resolve is being used
  • use of await within an array

'expect.hasAssertions()',
dedent`
it('pass', async () => {
expect();
})
`,
dedent`
Comment on lines 15 to 20
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you clean up the indentation of the code samples? the code should be indented one level from the indent level of the dedent, and the end backtick should be at the same indent as the dedent

i.e.

Suggested change
dedent`
it('pass', async () => {
expect();
})
`,
dedent`
dedent`
it('pass', async () => {
expect();
})
`,
dedent`

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. I'll amend in next commit.

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();
})
`,
],
invalid: [
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 Promise.all, to help prove the fixer works in more complex situations

{
code: dedent`
it('should be fix', async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The 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: 6,
column: 18,
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: 6,
column: 18,
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: 6,
column: 18,
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: 6,
column: 18,
messageId: 'noAsyncWrapperForExpectedPromise',
},
],
},
],
});
74 changes: 74 additions & 0 deletions src/rules/no-async-wrapper-for-expected-promise.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
import { AST_NODE_TYPES, type TSESTree } from '@typescript-eslint/utils';
import { createRule, parseJestFnCall } from './utils';

export default createRule({
name: __filename,
meta: {
docs: {
description:
'Disallow unnecessary async function wrapper for expected promises',
},
fixable: 'code',
messages: {
noAsyncWrapperForExpectedPromise:
'Rejected/resolved promises should not be wrapped in async function',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of a phrasing more like "Unneeded async function"? that way we don't have to deal with the "rejected/resolved", and I think it's a bit more positive to be conveying we think you can have less code as an optimization

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm cool with that 👍

},
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?.type !== AST_NODE_TYPES.ArrowFunctionExpression &&
awaitNode?.type !== AST_NODE_TYPES.FunctionExpression) ||
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we've got a utility for this

Suggested change
(awaitNode?.type !== AST_NODE_TYPES.ArrowFunctionExpression &&
awaitNode?.type !== AST_NODE_TYPES.FunctionExpression) ||
!awaitNode ||
!isFunction(awaitNode) ||

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL. I'll amend in next commit

!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),
),
];
},
});
}
},
};
},
});