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

fix(await-async-events): false positive reports on awaited expressions evaluating to promise #890

Merged

Conversation

Chamion
Copy link
Contributor

@Chamion Chamion commented Mar 22, 2024

Checks

Changes

In await-async-events, await-async-queries and await-async-utils when determining whether a promise is handled the checks are performed on a parent expression that evaluates to the call expression returning a promise instead of always the call expression. In the following cases the parent expression is used instead.

// sequence expression
(sideEffect(), promise());
// conditional expression
condition ? promise() : otherPromise();
// logical expression
maybePromise() ?? promise();
maybePromise() || promise();
condition && promise();

This change does not eliminate false positives from the three await-async- rules. The rules are fundamentally written in a brittle way where they look for specific reasons not to report and default to reporting. This means the rules are not robust against changes to the language so even if we defined perfect behaviour for all cases in current JavaScript a new language feature could introduce false positives again.

For the rules to be robust and guaranteed not to report false positives they would have to default to not report unless they find a specific reason to report. In 4d878d1 I took this approach but I noticed the other two rules seem to have a requirement of reporting in some unsafe cases (e.g. expect(query).toBeInTheDocument()) so I reverted the changes and opted to carve out exceptions for specific cases instead. Whether the rules ought to be safe with regard to false positives or if they're intentionally opinionated is another conversation. I left the safer approach in the commit history in case we want to revisit that idea. Let me know if you prefer a neatly squashed commit history instead.

Context

@sjarva invited me to contribute and I picked an issue off the top.

Fixes #887

Although the bug report was about conditional expressions the same bug could be reproduced with other expressions as well. This PR covers all the cases I could think of.

Copy link

codecov bot commented Mar 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.31%. Comparing base (93a6ab9) to head (7d0e285).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #890      +/-   ##
==========================================
+ Coverage   96.23%   96.31%   +0.08%     
==========================================
  Files          44       44              
  Lines        2419     2445      +26     
  Branches     1000     1023      +23     
==========================================
+ Hits         2328     2355      +27     
+ Misses         91       90       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Chamion
Copy link
Contributor Author

Chamion commented Mar 26, 2024

I've now shuffled around the statements such that the branch not visited in tests is on the same line as a branch that is visited. The code is worse as a result but hopefully this passes the code coverage check 😞.

@Belco90
Copy link
Member

Belco90 commented Apr 2, 2024

Thanks for your contribution @Chamion! I'll try to review this during the week.

Copy link
Member

@Belco90 Belco90 left a comment

Choose a reason for hiding this comment

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

LGTM, great implementation and detailed tests. Thanks for your contribution @Chamion

@Belco90 Belco90 changed the title Fix await-async-events false positive reports on awaited expressions evaluating to promise fix(await-async-events): false positive reports on awaited expressions evaluating to promise Apr 12, 2024
@Belco90 Belco90 added this pull request to the merge queue Apr 12, 2024
Merged via the queue into testing-library:main with commit 767f1be Apr 12, 2024
31 checks passed
Copy link

🎉 This PR is included in version 6.2.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@sjarva
Copy link
Collaborator

sjarva commented Apr 12, 2024

Thank you so much @Belco90 for reviewing! 🙇‍♀️

And huge thanks and congrats to @Chamion for contributing and getting your first PR (to this repo) reviewed and merged!! 🎉 🎈 🥳

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.

False positive for testing-library/await-async-events when returning using a ternary
3 participants