Skip to content

test: add unit tests for sortServicePortStatus and sortServiceLoadBalancerIngress #7710

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Goutham-Annem wants to merge 1 commit into
base: master
Choose a base branch
from
Open

test: add unit tests for sortServicePortStatus and sortServiceLoadBalancerIngress #7710

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
147 changes: 147 additions & 0 deletions pkg/util/helper/service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,3 +174,150 @@ func TestDedupeAndSortServiceLoadBalancerIngress(t *testing.T) {
})
}
}

func TestSortServicePortStatus(t *testing.T) {
tests := []struct {
name string
ports []corev1.PortStatus
want []corev1.PortStatus
}{
{
name: "sort by port number",
ports: []corev1.PortStatus{
{Port: 82, Protocol: "TCP"},
{Port: 80, Protocol: "TCP"},
{Port: 81, Protocol: "TCP"},
},
want: []corev1.PortStatus{
{Port: 80, Protocol: "TCP"},
{Port: 81, Protocol: "TCP"},
{Port: 82, Protocol: "TCP"},
},
},
{
name: "same port, sort by protocol",
ports: []corev1.PortStatus{
{Port: 80, Protocol: "UDP"},
{Port: 80, Protocol: "SCTP"},
{Port: 80, Protocol: "TCP"},
},
want: []corev1.PortStatus{
{Port: 80, Protocol: "SCTP"},
{Port: 80, Protocol: "TCP"},
{Port: 80, Protocol: "UDP"},
},
},
{
name: "same port and protocol, nil error sorts first",
ports: []corev1.PortStatus{
{Port: 80, Protocol: "TCP", Error: new("some-error")},
{Port: 80, Protocol: "TCP"},
},
Comment on lines +210 to +215
want: []corev1.PortStatus{
{Port: 80, Protocol: "TCP"},
{Port: 80, Protocol: "TCP", Error: new("some-error")},
},
},
{
name: "same port and protocol, sort by error string",
ports: []corev1.PortStatus{
{Port: 80, Protocol: "TCP", Error: new("error-2")},
{Port: 80, Protocol: "TCP", Error: new("error-1")},
},
want: []corev1.PortStatus{
{Port: 80, Protocol: "TCP", Error: new("error-1")},
{Port: 80, Protocol: "TCP", Error: new("error-2")},
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
for range 1000 { // eliminate randomness in sorting
sortServicePortStatus(tt.ports)
pass := assert.Equalf(t, tt.want, tt.ports, "sortServicePortStatus(%v)", tt.ports)
if !pass {
break
}
}
Comment on lines +235 to +241

@gemini-code-assist gemini-code-assist Bot Jul 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The for range 1000 loop is intended to eliminate randomness and ensure deterministic sorting. However, because sortServicePortStatus sorts the slice in-place, tt.ports is sorted in the very first iteration. In the remaining 999 iterations, the function is called on an already-sorted slice, which defeats the purpose of the loop.

To properly test determinism, you should create a fresh copy of the slice in each iteration. Note that the underlying sortServicePortStatus function currently violates strict weak ordering when both elements have a nil Error field (returning true in both directions), which makes deterministic testing even more critical.

			for range 1000 {
				ports := make([]corev1.PortStatus, len(tt.ports))
				copy(ports, tt.ports)
				sortServicePortStatus(ports)
				pass := assert.Equalf(t, tt.want, ports, "sortServicePortStatus(%v)", tt.ports)
				if !pass {
					break
				}
			}

})
}
Comment on lines +233 to +243
}

func TestSortServiceLoadBalancerIngress(t *testing.T) {
tests := []struct {
name string
ingresses []corev1.LoadBalancerIngress
want []corev1.LoadBalancerIngress
}{
{
name: "non-empty hostname sorts before empty hostname",
ingresses: []corev1.LoadBalancerIngress{
{IP: "1.1.1.1"},
{Hostname: "hostname-1"},
},
want: []corev1.LoadBalancerIngress{
{Hostname: "hostname-1"},
{IP: "1.1.1.1"},
},
},
{
name: "sort hostnames alphabetically",
ingresses: []corev1.LoadBalancerIngress{
{Hostname: "hostname-c"},
{Hostname: "hostname-a"},
{Hostname: "hostname-b"},
},
want: []corev1.LoadBalancerIngress{
{Hostname: "hostname-a"},
{Hostname: "hostname-b"},
{Hostname: "hostname-c"},
},
},
{
name: "same hostname, sort by IP",
ingresses: []corev1.LoadBalancerIngress{
{Hostname: "hostname-1", IP: "3.3.3.3"},
{Hostname: "hostname-1", IP: "1.1.1.1"},
{Hostname: "hostname-1", IP: "2.2.2.2"},
},
want: []corev1.LoadBalancerIngress{
{Hostname: "hostname-1", IP: "1.1.1.1"},
{Hostname: "hostname-1", IP: "2.2.2.2"},
{Hostname: "hostname-1", IP: "3.3.3.3"},
},
},
{
name: "same hostname and IP, non-nil IPMode sorts before nil",
ingresses: []corev1.LoadBalancerIngress{
{Hostname: "hostname-1", IP: "1.1.1.1"},
{Hostname: "hostname-1", IP: "1.1.1.1", IPMode: ptr.To(corev1.LoadBalancerIPModeVIP)},
},
want: []corev1.LoadBalancerIngress{
{Hostname: "hostname-1", IP: "1.1.1.1", IPMode: ptr.To(corev1.LoadBalancerIPModeVIP)},
{Hostname: "hostname-1", IP: "1.1.1.1"},
},
},
{
name: "same hostname and IP, sort by IPMode value",
ingresses: []corev1.LoadBalancerIngress{
{Hostname: "hostname-1", IP: "1.1.1.1", IPMode: ptr.To(corev1.LoadBalancerIPModeVIP)},
{Hostname: "hostname-1", IP: "1.1.1.1", IPMode: ptr.To(corev1.LoadBalancerIPModeProxy)},
},
want: []corev1.LoadBalancerIngress{
{Hostname: "hostname-1", IP: "1.1.1.1", IPMode: ptr.To(corev1.LoadBalancerIPModeProxy)},
{Hostname: "hostname-1", IP: "1.1.1.1", IPMode: ptr.To(corev1.LoadBalancerIPModeVIP)},
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
for range 1000 { // eliminate randomness in sorting
sortServiceLoadBalancerIngress(tt.ingresses)
pass := assert.Equalf(t, tt.want, tt.ingresses, "sortServiceLoadBalancerIngress(%v)", tt.ingresses)
if !pass {
break
}
}
Comment on lines +314 to +320

@gemini-code-assist gemini-code-assist Bot Jul 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Similar to TestSortServicePortStatus, sortServiceLoadBalancerIngress sorts the slice in-place. As a result, tt.ingresses is sorted in the first iteration, and the subsequent 999 iterations run on an already-sorted slice.

To ensure the test actually verifies determinism across multiple runs, create a copy of the slice inside the loop. Note that the underlying sortServiceLoadBalancerIngress function currently violates strict weak ordering when both elements have a nil IPMode field (returning true in both directions), which makes deterministic testing even more critical.

			for range 1000 {
				ingresses := make([]corev1.LoadBalancerIngress, len(tt.ingresses))
				copy(ingresses, tt.ingresses)
				sortServiceLoadBalancerIngress(ingresses)
				pass := assert.Equalf(t, tt.want, ingresses, "sortServiceLoadBalancerIngress(%v)", tt.ingresses)
				if !pass {
					break
				}
			}

})
}
Comment on lines +312 to +322
}
Loading