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: Replace t.fatalf with testify require #16038

Merged
merged 9 commits into from
Jun 14, 2024
Merged

test: Replace t.fatalf with testify require #16038

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e0eee4f
fix
Ari1009 Jun 2, 2024
6b67e76
update
Ari1009 Jun 3, 2024
91d9c25
Update_2
Ari1009 Jun 3, 2024
4239566
Updates_3
Ari1009 Jun 3, 2024
3c10f00
Update_4
Ari1009 Jun 4, 2024
6b4b485
Remove_UnecessaryPrint
Ari1009 Jun 4, 2024
e03a735
cleanup
Ari1009 Jun 4, 2024
49876f3
Cleanup_2
Ari1009 Jun 5, 2024
ccdc5f1
Merge branch 'vitessio:main' into fix
Ari1009 Jun 10, 2024
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
5 changes: 2 additions & 3 deletions go/tools/asthelpergen/asthelpergen_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"strings"
"testing"

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

Expand All @@ -42,8 +43,6 @@ func TestFullGeneration(t *testing.T) {
require.Contains(t, contents, "http://www.apache.org/licenses/LICENSE-2.0")
applyIdx := strings.Index(contents, "func (a *application) apply(parent, node AST, replacer replacerFunc)")
cloneIdx := strings.Index(contents, "CloneAST(in AST) AST")
if applyIdx == 0 && cloneIdx == 0 {
t.Fatalf("file doesn't contain expected contents")
}
assert.False(t, applyIdx == 0 && cloneIdx == 0, "file doesn't contain expected contents")
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like a redaction of severity from Fatalf to assert. Should this not be at least require?

}
}
4 changes: 2 additions & 2 deletions go/trace/trace_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,11 @@ func TestRegisterService(t *testing.T) {
closer := StartTracing(serviceName)
tracer, ok := closer.(*fakeTracer)
if !ok {
t.Fatalf("did not get the expected tracer, got %+v (%T)", tracer, tracer)
require.FailNow(t, fmt.Sprintf("did not get the expected tracer, got %+v (%T)", tracer, tracer))
Copy link
Contributor

Choose a reason for hiding this comment

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

shall we replace this if statement with require.True(t, ok, ...)?

}

if tracer.name != serviceName {
t.Fatalf("expected the name to be `%v` but it was `%v`", serviceName, tracer.name)
require.FailNow(t, fmt.Sprintf("expected the name to be `%v` but it was `%v`", serviceName, tracer.name))
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we replace with require.Equal(t, serviceName, tracer.name)?

}
}

Expand Down
19 changes: 8 additions & 11 deletions go/vt/binlog/binlog_streamer_rbr_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ import (
"reflect"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"vitess.io/vitess/go/mysql"
"vitess.io/vitess/go/mysql/binlog"
"vitess.io/vitess/go/mysql/collations"
Expand Down Expand Up @@ -519,17 +521,12 @@ func TestStreamerParseRBRNameEscapes(t *testing.T) {

go sendTestEvents(events, input)
_, err := bls.parseEvents(context.Background(), events, errs)
if err != ErrServerEOF {
t.Errorf("unexpected error: %v", err)
}
require.Equal(t, ErrServerEOF, err, "unexpected error: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use EqualError?


if !reflect.DeepEqual(got, want) {
t.Errorf("binlogConnStreamer.parseEvents(): got:\n%+v\nwant:\n%+v", got, want)
for i, fbt := range got {
t.Errorf("Got (%v)=%v", i, fbt.statements)
}
for i, fbt := range want {
t.Errorf("Want(%v)=%v", i, fbt.statements)
}
assert.True(t, reflect.DeepEqual(got, want), "binlogConnStreamer.parseEvents(): got:\n%+v\nwant:\n%+v", got, want)
Copy link
Contributor

Choose a reason for hiding this comment

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

assert.Equal uses reflect.DeepEqual under the hood. We should just use assert.Equal(t, want, got).


for i, fbt := range got {
assert.Equal(t, fbt.statements, want[i].statements, "Got (%v)=%v, want (%v)=%v", i, fbt.statements, i, want[i].statements)
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems superfluous given we've validated the equality of the entire slice, above?

}

}
17 changes: 5 additions & 12 deletions go/vt/binlog/tables_filter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"fmt"
"testing"

"github.com/stretchr/testify/assert"
binlogdatapb "vitess.io/vitess/go/vt/proto/binlogdata"
querypb "vitess.io/vitess/go/vt/proto/query"
)
Expand Down Expand Up @@ -60,9 +61,7 @@ func TestTablesFilterPass(t *testing.T) {
})
_ = f(eventToken, statements)
want := `statement: <6, "set1"> statement: <7, "dml1 /* _stream included1 (id ) (500 ); */"> statement: <7, "dml2 /* _stream included2 (id ) (500 ); */"> position: "MariaDB/0-41983-1" `
if want != got {
t.Errorf("want\n%s, got\n%s", want, got)
}
assert.Equal(t, want, got, "binlogConnStreamer.tablesFilterFunc(): got:\n%+v\nwant:\n%+v", got, want)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove the got:\n%+v\nwant:\n%+v output, since testify prints the same kind of output anyway.

}

