cleanup: replace dial with newclient #7975

janardhanvissa · 2024-12-31T09:11:25Z

Partially address: #7049

RELEASE NOTES: None

test/xds/xds_client_federation_test.go

test/local_creds_test.go

test/clientconn_state_transition_test.go

test/xds/xds_client_federation_test.go

test/local_creds_test.go

test/clientconn_state_transition_test.go

…tification channel

codecov · 2024-12-31T12:46:15Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.95%. Comparing base (724f450) to head (ab5f146).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7975      +/-   ##
==========================================
- Coverage   82.05%   81.95%   -0.10%     
==========================================
  Files         381      381              
  Lines       38539    38539              
==========================================
- Hits        31622    31585      -37     
- Misses       5602     5627      +25     
- Partials     1315     1327      +12

see 25 files with indirect coverage changes

test/local_creds_test.go

test/xds/xds_client_federation_test.go

arjan-bal · 2024-12-31T13:01:16Z

test/xds/xds_client_federation_test.go

 	if err == nil || !strings.Contains(err.Error(), wantErr) {
-		t.Fatalf("grpc.Dial(%q) returned %v, want: %s", target, err, wantErr)
+		t.Fatalf("Expected error containing %q, got: %v", wantErr, err)


The log message should contain the function that was called.

Suggested change

t.Fatalf("Expected error containing %q, got: %v", wantErr, err)

t.Fatalf("EmptyCall(_, _) = _, %v; want _, %q", wantErr, err)

The order of wantErr and err is inverted. err should come before wantErr.

arjan-bal · 2025-01-02T04:33:39Z

clientconn.go

+	// validation for the google-c2p scheme.
+	if cc.parsedTarget.URL.Scheme == "google-c2p" && cc.parsedTarget.URL.Host != "" {
+		return nil, fmt.Errorf("google-c2p URI scheme does not support authorities")
+	}
+


This is not how you get the test to pass :b. The validation is done by the resolver and not the gRPC channel. The gRPC channel is generic and doesn't care about the google-c2p scheme.

arjan-bal · 2025-01-02T04:36:45Z

xds/googledirectpath/googlec2p_test.go

@@ -315,15 +315,15 @@ func (s) TestBuildXDS(t *testing.T) {
 func (s) TestBuildFailsWhenCalledWithAuthority(t *testing.T) {
 	useCleanUniverseDomain(t)
 	uri := "google-c2p://an-authority/resource"
-	cc, err := grpc.Dial(uri, grpc.WithTransportCredentials(insecure.NewCredentials()))
+	cc, err := grpc.NewClient(uri, grpc.WithTransportCredentials(insecure.NewCredentials()))
 	defer func() {
 		if cc != nil {
 			cc.Close()
 		}
 	}()
 	wantErr := "google-c2p URI scheme does not support authorities"
 	if err == nil || !strings.Contains(err.Error(), wantErr) {


NewClient is lazy, so the the resolver will not be built until and rpc is made of cc.Connect is called. You would need to create a test service client, call client.EmptyCall and check for the error string in the returned error. This is very similar to the fix made in the federation test.

arjan-bal · 2025-01-02T04:39:40Z

test/end2end_test.go

+	lis, err := net.Listen("tcp", "localhost:0")
+	if err != nil {
+		t.Fatalf("Failed to listen: %v", err)
+	}
+	defer lis.Close()


This seems unnecessary. You can just pass the target URI as "passthrough:///", no need to create a listener and get it's address.

arjan-bal · 2025-01-02T04:41:12Z

test/balancer_switching_test.go

 	}
 	defer cc.Close()
-
+	cc.Connect()


You can avoid using cc.Connect here by replacing the r.UpdateState below with r.InitialState.

arjan-bal · 2025-01-02T04:42:01Z

test/balancer_switching_test.go

 	}
 	defer cc.Close()
-
+	cc.Connect()


You can avoid using cc.Connect here by replacing the r.UpdateState below with r.InitialState.

The resolver is getting as nil after updating to r.InitialState before creating a client conn.

arjan-bal · 2025-01-02T04:42:20Z

test/balancer_switching_test.go

 	}
 	defer cc.Close()
-
+	cc.Connect()


You can avoid using cc.Connect here by replacing the r.UpdateState below with r.InitialState.

@arjan-bal If we do like replacing r.UpdateState to r.InitialState, first we should ensure that the manual.Resolver.CC must be set grpc.NewClient before calling parseServiceConfig. So, we cannot update to r.InitialState before creating a NewClient.

You can use internal.ParseConfig to avoid depending on manual.Resolver.CC.

arjan-bal · 2025-01-02T04:42:40Z

test/balancer_switching_test.go

 	}
 	defer cc.Close()
-
+	cc.Connect()


You can avoid using cc.Connect here by replacing the r.UpdateState below with r.InitialState. Please check other places where the same change can be made.

…er cc

replace dial with newclient

7d2a53b