add unit tests for helpers and simplify collectOrderedGateways

Author: sarthyparty

internal/controller/state/graph/backend_tls_policy_test.go

@@ -402,7 +402,7 @@ func TestValidateBackendTLSPolicy(t *testing.T) {
 			},
 		},
 		{
-			name: "valid case with many ancestors (ancestor limit now handled during gateway assignment)",
+			name: "valid case with many ancestors",
 			tlsPolicy: &v1alpha3.BackendTLSPolicy{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      "tls-policy",

internal/controller/state/graph/policies.go

@@ -98,9 +98,9 @@ func collectOrderedGatewaysForService(
 	svc *ReferencedService,
 	gateways map[types.NamespacedName]*Gateway,
 	existingNGFGatewayAncestors map[types.NamespacedName]struct{},
-) (existingGateways []types.NamespacedName, newGateways []types.NamespacedName) {
-	existingGateways = make([]types.NamespacedName, 0, len(svc.GatewayNsNames))
-	newGateways = make([]types.NamespacedName, 0, len(svc.GatewayNsNames))
+) []types.NamespacedName {
+	existingGateways := make([]types.NamespacedName, 0, len(svc.GatewayNsNames))
+	newGateways := make([]types.NamespacedName, 0, len(svc.GatewayNsNames))
 
 	for gwNsName := range svc.GatewayNsNames {
 		if _, exists := existingNGFGatewayAncestors[gwNsName]; exists {
@@ -113,7 +113,8 @@ func collectOrderedGatewaysForService(
 	sortGatewaysByCreationTime(existingGateways, gateways)
 	sortGatewaysByCreationTime(newGateways, gateways)
 
-	return existingGateways, newGateways
+	existingGateways = append(existingGateways, newGateways...)
+	return existingGateways
 }
 
 func (g *Graph) attachPolicies(validator validation.PolicyValidator, ctlrName string, logger logr.Logger) {
@@ -158,11 +159,7 @@ func attachPolicyToService(
 	existingNGFGatewayAncestors := extractExistingNGFGatewayAncestorsForPolicy(policy, ctlrName)
 
 	// Collect and order gateways with existing gateway prioritization
-	existingGateways, newGateways := collectOrderedGatewaysForService(
-		svc, gws, existingNGFGatewayAncestors)
-
-	existingGateways = append(existingGateways, newGateways...)
-	orderedGateways := existingGateways
+	orderedGateways := collectOrderedGatewaysForService(svc, gws, existingNGFGatewayAncestors)
 
 	for _, gwNsName := range orderedGateways {
 		gw := gws[gwNsName]

internal/controller/state/graph/policy_ancestor.go

@@ -83,10 +83,7 @@ func parentRefEqual(ref1, ref2 v1.ParentReference) bool {
 	return true
 }
 
-// Helper functions to eliminate code duplication
-
-// getAncestorName generates a human-readable name for an ancestor from a ParentReference.
-// Returns namespace/name format if namespace is specified, otherwise just name.
+// getAncestorName returns namespace/name format if namespace is specified, otherwise just name.
 func getAncestorName(ancestorRef v1.ParentReference) string {
 	ancestorName := string(ancestorRef.Name)
 	if ancestorRef.Namespace != nil {
@@ -95,15 +92,14 @@ func getAncestorName(ancestorRef v1.ParentReference) string {
 	return ancestorName
 }
 
-// getPolicyName generates a human-readable name for a policy in namespace/name format.
+// getPolicyName returns a human-readable name for a policy in namespace/name format.
 func getPolicyName(policy policies.Policy) string {
 	return policy.GetNamespace() + "/" + policy.GetName()
 }
 
-// getPolicyKind returns the policy kind with defensive programming for test environments.
-// Returns "Policy" as fallback if GetObjectKind() returns nil.
+// getPolicyKind returns the policy kind or "Policy" if GetObjectKind() returns nil.
 func getPolicyKind(policy policies.Policy) string {
-	policyKind := "Policy" // Default fallback
+	policyKind := "Policy"
 	if objKind := policy.GetObjectKind(); objKind != nil {
 		policyKind = objKind.GroupVersionKind().Kind
 	}

internal/controller/state/graph/policy_ancestor_test.go

@@ -2,13 +2,19 @@ package graph
 
 import (
 	"testing"
+	"time"
 
+	"github.com/go-logr/logr/testr"
 	. "github.com/onsi/gomega"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	v1 "sigs.k8s.io/gateway-api/apis/v1"
 	"sigs.k8s.io/gateway-api/apis/v1alpha2"
 
 	ngfAPIv1alpha2 "github.com/nginx/nginx-gateway-fabric/v2/apis/v1alpha2"
+	"github.com/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/config/policies"
 	"github.com/nginx/nginx-gateway-fabric/v2/internal/framework/kinds"
 )
 
@@ -232,3 +238,231 @@ func TestParentRefEqual(t *testing.T) {
 		})
 	}
 }
+
+func TestLogAncestorLimitReached(t *testing.T) {
+	t.Parallel()
+	logger := testr.New(t)
+	logAncestorLimitReached(logger, "test-policy", "TestPolicy", "test-ancestor")
+}
+
+func TestGetAncestorName(t *testing.T) {
+	t.Parallel()
+	tests := []struct {
+		name     string
+		ref      v1.ParentReference
+		expected string
+	}{
+		{
+			name: "with namespace",
+			ref: v1.ParentReference{
+				Name:      "test-gw",
+				Namespace: func() *v1.Namespace { ns := v1.Namespace("test-ns"); return &ns }(),
+			},
+			expected: "test-ns/test-gw",
+		},
+		{
+			name: "without namespace",
+			ref: v1.ParentReference{
+				Name: "test-gw",
+			},
+			expected: "test-gw",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			t.Parallel()
+			g := NewWithT(t)
+			g.Expect(getAncestorName(test.ref)).To(Equal(test.expected))
+		})
+	}
+}
+
+type mockPolicy struct {
+	name      string
+	namespace string
+	kind      string
+}
+
+func (m *mockPolicy) GetName() string               { return m.name }
+func (m *mockPolicy) SetName(name string)           { m.name = name }
+func (m *mockPolicy) GetNamespace() string          { return m.namespace }
+func (m *mockPolicy) SetNamespace(namespace string) { m.namespace = namespace }
+func (m *mockPolicy) GetObjectKind() schema.ObjectKind {
+	if m.kind == "" {
+		return nil
+	}
+	return &mockObjectKind{kind: m.kind}
+}
+func (m *mockPolicy) GetTargetRefs() []v1alpha2.LocalPolicyTargetReference { return nil }
+func (m *mockPolicy) GetPolicyStatus() v1alpha2.PolicyStatus               { return v1alpha2.PolicyStatus{} }
+func (m *mockPolicy) SetPolicyStatus(_ v1alpha2.PolicyStatus)              {}
+func (m *mockPolicy) DeepCopyObject() runtime.Object                       { return m }
+func (m *mockPolicy) GetUID() types.UID                                    { return "" }
+func (m *mockPolicy) GetResourceVersion() string                           { return "" }
+func (m *mockPolicy) SetUID(_ types.UID)                                   {}
+func (m *mockPolicy) SetResourceVersion(_ string)                          {}
+func (m *mockPolicy) GetGeneration() int64                                 { return 0 }
+func (m *mockPolicy) SetGeneration(_ int64)                                {}
+func (m *mockPolicy) GetCreationTimestamp() metav1.Time                    { return metav1.Time{} }
+func (m *mockPolicy) SetCreationTimestamp(_ metav1.Time)                   {}
+func (m *mockPolicy) GetDeletionTimestamp() *metav1.Time                   { return nil }
+func (m *mockPolicy) SetDeletionTimestamp(_ *metav1.Time)                  {}
+func (m *mockPolicy) GetDeletionGracePeriodSeconds() *int64                { return nil }
+func (m *mockPolicy) SetDeletionGracePeriodSeconds(*int64)                 {}
+func (m *mockPolicy) GetLabels() map[string]string                         { return nil }
+func (m *mockPolicy) SetLabels(_ map[string]string)                        {}
+func (m *mockPolicy) GetAnnotations() map[string]string                    { return nil }
+func (m *mockPolicy) SetAnnotations(_ map[string]string)                   {}
+func (m *mockPolicy) GetFinalizers() []string                              { return nil }
+func (m *mockPolicy) SetFinalizers(_ []string)                             {}
+func (m *mockPolicy) GetOwnerReferences() []metav1.OwnerReference          { return nil }
+func (m *mockPolicy) SetOwnerReferences([]metav1.OwnerReference)           {}
+func (m *mockPolicy) GetManagedFields() []metav1.ManagedFieldsEntry        { return nil }
+func (m *mockPolicy) SetManagedFields(_ []metav1.ManagedFieldsEntry)       {}
+func (m *mockPolicy) GetGenerateName() string                              { return "" }
+func (m *mockPolicy) SetGenerateName(_ string)                             {}
+func (m *mockPolicy) GetSelfLink() string                                  { return "" }
+func (m *mockPolicy) SetSelfLink(_ string)                                 {}
+
+type mockObjectKind struct{ kind string }
+
+func (m *mockObjectKind) GroupVersionKind() schema.GroupVersionKind {
+	return schema.GroupVersionKind{Kind: m.kind}
+}
+func (m *mockObjectKind) SetGroupVersionKind(_ schema.GroupVersionKind) {}
+
+func TestGetPolicyName(t *testing.T) {
+	t.Parallel()
+	g := NewWithT(t)
+	policy := &mockPolicy{name: "test-policy", namespace: "test-ns"}
+	g.Expect(getPolicyName(policy)).To(Equal("test-ns/test-policy"))
+}
+
+func TestGetPolicyKind(t *testing.T) {
+	t.Parallel()
+	tests := []struct {
+		name     string
+		policy   policies.Policy
+		expected string
+	}{
+		{
+			name:     "with kind",
+			policy:   &mockPolicy{kind: "TestPolicy"},
+			expected: "TestPolicy",
+		},
+		{
+			name:     "without kind",
+			policy:   &mockPolicy{},
+			expected: "Policy",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			t.Parallel()
+			g := NewWithT(t)
+			g.Expect(getPolicyKind(test.policy)).To(Equal(test.expected))
+		})
+	}
+}
+
+func TestCompareNamespacedNames(t *testing.T) {
+	t.Parallel()
+	tests := []struct {
+		name     string
+		a, b     types.NamespacedName
+		expected bool
+	}{
+		{
+			name:     "same namespace, a name < b name",
+			a:        types.NamespacedName{Namespace: "ns", Name: "a"},
+			b:        types.NamespacedName{Namespace: "ns", Name: "b"},
+			expected: true,
+		},
+		{
+			name:     "same namespace, a name > b name",
+			a:        types.NamespacedName{Namespace: "ns", Name: "b"},
+			b:        types.NamespacedName{Namespace: "ns", Name: "a"},
+			expected: false,
+		},
+		{
+			name:     "a namespace < b namespace",
+			a:        types.NamespacedName{Namespace: "a", Name: "z"},
+			b:        types.NamespacedName{Namespace: "b", Name: "a"},
+			expected: true,
+		},
+		{
+			name:     "a namespace > b namespace",
+			a:        types.NamespacedName{Namespace: "b", Name: "a"},
+			b:        types.NamespacedName{Namespace: "a", Name: "z"},
+			expected: false,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			t.Parallel()
+			g := NewWithT(t)
+			g.Expect(compareNamespacedNames(test.a, test.b)).To(Equal(test.expected))
+		})
+	}
+}
+
+func TestSortGatewaysByCreationTime(t *testing.T) {
+	t.Parallel()
+	now := time.Now()
+	gw1Name := types.NamespacedName{Namespace: "test", Name: "gw1"}
+	gw2Name := types.NamespacedName{Namespace: "test", Name: "gw2"}
+
+	tests := []struct {
+		name     string
+		gateways map[types.NamespacedName]*Gateway
+		names    []types.NamespacedName
+		expected []types.NamespacedName
+	}{
+		{
+			name: "sort by creation time",
+			gateways: map[types.NamespacedName]*Gateway{
+				gw1Name: {Source: &v1.Gateway{
+					ObjectMeta: metav1.ObjectMeta{CreationTimestamp: metav1.NewTime(now.Add(time.Hour))},
+				}},
+				gw2Name: {Source: &v1.Gateway{
+					ObjectMeta: metav1.ObjectMeta{CreationTimestamp: metav1.NewTime(now)},
+				}},
+			},
+			names:    []types.NamespacedName{gw1Name, gw2Name},
+			expected: []types.NamespacedName{gw2Name, gw1Name},
+		},
+		{
+			name: "same creation time, sort by namespace/name",
+			gateways: map[types.NamespacedName]*Gateway{
+				gw2Name: {Source: &v1.Gateway{
+					ObjectMeta: metav1.ObjectMeta{CreationTimestamp: metav1.NewTime(now)},
+				}},
+				gw1Name: {Source: &v1.Gateway{
+					ObjectMeta: metav1.ObjectMeta{CreationTimestamp: metav1.NewTime(now)},
+				}},
+			},
+			names:    []types.NamespacedName{gw2Name, gw1Name},
+			expected: []types.NamespacedName{gw1Name, gw2Name},
+		},
+		{
+			name:     "nil gateway fallback to namespace/name",
+			gateways: map[types.NamespacedName]*Gateway{gw1Name: nil},
+			names:    []types.NamespacedName{gw2Name, gw1Name},
+			expected: []types.NamespacedName{gw1Name, gw2Name},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			t.Parallel()
+			g := NewWithT(t)
+			names := make([]types.NamespacedName, len(test.names))
+			copy(names, test.names)
+			sortGatewaysByCreationTime(names, test.gateways)
+			g.Expect(names).To(Equal(test.expected))
+		})
+	}
+}