-
Notifications
You must be signed in to change notification settings - Fork 230
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
feat(valid-expect): supporting automatically fixing missing await
in some cases
#1574
Conversation
@@ -3,6 +3,9 @@ | |||
💼 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 |
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.
can we have it say that "some patterns are fixable" or something like that? specifically now, only if the function is already async
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 attempted to fix it with commit 90028bf. What do you think?
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.
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
await
in some cases
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.
This looks like a good start to me, though noting it is by no means complete so I've updated the title to reflect that and removed the "resolving" of #1140
I think though it's fine to land as-is since it's an improvement regardless 🎉
docs/rules/valid-expect.md
Outdated
Note: Test function will be fixed if it is async and does not have await in the | ||
async assertion. |
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.
we could use fancy alerts here
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.
nice! I made the fix in commit ee54e6a.
# [28.4.0](v28.3.0...v28.4.0) (2024-05-03) ### Features * **valid-expect:** supporting automatically fixing missing `await` in some cases ([#1574](#1574)) ([a407098](a407098))
🎉 This PR is included in version 28.4.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
const findFirstAsyncFunction = ({ | ||
parent, | ||
}: TSESTree.Node): TSESTree.Node | null => { | ||
if (!parent) { | ||
return null; | ||
} | ||
|
||
return isFunction(parent) && parent.async | ||
? parent | ||
: findFirstAsyncFunction(parent); | ||
}; |
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.
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);
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.
Ah, I see the logic is changed in #1579 anyway.
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.
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
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.
No failing test case, just noticed this while reading the code.
When
await
is missing , it's addingawait
.Relates to #1140