func TestTablesFilterSkip(t *testing.T) {
Expand Down Expand Up @@ -90,9 +89,7 @@ func TestTablesFilterSkip(t *testing.T) {
})
_ = f(eventToken, statements)
want := `position: "MariaDB/0-41983-1" `
if want != got {
t.Errorf("want %s, got %s", want, got)
}
assert.Equal(t, want, got, "binlogConnStreamer.tablesFilterFunc(): got:\n%+v\nwant:\n%+v", got, want)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove the got:\n%+v\nwant:\n%+v output, since testify prints the same kind of output anyway.

}

func TestTablesFilterDDL(t *testing.T) {
Expand Down Expand Up @@ -120,9 +117,7 @@ func TestTablesFilterDDL(t *testing.T) {
})
_ = f(eventToken, statements)
want := `position: "MariaDB/0-41983-1" `
if want != got {
t.Errorf("want %s, got %s", want, got)
}
assert.Equal(t, want, got, "binlogConnStreamer.tablesFilterFunc(): got:\n%+v\nwant:\n%+v", got, want)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove the got:\n%+v\nwant:\n%+v output, since testify prints the same kind of output anyway.

}

func TestTablesFilterMalformed(t *testing.T) {
Expand Down Expand Up @@ -156,9 +151,7 @@ func TestTablesFilterMalformed(t *testing.T) {
})
_ = f(eventToken, statements)
want := `position: "MariaDB/0-41983-1" `
if want != got {
t.Errorf("want %s, got %s", want, got)
}
assert.Equal(t, want, got, "binlogConnStreamer.tablesFilterFunc(): got:\n%+v\nwant:\n%+v", got, want)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove the got:\n%+v\nwant:\n%+v output, since testify prints the same kind of output anyway.

}

func bltToString(tx *binlogdatapb.BinlogTransaction) string {
Expand Down
115 changes: 47 additions & 68 deletions go/vt/wrangler/tablet_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@ package wrangler

import (
"context"
"strings"
"testing"

"github.com/stretchr/testify/require"
"vitess.io/vitess/go/vt/logutil"
topodatapb "vitess.io/vitess/go/vt/proto/topodata"
"vitess.io/vitess/go/vt/topo"
Expand Down Expand Up @@ -48,20 +48,14 @@ func TestInitTabletShardConversion(t *testing.T) {
Shard: "80-C0",
}

if err := wr.TopoServer().InitTablet(context.Background(), tablet, false /*allowPrimaryOverride*/, true /*createShardAndKeyspace*/, false /*allowUpdate*/); err != nil {
t.Fatalf("InitTablet failed: %v", err)
}
err := wr.TopoServer().InitTablet(context.Background(), tablet, false /*allowPrimaryOverride*/, true /*createShardAndKeyspace*/, false /*allowUpdate*/)
require.NoError(t, err, "InitTablet failed")
Ari1009 marked this conversation as resolved.
Show resolved Hide resolved

ti, err := ts.GetTablet(context.Background(), tablet.Alias)
if err != nil {
t.Fatalf("GetTablet failed: %v", err)
}
if ti.Shard != "80-c0" {
t.Errorf("Got wrong tablet.Shard, got %v expected 80-c0", ti.Shard)
}
if string(ti.KeyRange.Start) != "\x80" || string(ti.KeyRange.End) != "\xc0" {
t.Errorf("Got wrong tablet.KeyRange, got %v expected 80-c0", ti.KeyRange)
}
require.NoError(t, err, "GetTablet failed")
require.Equal(t, "80-c0", ti.Shard, "Got wrong tablet.Shard")
require.Equal(t, "\x80", string(ti.KeyRange.Start), "Got wrong tablet.KeyRange start")
require.Equal(t, "\xc0", string(ti.KeyRange.End), "Got wrong tablet.KeyRange end")
Ari1009 marked this conversation as resolved.
Show resolved Hide resolved
}

// TestDeleteTabletBasic tests delete of non-primary tablet
Expand All @@ -82,17 +76,14 @@ func TestDeleteTabletBasic(t *testing.T) {
Keyspace: "test",
}

if err := wr.TopoServer().InitTablet(context.Background(), tablet, false /*allowPrimaryOverride*/, true /*createShardAndKeyspace*/, false /*allowUpdate*/); err != nil {
t.Fatalf("InitTablet failed: %v", err)
}
err := wr.TopoServer().InitTablet(context.Background(), tablet, false /*allowPrimaryOverride*/, true /*createShardAndKeyspace*/, false /*allowUpdate*/)
require.NoError(t, err, "InitTablet failed")

if _, err := ts.GetTablet(context.Background(), tablet.Alias); err != nil {
t.Fatalf("GetTablet failed: %v", err)
}
_, err = ts.GetTablet(context.Background(), tablet.Alias)
require.NoError(t, err, "GetTablet failed")

if err := wr.DeleteTablet(context.Background(), tablet.Alias, false); err != nil {
t.Fatalf("DeleteTablet failed: %v", err)
}
err = wr.DeleteTablet(context.Background(), tablet.Alias, false)
require.NoError(t, err, "DeleteTablet failed")
Ari1009 marked this conversation as resolved.
Show resolved Hide resolved
}

