test: add unit tests for SortIpAddresses utility function#6128
test: add unit tests for SortIpAddresses utility function#6128saiashok0981 wants to merge 1 commit into
Conversation
SortIpAddresses is an exported utility function in pkg/utils/net.go that performs IP address deduplication and byte-level sorting. Despite being used in CSI-related code paths and being a non-trivial operation, it had zero test coverage before this change. This commit introduces pkg/utils/net_test.go with 7 table-driven test cases covering the following scenarios: - empty_input: empty slice returns nil (consistent with append semantics) - single_ip: single IP is returned unchanged - already_sorted: pre-sorted IPs maintain their order - unsorted_ips: IPs are correctly reordered - duplicate_ips_are_deduplicated: the internal map deduplicates entries - cross_subnet_ordering: validates 10.x < 172.x < 192.x ordering - last_octet_ordering: numeric ordering (3 < 20 < 100), not lexicographic The tests use the standard table-driven pattern consistent with other test files in this package (e.g., slice_test.go, errors_test.go) and use only stdlib reflect and testing packages. All tests pass with: go test ./pkg/utils/... -run TestSortIpAddresses Signed-off-by: saiashok103@gmail.com
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @saiashok0981. Thanks for your PR. I'm waiting for a fluid-cloudnative member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new test file, pkg/utils/net_test.go, to verify the functionality of the SortIpAddresses utility. The reviewer suggests enhancing the test suite by adding coverage for edge cases, specifically invalid IP strings and mixed IPv4/IPv6 addresses, to ensure greater robustness and reliability.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| tests := []struct { | ||
| name string | ||
| ips []string | ||
| want []string | ||
| }{ | ||
| { | ||
| name: "empty_input", | ||
| ips: []string{}, | ||
| want: nil, | ||
| }, | ||
| { | ||
| name: "single_ip", | ||
| ips: []string{"10.0.0.1"}, | ||
| want: []string{"10.0.0.1"}, | ||
| }, | ||
| { | ||
| name: "already_sorted", | ||
| ips: []string{"10.0.0.1", "10.0.0.2", "10.0.0.3"}, | ||
| want: []string{"10.0.0.1", "10.0.0.2", "10.0.0.3"}, | ||
| }, | ||
| { | ||
| name: "unsorted_ips", | ||
| ips: []string{"10.0.0.3", "10.0.0.1", "10.0.0.2"}, | ||
| want: []string{"10.0.0.1", "10.0.0.2", "10.0.0.3"}, | ||
| }, | ||
| { | ||
| name: "duplicate_ips_are_deduplicated", | ||
| ips: []string{"10.0.0.1", "10.0.0.2", "10.0.0.1"}, | ||
| want: []string{"10.0.0.1", "10.0.0.2"}, | ||
| }, | ||
| { | ||
| name: "cross_subnet_ordering", | ||
| ips: []string{"192.168.1.1", "10.0.0.1", "172.16.0.1"}, | ||
| want: []string{"10.0.0.1", "172.16.0.1", "192.168.1.1"}, | ||
| }, | ||
| { | ||
| name: "last_octet_ordering", | ||
| ips: []string{"10.0.0.100", "10.0.0.20", "10.0.0.3"}, | ||
| want: []string{"10.0.0.3", "10.0.0.20", "10.0.0.100"}, | ||
| }, | ||
| } |
There was a problem hiding this comment.
The test suite is a great start, but it could be made more comprehensive by including tests for important edge cases. This would improve the robustness of the SortIpAddresses utility.
Specifically, please consider adding test cases for:
-
Invalid IP strings: The
SortIpAddressesfunction does not currently specify behavior for invalid IP strings (e.g.,"not-an-ip","").net.ParseIPreturnsnilfor these, andip.String()on anilnet.IPresults in the string"<nil>"in the output. A test should be added to assert the desired behavior, which would ideally be to filter out invalid IPs. -
Mixed IPv4 and IPv6 addresses: As noted in the pull request description,
bytes.Comparehas specific behavior for different IP versions. Adding a test with a mix of IPv4 and IPv6 addresses would ensure the sorting is correct and provide a valuable regression guard.
Here are some example test cases you could add:
{
name: "with_invalid_ips",
ips: []string{"10.0.0.2", "not-an-ip", "10.0.0.1"},
want: []string{"10.0.0.1", "10.0.0.2"},
},
{
name: "mixed_ipv4_and_ipv6",
ips: []string{"2001:db8::1", "::1", "192.168.1.1", "10.0.0.1"},
want: []string{"::1", "10.0.0.1", "192.168.1.1", "2001:db8::1"},
},
|
@saiashok0981 — this PR is blocked from merging by a failing DCO check. Please resolve this now. For a single-commit PR: For multiple commits on this branch: The code review side is finished and any merge labels already on the PR will take effect once DCO turns green. Reference: https://github.com/apps/dco |
|
/copilot review |
| name: "last_octet_ordering", | ||
| ips: []string{"10.0.0.100", "10.0.0.20", "10.0.0.3"}, | ||
| want: []string{"10.0.0.3", "10.0.0.20", "10.0.0.100"}, | ||
| }, |
There was a problem hiding this comment.
Nice, focused table here — the cases already pin down the important behaviors (dedup, and byte-wise numeric ordering rather than lexical, which last_octet_ordering and cross_subnet_ordering catch well). Two optional additions would round it out:
- A mixed IPv4/IPv6 case.
SortIpAddressesparses everything to 16-bytenet.IPand sorts withbytes.Compare, so cross-version ordering is worth locking down as a regression guard, e.g.["2001:db8::1", "::1", "192.168.1.1", "10.0.0.1"]->["::1", "10.0.0.1", "192.168.1.1", "2001:db8::1"]. - An invalid-IP case to document current behavior. Note the function does not filter invalid input:
net.ParseIP("not-an-ip")returns nil, andnil.String()yields"<nil>", which then appears in the output. So["10.0.0.1", "not-an-ip"]currently sorts to["<nil>", "10.0.0.1"](the nil parses to an empty slice and sorts first). Worth asserting the real behavior — or, if filtering is the intended contract, that would be a change to the function itself rather than the test.
Both are non-blocking.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6128 +/- ##
==========================================
+ Coverage 65.08% 65.13% +0.05%
==========================================
Files 485 485
Lines 33989 33989
==========================================
+ Hits 22122 22139 +17
+ Misses 10126 10109 -17
Partials 1741 1741 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
Description
Add comprehensive unit tests for the SortIpAddresses function in pkg/utils/net.go. Prior to this change, SortIpAddresses had zero test coverage despite being an exported utility function used across the codebase to sort IP addresses for CSI node operations.
Motivation
SortIpAddresses is a non-trivial function that performs IP address parsing, deduplication, and byte-level comparison sorting. The absence of tests makes it impossible to detect regressions if the sorting or deduplication behavior changes. This is particularly important because:
The function is called during CSI node publish operations where incorrect IP ordering could affect runtime behavior.
Test Name | Scenario -- | -- empty_input | Empty slice returns nil single_ip | Single IP is returned unchanged already_sorted | Pre-sorted IPs maintain order unsorted_ips | IPs correctly reordered (10.0.0.3, 10.0.0.1 → sorted) duplicate_ips_are_deduplicated | Duplicate IPs are removed cross_subnet_ordering | IPs across different /8 subnets sort correctly last_octet_ordering | Numeric last-octet ordering (not lexicographic)IP address sorting uses bytes.Compare on parsed net.IP values, which has subtle semantics for IPv4-mapped IPv6 addresses and needs regression guards.
The project's CI setup measures code coverage (see codecov.yml) — untested exported functions represent gaps in coverage metrics.
This PR brings SortIpAddresses up to the same testing standard as other utility functions in the package.
Signed-off-by: saiashok103@gmail.com