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.

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

Merged
merged 7 commits into from
May 3, 2024

Conversation

y-hsgw
Copy link
Contributor

@y-hsgw y-hsgw commented May 2, 2024

When await is missing , it's adding await.
Relates to #1140

@@ -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
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

@G-Rath G-Rath changed the title feat(valid-expect): make fixable feat(valid-expect): supporting automatically fixing missing await in some cases May 2, 2024
Copy link
Collaborator

@G-Rath G-Rath left a 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 🎉

Comment on lines 11 to 12
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

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.

@G-Rath G-Rath merged commit a407098 into jest-community:main May 3, 2024
36 checks passed
github-actions bot pushed a commit that referenced this pull request May 3, 2024
# [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))
Copy link

github-actions bot commented May 3, 2024

🎉 This PR is included in version 28.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@y-hsgw y-hsgw deleted the fixable-valid-expect branch May 3, 2024 03:09
Comment on lines +53 to +63
const findFirstAsyncFunction = ({
parent,
}: TSESTree.Node): TSESTree.Node | null => {
if (!parent) {
return null;
}

return isFunction(parent) && parent.async
? parent
: 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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants