net-utils: clean up deprecated methods and update unit tests by fkouteib · Pull Request #7508 · anza-xyz/agave

fkouteib · 2025-08-13T23:52:06Z

Problem

Several methods in net-utils have been deprecated and replaced by methods in net-utils::sockets. However, the unit tests all still called the deprecated methods.

Summary of Changes

Remove all deprecated code in net utils that's deprecated as of v2.3.2.
Update unit tests to use the new net-utils::sockets methods adn structs.
Fix test_bind() to adapt to re-use port flag no longer being set explicitly and directly in socket config.
Retire test_multi_bind_in_range_with_config_reuseport_disabled() as it's no longer valid.
Retire test_bind_with_any_port() as it's no longer valid.

fkouteib · 2025-08-13T23:55:02Z

        .is_err());
    }

-    #[test]


This test is no longer valid. We don't have direct/explicit control over setting/clearing re-use port flag. So multi-bind_*() always enables the re-use port and we can't test the combo desired in this test (multi-bind+re-use disabled).

codecov-commenter · 2025-08-14T00:42:15Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.4%. Comparing base (8cc1a62) to head (36ef207).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##           master    #7508     +/-   ##
=========================================
- Coverage    83.4%    83.4%   -0.1%     
=========================================
  Files         814      814             
  Lines      366068   365777    -291     
=========================================
- Hits       305416   305144    -272     
+ Misses      60652    60633     -19

🚀 New features to boost your workflow:

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

alexpyattaev · 2025-08-14T08:54:01Z

Unfortunately the deprecation backport got stuck in review limbo so we can not actually axe these functions now #6705 . Since 3.0 is about to get cut it is probably worth changing these to deprecated in 3.0 and also kill that backport PR.

fkouteib · 2025-08-14T17:52:56Z

Unfortunately the deprecation backport got stuck in review limbo so we can not actually axe these functions now #6705 . Since 3.0 is about to get cut it is probably worth changing these to deprecated in 3.0 and also kill that backport PR.

Thank you for the context, Alex. Understood. I will hold off on getting review sign-off on this PR until after v3.0 branch is cut. I have put up #7531 for deprecation version update.

alexpyattaev · 2025-08-14T20:56:41Z

Unfortunately the deprecation backport got stuck in review limbo so we can not actually axe these functions now #6705 . Since 3.0 is about to get cut it is probably worth changing these to deprecated in 3.0 and also kill that backport PR.

Thank you for the context, Alex. Understood. I will hold off on getting review sign-off on this PR until after v3.0 branch is cut. I have put up #7531 for deprecation version update.

Yes sounds good!

fkouteib · 2025-08-21T23:22:51Z

        }
    }

-    #[test]


Test is no longer valid. Bind to any port is no longer supported.

fkouteib · 2025-08-21T23:23:09Z

    }

