Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(valid-expect): supporting automatically fixing missing await in some cases #1574

Merged
merged 7 commits into from
May 3, 2024
Merged

feat(valid-expect): supporting automatically fixing missing await in some cases #1574

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e56ce4d
feat: add 'fixable' to valid-expect
y-hsgw May 2, 2024
545e505
test: add test to valid-expect
y-hsgw May 2, 2024
74890c4
docs: update doc for valid-expect
y-hsgw May 2, 2024
90028bf
docs: added additional note
y-hsgw May 2, 2024
eb7a2fc
fix: docs
y-hsgw May 2, 2024
ee54e6a
docs: using fancy alerts
y-hsgw May 2, 2024
a976141
fix: format
y-hsgw May 2, 2024
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
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ set to warn in.\
| [require-to-throw-message](docs/rules/require-to-throw-message.md) | Require a message for `toThrow()` | | | | |
| [require-top-level-describe](docs/rules/require-top-level-describe.md) | Require test cases and hooks to be inside a `describe` block | | | | |
| [valid-describe-callback](docs/rules/valid-describe-callback.md) | Enforce valid `describe()` callback | ✅ | | | |
| [valid-expect](docs/rules/valid-expect.md) | Enforce valid `expect()` usage | ✅ | | | |
| [valid-expect](docs/rules/valid-expect.md) | Enforce valid `expect()` usage | ✅ | | 🔧 | |
| [valid-expect-in-promise](docs/rules/valid-expect-in-promise.md) | Require promises that have expectations in their chain to be valid | ✅ | | | |
| [valid-title](docs/rules/valid-title.md) | Enforce valid titles | ✅ | | 🔧 | |

