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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
+32 −25
Open

cleanup: replace dial with newclient #8602

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 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
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
4 changes: 2 additions & 2 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("grpc.NewClient(%q) failed: %v", test.target, err)
}
defer cc.Close()
if cc.authority != test.wantAuthority {
Expand Down
17 changes: 12 additions & 5 deletions clientconn_parsed_target_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"time"

"github.com/google/go-cmp/cmp"
"google.golang.org/grpc/connectivity"
"google.golang.org/grpc/credentials/insecure"
"google.golang.org/grpc/internal"
"google.golang.org/grpc/internal/testutils"
Expand Down Expand Up @@ -195,10 +196,15 @@ func (s) TestParsedTarget_Failure_WithoutCustomDialer(t *testing.T) {

for _, target := range targets {
t.Run(target, func(t *testing.T) {
if cc, err := Dial(target, WithTransportCredentials(insecure.NewCredentials())); err == nil {
defer cc.Close()
t.Fatalf("Dial(%q) succeeded cc.parsedTarget = %+v, expected to fail", target, cc.parsedTarget)
ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
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("grpc.NewClient(%q) failed: %v, expected to succeed", target, err)
}
defer cc.Close()
cc.Connect()
testutils.AwaitState(ctx, t, cc, connectivity.Idle)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is pretty much a no-op since the channel starts in IDLE. A better check would be to assert testutils.AwaitState with a short context. We should wait for a reply on my other comment though, it may change the assertion.

})
}
}
Expand Down Expand Up @@ -273,11 +279,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("grpc.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("grpc.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("grpc.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("grpc.NewClient received unexpected error: %v, want error containing \"%v\"", 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("grpc.NewClient received unexpected error: %v, want error containing \"%v\"", err, noTSecStr)
}
cc, err := NewClient("passthrough:///nice")
if err != nil {
t.Fatalf("Dialing with insecure credentials failed: %v", err)
t.Fatalf("grpc.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("grpc.NewClient with insecure credentials failed: %v", err)
}
defer cc.Close()
if cc.dopts.copts.ReadBufferSize != maxRecvSize {
Expand Down
10 changes: 5 additions & 5 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 grpc.NewClient(%q) error: %v", target, err)
}
cc.Connect()
if got, want := <-addrCh, "localhost:1234"; got != want {
Expand All @@ -85,12 +85,12 @@ 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 grpc.NewClient(%q) error: %v", target, err)
}
cc.Connect()
if got, want := <-addrCh, target; got != want {
if got, want := <-addrCh, "caseTest2:///localhost:1234"; got != want {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we revert this change? It seems to be a no-op since the target variable has the same value.

cc.Close()
t.Fatalf("Dialer got address %q; wanted %q", got, want)
}
Expand Down