Skip to content
Merged
Changes from 3 commits
Commits
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
187 changes: 186 additions & 1 deletion internal/balancergroup/balancergroup_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Contributor

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 to t.Name(). Here and elsewhere.

called := false

stub.Register(balancerName, stub.BalancerFuncs{
UpdateClientConnState: func(_ *stub.BalancerData, _ balancer.ClientConnState) error {
called = true
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"
Copy link
Member

Choose a reason for hiding this comment

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

We can use t.Name() everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 t.Name()?

Copy link
Member

Choose a reason for hiding this comment

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

No, t.Name() will basically give you the name of the test. So if you are using one balancer it should work fine. But if you use it for multiple balancers t will not work, since we will be trying to create multiple balancers with same name.

called := false

stub.Register(balancerName, stub.BalancerFuncs{
ResolverError: func(_ *stub.BalancerData, _ error) {
called = true
},
})

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"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@easwars i just got nitpicking question.
in this pr, i got reviewed to use t.Name() for balancerName. but some of the cases are still using hard-coded name. Is there any specific reason for it? or would it be better to be updated as t.name() too?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to change them to use t.Name as well, but not a priority.

exitIdleCh := make(chan struct{}, 1)
Expand All @@ -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):
Expand All @@ -514,6 +571,58 @@ func (s) TestBalancerExitIdleOne(t *testing.T) {
}
}

func (s) TestBalancerGroup_ExitIdleOne_AfterClose(t *testing.T) {
balancerName := "stub-balancer-test-exit-idle-one-after-close"
called := false

stub.Register(balancerName, stub.BalancerFuncs{
ExitIdle: func(_ *stub.BalancerData) {
called = true
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
called = true
close(exitIdleCh)

Here instead of a variable , a channel can be closed and waited for in a select block since this operation is done only once.

},
})

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])

if called {
t.Fatalf("ExitIdleOne called ExitIdle on sub-balancer after BalancerGroup was closed")
}
}

func (s) TestBalancerGroup_ExitIdleOne_NonExistentID(t *testing.T) {
balancerName := "stub-balancer-test-exit-idle-one-missing-id"
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be t.Name() as well, right? Any reason for that not to be the case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i missed to fixed it,
just updated as t.Name() here too

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 {
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

chaged to declare channel as exitIdleCh

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
Expand Down Expand Up @@ -640,3 +749,79 @@ 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"
Copy link
Contributor

Choose a reason for hiding this comment

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

Does three balancers add any more value than two?

And why not?
balancer1 := t.Name() + "-1"
balancer2 := t.Name() + "-2"


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
},
})
}
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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:
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))
Copy link
Member

Choose a reason for hiding this comment

The 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.
eg:
t.Fatalf("Timeout waiting for ExitIdle to be called for %v , name")

}
}

for _, expected := range balancerNames {
if !called[expected] {
t.Errorf("ExitIdle was not called for sub-balancer registered as %q", expected)
}
}
}

func (s) TestBalancerGroup_ExitIdle_AfterClose(t *testing.T) {
balancerName := "stub-balancer-test-balancer-group-exit-idle-after-close"
Copy link
Contributor

Choose a reason for hiding this comment

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

t.Name here as well.

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,
})

bg.Add(testBalancerIDs[0], balancer.Get(balancerName))
bg.Close()
bg.ExitIdle()

if called {
t.Fatalf("ExitIdle was called on sub-balancer even after BalancerGroup was closed")
}
}