testing-library / eslint-plugin-testing-library

ESLint plugin to follow best practices and anticipate common mistakes when writing tests with Testing Library
https://npm.im/eslint-plugin-testing-library
MIT License
967 stars 135 forks source link

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

Closed Chamion closed 2 months ago

Chamion commented 3 months ago

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.

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Chamion commented 3 months ago

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 commented 2 months ago

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

github-actions[bot] commented 2 months ago

:tada: This PR is included in version 6.2.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

sjarva commented 2 months ago

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!! πŸŽ‰ 🎈 πŸ₯³