Skip to content
Merged
Changes from 5 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
188 changes: 187 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,60 @@ func (s) TestBalancerExitIdleOne(t *testing.T) {
}
}

func (s) TestBalancerGroup_ExitIdleOne_AfterClose(t *testing.T) {
balancerName := "stub-balancer-test-exit-idle-one-after-close"
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):
case <-exitIdleCh:
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 +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"
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:
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))
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")

}
}
}

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.

exitIdleCh := make(chan struct{})
Copy link
Contributor

Choose a reason for hiding this comment

The 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 1. Otherwise, the writer would block if there is no reader.

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 fixed here 705625a.
thx for the helpful review 👍🏻


stub.Register(balancerName, stub.BalancerFuncs{
ExitIdle: func(_ *stub.BalancerData) {
close(exitIdleCh)
Copy link
Contributor

Choose a reason for hiding this comment

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

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 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):
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
case <-time.After(defaultTestTimeout):
case <-time.After(defaultShortTestTimeout):

We use defaultShortTestTimeout to wait for some time for cases that are not expected to happen , and use defaultTestTimeout for cases that are expected to happen.

}
}
Loading