Fix unit tests to avoid data race in pipelines (#2602)

Fix unit tests to avoid data race in pipelines

Problem: Users want to parallelize unit tests without having data race issues
Solution: Modify unit tests to be independent of variables, maps/structs to avoid data race when tests run in parallel.

Author: salonichf5

docs/developer/testing.md

@@ -36,6 +36,8 @@ the [Counterfeiter](https://github.com/maxbrunsfeld/counterfeiter) tool. Counter
 implementations of internal and public interfaces, allowing us to isolate and control dependencies during testing. It
 simplifies the process of mocking and stubbing, making our tests more robust and flexible.
 
+**Parallelize unit tests**: In general, all tests should be designed to run in parallel for faster execution and help uncover bugs. For standard Go tests, this requires adding `t.Parallel()` to every test and subtest. Ginkgo tests, on the other hand, automatically run in parallel without the need for additional configuration. If a component under test requires sequential execution, you can run tests sequentially by using an [ordered container](https://onsi.github.io/ginkgo/#ordered-containers) for Ginkgo tests or by omitting `t.Parallel()` from the go test or subtest. In such cases, it’s essential to include a comment explaining why parallel execution is not possible.
+
 By combining BDD style tests, unit tests, and mock generation, we aim to achieve a comprehensive and maintainable
 testing strategy. This approach enables us to ensure the correctness, reliability, and flexibility of our codebase while
 promoting efficient refactoring and continuous development.

internal/mode/static/state/graph/policies_test.go

@@ -25,60 +25,20 @@ var testNs = "test"
 func TestAttachPolicies(t *testing.T) {
 	t.Parallel()
 	policyGVK := schema.GroupVersionKind{Group: "Group", Version: "Version", Kind: "Policy"}
-
-	gwPolicyKey := createTestPolicyKey(policyGVK, "gw-policy")
-	gwPolicy := &Policy{
-		Valid:  true,
-		Source: &policiesfakes.FakePolicy{},
-		TargetRefs: []PolicyTargetRef{
-			{
-				Kind:   kinds.Gateway,
-				Group:  v1.GroupName,
-				Nsname: types.NamespacedName{Namespace: testNs, Name: "gateway"},
-			},
-			{
-				Kind:   kinds.Gateway,
-				Group:  v1.GroupName,
-				Nsname: types.NamespacedName{Namespace: testNs, Name: "gateway2"}, // ignored
-			},
-		},
-	}
-
-	routePolicyKey := createTestPolicyKey(policyGVK, "route-policy")
-	routePolicy := &Policy{
-		Valid:  true,
-		Source: &policiesfakes.FakePolicy{},
-		TargetRefs: []PolicyTargetRef{
-			{
-				Kind:   kinds.HTTPRoute,
-				Group:  v1.GroupName,
-				Nsname: types.NamespacedName{Namespace: testNs, Name: "hr-route"},
-			},
-			{
-				Kind:   kinds.HTTPRoute,
-				Group:  v1.GroupName,
-				Nsname: types.NamespacedName{Namespace: testNs, Name: "hr2-route"},
-			},
-		},
-	}
-
-	grpcRoutePolicyKey := createTestPolicyKey(policyGVK, "grpc-route-policy")
-	grpcRoutePolicy := &Policy{
-		Valid:  true,
-		Source: &policiesfakes.FakePolicy{},
-		TargetRefs: []PolicyTargetRef{
-			{
-				Kind:   kinds.GRPCRoute,
+	createPolicy := func(targetRefsNames []string, refKind v1.Kind) *Policy {
+		targetRefs := make([]PolicyTargetRef, 0, len(targetRefsNames))
+		for _, name := range targetRefsNames {
+			targetRefs = append(targetRefs, PolicyTargetRef{
+				Kind:   refKind,
 				Group:  v1.GroupName,
-				Nsname: types.NamespacedName{Namespace: testNs, Name: "grpc-route"},
-			},
-		},
-	}
-
-	ngfPolicies := map[PolicyKey]*Policy{
-		gwPolicyKey:        gwPolicy,
-		routePolicyKey:     routePolicy,
-		grpcRoutePolicyKey: grpcRoutePolicy,
+				Nsname: types.NamespacedName{Namespace: testNs, Name: name},
+			})
+		}
+		return &Policy{
+			Valid:      true,
+			Source:     &policiesfakes.FakePolicy{},
+			TargetRefs: targetRefs,
+		}
 	}
 
 	createRouteKey := func(name string, routeType RouteType) RouteKey {
@@ -88,77 +48,40 @@ func TestAttachPolicies(t *testing.T) {
 		}
 	}
 
-	newGraph := func() *Graph {
-		return &Graph{
-			Gateway: &Gateway{
-				Source: &v1.Gateway{
-					ObjectMeta: metav1.ObjectMeta{
-						Name:      "gateway",
-						Namespace: testNs,
-					},
+	createGateway := func(name string) *Gateway {
+		return &Gateway{
+			Source: &v1.Gateway{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      name,
+					Namespace: testNs,
 				},
-				Valid: true,
 			},
-			Routes: map[RouteKey]*L7Route{
-				createRouteKey("hr-route", RouteTypeHTTP): {
-					Source: &v1.HTTPRoute{
-						ObjectMeta: metav1.ObjectMeta{
-							Name:      "hr-route",
-							Namespace: testNs,
-						},
-					},
-					ParentRefs: []ParentRef{
-						{
-							Attachment: &ParentRefAttachmentStatus{
-								Attached: true,
-							},
-						},
-					},
-					Valid:      true,
-					Attachable: true,
-				},
-				createRouteKey("hr2-route", RouteTypeHTTP): {
-					Source: &v1.HTTPRoute{
-						ObjectMeta: metav1.ObjectMeta{
-							Name:      "hr2-route",
-							Namespace: testNs,
-						},
-					},
-					ParentRefs: []ParentRef{
-						{
-							Attachment: &ParentRefAttachmentStatus{
-								Attached: true,
-							},
-						},
+			Valid: true,
+		}
+	}
+
+	createRoutesForGraph := func(routes map[string]RouteType) map[RouteKey]*L7Route {
+		routesMap := make(map[RouteKey]*L7Route, len(routes))
+		for routeName, routeType := range routes {
+			routesMap[createRouteKey(routeName, routeType)] = &L7Route{
+				Source: &v1.HTTPRoute{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      routeName,
+						Namespace: testNs,
 					},
-					Valid:      true,
-					Attachable: true,
 				},
-				createRouteKey("grpc-route", RouteTypeGRPC): {
-					Source: &v1alpha2.GRPCRoute{
-						ObjectMeta: metav1.ObjectMeta{
-							Name:      "grpc-route",
-							Namespace: testNs,
-						},
-					},
-					ParentRefs: []ParentRef{
-						{
-							Attachment: &ParentRefAttachmentStatus{
-								Attached: true,
-							},
+				ParentRefs: []ParentRef{
+					{
+						Attachment: &ParentRefAttachmentStatus{
+							Attached: true,
 						},
 					},
-					Valid:      true,
-					Attachable: true,
 				},
-			},
-
-			NGFPolicies: ngfPolicies,
+				Valid:      true,
+				Attachable: true,
+			}
 		}
-	}
-
-	newModifiedGraph := func(mod func(g *Graph) *Graph) *Graph {
-		return mod(newGraph())
+		return routesMap
 	}
 
 	expectNoPolicyAttachment := func(g *WithT, graph *Graph) {
@@ -192,30 +115,63 @@ func TestAttachPolicies(t *testing.T) {
 	}
 
 	tests := []struct {
-		graph  *Graph
-		expect func(g *WithT, graph *Graph)
-		name   string
+		gateway     *Gateway
+		routes      map[RouteKey]*L7Route
+		ngfPolicies map[PolicyKey]*Policy
+		expect      func(g *WithT, graph *Graph)
+		name        string
 	}{
 		{
 			name: "nil Gateway",
-			graph: newModifiedGraph(func(g *Graph) *Graph {
-				g.Gateway = nil
-				return g
-			}),
+			routes: createRoutesForGraph(
+				map[string]RouteType{
+					"hr1-route":  RouteTypeHTTP,
+					"hr2-route":  RouteTypeHTTP,
+					"grpc-route": RouteTypeGRPC,
+				},
+			),
+			ngfPolicies: map[PolicyKey]*Policy{
+				createTestPolicyKey(policyGVK, "gw-policy"): createPolicy([]string{"gateway", "gateway1"}, kinds.Gateway),
+				createTestPolicyKey(policyGVK, "route-policy"): createPolicy(
+					[]string{"hr1-route", "hr2-route"},
+					kinds.HTTPRoute,
+				),
+				createTestPolicyKey(policyGVK, "grpc-route-policy"): createPolicy([]string{"grpc-route"}, kinds.GRPCRoute),
+			},
 			expect: expectNoPolicyAttachment,
 		},
 		{
-			name: "nil routes",
-			graph: newModifiedGraph(func(g *Graph) *Graph {
-				g.Routes = nil
-				return g
-			}),
+			name:    "nil routes",
+			gateway: createGateway("gateway"),
+			ngfPolicies: map[PolicyKey]*Policy{
+				createTestPolicyKey(policyGVK, "gw-policy1"): createPolicy([]string{"gateway", "gateway1"}, kinds.Gateway),
+				createTestPolicyKey(policyGVK, "route-policy1"): createPolicy(
+					[]string{"hr1-route", "hr2-route"},
+					kinds.HTTPRoute,
+				),
+				createTestPolicyKey(policyGVK, "grpc-route-policy1"): createPolicy([]string{"grpc-route"}, kinds.GRPCRoute),
+			},
 			expect: expectGatewayPolicyAttachment,
 		},
 		{
-			name:   "normal",
-			graph:  newGraph(),
-			expect: expectPolicyAttachment,
+			name: "normal",
+			routes: createRoutesForGraph(
+				map[string]RouteType{
+					"hr-1":   RouteTypeHTTP,
+					"hr-2":   RouteTypeHTTP,
+					"grpc-1": RouteTypeGRPC,
+				},
+			),
+			ngfPolicies: map[PolicyKey]*Policy{
+				createTestPolicyKey(policyGVK, "gw-policy2"): createPolicy([]string{"gateway2", "gateway3"}, kinds.Gateway),
+				createTestPolicyKey(policyGVK, "route-policy2"): createPolicy(
+					[]string{"hr-1", "hr-2"},
+					kinds.HTTPRoute,
+				),
+				createTestPolicyKey(policyGVK, "grpc-route-policy2"): createPolicy([]string{"grpc-1"}, kinds.GRPCRoute),
+			},
+			gateway: createGateway("gateway2"),
+			expect:  expectPolicyAttachment,
 		},
 	}
 
@@ -224,8 +180,14 @@ func TestAttachPolicies(t *testing.T) {
 			t.Parallel()
 			g := NewWithT(t)
 
-			test.graph.attachPolicies("nginx-gateway")
-			test.expect(g, test.graph)
+			graph := &Graph{
+				Gateway:     test.gateway,
+				Routes:      test.routes,
+				NGFPolicies: test.ngfPolicies,
+			}
+
+			graph.attachPolicies("nginx-gateway")
+			test.expect(g, graph)
 		})
 	}
 }

internal/mode/static/state/graph/reference_grant_test.go

@@ -345,63 +345,57 @@ func TestRefAllowedFrom(t *testing.T) {
 		},
 	}
 
-	resolver := newReferenceGrantResolver(refGrants)
-	refAllowedFromGRPCRoute := resolver.refAllowedFrom(fromGRPCRoute(grNs))
-	refAllowedFromHTTPRoute := resolver.refAllowedFrom(fromHTTPRoute(hrNs))
-	refAllowedFromTLSRoute := resolver.refAllowedFrom(fromTLSRoute(trNs))
-	refAllowedFromGateway := resolver.refAllowedFrom(fromGateway(gwNs))
-
 	tests := []struct {
 		name           string
-		refAllowedFrom func(resource toResource) bool
+		refAllowedFrom fromResource
 		toResource     toResource
 		expAllowed     bool
 	}{
 		{
 			name:           "ref allowed from gateway to secret",
-			refAllowedFrom: refAllowedFromGateway,
+			refAllowedFrom: fromGateway(gwNs),
 			toResource:     toSecret(allowedGatewayNsName),
 			expAllowed:     true,
 		},
 		{
 			name:           "ref not allowed from gateway to secret",
-			refAllowedFrom: refAllowedFromGateway,
+			refAllowedFrom: fromGateway(gwNs),
 			toResource:     toSecret(notAllowedNsName),
 			expAllowed:     false,
 		},
 		{
 			name:           "ref allowed from httproute to service",
-			refAllowedFrom: refAllowedFromHTTPRoute,
+			refAllowedFrom: fromHTTPRoute(hrNs),
 			toResource:     toService(allowedHTTPRouteNsName),
 			expAllowed:     true,
 		},
 		{
 			name:           "ref not allowed from httproute to service",
-			refAllowedFrom: refAllowedFromHTTPRoute,
+			refAllowedFrom: fromHTTPRoute(hrNs),
 			toResource:     toService(notAllowedNsName),
 			expAllowed:     false,
 		},
 		{
 			name:           "ref allowed from grpcroute to service",
-			refAllowedFrom: refAllowedFromGRPCRoute,
+			refAllowedFrom: fromGRPCRoute(grNs),
 			toResource:     toService(allowedGRPCRouteNsName),
 			expAllowed:     true,
 		},
 		{
 			name:           "ref not allowed from grpcroute to service",
-			refAllowedFrom: refAllowedFromGRPCRoute,
+			refAllowedFrom: fromGRPCRoute(grNs),
 			toResource:     toService(notAllowedNsName),
 			expAllowed:     false,
 		},
 		{
 			name:           "ref allowed from tlsroute to service",
-			refAllowedFrom: refAllowedFromTLSRoute,
+			refAllowedFrom: fromTLSRoute(trNs),
 			toResource:     toService(allowedTLSRouteNsName),
 			expAllowed:     true,
 		},
 		{
 			name:           "ref not allowed from tlsroute to service",
-			refAllowedFrom: refAllowedFromTLSRoute,
+			refAllowedFrom: fromTLSRoute(trNs),
 			toResource:     toService(notAllowedNsName),
 			expAllowed:     false,
 		},
@@ -411,8 +405,11 @@ func TestRefAllowedFrom(t *testing.T) {
 		t.Run(test.name, func(t *testing.T) {
 			t.Parallel()
 
+			resolver := newReferenceGrantResolver(refGrants)
+			refAllowed := resolver.refAllowedFrom(test.refAllowedFrom)
+
 			g := NewWithT(t)
-			g.Expect(test.refAllowedFrom(test.toResource)).To(Equal(test.expAllowed))
+			g.Expect(refAllowed(test.toResource)).To(Equal(test.expAllowed))
 		})
 	}
 }