net-utils (& others): swap tests to non-overlapping port allocation by puhtaytow · Pull Request #6957 · anza-xyz/agave

puhtaytow · 2025-07-14T11:29:58Z

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

Summary of Changes

Swapped tests to non-overlapping port allocation.
Added helper function to convert Range to (u16, u16) as still some functions expect the tuple.
Added helper functions to allocate unique ports for localhost, range and single scenarios.
Reduced amount of imports and lines / less verbose overall.
Swapped UNSPECIFIED to LOCALHOST at many points.

codecov-commenter · 2025-07-14T12:38:20Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.2%. Comparing base (fac7555) to head (ff43cf2).
Report is 53 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #6957   +/-   ##
=======================================
  Coverage    83.2%    83.2%           
=======================================
  Files         856      856           
  Lines      376845   376862   +17     
=======================================
+ Hits       313722   313754   +32     
+ Misses      63123    63108   -15

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

alexpyattaev

Thank you!

puhtaytow · 2025-07-14T12:52:50Z

Thank you!

Thank you! 🙏

lijunwangs · 2025-07-17T08:21:03Z

        let mut duplicate_slot_repair_statuses = HashMap::new();
        let dead_slot = 9;
-        let receive_socket = &bind_to_unspecified().unwrap();
+        let receive_socket = &bind_to(


These code are so much repeated, maybe it is worth to create a new bind_to_unspecified utility function.

Yes, that might be nice, but let us fix our CI first. Just changing semantics of bind_to_unspecified is not a great plan either as there are legit cases where you might want to call it.

KirillLykov · 2025-07-17T08:32:05Z

I agree with Lijun that would be nice to not repeat this code everywhere.

alexpyattaev · 2025-07-17T08:35:23Z

Ok, fine. @puhtaytow seems like we have to come up with a way to have a new bind_to_unspecified that works via the same mechanism as localhost_port_range_for_tests but does not allocate a full 20-port span every time.

puhtaytow · 2025-07-17T09:55:11Z

Surely. Let me look at this 🙏

steviez · 2025-07-17T16:43:53Z

Minor note but:

net-utils / core, rpc, streamer, turbine, validator: introduce new helper function / swap tests to non-overlapping ports

certainly exceeds the suggested character limit for commit titles

lijunwangs

LGTM

puhtaytow · 2025-07-17T17:38:45Z

Thank you guys! 🙏

gregcusack · 2025-07-17T18:32:37Z

-    assert!(start < u16::MAX - 20, "ran out of port numbers!");
-    (start, start + 20)
+    assert!(start < u16::MAX - size, "ran out of port numbers!");
+    (start, start + size)


do we have an off by one error here? If start is 10000, and i want 20 ports, i get back (10000, 10020) which is 21 ports.

Very good question, i wasn't confident enough so decided to check it with the test and the code below pass positively.

let (pr_s, pr_e) = localhost_port_range_for_tests(); assert_eq!(pr_e - pr_s, 20); ...

well ya the difference between 10020 and 10000 is 20. but if we return both of them, the user will use them all. but that is 21 ports returned even though the difference is 20.
in your example above, if i use every port from pr_e to pr_s, that is 21 ports. if we count it out: 0, 1, 2, 3, ..., 19, 20. that is 21.

right, would you suggest to add note in the function docs or maybe return Range? instead?

switching to range would be great, yes. Since this is a new function we might as well have proper API for it.

…han one Co-authored-by: Greg Cusack <greg.cusack@anza.xyz>

puhtaytow · 2025-07-18T15:52:36Z

    url::Url,
 };

+/// Helper for code that still expects port ranges as `(u16, u16)`


Should it be here or somewhere else?

…x after

puhtaytow · 2025-07-18T16:40:33Z

The Range was fixed in as suggested. I used this occasion to fix more stuff that didn't look like it makes much sense.

Please tell me WDYT 🙏

alexpyattaev · 2025-07-19T15:39:00Z

The Range was fixed in as suggested. I used this occasion to fix more stuff that didn't look like it makes much sense.

Generally it is not advised to do "drive-by" fixes for things unrelated to the subject of the PR. Not a huge deal here, but please avoid when possible.

alexpyattaev

This looks like it is going in the right direction, but I feel like we should split this into 2 PRs: one to introduce new functions in net-utils, another to patch them into the codebase. Also left a few nits and bikeshedding suggestions.

alexpyattaev · 2025-07-19T15:54:55Z

+///
+/// 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_single_for_tests() -> u16 {


Is this function redundant? If not, maybe it should be called unique_port_for_tests ?

alexpyattaev · 2025-07-19T15:55:44Z