-    #[test]
-    fn test_bind_with_any_port() {


Test is no longer valid. Bind to any port is no longer supported.

fkouteib

Rebased to the latest master after version bump to 3.1 and v3.0 branch cut, and removed additional functions and tests that have changed since the last iteration on these changes.

gregcusack

looks good just the one question/comment on test location

gregcusack · 2025-08-21T23:51:29Z

+        crate::sockets::{
+            bind_common_in_range_with_config, bind_more_with_config, bind_to, bind_to_with_config,
+            bind_two_in_range_with_offset_and_config, multi_bind_in_range_with_config,
+            unique_port_range_for_tests, SocketConfiguration as SocketConfig,
+        },


for the tests using these, should we just move those tests into sockets.rs instead of leaving them here?

Good catch. Some of these were actually duplicate tests with identical name and purpose [test_bind(), test_bind_in_range_nil(), test_bind_common_in_range(), test_bind_two_in_range_with_offset()]. So I removed them. The others like test_get_public_ip_addr_reachable() are testing higher level functionality relevant to libary methods in the file. So I think they should stay here and they call socket methods as part of their config setup.

gregcusack

lgtm! but should probably let @alexpyattaev review before merging

alexpyattaev

LGTM but I am not 100% we can make this change on a minor release. Merge at your own peril.

fkouteib · 2025-08-22T04:54:54Z

LGTM but I am not 100% we can make this change on a minor release. Merge at your own peril.

Thank you for the review and the feedback. Would you mind elaborating on the concern? This will not be backported to v3 branch. It's going to master and released in the next quarterly cut release.

alexpyattaev · 2025-08-22T04:59:49Z

LGTM but I am not 100% we can make this change on a minor release. Merge at your own peril.

Thank you for the review and the feedback. Would you mind elaborating on the concern? This will not be backported to v3 branch. It's going to master and released in the next quarterly cut release.

Master is at 3.1 now, which is a minor release. Removing deprecated items is a major API break. But I guess in agave we do not follow strict interpretation of semver.

fkouteib · 2025-08-22T05:13:38Z

Master is at 3.1 now, which is a minor release. Removing deprecated items is a major API break. But I guess in agave we do not follow strict interpretation of semver.

Ah yes. I agree but my sense is in Agave that we treat the quarterly releases as "major" when the release branch is first cut, and any BP changes into a release branch as "minor" releases.

@steviez what is the official agave policy on what's considered a "major release"? Apologies if it's already documented and published somewhere and I missed it. Thank you.

steviez · 2025-08-23T07:01:17Z

Master is at 3.1 now, which is a minor release. Removing deprecated items is a major API break. But I guess in agave we do not follow strict interpretation of semver.

Ah yes. I agree but my sense is in Agave that we treat the quarterly releases as "major" when the release branch is first cut, and any BP changes into a release branch as "minor" releases.

@steviez what is the official agave policy on what's considered a "major release"? Apologies if it's already documented and published somewhere and I missed it. Thank you.

Historically, we haven't been great about observing semver in Agave. However, that isn't necessarily a good thing and we're trying to do better. Some discussion here including a note I just added: #3724

If we want to do the "right thing", we should probably defer this until v4.0. @jacobcreech might be able to comment about any ecosystem repercussions of us breaking stuff. With that all said, I'm guessing nobody would actually miss what this PR is ripping out.

fkouteib · 2025-08-23T08:00:48Z

Thanks Steve. Deferring this to v4.0 release.

fkouteib commented Aug 13, 2025

View reviewed changes

fkouteib requested review from alexpyattaev and gregcusack August 13, 2025 23:55

fkouteib mentioned this pull request Aug 14, 2025

net-utils: update deprecated methods to dropped since 3.0 #7531

Merged

fkouteib force-pushed the net-utils-cleanup branch from d112fd4 to 864f6b4 Compare August 21, 2025 23:21

fkouteib requested a review from a team as a code owner August 21, 2025 23:21

net-utils: Clean up deprecated methods and update unit tests

864f6b4

fkouteib commented Aug 21, 2025

View reviewed changes

Comment thread net-utils/src/sockets.rs

}

}

#[test]

Copy link
Copy Markdown

Author

fkouteib Aug 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test is no longer valid. Bind to any port is no longer supported.

fkouteib commented Aug 21, 2025

View reviewed changes

gregcusack reviewed Aug 21, 2025

View reviewed changes

Remove duplicate tests

36ef207

gregcusack self-requested a review August 22, 2025 00:13

gregcusack approved these changes Aug 22, 2025

View reviewed changes

alexpyattaev approved these changes Aug 22, 2025

View reviewed changes

fkouteib closed this Aug 23, 2025

alexpyattaev added the wait for major release This PR should be reconsidered at next major release label Aug 23, 2025

                       }
                   }
-                  #[test]

Conversation

fkouteib commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Summary of Changes

Uh oh!

fkouteib Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

alexpyattaev commented Aug 14, 2025

Uh oh!

fkouteib commented Aug 14, 2025

Uh oh!

alexpyattaev commented Aug 14, 2025

Uh oh!

fkouteib Aug 21, 2025

Choose a reason for hiding this comment

Uh oh!

fkouteib Aug 21, 2025

Choose a reason for hiding this comment

Uh oh!

fkouteib left a comment

Choose a reason for hiding this comment

Uh oh!

gregcusack left a comment

Choose a reason for hiding this comment

Uh oh!

gregcusack Aug 21, 2025

Choose a reason for hiding this comment

Uh oh!

fkouteib Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

gregcusack left a comment

Choose a reason for hiding this comment

Uh oh!

alexpyattaev left a comment

Choose a reason for hiding this comment

Uh oh!

fkouteib commented Aug 22, 2025

Uh oh!

alexpyattaev commented Aug 22, 2025

Uh oh!

fkouteib commented Aug 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steviez commented Aug 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fkouteib commented Aug 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

fkouteib commented Aug 13, 2025 •

edited

Loading

codecov-commenter commented Aug 14, 2025 •

edited

Loading

fkouteib commented Aug 22, 2025 •

edited

Loading

steviez commented Aug 23, 2025 •

edited

Loading