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

Improve forgot-await detection by tracking async calls #216

Merged
merged 6 commits into from
Aug 19, 2021

Conversation

calebeby
Copy link
Member

@calebeby calebeby commented Aug 17, 2021

When @spaceninja and I were working on some tests, we found a bug where the forgot-await detection didn't work:

const buttonEl = await screen.getByRole("button", { name: /count/i });
await expect(buttonEl).toHaveTextContent(/count is: 0/i);
user.click(buttonEl); // Should trigger forgot-await error
await expect(buttonEl).toHaveTextContent(/count is: 3/i);

This is because the current implementation usually only works if the call with await missing is the last call inside withBrowser, because it depends on Jest's feature of listening for unhandled promise rejections.

This PR reimplements the forgot-await detection. Now it is centralized, and it works differently. It works by keeping track of all async calls that are started inside withBrowser. When withBrowser finishes, it checks to see if any of the calls did not finish yet. If the developer remembered to use await on all the calls, then all the calls should have already finished, so nothing happens. If the developer forgot await on one or more of the calls, withBrowser can see that, and throw the error.

This does not handle the case where the call that is missing await executes fast enough that it finishes before the rest of the things in withBrowser.

Review with whitespace changes turned off, otherwise the diff is a lot bigger than it needs to be

@calebeby calebeby changed the title Improve forgot-await detection using an async hook tracker Improve forgot-await detection by tracking async calls Aug 17, 2021
Copy link
Member

@gerardo-rodriguez gerardo-rodriguez left a comment

Choose a reason for hiding this comment

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

Reviewing this now. Gotta love the tests, they are always the most helpful to understand the intent of the changes. 🙂

Copy link
Member

@gerardo-rodriguez gerardo-rodriguez left a comment

Choose a reason for hiding this comment

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

LGTM! I didn't see anything that obviously jumped out at me. Great work, @calebeby! 🎉

@calebeby calebeby merged commit 0c25d10 into main Aug 19, 2021
@calebeby calebeby deleted the async-hook-tracker branch August 19, 2021 23:54
@github-actions github-actions bot mentioned this pull request Aug 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants