Skip to content

cleanup: replace dial with newclient #8602

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 15 commits into from
Oct 21, 2025
Merged

cleanup: replace dial with newclient #8602

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
65f9bbe
Updated NewClient instead of Dial
vinothkumarr227 Sep 23, 2025
578671c
Merge remote-tracking branch 'origin/master' into update-docs-example…
vinothkumarr227 Sep 23, 2025
c83de4b
Fixed the test issues
vinothkumarr227 Sep 23, 2025
714bd88
small tweaks
vinothkumarr227 Sep 23, 2025
b060b0c
small tweaks
vinothkumarr227 Sep 23, 2025
98695b5
Fixed the review changes
vinothkumarr227 Sep 30, 2025
1a53f2b
small tweaks
vinothkumarr227 Sep 30, 2025
889cf0c
small tweaks
vinothkumarr227 Sep 30, 2025
66a1b3c
Fixed the review changes
vinothkumarr227 Oct 3, 2025
f1c77ee
small tweaks
vinothkumarr227 Oct 7, 2025
115b4f7
small tweaks
vinothkumarr227 Oct 13, 2025
f4de7e2
Fixed the test issues
vinothkumarr227 Oct 14, 2025
02196ff
Fixed the review changes
vinothkumarr227 Oct 16, 2025
40a938e
small tweaks
vinothkumarr227 Oct 17, 2025
2ced611
small tweaks
vinothkumarr227 Oct 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions clientconn_authority_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,9 +109,9 @@ func (s) TestClientConnAuthority(t *testing.T) {

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
cc, err := Dial(test.target, test.opts...)
cc, err := NewClient(test.target, test.opts...)
if err != nil {
t.Fatalf("Dial(%q) failed: %v", test.target, err)
t.Fatalf("NewClient(%q) failed: %v", test.target, err)
}
defer cc.Close()
if cc.authority != test.wantAuthority {
Expand All @@ -130,6 +130,6 @@ func (s) TestClientConnAuthority_CredsAndDialOptionMismatch(t *testing.T) {
opts := []DialOption{WithTransportCredentials(creds), WithAuthority("authority-override")}
if cc, err := NewClient("Non-Existent.Server:8000", opts...); err == nil {
cc.Close()
t.Fatal("grpc.NewClient() succeeded when expected to fail")
t.Fatal("NewClient() succeeded when expected to fail")
}
}
34 changes: 32 additions & 2 deletions clientconn_parsed_target_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"errors"
"fmt"
"net"
"strings"
"testing"
"time"

Expand Down Expand Up @@ -203,6 +204,34 @@ func (s) TestParsedTarget_Failure_WithoutCustomDialer(t *testing.T) {
}
}

func (s) TestParsedTarget_Failure_WithoutCustomDialer_WithNewClient(t *testing.T) {
targets := []string{
"",
"unix://a/b/c",
"unix://authority",
"unix-abstract://authority/a/b/c",
"unix-abstract://authority",
}

for _, target := range targets {
t.Run(target, func(t *testing.T) {
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a const defaultTestTimeout defined in this package that is also set to 10s. Please use that instead of hardcoding the value here. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

defer cancel()
cc, err := NewClient(target, WithTransportCredentials(insecure.NewCredentials()))
if err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

t.Fatalf("NewClient(%q) failed: %v", target, err)
}
defer cc.Close()
wantErrSubstr := "failed to exit idle mode"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Make this const.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if _, err := cc.NewStream(ctx, &StreamDesc{}, "/my.service.v1.MyService/UnaryCall"); err == nil {
t.Fatalf("NewStream() succeeded with target = %q, cc.parsedTarget = %+v, expected to fail", target, cc.parsedTarget)
} else if !strings.Contains(err.Error(), wantErrSubstr) {
t.Fatalf("NewStream() with target = %q returned unexpected error: got %v, want substring %q", target, err, wantErrSubstr)
}
})
}
}

func (s) TestParsedTarget_WithCustomDialer(t *testing.T) {
resetInitialResolverState()
defScheme := resolver.GetDefaultScheme()
Expand Down Expand Up @@ -273,11 +302,12 @@ func (s) TestParsedTarget_WithCustomDialer(t *testing.T) {
return nil, errors.New("dialer error")
}

cc, err := Dial(test.target, WithTransportCredentials(insecure.NewCredentials()), WithContextDialer(dialer))
cc, err := NewClient(test.target, WithTransportCredentials(insecure.NewCredentials()), withDefaultScheme(defScheme), WithContextDialer(dialer))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your feedback, I used withDefaultScheme only in the specific tests where it’s needed, as suggested by Arjan. For checking the default NewClient behavior with a custom dialer, I’m not overriding the scheme. If you’d like a separate test specifically for NewClient's default behavior, let me know and I can add it.

if err != nil {
t.Fatalf("Dial(%q) failed: %v", test.target, err)
t.Fatalf("grpc.NewClient(%q) failed: %v", test.target, err)
}
defer cc.Close()
cc.Connect()

select {
case addr := <-addrCh:
Expand Down
26 changes: 13 additions & 13 deletions default_dial_option_server_option_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,12 @@ import (
)

func (s) TestAddGlobalDialOptions(t *testing.T) {
// Ensure the Dial fails without credentials
if _, err := Dial("fake"); err == nil {
t.Fatalf("Dialing without a credential did not fail")
// Ensure the NewClient fails without credentials
if _, err := NewClient("fake"); err == nil {
t.Fatalf("NewClient without a credential did not fail")
} else {
if !strings.Contains(err.Error(), "no transport security set") {
t.Fatalf("Dialing failed with unexpected error: %v", err)
t.Fatalf("NewClient failed with unexpected error: %v", err)
}
}

Expand All @@ -48,9 +48,9 @@ func (s) TestAddGlobalDialOptions(t *testing.T) {
}
}

// Ensure the Dial passes with the extra dial options
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("fake"); err != nil {
t.Fatalf("NewClient with insecure credential failed: %v", err)
} else {
cc.Close()
}
Expand All @@ -71,8 +71,8 @@ func (s) TestDisableGlobalOptions(t *testing.T) {
// due to the global dial options with credentials not being picked up due
// to global options being disabled.
noTSecStr := "no transport security set"
if _, err := Dial("fake", internal.DisableGlobalDialOptions.(func() DialOption)()); !strings.Contains(fmt.Sprint(err), noTSecStr) {
t.Fatalf("Dialing received unexpected error: %v, want error containing \"%v\"", err, noTSecStr)
if _, err := NewClient("fake", internal.DisableGlobalDialOptions.(func() DialOption)()); !strings.Contains(fmt.Sprint(err), noTSecStr) {
t.Fatalf("NewClient received unexpected error: %v, want error containing %q", err, noTSecStr)
}
}

Expand All @@ -95,11 +95,11 @@ func (s) TestGlobalPerTargetDialOption(t *testing.T) {
defer internal.ClearGlobalPerTargetDialOptions()
noTSecStr := "no transport security set"
if _, err := NewClient("dns:///fake"); !strings.Contains(fmt.Sprint(err), noTSecStr) {
t.Fatalf("Dialing received unexpected error: %v, want error containing \"%v\"", err, noTSecStr)
t.Fatalf("NewClient received unexpected error: %v, want error containing %q", err, noTSecStr)
}
cc, err := NewClient("passthrough:///nice")
if err != nil {
t.Fatalf("Dialing with insecure credentials failed: %v", err)
t.Fatalf("NewClient with insecure credentials failed: %v", err)
}
cc.Close()
}
Expand Down Expand Up @@ -135,9 +135,9 @@ func (s) TestJoinDialOption(t *testing.T) {
const maxRecvSize = 998765
const initialWindowSize = 100
jdo := newJoinDialOption(WithTransportCredentials(insecure.NewCredentials()), WithReadBufferSize(maxRecvSize), WithInitialWindowSize(initialWindowSize))
cc, err := Dial("fake", jdo)
cc, err := NewClient("fake", jdo)
if err != nil {
t.Fatalf("Dialing with insecure credentials failed: %v", err)
t.Fatalf("NewClient with insecure credentials failed: %v", err)
}
defer cc.Close()
if cc.dopts.copts.ReadBufferSize != maxRecvSize {
Expand Down
12 changes: 6 additions & 6 deletions resolver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,9 @@ func (s) TestResolverCaseSensitivity(t *testing.T) {
return nil, fmt.Errorf("not dialing with custom dialer")
}

cc, err := Dial(target, WithContextDialer(customDialer), WithTransportCredentials(insecure.NewCredentials()))
cc, err := NewClient(target, WithContextDialer(customDialer), WithTransportCredentials(insecure.NewCredentials()))
if err != nil {
t.Fatalf("Unexpected Dial(%q) error: %v", target, err)
t.Fatalf("Unexpected NewClient(%q) error: %v", target, err)
}
cc.Connect()
if got, want := <-addrCh, "localhost:1234"; got != want {
Expand All @@ -85,9 +85,9 @@ func (s) TestResolverCaseSensitivity(t *testing.T) {
// This results in "passthrough" being used with the address as the whole
// target.
target = "caseTest2:///localhost:1234"
cc, err = Dial(target, WithContextDialer(customDialer), WithResolvers(res), WithTransportCredentials(insecure.NewCredentials()))
cc, err = NewClient(target, WithContextDialer(customDialer), withDefaultScheme("passthrough"), WithResolvers(res), WithTransportCredentials(insecure.NewCredentials()))
if err != nil {
t.Fatalf("Unexpected Dial(%q) error: %v", target, err)
t.Fatalf("Unexpected NewClient(%q) error: %v", target, err)
}
cc.Connect()
if got, want := <-addrCh, target; got != want {
Expand Down Expand Up @@ -125,7 +125,7 @@ func (s) TestResolverAddressesToEndpoints(t *testing.T) {
WithResolvers(r),
WithDefaultServiceConfig(fmt.Sprintf(`{"loadBalancingConfig": [{"%s":{}}]}`, balancerName)))
if err != nil {
t.Fatalf("grpc.NewClient() failed: %v", err)
t.Fatalf("NewClient() failed: %v", err)
}
cc.Connect()
defer cc.Close()
Expand Down Expand Up @@ -176,7 +176,7 @@ func (s) TestResolverAddressesToEndpointsUsingNewAddresses(t *testing.T) {
WithResolvers(r),
WithDefaultServiceConfig(fmt.Sprintf(`{"loadBalancingConfig": [{"%s":{}}]}`, balancerName)))
if err != nil {
t.Fatalf("grpc.NewClient() failed: %v", err)
t.Fatalf("NewClient() failed: %v", err)
}
cc.Connect()
defer cc.Close()
Expand Down