// TestDeleteTabletTruePrimary tests that you can delete a true primary tablet
Expand All @@ -115,31 +106,27 @@ func TestDeleteTabletTruePrimary(t *testing.T) {
Type: topodatapb.TabletType_PRIMARY,
}

if err := wr.TopoServer().InitTablet(context.Background(), tablet, false /*allowPrimaryOverride*/, true /*createShardAndKeyspace*/, false /*allowUpdate*/); err != nil {
t.Fatalf("InitTablet failed: %v", err)
}
if _, err := ts.GetTablet(context.Background(), tablet.Alias); err != nil {
t.Fatalf("GetTablet failed: %v", err)
}
err := wr.TopoServer().InitTablet(context.Background(), tablet, false /*allowPrimaryOverride*/, true /*createShardAndKeyspace*/, false /*allowUpdate*/)
require.NoError(t, err, "InitTablet failed")

_, err = ts.GetTablet(context.Background(), tablet.Alias)
require.NoError(t, err, "GetTablet failed")

// set PrimaryAlias and PrimaryTermStartTime on shard to match chosen primary tablet
if _, err := ts.UpdateShardFields(context.Background(), "test", "0", func(si *topo.ShardInfo) error {
_, err = ts.UpdateShardFields(context.Background(), "test", "0", func(si *topo.ShardInfo) error {
si.PrimaryAlias = tablet.Alias
si.PrimaryTermStartTime = tablet.PrimaryTermStartTime
return nil
}); err != nil {
t.Fatalf("UpdateShardFields failed: %v", err)
}
})
require.NoError(t, err, "UpdateShardFields failed")
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for the last string here either.


err := wr.DeleteTablet(context.Background(), tablet.Alias, false)
err = wr.DeleteTablet(context.Background(), tablet.Alias, false)
wantError := "as it is a primary, use allow_primary flag"
if err == nil || !strings.Contains(err.Error(), wantError) {
t.Fatalf("DeleteTablet on primary: want error = %v, got error = %v", wantError, err)
}
require.Error(t, err, "DeleteTablet on primary: want error")
require.Contains(t, err.Error(), wantError, "DeleteTablet on primary: want specific error message")
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use a single ErrorContains test instead of the two tests above.


if err := wr.DeleteTablet(context.Background(), tablet.Alias, true); err != nil {
t.Fatalf("DeleteTablet failed: %v", err)
}
err = wr.DeleteTablet(context.Background(), tablet.Alias, true)
require.NoError(t, err, "DeleteTablet failed")
Copy link
Contributor

Choose a reason for hiding this comment

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

@Ari1009 I think you can remove this one.

}

// TestDeleteTabletFalsePrimary tests that you can delete a false primary tablet
Expand All @@ -162,9 +149,8 @@ func TestDeleteTabletFalsePrimary(t *testing.T) {
Type: topodatapb.TabletType_PRIMARY,
}

if err := wr.TopoServer().InitTablet(context.Background(), tablet1, false /*allowPrimaryOverride*/, true /*createShardAndKeyspace*/, false /*allowUpdate*/); err != nil {
t.Fatalf("InitTablet failed: %v", err)
}
err := wr.TopoServer().InitTablet(context.Background(), tablet1, false /*allowPrimaryOverride*/, true /*createShardAndKeyspace*/, false /*allowUpdate*/)
require.NoError(t, err, "InitTablet failed")

