core, send-tx-service, quic-client: switch tests to use non-overlapping socket bind api#6897
Conversation
puhtaytow
changed the title
|
@lijunwangs when you get a chance, can you take a look at this? |
@0xbrw will do |
| let port_range = localhost_port_range_for_tests(); | ||
| ( | ||
| bind_to_localhost().unwrap(), | ||
| bind_to(IpAddr::V4(Ipv4Addr::LOCALHOST), port_range.0).expect("should bind"), |
There was a problem hiding this comment.
what happens if it fail to bind the first port in the range?
There was a problem hiding this comment.
If it fail to bind the port, then indeed there is no retry mechanism. It would panic with the expect message, rendering the test as failed.
Since the localhost_port_range_for_tests, use two mechanisms to prevent overlapping of the ports in standard tests and nextest runners scenarios, the situation would be unlikely to happen in the CI scenario, but possible in e.g. local testing where one run tests, these bind socks but before returning being interrupted and then tests being run again before releasing the previously allocated port or when the port was already in use.
This are the top of my head scenarios. Do you suggest to apply changes to cover edge cases like this?
Thank you for the question 🙏 🍀
There was a problem hiding this comment.
Lijun, this should literally be impossible. If it ever happens, we fix localhost_port_range_for_tests to do its job.
There was a problem hiding this comment.
Does it work when the unit tests are executed in parallel?
There was a problem hiding this comment.
yes, as long as you use cargo test or cargo nextest. I have not tested with other test runners.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #6897 +/- ##
=======================================
Coverage 83.2% 83.2%
=======================================
Files 852 852
Lines 376974 376982 +8
=======================================
+ Hits 313890 313912 +22
+ Misses 63084 63070 -14 🚀 New features to boost your workflow:
|
…these tests aren't running at the same time / part of ledger tool
|
btw @alexpyattaev added the missing repair_service |
alexpyattaev
left a comment
alexpyattaev
left a comment
There was a problem hiding this comment.
Looks good, thank you! @lijunwangs are you happy with this one?
|
Thank you @alexpyattaev @lijunwangs 🙏 🙏 |
puhtaytow
deleted the
core-send-tx-service-quic-client-non-overlapping-socket-bind-api-0
branch
6 participants
Problem
A bunch of tests still relies on either hardcoded ports or bind_to_localhost and bind_to_unspecified. This creates flaky tests in cases where unique port ranges are needed.
Related to #6886 and #6895
Summary of Changes
This PR should close the topic of migration to non-overlapping bindings.