-
Notifications
You must be signed in to change notification settings - Fork 5k
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: flaky test Swap-Send ETH to non-contract address with data that matches swap data signature submits a transaction successfully
#25545
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
'Pending', | ||
'-1 ETH', | ||
'', | ||
); | ||
await swapSendPage.verifyHistoryEntry( | ||
'Send ETH as TST', | ||
'Confirmed', |
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 are asserting that the expected tx is there and confirmed, which is what we really want. If we wanted to test transaction states (from unapproved, to pending to confirmed) this would need to be done in a separate test, and have a different approach (lowering the gas, to make sure that the pending tx stays there until speed up/cancel, for example)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #25545 +/- ##
========================================
Coverage 69.68% 69.68%
========================================
Files 1349 1349
Lines 47852 47852
Branches 13195 13195
========================================
Hits 33342 33342
Misses 14510 14510 ☔ View full report in Codecov by Sentry. |
Builds ready [d2f8b4c]
Page Load Metrics (240 ± 277 ms)
Bundle size diffs
|
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.
LGTM !
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.
LGTM
Description
The problem is that we are looking for a pending transaction and then a confirmed transaction. If the transaction is confirmed very fast, the pending transaction is never found, thus making the test fail.
This is a bad pattern that introduces flakiness, as we are trying to find an element by its transient state, which is variable. What we really want to assert is that the transaction is in the end confirmed, so we shouldn't try to look for the pending transaction (something that disappears fast) in the first place.
Error:
ci error
Related issues
Fixes: #25546
Manual testing steps
Screenshots/Recordings
See the screenshot in ci, how the expected transaction is indeed there (confirmed). However, on the error we couldn't locate the element when it was pending (possibly due to being really fast at occasions)
Pre-merge author checklist
Pre-merge reviewer checklist