Skip to content

Commit

Permalink
test: Use testify require/assert instead of t.Fatal/Error in `go/vt/t…
Browse files Browse the repository at this point in the history
…hrottler` (#15703)

Signed-off-by: Noble Mittal <noblemittal@outlook.com>
  • Loading branch information
beingnoble03 committed Apr 15, 2024
1 parent 81e8f0c commit cc6b5a6
Show file tree
Hide file tree
Showing 10 changed files with 282 additions and 375 deletions.
7 changes: 4 additions & 3 deletions go/vt/throttler/aggregated_interval_history_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,15 @@ package throttler
import (
"testing"
"time"

"github.com/stretchr/testify/assert"
)

func TestAggregatedIntervalHistory(t *testing.T) {
h := newAggregatedIntervalHistory(10, 1*time.Second, 2)
h.addPerThread(0, record{sinceZero(0 * time.Second), 1000})
h.addPerThread(1, record{sinceZero(0 * time.Second), 2000})

if got, want := h.average(sinceZero(250*time.Millisecond), sinceZero(750*time.Millisecond)), 3000.0; got != want {
t.Errorf("average(0.25s, 0.75s) across both threads = %v, want = %v", got, want)
}
got := h.average(sinceZero(250*time.Millisecond), sinceZero(750*time.Millisecond))
assert.Equal(t, 3000.0, got)
}
35 changes: 13 additions & 22 deletions go/vt/throttler/interval_history_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,11 @@ limitations under the License.
package throttler

import (
"strings"
"testing"
"time"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestIntervalHistory_AverageIncludesPartialIntervals(t *testing.T) {
Expand All @@ -33,19 +35,17 @@ func TestIntervalHistory_AverageIncludesPartialIntervals(t *testing.T) {
h.add(record{sinceZero(3 * time.Second), 10000000})
// Rate within [1s, 2s) = 1000 and within [2s, 3s) = 2000 = average of 1500
want := 1500.0
if got := h.average(sinceZero(1500*time.Millisecond), sinceZero(2500*time.Millisecond)); got != want {
t.Errorf("average(1.5s, 2.5s) = %v, want = %v", got, want)
}
got := h.average(sinceZero(1500*time.Millisecond), sinceZero(2500*time.Millisecond))
assert.Equal(t, want, got)
}

func TestIntervalHistory_AverageRangeSmallerThanInterval(t *testing.T) {
h := newIntervalHistory(10, 1*time.Second)

h.add(record{sinceZero(0 * time.Second), 10000})
want := 10000.0
if got := h.average(sinceZero(250*time.Millisecond), sinceZero(750*time.Millisecond)); got != want {
t.Errorf("average(0.25s, 0.75s) = %v, want = %v", got, want)
}
got := h.average(sinceZero(250*time.Millisecond), sinceZero(750*time.Millisecond))
assert.Equal(t, want, got)
}

func TestIntervalHistory_GapsCountedAsZero(t *testing.T) {
Expand All @@ -55,22 +55,17 @@ func TestIntervalHistory_GapsCountedAsZero(t *testing.T) {
h.add(record{sinceZero(3 * time.Second), 1000})

want := 500.0
if got := h.average(sinceZero(0*time.Second), sinceZero(4*time.Second)); got != want {
t.Errorf("average(0s, 4s) = %v, want = %v", got, want)
}
got := h.average(sinceZero(0*time.Second), sinceZero(4*time.Second))
assert.Equal(t, want, got)
}

func TestIntervalHistory_AddNoDuplicateInterval(t *testing.T) {
defer func() {
r := recover()
require.NotNil(t, r, "add() did not panic")

if r == nil {
t.Fatal("add() did not panic")
}
want := "BUG: cannot add record because it is already covered by a previous entry"
if !strings.Contains(r.(string), want) {
t.Fatalf("add() did panic for the wrong reason: got = %v, want = %v", r, want)
}
require.Contains(t, r, want, "add() did panic for the wrong reason")
}()

h := newIntervalHistory(10, 1*time.Second)
Expand All @@ -82,14 +77,10 @@ func TestIntervalHistory_AddNoDuplicateInterval(t *testing.T) {
func TestIntervalHistory_RecordDoesNotStartAtInterval(t *testing.T) {
defer func() {
r := recover()
require.NotNil(t, r, "add() did not panic")

if r == nil {
t.Fatal("add() did not panic")
}
want := "BUG: cannot add record because it does not start at the beginning of the interval"
if !strings.Contains(r.(string), want) {
t.Fatalf("add() did panic for the wrong reason: got = %v, want = %v", r, want)
}
require.Contains(t, r, want, "add() did panic for the wrong reason")
}()

h := newIntervalHistory(1, 1*time.Second)
Expand Down
153 changes: 62 additions & 91 deletions go/vt/throttler/manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,12 @@ import (
"fmt"
"reflect"
"sort"
"strings"
"testing"
"time"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

throttlerdatapb "vitess.io/vitess/go/vt/proto/throttlerdata"
)

Expand Down Expand Up @@ -60,12 +62,11 @@ func (f *managerTestFixture) tearDown() {
func TestManager_Registration(t *testing.T) {
m := newManager()
t1, err := newThrottler(m, "t1", "TPS", 1 /* threadCount */, MaxRateModuleDisabled, ReplicationLagModuleDisabled, time.Now)
if err != nil {
t.Fatal(err)
}
if err := m.registerThrottler("t1", t1); err == nil {
t.Fatalf("manager should not accept a duplicate registration of a throttler: %v", err)
}
require.NoError(t, err)

err = m.registerThrottler("t1", t1)
require.Error(t, err, "manager should not accept a duplicate registration of a throttler")

t1.Close()

// Unregistering an unregistered throttler should log an error.
Expand All @@ -81,18 +82,16 @@ func TestManager_SetMaxRate(t *testing.T) {

// Test SetMaxRate().
want := []string{"t1", "t2"}
if got := f.m.SetMaxRate(23); !reflect.DeepEqual(got, want) {
t.Errorf("manager did not set the rate on all throttlers. got = %v, want = %v", got, want)
}
got := f.m.SetMaxRate(23)
assert.Equal(t, want, got, "manager did not set the rate on all throttlers")

// Test MaxRates().
wantRates := map[string]int64{
"t1": 23,
"t2": 23,
}
if gotRates := f.m.MaxRates(); !reflect.DeepEqual(gotRates, wantRates) {
t.Errorf("manager did not set the rate on all throttlers. got = %v, want = %v", gotRates, wantRates)
}
gotRates := f.m.MaxRates()
assert.Equal(t, wantRates, gotRates, "manager did not set the rate on all throttlers")
}

func TestManager_GetConfiguration(t *testing.T) {
Expand All @@ -108,89 +107,68 @@ func TestManager_GetConfiguration(t *testing.T) {
"t2": defaultMaxReplicationLagModuleConfig.Clone().Configuration,
}
got, err := f.m.GetConfiguration("" /* all */)
if err != nil {
t.Fatal(err)
}
if !reflect.DeepEqual(got, want) {
t.Errorf("manager did not return the correct initial config for all throttlers. got = %v, want = %v", got, want)
}
require.NoError(t, err)
assert.Equal(t, want, got, "manager did not return the correct initial config for all throttlers")

// Test GetConfiguration() when a specific throttler is requested.
wantT2 := map[string]*throttlerdatapb.Configuration{
"t2": defaultMaxReplicationLagModuleConfig.Clone().Configuration,
}
gotT2, err := f.m.GetConfiguration("t2")
if err != nil {
t.Fatal(err)
}
if !reflect.DeepEqual(gotT2, wantT2) {
t.Errorf("manager did not return the correct initial config for throttler: %v got = %v, want = %v", "t2", gotT2, wantT2)
}
require.NoError(t, err)
assert.Equal(t, wantT2, gotT2, "manager did not return the correct initial config for throttler: t2")

// Now change the config and then reset it back.
newConfig := &throttlerdatapb.Configuration{
TargetReplicationLagSec: defaultTargetLag + 1,
IgnoreNSlowestReplicas: defaultIgnoreNSlowestReplicas + 1,
}
allNames, err := f.m.UpdateConfiguration("", newConfig, false /* copyZeroValues */)
if err != nil {
t.Fatal(err)
}
// Verify it was changed.
if err := checkConfig(f.m, []string{"t1", "t2"}, allNames, defaultTargetLag+1, defaultIgnoreNSlowestReplicas+1); err != nil {
t.Fatal(err)
}
require.NoError(t, err)

err = checkConfig(f.m, []string{"t1", "t2"}, allNames, defaultTargetLag+1, defaultIgnoreNSlowestReplicas+1)
require.NoError(t, err)

// Reset only "t2".
if names, err := f.m.ResetConfiguration("t2"); err != nil || !reflect.DeepEqual(names, []string{"t2"}) {
t.Fatalf("Reset failed or returned wrong throttler names: %v err: %v", names, err)
}
names, err := f.m.ResetConfiguration("t2")
require.NoError(t, err)
assert.Equal(t, []string{"t2"}, names, "Reset failed or returned wrong throttler names")

gotT2AfterReset, err := f.m.GetConfiguration("t2")
if err != nil {
t.Fatal(err)
}
if !reflect.DeepEqual(gotT2AfterReset, wantT2) {
t.Errorf("manager did not return the correct initial config for throttler %v after reset: got = %v, want = %v", "t2", gotT2AfterReset, wantT2)
}
require.NoError(t, err)
assert.Equal(t, wantT2, gotT2AfterReset, "manager did not return the correct initial config for throttler t2 after reset")

// Reset all throttlers.
if names, err := f.m.ResetConfiguration(""); err != nil || !reflect.DeepEqual(names, []string{"t1", "t2"}) {
t.Fatalf("Reset failed or returned wrong throttler names: %v err: %v", names, err)
}

names, err = f.m.ResetConfiguration("")
require.NoError(t, err)
assert.Equal(t, []string{"t1", "t2"}, names, "Reset failed or returned wrong throttler names")

gotAfterReset, err := f.m.GetConfiguration("")
if err != nil {
t.Fatal(err)
}
if !reflect.DeepEqual(gotAfterReset, want) {
t.Errorf("manager did not return the correct initial config for all throttlers after reset. got = %v, want = %v", got, want)
}
require.NoError(t, err)
assert.Equal(t, want, gotAfterReset, "manager did not return the correct initial config for all throttlers after reset")
}

func TestManager_UpdateConfiguration_Error(t *testing.T) {
f := &managerTestFixture{}
if err := f.setUp(); err != nil {
t.Fatal(err)
}
err := f.setUp()
require.NoError(t, err)
defer f.tearDown()

// Check that errors from Verify() are correctly propagated.
invalidConfig := &throttlerdatapb.Configuration{
// max < 2 is not allowed.
MaxReplicationLagSec: 1,
}
if _, err := f.m.UpdateConfiguration("t2", invalidConfig, false /* copyZeroValues */); err == nil {
t.Fatal("expected error but got nil")
} else {
want := "max_replication_lag_sec must be >= 2"
if !strings.Contains(err.Error(), want) {
t.Fatalf("received wrong error. got = %v, want contains = %v", err, want)
}
}
_, err = f.m.UpdateConfiguration("t2", invalidConfig, false /* copyZeroValues */)
wantErr := "max_replication_lag_sec must be >= 2"
require.ErrorContains(t, err, wantErr)
}

func TestManager_UpdateConfiguration_Partial(t *testing.T) {
f := &managerTestFixture{}
if err := f.setUp(); err != nil {
t.Fatal(err)
}
err := f.setUp()
require.NoError(t, err)
defer f.tearDown()

// Verify that a partial update only updates that one field.
Expand All @@ -199,47 +177,40 @@ func TestManager_UpdateConfiguration_Partial(t *testing.T) {
IgnoreNSlowestReplicas: wantIgnoreNSlowestReplicas,
}
names, err := f.m.UpdateConfiguration("t2", partialConfig, false /* copyZeroValues */)
if err != nil {
t.Fatal(err)
}
if err := checkConfig(f.m, []string{"t2"}, names, defaultTargetLag, wantIgnoreNSlowestReplicas); err != nil {
t.Fatal(err)
}
require.NoError(t, err)

err = checkConfig(f.m, []string{"t2"}, names, defaultTargetLag, wantIgnoreNSlowestReplicas)
require.NoError(t, err)

// Repeat test for all throttlers.
allNames, err := f.m.UpdateConfiguration("" /* all */, partialConfig, false /* copyZeroValues */)
if err != nil {
t.Fatal(err)
}
if err := checkConfig(f.m, []string{"t1", "t2"}, allNames, defaultTargetLag, wantIgnoreNSlowestReplicas); err != nil {
t.Fatal(err)
}
require.NoError(t, err)

err = checkConfig(f.m, []string{"t1", "t2"}, allNames, defaultTargetLag, wantIgnoreNSlowestReplicas)
require.NoError(t, err)
}

func TestManager_UpdateConfiguration_ZeroValues(t *testing.T) {
f := &managerTestFixture{}
if err := f.setUp(); err != nil {
t.Fatal(err)
}
err := f.setUp()
require.NoError(t, err)
defer f.tearDown()

// Test the explicit copy of zero values.
zeroValueConfig := defaultMaxReplicationLagModuleConfig.Configuration.CloneVT()
zeroValueConfig.IgnoreNSlowestReplicas = 0
names, err := f.m.UpdateConfiguration("t2", zeroValueConfig, true /* copyZeroValues */)
if err != nil {
t.Fatal(err)
}
if err := checkConfig(f.m, []string{"t2"}, names, defaultTargetLag, 0); err != nil {
t.Fatal(err)
}
require.NoError(t, err)

err = checkConfig(f.m, []string{"t2"}, names, defaultTargetLag, 0)
require.NoError(t, err)

// Repeat test for all throttlers.
allNames, err := f.m.UpdateConfiguration("" /* all */, zeroValueConfig, true /* copyZeroValues */)
if err != nil {
t.Fatal(err)
}
if err := checkConfig(f.m, []string{"t1", "t2"}, allNames, defaultTargetLag, 0); err != nil {
t.Fatal(err)
}
require.NoError(t, err)

err = checkConfig(f.m, []string{"t1", "t2"}, allNames, defaultTargetLag, 0)
require.NoError(t, err)
}

func checkConfig(m *managerImpl, throttlers []string, updatedThrottlers []string, targetLag int64, ignoreNSlowestReplicas int32) error {
Expand Down
Loading

0 comments on commit cc6b5a6

Please sign in to comment.