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

janardhanvissa · 2024-10-22T14:35:52Z

Addresses: #7686

RELEASE NOTES: None

codecov · 2024-10-22T14:41:33Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.94%. Comparing base (4bb0170) to head (cd4f22e).
Report is 34 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7768      +/-   ##
==========================================
+ Coverage   80.25%   81.94%   +1.69%     
==========================================
  Files         367      373       +6     
  Lines       36622    37880    +1258     
==========================================
+ Hits        29391    31042    +1651     
+ Misses       6043     5545     -498     
- Partials     1188     1293     +105

see 70 files with indirect coverage changes

test/clientconn_state_transition_test.go

arjan-bal · 2024-10-22T17:52:12Z

Please merge the latest master. There was a fix for the failing vet test added recently.

arjan-bal

LGTM, adding a second reviewer.

dfawley · 2024-10-25T23:07:10Z

test/clientconn_state_transition_test.go

+
+	cc.Connect()
+
+	// TODO: Change this assertion to testutils.AwaitState(shortCtx, t, cc,


Please instead assert the proper behavior and use t.Skip instead with a message that refers to the issue.

Used t.Skip with a message.

test/clientconn_state_transition_test.go

dfawley · 2024-10-25T23:09:23Z

test/clientconn_state_transition_test.go

+		}
+	}()
+
+	// TODO: Change this assertion to testutils.AwaitState(shortCtx, t, cc,


I'm not sure what this part is attempting to test. It should already be CONNECTING per the above assertion.

test/clientconn_state_transition_test.go

janardhanvissa · 2024-10-26T15:59:40Z

Addressed above mentioned comments by using t.Skip instead with a message and channel and goroutine

dfawley · 2024-10-29T21:10:57Z

test/clientconn_state_transition_test.go

+
+	done := make(chan struct{})
+	go func() {
+		_, err := testgrpc.NewTestServiceClient(cc).EmptyCall(shortCtx, &testgrpc.Empty{})


This shouldn't use the shortCtx - we only use that for operations we expect to not happen. We want this one to succeed.

Actually...literally everywhere you use shortCtx is wrong. Use ctx instead.

But, we should use shortCtx in one check: before we produce the proper address from the resolver, and after we've asserted that we entered CONNECTING, we should do testutils.AwaitNoStateChange(shortCtx, t, cc, connectivity.Connecting)

used ctx instead of shortCtx

I don't see the AwaitNoStateChange.

test/clientconn_state_transition_test.go

dfawley · 2024-10-29T21:16:21Z

test/clientconn_state_transition_test.go

+	testutils.AwaitState(shortCtx, t, cc, connectivity.Ready)
+	t.Skipf("After entering READY state: current state = %v\n", cc.GetState())
+
+	mr.UpdateState(resolver.State{Addresses: nil})


What's this about?

dfawley · 2024-10-29T22:48:31Z

test/clientconn_state_transition_test.go

+// 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() {


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.

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

@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.")

dfawley · 2024-10-29T22:48:47Z

test/clientconn_state_transition_test.go

+
+	cc.Connect()
+
+	testutils.AwaitState(ctx, t, cc, connectivity.Idle)


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

connectivity.Idle moved up before cc.Connect()

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.

test/clientconn_state_transition_test.go

dfawley · 2024-10-29T22:49:56Z

test/clientconn_state_transition_test.go

+
+	done := make(chan struct{})
+	go func() {
+		_, err := testgrpc.NewTestServiceClient(cc).EmptyCall(shortCtx, &testgrpc.Empty{})


I don't see the AwaitNoStateChange.

…Connect()

purnesh42H · 2024-10-30T05:33:26Z

@janardhanvissa let's re-review it in BLR once since there were many changes after first commit. Closing it for now

test/clientconn_state_transition_test.go

arjan-bal · 2024-11-05T06:04:50Z

test/clientconn_state_transition_test.go

+// 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() {


@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.")

Re-reviewing changes requested by the second reviewer.

arjan-bal

Can you add another test case similar to the existing one, but that makes an RPC instead of calling cc.Connect()?

arjan-bal · 2024-11-05T07:54:09Z

test/clientconn_state_transition_test.go

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


nit: Please remove blank line at the beginning of the method.

removed blank line

test/clientconn_state_transition_test.go

…dition

janardhanvissa · 2024-11-05T11:13:26Z

Added a new test with RPC call instead of cc.Connect()

arjan-bal

Couple of minor comments, otherwise LGTM!

arjan-bal · 2024-11-06T07:59:56Z

test/clientconn_state_transition_test.go

+		}
+	}()
+
+	// The channel should transition to CONNECTING automatically when Connect()


nit: This comment needs to say "when an RPC is made" instead of "when Connect() is called".

done with the change

test/clientconn_state_transition_test.go

dfawley

Thanks! Looks better, just a couple minor things.

dfawley · 2024-11-11T22:59:57Z

test/clientconn_state_transition_test.go

