Skip to content
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

Jump to bottom

test: Add unit test for channel state waiting for first resolver update #7768

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

test: Add unit test for channel state waiting for first resolver update #7768

Changes from 7 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
49329cf
Add unit test for channel state waiting for first resolver update
janardhankrishna-sai Oct 22, 2024
f314b62
removing space
janardhankrishna-sai Oct 22, 2024
6ad9713
Moving TODO comments
janardhankrishna-sai Oct 23, 2024
7d8ed8f
Used t.Skip instead with a message and used channel and goroutine
janardhankrishna-sai Oct 26, 2024
b74dcd2
removing comment
janardhankrishna-sai Oct 26, 2024
2061c46
Moving t.Skip to up and updated AwaitState, shortCtx to ctx
janardhankrishna-sai Oct 29, 2024
f3c6931
Moving t.Skip to up and updated AwaitState, shortCtx to ctx
janardhankrishna-sai Oct 29, 2024
7829dd6
Moving Idle before cc.Connect() and connectivity.Connecting after cc.…
janardhankrishna-sai Oct 30, 2024
69c8a63
Adding time.Sleep
janardhankrishna-sai Oct 30, 2024
3b33b19
removed the updateState and Ready after Connecting and updated message
janardhankrishna-sai Nov 5, 2024
a6d2b49
adding new test case with rpc call and adding awaitnostatechannel con…
janardhankrishna-sai Nov 5, 2024
9a85a50
updating the comment
janardhankrishna-sai Nov 6, 2024
bc03ad2
updating the comment
janardhankrishna-sai Nov 6, 2024
cd4f22e
Updating skip msg and moving Idle connectivity before context
janardhankrishna-sai Nov 12, 2024
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
36 changes: 36 additions & 0 deletions test/clientconn_state_transition_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import (
"google.golang.org/grpc/internal/balancer/stub"
"google.golang.org/grpc/internal/envconfig"
"google.golang.org/grpc/internal/grpcsync"
"google.golang.org/grpc/internal/stubserver"
"google.golang.org/grpc/internal/testutils"
"google.golang.org/grpc/resolver"
"google.golang.org/grpc/resolver/manual"
Expand Down Expand Up @@ -616,3 +617,38 @@ func (s) TestConnectivityStateSubscriber(t *testing.T) {
}
}
}

// TestChannelStateWaitingForFirstResolverUpdate verifies the initial
// state of the channel when a manual name resolver doesn't provide any updates.
func (s) TestChannelStateWaitingForFirstResolverUpdate(t *testing.T) {
if testing.Short() {
Copy link
Member

Choose a reason for hiding this comment

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

What is this and why?

I think we have a bug, which this test doesn't actually appear to be testing, but which is described in the issue that asked for this test. IF there is a bug, we need to always skip the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we have a bug that's why we added this test to skip.

Copy link
Contributor

Choose a reason for hiding this comment

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

@janardhanvissa you can skip the test unconditionally, i.e. remote the if testing.Short(). Also add more details in the skip message.

t.Skip("The channel remains in IDLE until the LB policy updates the state to CONNECTING. This is a bug and the channel should transition to CONNECTING as soon as Connect() is called.")

t.Skip("Skipping test in short mode.")
}

backend := stubserver.StartTestService(t, nil)
defer backend.Stop()
dfawley marked this conversation as resolved.
Show resolved Hide resolved

mr := manual.NewBuilderWithScheme("e2e-test")
defer mr.Close()

cc, err := grpc.NewClient(mr.Scheme()+":///", grpc.WithResolvers(mr), grpc.WithTransportCredentials(insecure.NewCredentials()))
if err != nil {
t.Fatalf("Failed to create new client: %v", err)
}
defer cc.Close()

ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
defer cancel()

cc.Connect()

testutils.AwaitState(ctx, t, cc, connectivity.Idle)
Copy link
Member

Choose a reason for hiding this comment

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

This has to be before Connect or else it will race..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

connectivity.Idle moved up before cc.Connect()

Copy link
Member

Choose a reason for hiding this comment

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

This should go above the context creation and just be an assertion that State() == IDLE. No need to Await since it should always be in this state upon creation.


arjan-bal marked this conversation as resolved.
Show resolved Hide resolved
mr.UpdateState(resolver.State{
Addresses: []resolver.Address{{Addr: backend.Address}},
})

testutils.AwaitState(ctx, t, cc, connectivity.Connecting)
arjan-bal marked this conversation as resolved.
Show resolved Hide resolved
arjan-bal marked this conversation as resolved.
Show resolved Hide resolved

arjan-bal marked this conversation as resolved.
Show resolved Hide resolved
testutils.AwaitState(ctx, t, cc, connectivity.Ready)
}