-    bind_to_async(IpAddr::V4(Ipv4Addr::LOCALHOST), 0).await
+    bind_to_async(
+        IpAddr::V4(Ipv4Addr::LOCALHOST),
+        localhost_port_range_for_tests().start,


why are we eating an entire port range for this one?

alexpyattaev · 2025-07-19T15:57:14Z

    #[test]
    pub fn test_multicast_msg() {
-        let reader = bind_to_localhost().expect("bind");
+        let reader = bind_to_localhost_unique().expect("should bind");


if we are changing the messages, maybe giving them numbers would be nice?

puhtaytow · 2025-07-21T07:10:09Z

Turned this PR into draft with no intentions to ever merge it. As suggested by @alexpyattaev it would be splitted into multiple PRs starting from this one #7055

Thank you and sorry for the chaos 🙏

puhtaytow added 2 commits July 14, 2025 11:46

move to bind_to / bind_to_async / ditch bind_to_unspecified/ async

9eea7cf

swap remaining tests

a26d7d5

mergify Bot added community need:merge-assist labels Jul 14, 2025

mergify Bot requested a review from a team July 14, 2025 11:30

inline bind_to in meta

d498b42

alexpyattaev added the CI Pull Request is ready to enter CI label Jul 14, 2025

anza-team removed the CI Pull Request is ready to enter CI label Jul 14, 2025

alexpyattaev previously approved these changes Jul 14, 2025

View reviewed changes

alexpyattaev mentioned this pull request Jul 14, 2025

streamer, tests: migrate to non-overlapping ports #6895

Merged

alexpyattaev requested a review from lijunwangs July 15, 2025 11:59

lijunwangs reviewed Jul 17, 2025

View reviewed changes

puhtaytow added 13 commits July 17, 2025 14:40

support range and single ports scenarios

a45ca39

fix typo

66e0081

move tests to new bind_to api

073dee6

fix import

a6dd863

move to new unique api

c50db86

finish it

c396d24

cosmetics / bring docs part / change function names to more explicit

f62e018

remove obsolete function

975fa39

simplify the code

ccb3dac

further reduce code

2d489d6

more explicit function naming

5140795

reduce even more code

5123fea

adjust func name

993e52c

lijunwangs previously approved these changes Jul 17, 2025

View reviewed changes

lijunwangs added the CI Pull Request is ready to enter CI label Jul 17, 2025

anza-team removed the CI Pull Request is ready to enter CI label Jul 17, 2025

puhtaytow changed the title ~~net-utils / core, rpc, streamer, turbine, validator: introduce new helper function / swap tests to non-overlapping ports~~ net-utils, core, rpc, streamer, turbine, validator: swap test to non-overlapping port solution Jul 17, 2025

puhtaytow changed the title ~~net-utils, core, rpc, streamer, turbine, validator: swap test to non-overlapping port solution~~ net-utils, core, rpc, streamer, turbine, validator: swap tests to non-overlapping port solution Jul 17, 2025

gregcusack self-requested a review July 17, 2025 18:09

gregcusack reviewed Jul 17, 2025

View reviewed changes

allocate single port instead of many for cases that don't need more t…

807dce4

…han one Co-authored-by: Greg Cusack <greg.cusack@anza.xyz>

puhtaytow dismissed lijunwangs’s stale review via 807dce4 July 17, 2025 18:38

move to range / adjust code

4d2894b

puhtaytow marked this pull request as draft July 18, 2025 06:14

alexpyattaev changed the title ~~net-utils, core, rpc, streamer, turbine, validator: swap tests to non-overlapping port solution~~ net-utils (& others): swap tests to non-overlapping port allocation solution Jul 18, 2025

alexpyattaev changed the title ~~net-utils (& others): swap tests to non-overlapping port allocation solution~~ net-utils (& others): swap tests to non-overlapping port allocation Jul 18, 2025

puhtaytow added 3 commits July 18, 2025 12:41

cleanup

b5e1bc0

cleanup round 2

f836247

cleanup round 3

738c73a

puhtaytow commented Jul 18, 2025

View reviewed changes

puhtaytow added 2 commits July 18, 2025 17:58

add deprecation for bind_to_unspecified

47c0048

remove the deprecation of bind_to_unspecified / too many things to fi…

796b912

…x after

puhtaytow marked this pull request as ready for review July 18, 2025 16:27

gregcusack self-requested a review July 18, 2025 17:05

alexpyattaev reviewed Jul 19, 2025

View reviewed changes

puhtaytow marked this pull request as draft July 21, 2025 05:26

puhtaytow closed this Jul 21, 2025

alexpyattaev mentioned this pull request Jul 21, 2025

net-utils: introduce new helper functions #7055

Merged

Conversation

puhtaytow commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Summary of Changes

Uh oh!

codecov-commenter commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

alexpyattaev left a comment

Choose a reason for hiding this comment

Uh oh!

puhtaytow commented Jul 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KirillLykov commented Jul 17, 2025

Uh oh!

alexpyattaev commented Jul 17, 2025

Uh oh!

puhtaytow commented Jul 17, 2025

Uh oh!

steviez commented Jul 17, 2025

Uh oh!

lijunwangs left a comment

Choose a reason for hiding this comment

Uh oh!

puhtaytow commented Jul 17, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

puhtaytow commented Jul 18, 2025

Uh oh!

alexpyattaev commented Jul 19, 2025

Uh oh!

alexpyattaev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

puhtaytow commented Jul 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

puhtaytow commented Jul 14, 2025 •

edited

Loading

codecov-commenter commented Jul 14, 2025 •

edited

Loading