+// TestChannelStateWaitingForFirstResolverUpdate verifies the initial
+// state of the channel when a manual name resolver doesn't provide any updates.
+func (s) TestChannelStateWaitingForFirstResolverUpdate(t *testing.T) {
+	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 soon as Connect() is called. See issue #7686.")


as soon as Connect() is called soon as Connect() is called -> as soon as Connect() is called

Updated the comment.

dfawley · 2024-11-11T23:00:41Z

test/clientconn_state_transition_test.go

+}
+
+func (s) TestChannelStateTransitionWithRPC(t *testing.T) {
+	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 an RPC call is made soon as Connect() is called. See issue #7686.")


Copy/paste issues here too

Updated the comment here as well.

dfawley · 2024-11-11T23:01:36Z

test/clientconn_state_transition_test.go

+
+	cc.Connect()
+
+	testutils.AwaitState(ctx, t, cc, connectivity.Idle)


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.

janardhanvissa · 2024-11-12T06:42:39Z

Replaced the initial AwaitState() call with a direct GetState() check.

arjan-bal reviewed Oct 22, 2024

View reviewed changes

test/clientconn_state_transition_test.go Outdated Show resolved Hide resolved

arjan-bal assigned janardhanvissa Oct 22, 2024

arjan-bal added the Type: Testing label Oct 22, 2024

arjan-bal added this to the 1.69 Release milestone Oct 22, 2024

janardhankrishna-sai added 3 commits October 23, 2024 04:49

Add unit test for channel state waiting for first resolver update

49329cf

removing space

f314b62

Moving TODO comments

6ad9713

janardhanvissa force-pushed the statechannel branch from 1edec56 to 6ad9713 Compare October 23, 2024 04:52

arjan-bal previously approved these changes Oct 23, 2024

View reviewed changes

arjan-bal unassigned janardhanvissa Oct 23, 2024

arjan-bal requested a review from dfawley October 23, 2024 05:12

arjan-bal assigned dfawley Oct 23, 2024

dfawley reviewed Oct 25, 2024

View reviewed changes

dfawley assigned janardhanvissa and unassigned dfawley Oct 25, 2024

janardhankrishna-sai added 2 commits October 26, 2024 13:10

Used t.Skip instead with a message and used channel and goroutine

7d8ed8f

removing comment

b74dcd2

purnesh42H assigned dfawley and unassigned janardhanvissa Oct 29, 2024

dfawley reviewed Oct 29, 2024

View reviewed changes

dfawley assigned janardhanvissa and unassigned dfawley Oct 29, 2024

janardhankrishna-sai added 2 commits October 29, 2024 22:02

Moving t.Skip to up and updated AwaitState, shortCtx to ctx

2061c46

Moving t.Skip to up and updated AwaitState, shortCtx to ctx

f3c6931

dfawley reviewed Oct 29, 2024

View reviewed changes

janardhankrishna-sai added 2 commits October 30, 2024 03:55

Moving Idle before cc.Connect() and connectivity.Connecting after cc.…

7829dd6

…Connect()

Adding time.Sleep

69c8a63

purnesh42H closed this Oct 30, 2024

arjan-bal assigned arjan-bal and unassigned janardhanvissa Nov 5, 2024

arjan-bal reopened this Nov 5, 2024

arjan-bal reviewed Nov 5, 2024

View reviewed changes

arjan-bal assigned janardhanvissa and unassigned arjan-bal Nov 5, 2024

removed the updateState and Ready after Connecting and updated message

3b33b19

arjan-bal reviewed Nov 5, 2024

View reviewed changes

adding new test case with rpc call and adding awaitnostatechannel con…

a6d2b49

…dition

arjan-bal assigned arjan-bal and unassigned janardhanvissa Nov 6, 2024

arjan-bal approved these changes Nov 6, 2024

View reviewed changes

arjan-bal assigned janardhanvissa and unassigned arjan-bal Nov 6, 2024

janardhankrishna-sai added 2 commits November 6, 2024 10:29

updating the comment

9a85a50

updating the comment

bc03ad2

arjan-bal assigned dfawley and unassigned janardhanvissa Nov 6, 2024

purnesh42H requested a review from dfawley November 11, 2024 08:22

dfawley reviewed Nov 11, 2024

View reviewed changes

dfawley assigned janardhanvissa and unassigned dfawley Nov 11, 2024

Updating skip msg and moving Idle connectivity before context

cd4f22e

arjan-bal assigned dfawley and unassigned janardhanvissa Nov 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

janardhanvissa commented Oct 22, 2024

codecov bot commented Oct 22, 2024 •

edited

Loading

arjan-bal commented Oct 22, 2024

arjan-bal left a comment

dfawley Oct 25, 2024

janardhanvissa Oct 29, 2024

dfawley Oct 25, 2024

janardhanvissa commented Oct 26, 2024

dfawley Oct 29, 2024

janardhanvissa Oct 29, 2024

dfawley Oct 29, 2024

dfawley Oct 29, 2024

dfawley Oct 29, 2024

janardhanvissa Oct 30, 2024

arjan-bal Nov 5, 2024

dfawley Oct 29, 2024

janardhanvissa Oct 30, 2024

dfawley Nov 11, 2024

dfawley Oct 29, 2024

purnesh42H commented Oct 30, 2024

arjan-bal Nov 5, 2024

arjan-bal left a comment

arjan-bal Nov 5, 2024

janardhanvissa Nov 5, 2024

janardhanvissa commented Nov 5, 2024

arjan-bal left a comment

arjan-bal Nov 6, 2024

janardhanvissa Nov 6, 2024

dfawley left a comment

dfawley Nov 11, 2024

janardhanvissa Nov 12, 2024

dfawley Nov 11, 2024

janardhanvissa Nov 12, 2024

dfawley Nov 11, 2024

janardhanvissa commented Nov 12, 2024


		cc.Connect()

		// TODO: Change this assertion to testutils.AwaitState(shortCtx, t, cc,


		cc.Connect()

		testutils.AwaitState(ctx, t, cc, connectivity.Idle)

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

Are you sure you want to change the base?

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

Conversation

janardhanvissa commented Oct 22, 2024

codecov bot commented Oct 22, 2024 • edited Loading

Codecov Report

arjan-bal commented Oct 22, 2024

arjan-bal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janardhanvissa commented Oct 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purnesh42H commented Oct 30, 2024

Choose a reason for hiding this comment

arjan-bal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janardhanvissa commented Nov 5, 2024

arjan-bal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfawley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janardhanvissa commented Nov 12, 2024

codecov bot commented Oct 22, 2024 •

edited

Loading