tablet2 := &topodatapb.Tablet{
Alias: &topodatapb.TabletAlias{
Expand All @@ -175,23 +161,20 @@ func TestDeleteTabletFalsePrimary(t *testing.T) {
Shard: "0",
Type: topodatapb.TabletType_PRIMARY,
}
if err := wr.TopoServer().InitTablet(context.Background(), tablet2, true /*allowPrimaryOverride*/, false /*createShardAndKeyspace*/, false /*allowUpdate*/); err != nil {
t.Fatalf("InitTablet failed: %v", err)
}
err = wr.TopoServer().InitTablet(context.Background(), tablet2, true /*allowPrimaryOverride*/, false /*createShardAndKeyspace*/, false /*allowUpdate*/)
require.NoError(t, err, "InitTablet failed")

// set PrimaryAlias and PrimaryTermStartTime on shard to match chosen primary tablet
if _, err := ts.UpdateShardFields(context.Background(), "test", "0", func(si *topo.ShardInfo) error {
_, err = ts.UpdateShardFields(context.Background(), "test", "0", func(si *topo.ShardInfo) error {
si.PrimaryAlias = tablet2.Alias
si.PrimaryTermStartTime = tablet2.PrimaryTermStartTime
return nil
}); err != nil {
t.Fatalf("UpdateShardFields failed: %v", err)
}
})
require.NoError(t, err, "UpdateShardFields failed")
Copy link
Contributor

Choose a reason for hiding this comment

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

Here as well.


// Should be able to delete old (false) primary with allowPrimary = false
if err := wr.DeleteTablet(context.Background(), tablet1.Alias, false); err != nil {
t.Fatalf("DeleteTablet failed: %v", err)
}
err = wr.DeleteTablet(context.Background(), tablet1.Alias, false)
require.NoError(t, err, "DeleteTablet failed")
Copy link
Contributor

Choose a reason for hiding this comment

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

Here too.

}

// TestDeleteTabletShardNonExisting tests that you can delete a true primary
Expand All @@ -214,29 +197,25 @@ func TestDeleteTabletShardNonExisting(t *testing.T) {
Type: topodatapb.TabletType_PRIMARY,
}

if err := wr.TopoServer().InitTablet(context.Background(), tablet, false /*allowPrimaryOverride*/, true /*createShardAndKeyspace*/, false /*allowUpdate*/); err != nil {
t.Fatalf("InitTablet failed: %v", err)
}
if _, err := ts.GetTablet(context.Background(), tablet.Alias); err != nil {
t.Fatalf("GetTablet failed: %v", err)
}
err := wr.TopoServer().InitTablet(context.Background(), tablet, false /*allowPrimaryOverride*/, true /*createShardAndKeyspace*/, false /*allowUpdate*/)
require.NoError(t, err, "InitTablet failed")

_, err = ts.GetTablet(context.Background(), tablet.Alias)
require.NoError(t, err, "GetTablet failed")
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for the additional string here.


// set PrimaryAlias and PrimaryTermStartTime on shard to match chosen primary tablet
if _, err := ts.UpdateShardFields(context.Background(), "test", "0", func(si *topo.ShardInfo) error {
_, err = ts.UpdateShardFields(context.Background(), "test", "0", func(si *topo.ShardInfo) error {
si.PrimaryAlias = tablet.Alias
si.PrimaryTermStartTime = tablet.PrimaryTermStartTime
return nil
}); err != nil {
t.Fatalf("UpdateShardFields failed: %v", err)
}
})
require.NoError(t, err, "UpdateShardFields failed")
Copy link
Contributor

Choose a reason for hiding this comment

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

No need here either.


// trigger a shard deletion
if err := ts.DeleteShard(context.Background(), "test", "0"); err != nil {
t.Fatalf("DeleteShard failed: %v", err)
}
err = ts.DeleteShard(context.Background(), "test", "0")
require.NoError(t, err, "DeleteShard failed")
Copy link
Contributor

Choose a reason for hiding this comment

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

No need here either.


// DeleteTablet should not fail if a shard no longer exist
if err := wr.DeleteTablet(context.Background(), tablet.Alias, true); err != nil {
t.Fatalf("DeleteTablet failed: %v", err)
}
err = wr.DeleteTablet(context.Background(), tablet.Alias, true)
require.NoError(t, err, "DeleteTablet failed")
}
9 changes: 3 additions & 6 deletions go/vt/wrangler/vexec_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"testing"
"time"

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

"vitess.io/vitess/go/sqltypes"
Expand Down Expand Up @@ -151,15 +152,11 @@ func TestVExec(t *testing.T) {
if testCase.errorString == "" {
require.NoError(t, err)
for _, result := range results {
if !testCase.result.Equal(result) {
t.Errorf("mismatched result:\nwant: %v\ngot: %v", testCase.result, result)
}
assert.True(t, testCase.result.Equal(result), "mismatched result:\nwant: %v\ngot: %v", testCase.result, result)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove the got:\n%+v\nwant:\n%+v output, since testify prints the same kind of output anyway.

}
} else {
require.Error(t, err)
if !strings.Contains(err.Error(), testCase.errorString) {
t.Fatalf("Wrong error, want %s, got %s", testCase.errorString, err.Error())
}
assert.Contains(t, err.Error(), testCase.errorString, "Wrong error, want %s, got %s", testCase.errorString, err.Error())
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use ErrorContains() instead.

}
})
}
Expand Down
Loading
Loading