-
Notifications
You must be signed in to change notification settings - Fork 116
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(test): resolve a timing issue in fake_peer_set setup #5398
Conversation
… adds match arm for Response::Nil to MempoolTransactionIds request
c724e69
to
de9ce41
Compare
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.
Let's give it a shot!
This fix does not work on macOS:
|
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 need a fix that works on macOS, or we need to turn off this test on macOS.
@teor2345 Thank you for updating the PR description.
Not a MacOS-specific issue, it caused a |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #5398 +/- ##
==========================================
- Coverage 79.10% 79.05% -0.06%
==========================================
Files 308 308
Lines 39752 39752
==========================================
- Hits 31447 31427 -20
- Misses 8305 8325 +20 |
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.
This seems to work now
Motivation
To fix a mempool test that's failing occasionally.
Closes #5384
Solution
Related cleanups:
unreachable!(..)
withassert!(added_transactions.is_empty(), ..)
in theNil
case where the response is being unwrapped with a match statementunreachable
andassert
messages to sayMempoolTransactionIds
requests should respondOk(Vec<UnminedTxId> | Nil)
Review
This is low priority.
Reviewer Checklist