Skip to content
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.

Sign up for GitHub

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

test: Use testify require/assert instead of t.Fatal/Error in go/vt/throttler #15703

Merged
merged 1 commit into from
Apr 15, 2024
Merged

test: Use testify require/assert instead of t.Fatal/Error in go/vt/throttler #15703

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 4 additions & 3 deletions go/vt/throttler/aggregated_interval_history_test.go
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
Loading