net-utils: introduce new helper functions#7055
Conversation
| @@ -1,22 +1,21 @@ | |||
| #[cfg(feature = "dev-context-only-utils")] | |||
There was a problem hiding this comment.
Its really odd that this one got migrated up...
There was a problem hiding this comment.
Auto-formatting it is. Do you want me to bring it back to the previous location?
There was a problem hiding this comment.
nope, it does not matter just odd.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7055 +/- ##
=======================================
Coverage 83.2% 83.2%
=======================================
Files 853 853
Lines 374698 374708 +10
=======================================
+ Hits 311796 311854 +58
+ Misses 62902 62854 -48 🚀 New features to boost your workflow:
|
gregcusack
left a comment
gregcusack
left a comment
There was a problem hiding this comment.
looks mostly good. just the one comment! thank you!
| /// | ||
| /// When running without nextest, this will only bump an atomic and eventually | ||
| /// panic when it runs out of port numbers to assign. | ||
| pub fn localhost_port_range_for_tests() -> (u16, u16) { |
There was a problem hiding this comment.
should this also return a Range<u16>? Feels like we should return port ranges in one way, rather than two.
There was a problem hiding this comment.
We have a whole bunch of code that expects this port range format. Legacy:)
There was a problem hiding this comment.
ahh ok sad!
we do have the off by one error here still. Range<u16> is great since it returns inclusive on lower end and exclusive on upper end. But if we explicitly use (pr.start, pr.end), this returns a range of 21 usable ports by the caller. so unique_port_range_for_tests() only reserves 20 ports but localhost_port_range_for_tests() returns 21 ports to the caller. Maybe we can make a note here or something saying that localhost_port_range_for_tests() returns the range, inclusive start, exclusive end. Do not use the end port.
There was a problem hiding this comment.
You are absolutely right. I was convinced that the fact, that the localhost_port_range_for_tests() would get Range from the unique_port_range_for_tests resolves the issue, but i just checked it in tests and was wrong indeed. Thank you for pointing this issue again.
Since in the tests that base on the localhost_port_range_for_tests we use ports in the range with exclusive end, these getting as expected 20 ports, but i agree it might be used incorrectly.
I scanned thru the repo and there is 35 occurrences of the localhost_port_range_for_tests. If we fix it in this PR it would be quite extensive again, hence why, as you suggested, I'll add the comment and in the next PR, we might either switch to Range in this function as well, or even remove it entirely and use directly the unique_port_range_for_tests
Thank you 🙏
There was a problem hiding this comment.
As I understand it, the code as proposed will fix the localhost_port_range_for_tests, so it will work perfectly fine returning 1 less port. No need to change its users. I'd remove the note there, it does nothing but add confusion.
There was a problem hiding this comment.
sorry i'm late on this. if we call unique_port_range_for_tests(20), we are allocating/reserving 20 ports, say port 10000 -> 10019. But the range returned is [10000, 10020). Which is exactly what we want. But then localhost_port_range_for_tests() will return ports 10000 -> 10020 (inclusive). So if a test binds to 10020 and another test calls unique_port_range_for_tests(20), returning [10020, 10040), we will have a one port overlap (10020), causing a test to fail for the same reason we are trying to fix.
puhtaytow
force-pushed
the
net-utils-helpers-0
branch
from
d198b06 to
8d86cec
Compare
KirillLykov
left a comment
KirillLykov
left a comment
There was a problem hiding this comment.
Looks good to me, but please wait for other reviewers approve as well.
|
Thank you! |
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.
This PR is the step in the journey toward resolving the issue.
Related to #6886
and #6957
Summary of Changes
Introduce
unique_port_range_for_testsbind_to_localhost_uniqueAdjust
localhost_port_range_for_tests