-
Notifications
You must be signed in to change notification settings - Fork 4.6k
grpctest: add test coverages of ExitIdle
#8375
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.
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
Changes from 5 commits
ef292d2
df3f0f0
6ed4f3b
c74418d
f2b8e02
4994ba1
73ddce7
fd47c7d
347b549
1137e54
0fda6d9
cae4825
c33ece3
bd2f761
a329773
143c2be
9d703f2
3604f68
705625a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -484,6 +484,63 @@ func (s) TestBalancerGroupBuildOptions(t *testing.T) { | |||||
} | ||||||
} | ||||||
|
||||||
func (s) TestBalancerGroup_UpdateClientConnState_AfterClose(t *testing.T) { | ||||||
balancerName := "stub-balancer-test-update-client-state-after-close" | ||||||
called := false | ||||||
|
||||||
stub.Register(balancerName, stub.BalancerFuncs{ | ||||||
UpdateClientConnState: func(_ *stub.BalancerData, _ balancer.ClientConnState) error { | ||||||
called = true | ||||||
eshitachandwani marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
return nil | ||||||
}, | ||||||
}) | ||||||
|
||||||
bg := New(Options{ | ||||||
CC: testutils.NewBalancerClientConn(t), | ||||||
BuildOpts: balancer.BuildOptions{}, | ||||||
StateAggregator: nil, | ||||||
Logger: nil, | ||||||
}) | ||||||
|
||||||
bg.Add(testBalancerIDs[0], balancer.Get(balancerName)) | ||||||
bg.Close() | ||||||
|
||||||
err := bg.UpdateClientConnState(testBalancerIDs[0], balancer.ClientConnState{}) | ||||||
if err != nil { | ||||||
t.Fatalf("Expected nil error, got %v", err) | ||||||
} | ||||||
if called { | ||||||
t.Fatalf("UpdateClientConnState was called after BalancerGroup was closed") | ||||||
} | ||||||
} | ||||||
|
||||||
func (s) TestBalancerGroup_ResolverError_AfterClose(t *testing.T) { | ||||||
balancerName := "stub-balancer-test-resolver-error-after-close" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @eshitachandwani if i have to use multiple balancerName, then can i just declare multiple There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, |
||||||
called := false | ||||||
|
||||||
stub.Register(balancerName, stub.BalancerFuncs{ | ||||||
ResolverError: func(_ *stub.BalancerData, _ error) { | ||||||
called = true | ||||||
eshitachandwani marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
}, | ||||||
}) | ||||||
|
||||||
bg := New(Options{ | ||||||
CC: testutils.NewBalancerClientConn(t), | ||||||
BuildOpts: balancer.BuildOptions{}, | ||||||
StateAggregator: nil, | ||||||
Logger: nil, | ||||||
}) | ||||||
|
||||||
bg.Add(testBalancerIDs[0], balancer.Get(balancerName)) | ||||||
bg.Close() | ||||||
|
||||||
bg.ResolverError(errors.New("test error")) | ||||||
|
||||||
if called { | ||||||
t.Fatalf("ResolverError was called on sub-balancer after BalancerGroup was closed") | ||||||
} | ||||||
} | ||||||
|
||||||
func (s) TestBalancerExitIdleOne(t *testing.T) { | ||||||
const balancerName = "stub-balancer-test-balancergroup-exit-idle-one" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @easwars i just got nitpicking question. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be nice to change them to use |
||||||
exitIdleCh := make(chan struct{}, 1) | ||||||
|
@@ -505,7 +562,7 @@ func (s) TestBalancerExitIdleOne(t *testing.T) { | |||||
builder := balancer.Get(balancerName) | ||||||
bg.Add(testBalancerIDs[0], builder) | ||||||
|
||||||
// Call ExitIdle on the child policy. | ||||||
// Call ExitIdleOne on the child policy. | ||||||
bg.ExitIdleOne(testBalancerIDs[0]) | ||||||
select { | ||||||
case <-time.After(time.Second): | ||||||
|
@@ -514,6 +571,60 @@ func (s) TestBalancerExitIdleOne(t *testing.T) { | |||||
} | ||||||
} | ||||||
|
||||||
func (s) TestBalancerGroup_ExitIdleOne_AfterClose(t *testing.T) { | ||||||
balancerName := "stub-balancer-test-exit-idle-one-after-close" | ||||||
eshitachandwani marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
exitIdleCh := make(chan struct{}) | ||||||
|
||||||
stub.Register(balancerName, stub.BalancerFuncs{ | ||||||
ExitIdle: func(_ *stub.BalancerData) { | ||||||
close(exitIdleCh) | ||||||
}, | ||||||
}) | ||||||
|
||||||
bg := New(Options{ | ||||||
CC: testutils.NewBalancerClientConn(t), | ||||||
BuildOpts: balancer.BuildOptions{}, | ||||||
StateAggregator: nil, | ||||||
Logger: nil, | ||||||
}) | ||||||
|
||||||
bg.Add(testBalancerIDs[0], balancer.Get(balancerName)) | ||||||
bg.Close() | ||||||
bg.ExitIdleOne(testBalancerIDs[0]) | ||||||
|
||||||
select { | ||||||
case <-time.After(time.Second): | ||||||
eshitachandwani marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
case <-exitIdleCh: | ||||||
t.Fatalf("ExitIdleOne called ExitIdle on sub-balancer after BalancerGroup was closed") | ||||||
} | ||||||
} | ||||||
|
||||||
func (s) TestBalancerGroup_ExitIdleOne_NonExistentID(t *testing.T) { | ||||||
easwars marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
balancerName := "stub-balancer-test-exit-idle-one-missing-id" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i missed to fixed it, |
||||||
called := false | ||||||
|
||||||
stub.Register(balancerName, stub.BalancerFuncs{ | ||||||
ExitIdle: func(_ *stub.BalancerData) { | ||||||
called = true | ||||||
}, | ||||||
}) | ||||||
|
||||||
bg := New(Options{ | ||||||
CC: testutils.NewBalancerClientConn(t), | ||||||
BuildOpts: balancer.BuildOptions{}, | ||||||
StateAggregator: nil, | ||||||
Logger: nil, | ||||||
}) | ||||||
defer bg.Close() | ||||||
|
||||||
bg.Add(testBalancerIDs[0], balancer.Get(balancerName)) | ||||||
bg.ExitIdleOne("non-existent-id") | ||||||
|
||||||
if called { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use a channel as used in the other tests instead of a local variable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. chaged to declare channel as |
||||||
t.Fatalf("ExitIdleOne called ExitIdle on wrong sub-balancer ID") | ||||||
} | ||||||
} | ||||||
|
||||||
// TestBalancerGracefulSwitch tests the graceful switch functionality for a | ||||||
// child of the balancer group. At first, the child is configured as a round | ||||||
// robin load balancer, and thus should behave accordingly. The test then | ||||||
|
@@ -640,3 +751,78 @@ func (s) TestBalancerGracefulSwitch(t *testing.T) { | |||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
func (s) TestBalancerExitIdle_All(t *testing.T) { | ||||||
balancerOne := "stub-balancer-test-balancer-group-exit-idle-one" | ||||||
balancerTwo := "stub-balancer-test-balancer-group-exit-idle-two" | ||||||
balancerThree := "stub-balancer-test-balancer-group-exit-idle-three" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does three balancers add any more value than two? And why not? |
||||||
|
||||||
balancerNames := []string{balancerOne, balancerTwo, balancerThree} | ||||||
testIDs := []string{testBalancerIDs[0], testBalancerIDs[1], testBalancerIDs[2]} | ||||||
|
||||||
exitIdleCh := make(chan string, len(balancerNames)) | ||||||
|
||||||
for _, name := range balancerNames { | ||||||
stub.Register(name, stub.BalancerFuncs{ | ||||||
ExitIdle: func(_ *stub.BalancerData) { | ||||||
exitIdleCh <- name | ||||||
}, | ||||||
}) | ||||||
} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would get rid of the slices and the loops and instead keep this downright simple and dump. // Have two test IDs: testID1 and testID2
// Have two channels, exitIdleCh1 and exitIdleCh2
// Register two stub balancers, the first one for balancer1 that writes to exitIdleCh1 and the second one for balancer2 that writes to exitIdleCh2
// Create the balancer group, and add the child balancers in there
// Call ExitIdle
// Validate that both exitIdleCh1 and exitIdleCh2 are written to. (I don't think it's an issue if one of those are written to more than once). In this case though, you might have to spawn goroutines to validate that both channels are written to, since we cannot guarantee the order in which they would be written to. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. applied in 9d703f2 |
||||||
|
||||||
cc := testutils.NewBalancerClientConn(t) | ||||||
bg := New(Options{ | ||||||
CC: cc, | ||||||
BuildOpts: balancer.BuildOptions{}, | ||||||
StateAggregator: nil, | ||||||
Logger: nil, | ||||||
}) | ||||||
defer bg.Close() | ||||||
|
||||||
bg.Add(testIDs[0], balancer.Get(balancerOne)) | ||||||
bg.Add(testIDs[1], balancer.Get(balancerTwo)) | ||||||
bg.Add(testIDs[2], balancer.Get(balancerThree)) | ||||||
|
||||||
bg.ExitIdle() | ||||||
|
||||||
called := make(map[string]bool) | ||||||
for i := 0; i < len(balancerNames); i++ { | ||||||
select { | ||||||
case name := <-exitIdleCh: | ||||||
if called[name] { | ||||||
t.Fatalf("ExitIdle was called multiple times for sub-balancer %q", name) | ||||||
} | ||||||
called[name] = true | ||||||
case <-time.After(time.Second): | ||||||
t.Fatalf("Timeout: ExitIdle not called for all sub-balancers, got %d/%d", len(called), len(balancerNames)) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The error can be changed to give specific name of the balancer for which it was not called. |
||||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
func (s) TestBalancerGroup_ExitIdle_AfterClose(t *testing.T) { | ||||||
balancerName := "stub-balancer-test-balancer-group-exit-idle-after-close" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||
exitIdleCh := make(chan struct{}) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops. Sorry, didn't catch this earlier. This channel needs to have a buffer of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||
|
||||||
stub.Register(balancerName, stub.BalancerFuncs{ | ||||||
ExitIdle: func(_ *stub.BalancerData) { | ||||||
close(exitIdleCh) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In some places, you write an empty struct to the channel, while in other places, you close the channel. I understand this is minor and super nit-picky, but can you please choose one approach and stay consistent across the PR. Thanks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i decided to send an empty struct to a buffered channel, it's more robust and consistent approach i think. |
||||||
}, | ||||||
}) | ||||||
|
||||||
bg := New(Options{ | ||||||
CC: testutils.NewBalancerClientConn(t), | ||||||
BuildOpts: balancer.BuildOptions{}, | ||||||
StateAggregator: nil, | ||||||
Logger: nil, | ||||||
}) | ||||||
|
||||||
bg.Add(testBalancerIDs[0], balancer.Get(balancerName)) | ||||||
bg.Close() | ||||||
bg.ExitIdle() | ||||||
|
||||||
select { | ||||||
case <-exitIdleCh: | ||||||
t.Fatalf("ExitIdle was called on sub-balancer even after BalancerGroup was closed") | ||||||
case <-time.After(defaultTestTimeout): | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
We use |
||||||
} | ||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
balancerName
could be set tot.Name()
. Here and elsewhere.