diff --git a/docs/developer/testing.md b/docs/developer/testing.md index 205ae8f37b..7804930ff3 100644 --- a/docs/developer/testing.md +++ b/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. diff --git a/internal/mode/static/state/graph/policies_test.go b/internal/mode/static/state/graph/policies_test.go index 499b7e8386..340d3f10f0 100644 --- a/internal/mode/static/state/graph/policies_test.go +++ b/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) }) } } diff --git a/internal/mode/static/state/graph/reference_grant_test.go b/internal/mode/static/state/graph/reference_grant_test.go index 618cba181d..cb9f780a1d 100644 --- a/internal/mode/static/state/graph/reference_grant_test.go +++ b/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)) }) } }