Expand Down
6 changes: 6 additions & 0 deletions docs/rules/valid-expect.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,14 @@
💼 This rule is enabled in the ✅ `recommended`
[config](https://github.com/jest-community/eslint-plugin-jest/blob/main/README.md#shareable-configurations).

🔧 This rule is automatically fixable by the
Copy link
Member

Choose a reason for hiding this comment

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

can we have it say that "some patterns are fixable" or something like that? specifically now, only if the function is already async

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 attempted to fix it with commit 90028bf. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

fwiw it seems currently the docs generator doesn't support customizing this in anyway, so I think what you've done is fine for now - @bmish might be something worth adding; even just the ability to explicitly name particular rules as "partially fixable" in the config which would case this string to include with "in some cases" suffix

[`--fix` CLI option](https://eslint.org/docs/latest/user-guide/command-line-interface#--fix).

<!-- end auto-generated rule header -->

Note: Test function will be fixed if it is async and does not have await in the
async assertion.
Copy link
Collaborator

@G-Rath G-Rath May 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

we could use fancy alerts here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice! I made the fix in commit ee54e6a.


Ensure `expect()` is called with a single argument and there is an actual
expectation made.

Expand Down
69 changes: 69 additions & 0 deletions src/rules/__tests__/valid-expect.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -571,6 +571,8 @@ ruleTester.run('valid-expect', rule, {
// usages in async function
{
code: 'test("valid-expect", async () => { expect(Promise.resolve(2)).resolves.toBeDefined(); });',
output:
'test("valid-expect", async () => { await expect(Promise.resolve(2)).resolves.toBeDefined(); });',
errors: [
{
column: 36,
Expand All @@ -582,6 +584,8 @@ ruleTester.run('valid-expect', rule, {
},
{
code: 'test("valid-expect", async () => { expect(Promise.resolve(2)).resolves.not.toBeDefined(); });',
output:
'test("valid-expect", async () => { await expect(Promise.resolve(2)).resolves.not.toBeDefined(); });',
errors: [
{
column: 36,
Expand Down Expand Up @@ -621,6 +625,12 @@ ruleTester.run('valid-expect', rule, {
expect(Promise.resolve(1)).rejects.toBeDefined();
});
`,
output: dedent`
test("valid-expect", async () => {
await expect(Promise.resolve(2)).resolves.not.toBeDefined();
await expect(Promise.resolve(1)).rejects.toBeDefined();
});
`,
errors: [
{
line: 2,
Expand All @@ -646,6 +656,12 @@ ruleTester.run('valid-expect', rule, {
expect(Promise.resolve(1)).rejects.toBeDefined();
});
`,
output: dedent`
test("valid-expect", async () => {
await expect(Promise.resolve(2)).resolves.not.toBeDefined();
await expect(Promise.resolve(1)).rejects.toBeDefined();
});
`,
errors: [
{
line: 3,
Expand All @@ -667,6 +683,12 @@ ruleTester.run('valid-expect', rule, {
return expect(Promise.resolve(1)).rejects.toBeDefined();
});
`,
output: dedent`
test("valid-expect", async () => {
await expect(Promise.resolve(2)).resolves.not.toBeDefined();
await expect(Promise.resolve(1)).rejects.toBeDefined();
});
`,
options: [{ alwaysAwait: true }],
errors: [
{
Expand All @@ -691,6 +713,12 @@ ruleTester.run('valid-expect', rule, {
return expect(Promise.resolve(1)).rejects.toBeDefined();
});
`,
output: dedent`
test("valid-expect", async () => {
await expect(Promise.resolve(2)).resolves.not.toBeDefined();
return expect(Promise.resolve(1)).rejects.toBeDefined();
});
`,
errors: [
{
line: 2,
Expand All @@ -709,6 +737,12 @@ ruleTester.run('valid-expect', rule, {
return expect(Promise.resolve(1)).rejects.toBeDefined();
});
`,
output: dedent`
test("valid-expect", async () => {
await expect(Promise.resolve(2)).resolves.not.toBeDefined();
await expect(Promise.resolve(1)).rejects.toBeDefined();
});
`,
options: [{ alwaysAwait: true }],
errors: [
{
Expand All @@ -726,6 +760,12 @@ ruleTester.run('valid-expect', rule, {
return expect(Promise.resolve(1)).toReject();
});
`,
output: dedent`
test("valid-expect", async () => {
await expect(Promise.resolve(2)).toResolve();
await expect(Promise.resolve(1)).toReject();
});
`,
options: [{ alwaysAwait: true }],
errors: [
{
Expand Down Expand Up @@ -771,6 +811,27 @@ ruleTester.run('valid-expect', rule, {
},
],
},
{
code: dedent`
test("valid-expect", async () => {
Promise.reject(expect(Promise.resolve(2)).resolves.not.toBeDefined());
});
`,
output: dedent`
test("valid-expect", async () => {
await Promise.reject(expect(Promise.resolve(2)).resolves.not.toBeDefined());
});
`,
errors: [
{
line: 2,
column: 3,
endColumn: 72,
messageId: 'promisesWithAsyncAssertionsMustBeAwaited',
data: { orReturned: ' or returned' },
},
],
},
{
code: dedent`
test("valid-expect", () => {
Expand Down Expand Up @@ -961,6 +1022,14 @@ ruleTester.run('valid-expect', rule, {
});
});
`,
output: dedent`
test("valid-expect", () => {
return expect(functionReturningAPromise()).resolves.toEqual(1).then(async () => {
await expect(Promise.resolve(2)).resolves.toBe(1);
await expect(Promise.resolve(4)).resolves.toBe(4);
});
});
`,
errors: [
{
line: 4,
Expand Down
34 changes: 34 additions & 0 deletions src/rules/valid-expect.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import {
ModifierName,
createRule,
getAccessorValue,
getSourceCode,
isFunction,
isSupportedAccessor,
parseJestFnCallWithReason,
} from './utils';
Expand Down Expand Up @@ -48,6 +50,18 @@ const findPromiseCallExpressionNode = (node: TSESTree.Node) =>
? getPromiseCallExpressionNode(node.parent)
: null;

const findFirstAsyncFunction = ({
parent,
}: TSESTree.Node): TSESTree.Node | null => {
if (!parent) {
return null;
}

return isFunction(parent) && parent.async
? parent
: findFirstAsyncFunction(parent);
};
Comment on lines +53 to +63
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the late response; I only saw this after the new release.

Doesn't this cause false-positives for non-async functions contained in async functions? It's probably a really rare situation in test files, but I think it would be more correct like this:

  if (!parent) {
    return null;
  }

  if (isFunction(parent)) {
    return parent.async ? parent : null;
  }

  return findFirstAsyncFunction(parent);

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see the logic is changed in #1579 anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you have a test case that's failing? If so please share either way so we can ensure that going forward it doesn't fail

Copy link
Contributor

Choose a reason for hiding this comment

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

No failing test case, just noticed this while reading the code.


const getParentIfThenified = (node: TSESTree.Node): TSESTree.Node => {
const grandParentNode = node.parent?.parent;

Expand Down Expand Up @@ -127,6 +141,7 @@ export default createRule<[Options], MessageIds>({
promisesWithAsyncAssertionsMustBeAwaited:
'Promises which return async assertions must be awaited{{ orReturned }}',
},
fixable: 'code',
type: 'suggestion',
schema: [
{
Expand Down Expand Up @@ -339,6 +354,25 @@ export default createRule<[Options], MessageIds>({
? 'asyncMustBeAwaited'
: 'promisesWithAsyncAssertionsMustBeAwaited',
node,
fix(fixer) {
if (!findFirstAsyncFunction(finalNode)) {
return [];
}
const returnStatement =
finalNode.parent?.type === AST_NODE_TYPES.ReturnStatement
? finalNode.parent
: null;

if (alwaysAwait && returnStatement) {
const sourceCodeText =
getSourceCode(context).getText(returnStatement);
const replacedText = sourceCodeText.replace('return', 'await');

return fixer.replaceText(returnStatement, replacedText);
}

return fixer.insertTextBefore(finalNode, 'await ');
},
});

if (isParentArrayExpression) {
Expand Down
Loading