xds/clusterimpl: Convert existing unit tests to e2e style (3/N) #8616
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8616 +/- ##
==========================================
+ Coverage 82.05% 82.11% +0.05%
==========================================
Files 415 415
Lines 40694 40697 +3
==========================================
+ Hits 33393 33418 +25
+ Misses 5910 5893 -17
+ Partials 1391 1386 -5 🚀 New features to boost your workflow:
|
| Listeners: []*v3listenerpb.Listener{e2e.DefaultClientListener(serviceName, routeName)}, | ||
| Routes: []*v3routepb.RouteConfiguration{e2e.DefaultRouteConfig(routeName, serviceName, clusterName)}, | ||
| Clusters: []*v3clusterpb.Cluster{e2e.DefaultCluster(clusterName, endpointsName, e2e.SecurityLevelNone)}, | ||
| Endpoints: []*v3endpointpb.ClusterLoadAssignment{e2e.DefaultEndpoint(endpointsName, "localhost", []uint32{testutils.ParsePort(t, server.Address)})}, |
There was a problem hiding this comment.
Is it possible to use e2e.DefaultClientResources instead?
| updatedChildPolicy := "" | ||
| var mu sync.Mutex |
There was a problem hiding this comment.
Please consider using atomic.Pointer[string] instead. You can see lot of examples of atomic.Pointer used in our codebase.
| select { | ||
| case lbCfgCh <- ccs.BalancerConfig: | ||
| default: | ||
| } |
There was a problem hiding this comment.
Please avoid non-blocking writes to channels in test. This can lead to non-determinism which can lead to flakes. Instead of the default case, use a case where you block on <-ctx.Done()
| t.Fatalf("Failed to update xDS resources: %v", err) | ||
| } | ||
|
|
||
| // Register stub pickfirst LB policy so that we can catch config changes. |
There was a problem hiding this comment.
It would make sense to move this definition of the stub policy to be after creation of the gRPC channel and the first RPC.
| t.Fatalf("client.EmptyCall() failed: %v", err) | ||
| } | ||
|
|
||
| // Now update the cluster to use "pick_first" as the endpoint picking policy. |
There was a problem hiding this comment.
It would be worth documenting somewhere that the e2e.DefaultCluster configures weighted target as the load balancing policy, which results in round_robin being used.
There was a problem hiding this comment.
But round_robin also delegates to pick_first. So, wouldn't your wrapped pick_first be used even before this update?
| // Start a server backend exposing the test service. | ||
| server := stubserver.StartTestService(t, nil) | ||
| defer server.Stop() |
There was a problem hiding this comment.
You don't need to spin up a backend for this test since you don't expect the RPC to ever succeed.
There was a problem hiding this comment.
Hmm ... maybe its fine to spin up the backend and include it in the endpoint resource like you do. Since you actually perform an RPC and verify that it does not succeed.
| const ( | ||
| serviceName = "test-child-policy" | ||
| routeName = "route-" + serviceName | ||
| clusterName = "cluster-" + serviceName | ||
| endpointsName = "endpoints-" + serviceName | ||
| ) | ||
|
|
||
| // Configure the xDS management server with default resources. Override the | ||
| // default cluster to include pickfirst as an LbPolicy. | ||
| resources := e2e.UpdateOptions{ | ||
| NodeID: nodeID, | ||
| Listeners: []*v3listenerpb.Listener{e2e.DefaultClientListener(serviceName, routeName)}, | ||
| Routes: []*v3routepb.RouteConfiguration{e2e.DefaultRouteConfig(routeName, serviceName, clusterName)}, | ||
| Clusters: []*v3clusterpb.Cluster{e2e.DefaultCluster(clusterName, endpointsName, e2e.SecurityLevelNone)}, | ||
| Endpoints: []*v3endpointpb.ClusterLoadAssignment{e2e.DefaultEndpoint(endpointsName, "localhost", []uint32{testutils.ParsePort(t, server.Address)})}, | ||
| } |
There was a problem hiding this comment.
Same here. Can we use e2e.DefaultClientResources and save some lines of code.
|
|
||
| client := testgrpc.NewTestServiceClient(cc) | ||
| if _, err := client.EmptyCall(ctx, &testpb.Empty{}); err == nil { | ||
| t.Fatalf("Unexpected error, got <nil> want %v", err) |
There was a problem hiding this comment.
This error message is going to very confusing because err is also nil. So, something like
"EmptyCall RPC succeeded when expected to fail" would be more meaningful.
Partially addresses: #6113
Added
TestChildPolicyChangeOnConfigUpdateandTestFailedToParseChildPolicyConfigend2end Tests in xds/clusterimpl/tests Mirroring Existing Unit Test and delete them.RELEASE NOTES: None