cleanup: replace dial with newclient #8602

vinothkumarr227 · 2025-09-23T13:31:25Z

RELEASE NOTES: None

…s-tests-newclient

codecov · 2025-09-23T13:35:17Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.08%. Comparing base (7235bb7) to head (66a1b3c).
⚠️ Report is 17 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8602      +/-   ##
==========================================
+ Coverage   81.86%   82.08%   +0.21%     
==========================================
  Files         415      415              
  Lines       40694    40711      +17     
==========================================
+ Hits        33316    33416     +100     
+ Misses       5993     5907      -86     
- Partials     1385     1388       +3

see 31 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

arjan-bal · 2025-09-25T08:28:10Z

clientconn_parsed_target_test.go

 func (s) TestParsedTarget_Success_WithoutCustomDialer(t *testing.T) {
 	tests := []struct {
 		target             string
-		wantDialParse      resolver.Target


Please keep this test param and related assertions. We need to keep tests that test Dial specifically, since Dial will be supported for gRPC Go 1.x. We only need to replace Dial in tests which are not intended to verify the behaviour of Dial.

arjan-bal · 2025-09-25T08:38:57Z

clientconn_parsed_target_test.go

-		"",
-		"unix://a/b/c",
-		"unix://authority",
-		"unix-abstract://authority/a/b/c",
-		"unix-abstract://authority",


We should not remove these test cases. Since NewClient doesn't connect like Dial, we need to do the following to get an error instead:

Call cc.Connect()

Wait for TRANSIENT_FAILURE state using AwaitState. If using this function causes a circular dependency, copy it over to this file, adding a comment that mentioning the reason for the duplication.

If context times out before TF state, fail the test.

I am using Idle because these malformed targets result in zero resolved addresses. The client never attempts a connection, so it stays IDLE and cannot reach TRANSIENT_FAILURE

arjan-bal · 2025-09-25T08:41:29Z

clientconn_parsed_target_test.go

 		},
 		{
-			target:            "",
+			target:            "passthrough:///",


Instead of changing all the test cases, you can instead use the private withDefaultScheme dial option to change the default scheme from dns to passthrough.

grpc-go/dialoptions.go

Lines 733 to 739 in 8ae3c07

// withDefaultScheme is used to allow Dial to use "passthrough" as the default

// name resolver, while NewClient uses "dns" otherwise.

func withDefaultScheme(s string) DialOption {

return newFuncDialOption(func(o *dialOptions) {

o.defaultScheme = s

})

}

arjan-bal · 2025-09-25T08:43:26Z

clientconn_test.go

 	r := manual.NewBuilderWithScheme("whatever")
 	r.InitialState(resolver.State{Addresses: []resolver.Address{lisAddr}})
-	client, err := Dial(r.Scheme()+":///test.server", WithTransportCredentials(insecure.NewCredentials()), WithResolvers(r), WithTimeout(5*time.Second))
+	client, err := NewClient(r.Scheme()+":///test.server", WithTransportCredentials(insecure.NewCredentials()), WithResolvers(r), WithTimeout(5*time.Second))


Please revert this change. The WithTimeout dial option is a no-op with NewClient, see godoc. We should continue to test it with Dial.

arjan-bal · 2025-09-25T08:46:32Z

default_dial_option_server_option_test.go

-	if cc, err := Dial("fake"); err != nil {
-		t.Fatalf("Dialing with insecure credential failed: %v", err)
+	// Ensure the NewClient passes with the extra dial options
+	if cc, err := NewClient("dns:///fake"); err != nil {


Why do we need to set the dns scheme explicitly here? Doesn't NewClient add it automatically?

Thanks for pointing this out. I have reverted it

arjan-bal · 2025-09-25T08:48:17Z

resolver_test.go

 	// target.
-	target = "caseTest2:///localhost:1234"
-	cc, err = Dial(target, WithContextDialer(customDialer), WithResolvers(res), WithTransportCredentials(insecure.NewCredentials()))
+	target = "passthrough:///caseTest2:///localhost:1234"


Why are we adding 2 schemes here? Due to passthrough, the wrapped resolver created above wouldn't even be called.

No need a two schema here, I reverted it

eshitachandwani · 2025-10-02T09:09:05Z

clientconn_parsed_target_test.go

+			ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
+			defer cancel()
+			cc, err := NewClient(target, WithTransportCredentials(insecure.NewCredentials()))
+			if err != nil {


We are changing the check condition here , is it intentional? If it is , the error message isn't correct. We are checking err!=nil which means NewClient failed and the error message says NewClient succeeded which is not right.

eshitachandwani · 2025-10-02T19:39:41Z

clientconn_parsed_target_test.go

 			}

-			cc, err := Dial(test.target, WithTransportCredentials(insecure.NewCredentials()), WithContextDialer(dialer))
+			cc, err := NewClient(test.target, WithTransportCredentials(insecure.NewCredentials()), withDefaultScheme(defScheme), WithContextDialer(dialer))


I don't think we should override the default scheme to passthrough with new client because we want to check the behavior of parsed strings with defualt NewClient function with custom dialer. And the behavior might be different , because dns resolver also does its own parsing , so the results might be different. Correct me if I have understood incorrectly. But I think we should have a separate test for NewClient's default parsed address behavior with custom dialer

eshitachandwani · 2025-10-02T19:40:41Z

clientconn_test.go

 	if err != nil {
 		t.Fatalf("Dial failed. Err: %v", err)
 	}
+	client.Connect()


Why did we add this here? We did not change from Dial to NewClient in the test and Dial will connect automatically?

vinothkumarr227 added 5 commits September 23, 2025 02:14

Updated NewClient instead of Dial

65f9bbe

Merge remote-tracking branch 'origin/master' into update-docs-example…

578671c

…s-tests-newclient

Fixed the test issues

c83de4b

small tweaks

714bd88

small tweaks

b060b0c

arjan-bal reviewed Sep 25, 2025

View reviewed changes

arjan-bal assigned vinothkumarr227 Sep 25, 2025

arjan-bal added the Type: Internal Cleanup Refactors, etc label Sep 25, 2025

arjan-bal added this to the 1.77 Release milestone Sep 25, 2025

vinothkumarr227 added 3 commits September 30, 2025 12:41

Fixed the review changes

98695b5

small tweaks

1a53f2b

small tweaks

889cf0c

eshitachandwani reviewed Oct 2, 2025

View reviewed changes

Fixed the review changes

66a1b3c

	// withDefaultScheme is used to allow Dial to use "passthrough" as the default
	// name resolver, while NewClient uses "dns" otherwise.
	func withDefaultScheme(s string) DialOption {
	return newFuncDialOption(func(o *dialOptions) {
	o.defaultScheme = s
	})
	}

cleanup: replace dial with newclient #8602

Are you sure you want to change the base?

cleanup: replace dial with newclient #8602

Uh oh!

Conversation

vinothkumarr227 commented Sep 23, 2025

Uh oh!

codecov bot commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Sep 23, 2025 •

edited

Loading