Skip to content

Commit

Permalink
requestbatcher: deflake TestBatchTimeout
Browse files Browse the repository at this point in the history
Fixes #97832.

This commit deflakes `TestBatchTimeout/WithTimeoutAndPagination`. The
test was setting an aggressive MaxTimeout and then not handling all
cases where the timeout could fire. Luckily, the test did not ever
actually want the timeout to fire, so we can deflake it by simply
setting a large (but non-zero) MaxTimeout. This still allows us to
assert that time timeout is applied across all sub-batches in a
paginated request.

Release note: None
  • Loading branch information
nvanbenschoten committed Mar 13, 2023
1 parent a3a63c6 commit 88b4e99
Showing 1 changed file with 11 additions and 8 deletions.
19 changes: 11 additions & 8 deletions pkg/internal/client/requestbatcher/batcher_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,6 @@ func TestBatchTimeout(t *testing.T) {
const timeout = 5 * time.Millisecond
stopper := stop.NewStopper()
defer stopper.Stop(context.Background())
sc := make(chanSender)
testCases := []struct {
requestTimeout time.Duration
maxTimeout time.Duration
Expand Down Expand Up @@ -350,6 +349,7 @@ func TestBatchTimeout(t *testing.T) {
for _, tc := range testCases {
t.Run(fmt.Sprintf("With%sRequestTimeout%sMaxTimeout", tc.requestTimeout, tc.maxTimeout),
func(t *testing.T) {
sc := make(chanSender)
b := New(Config{
// MaxMsgsPerBatch of 1 is chosen so that the first call to Send will
// immediately lead to a batch being sent.
Expand Down Expand Up @@ -387,8 +387,9 @@ func TestBatchTimeout(t *testing.T) {
)
}
t.Run("WithTimeoutAndPagination", func(t *testing.T) {
maxTimeout := 20 * time.Millisecond
maxTimeout := 45 * time.Second
firstAndSecondSendTimeDiff := 10 * time.Millisecond
sc := make(chanSender)
b := New(Config{
// MaxMsgsPerBatch of 1 is chosen so that the first call to Send will
// immediately lead to a batch being sent.
Expand All @@ -398,13 +399,14 @@ func TestBatchTimeout(t *testing.T) {
MaxTimeout: maxTimeout,
})
// This test will simulate multiple calls to Send (due to pagination) and
// test that the MaxTimeout set is a timeout per batch rather than a
// timeout per request.
// test that the MaxTimeout set is a timeout per batch rather than a timeout
// per request. MaxTimeout is set to a large value, so the timeout should
// never actually be hit. This means that the subtest does not face the same
// timing issues that the subtests above do.
ctx := context.Background()
respChan := make(chan Response, 1)
if err := b.SendWithChan(context.Background(), respChan, 1, &kvpb.GetRequest{}); err != nil {
testutils.IsError(err, context.DeadlineExceeded.Error())
return
}
err := b.SendWithChan(ctx, respChan, 1, &kvpb.GetRequest{})
require.NoError(t, err)
// First call to Send.
s := <-sc
time.Sleep(firstAndSecondSendTimeDiff)
Expand Down Expand Up @@ -432,6 +434,7 @@ func TestBatchTimeout(t *testing.T) {
s.respChan <- batchResp{}
})
t.Run("NoTimeout", func(t *testing.T) {
sc := make(chanSender)
b := New(Config{
// MaxMsgsPerBatch of 2 is chosen so that the second call to Send will
// immediately lead to a batch being sent.
Expand Down

0 comments on commit 88b4e99

Please sign in to comment.