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

chore: modernize tests #15244

Merged
merged 18 commits into from
Feb 22, 2024
Merged

chore: modernize tests #15244

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
06ba65a
chore: remove loopclosure captures from tests
Maniktherana Feb 15, 2024
3b4b428
chore: convert errorf to assert
Maniktherana Feb 15, 2024
ebcd8b9
Merge pull request #3 from vitessio/main
Maniktherana Feb 15, 2024
56e4628
chore: remove loopclosure captures from tests
Maniktherana Feb 15, 2024
790243f
chore: convert errorf to assert
Maniktherana Feb 15, 2024
292e9c5
Merge changes from origin/modernize-tests
Maniktherana Feb 15, 2024
aff9e49
chore: replace errof with assert using test-rewrite tool
Maniktherana Feb 15, 2024
3ccab1e
Merge branch 'main' into modernize-tests
Maniktherana Feb 15, 2024
fec218e
chore: replace with assert using test-rewriter + fix failing test
Maniktherana Feb 15, 2024
8f9426a
chore: remove unneeded assert messages
Maniktherana Feb 16, 2024
b8f5b3e
chore: continue remove unneeded assert messages
Maniktherana Feb 16, 2024
fed3dbc
lint
Maniktherana Feb 16, 2024
c7de6a8
chore: readd messages on test failure
Maniktherana Feb 20, 2024
d9d6bda
refac: use proper assertion methods
Maniktherana Feb 20, 2024
bc8b48c
chore: format tests
Maniktherana Feb 21, 2024
3e9e55a
refac: use assert.EqualValues instead of equal
Maniktherana Feb 21, 2024
303c47e
refac: use assert.Zero/NotZero
Maniktherana Feb 21, 2024
faadfce
refac: replace assert.equal with appropriate assertions
Maniktherana Feb 21, 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
34 changes: 10 additions & 24 deletions go/cache/lru_cache_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ package cache

import (
"testing"

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

type CacheValue struct {
Expand All @@ -27,24 +29,12 @@ type CacheValue struct {
func TestInitialState(t *testing.T) {
cache := NewLRUCache[*CacheValue](5)
l, sz, c, e, h, m := cache.Len(), cache.UsedCapacity(), cache.MaxCapacity(), cache.Evictions(), cache.Hits(), cache.Misses()
if l != 0 {
t.Errorf("length = %v, want 0", l)
}
if sz != 0 {
t.Errorf("size = %v, want 0", sz)
}
if c != 5 {
t.Errorf("capacity = %v, want 5", c)
}
if e != 0 {
t.Errorf("evictions = %v, want 0", c)
}
if h != 0 {
t.Errorf("hits = %v, want 0", c)
}
if m != 0 {
t.Errorf("misses = %v, want 0", c)
}
assert.Equal(t, 0, l)
assert.Equal(t, int64(0), sz)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick: you can use assert.EqualValue to avoid casting the literals to int64

Copy link
Contributor Author

@Maniktherana Maniktherana Feb 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

done

assert.Equal(t, int64(5), c)
assert.Equal(t, int64(0), e)
assert.Equal(t, int64(0), h)
assert.Equal(t, int64(0), m)
}

func TestSetInsertsValue(t *testing.T) {
Expand Down Expand Up @@ -137,12 +127,8 @@ func TestCapacityIsObeyed(t *testing.T) {
// Insert one more; something should be evicted to make room.
cache.Set("key4", value)
sz, evictions := cache.UsedCapacity(), cache.Evictions()
if sz != size {
t.Errorf("post-evict cache.UsedCapacity() = %v, expected %v", sz, size)
}
if evictions != 1 {
t.Errorf("post-evict cache.Evictions() = %v, expected 1", evictions)
}
assert.Equal(t, size, sz)
assert.Equal(t, int64(1), evictions)

// Check various other stats
if l := cache.Len(); int64(l) != size {
Expand Down
17 changes: 9 additions & 8 deletions go/cache/theine/singleflight_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ func TestDo(t *testing.T) {
return "bar", nil
})

assert.Equal(t, "bar (string)", fmt.Sprintf("%v (%T)", v, v), "incorrect Do value")
assert.NoError(t, err, "got Do error")
assert.Equal(t, "bar (string)", fmt.Sprintf("%v (%T)", v, v))
assert.NoError(t, err)
}

func TestDoErr(t *testing.T) {
Expand All @@ -54,8 +54,8 @@ func TestDoErr(t *testing.T) {
return "", someErr
})

assert.ErrorIs(t, err, someErr, "incorrect Do error")
assert.Empty(t, v, "unexpected non-empty value")
assert.Equal(t, someErr, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

why did we change from ErrorIs to Equal here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this may have been due to me using the test-rewriter and then resolving merge conflicts when rebasing. I'll revert this

assert.Equal(t, "", v)
Copy link
Contributor

Choose a reason for hiding this comment

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

why not assert.Empty?

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'll revert this one too

}

func TestDoDupSuppress(t *testing.T) {
Expand Down Expand Up @@ -85,11 +85,11 @@ func TestDoDupSuppress(t *testing.T) {
defer wg2.Done()
wg1.Done()
v, err, _ := g.Do("key", fn)
if !assert.NoError(t, err, "unexpected Do error") {
if !assert.NoError(t, err) {
return
}

assert.Equal(t, "bar", v, "unexpected Do value")
assert.Equal(t, "bar", v)
}()
}
wg1.Wait()
Expand All @@ -98,7 +98,7 @@ func TestDoDupSuppress(t *testing.T) {
c <- "bar"
wg2.Wait()
got := atomic.LoadInt32(&calls)
assert.True(t, got > 0 && got < n, "number of calls not between 0 and %d", n)
assert.True(t, got > 0 && got < n)
Copy link
Contributor

Choose a reason for hiding this comment

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

assert.Greater and assert.Lesser might produce more useful error messages

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, these are mostly from using test writer tool you mentioned. I'll go back and fix the tests

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah that tool isn't perfect but it's definitely a start over doing this all by hand! just needs some touching up after

}

// Test singleflight behaves correctly after Do panic.
Expand Down Expand Up @@ -131,7 +131,7 @@ func TestPanicDo(t *testing.T) {

select {
case <-done:
assert.Equal(t, int32(n), panicCount, "unexpected number of panics")
assert.Equal(t, int32(n), panicCount)
case <-time.After(time.Second):
require.Fail(t, "Do hangs")
}
Expand All @@ -152,6 +152,7 @@ func TestGoexitDo(t *testing.T) {
var err error
defer func() {
assert.NoError(t, err)

if atomic.AddInt32(&waited, -1) == 0 {
close(done)
}
Expand Down
38 changes: 10 additions & 28 deletions go/event/event_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import (
"reflect"
"testing"
"time"

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

type testInterface1 interface {
Expand Down Expand Up @@ -56,10 +58,7 @@ func TestStaticListener(t *testing.T) {
AddListener(func(testEvent1) { triggered = true })
AddListener(func(testEvent2) { t.Errorf("wrong listener type triggered") })
Dispatch(testEvent1{})

if !triggered {
t.Errorf("static listener failed to trigger")
}
assert.True(t, triggered)
Copy link
Contributor

Choose a reason for hiding this comment

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

this will read just as expected true, got false, so we might want a more descriptive message here (i.e. "expected static listener to trigger")

Copy link
Contributor

Choose a reason for hiding this comment

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

same applies to other assert.True and assert.False instances

Copy link
Member

@deepthi deepthi Feb 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Agree with @ajm188 on this. If you are up to doing a PR against the test rewriter tool, it will be good to try and preserve the custom messages.

}

func TestPointerListener(t *testing.T) {
Expand All @@ -69,10 +68,7 @@ func TestPointerListener(t *testing.T) {
AddListener(func(ev *testEvent2) { ev.triggered = true })
AddListener(func(testEvent2) { t.Errorf("non-pointer listener triggered on pointer type") })
Dispatch(testEvent)

if !testEvent.triggered {
t.Errorf("pointer listener failed to trigger")
}
assert.True(t, testEvent.triggered)
}

func TestInterfaceListener(t *testing.T) {
Expand All @@ -82,10 +78,7 @@ func TestInterfaceListener(t *testing.T) {
AddListener(func(testInterface1) { triggered = true })
AddListener(func(testInterface2) { t.Errorf("interface listener triggered on non-matching type") })
Dispatch(testEvent1{})

if !triggered {
t.Errorf("interface listener failed to trigger")
}
assert.True(t, triggered)
}

func TestEmptyInterfaceListener(t *testing.T) {
Expand All @@ -94,10 +87,8 @@ func TestEmptyInterfaceListener(t *testing.T) {
triggered := false
AddListener(func(any) { triggered = true })
Dispatch("this should match any")
assert.True(t, triggered)

if !triggered {
t.Errorf("any listener failed to trigger")
}
}

func TestMultipleListeners(t *testing.T) {
Expand Down Expand Up @@ -144,7 +135,6 @@ func TestBadListenerWrongType(t *testing.T) {

defer func() {
err := recover()

if err == nil {
t.Errorf("bad listener type (not a func) failed to trigger panic")
}
Expand Down Expand Up @@ -186,10 +176,8 @@ func TestDispatchPointerToValueInterfaceListener(t *testing.T) {
triggered = true
})
Dispatch(&testEvent1{})
assert.True(t, triggered)

if !triggered {
t.Errorf("Dispatch by pointer failed to trigger interface listener")
}
}

func TestDispatchValueToValueInterfaceListener(t *testing.T) {
Expand All @@ -200,10 +188,8 @@ func TestDispatchValueToValueInterfaceListener(t *testing.T) {
triggered = true
})
Dispatch(testEvent1{})
assert.True(t, triggered)

if !triggered {
t.Errorf("Dispatch by value failed to trigger interface listener")
}
}

func TestDispatchPointerToPointerInterfaceListener(t *testing.T) {
Expand All @@ -212,10 +198,8 @@ func TestDispatchPointerToPointerInterfaceListener(t *testing.T) {
triggered := false
AddListener(func(testInterface2) { triggered = true })
Dispatch(&testEvent2{})
assert.True(t, triggered)

if !triggered {
t.Errorf("interface listener failed to trigger for pointer")
}
}

func TestDispatchValueToPointerInterfaceListener(t *testing.T) {
Expand Down Expand Up @@ -245,10 +229,8 @@ func TestDispatchUpdate(t *testing.T) {

ev := &testUpdateEvent{}
DispatchUpdate(ev, "hello")
assert.True(t, triggered)
Copy link
Member

Choose a reason for hiding this comment

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

You missed this one, it should have a message passed in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


if !triggered {
t.Errorf("listener failed to trigger on DispatchUpdate()")
}
want := "hello"
if got := ev.update.(string); got != want {
t.Errorf("ev.update = %#v, want %#v", got, want)
Expand Down
63 changes: 21 additions & 42 deletions go/event/syslogger/syslogger_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import (
"testing"

"vitess.io/vitess/go/event"

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

type TestEvent struct {
Expand Down Expand Up @@ -70,10 +72,8 @@ func TestSyslog(t *testing.T) {

ev := new(TestEvent)
event.Dispatch(ev)
assert.True(t, ev.triggered)

if !ev.triggered {
t.Errorf("Syslog() was not called on event that implements Syslogger")
}
}

// TestBadWriter verifies we are still triggering (to normal logs) if
Expand All @@ -87,55 +87,40 @@ func TestBadWriter(t *testing.T) {
wantLevel := "ERROR"
ev := &TestEvent{priority: syslog.LOG_ALERT, message: wantMsg}
event.Dispatch(ev)
if !strings.Contains(tl.getLog().msg, wantMsg) {
t.Errorf("error log msg [%s], want msg [%s]", tl.getLog().msg, wantMsg)
}
if !strings.Contains(tl.getLog().level, wantLevel) {
t.Errorf("error log level [%s], want level [%s]", tl.getLog().level, wantLevel)
}
assert.True(t, strings.Contains(tl.getLog().msg, wantMsg))
assert.True(t, strings.Contains(tl.getLog().level, wantLevel))

ev = &TestEvent{priority: syslog.LOG_CRIT, message: wantMsg}
event.Dispatch(ev)
if !strings.Contains(tl.getLog().level, wantLevel) {
t.Errorf("error log level [%s], want level [%s]", tl.getLog().level, wantLevel)
}
assert.True(t, strings.Contains(tl.getLog().level, wantLevel))

ev = &TestEvent{priority: syslog.LOG_ERR, message: wantMsg}
event.Dispatch(ev)
if !strings.Contains(tl.getLog().level, wantLevel) {
t.Errorf("error log level [%s], want level [%s]", tl.getLog().level, wantLevel)
}
assert.True(t, strings.Contains(tl.getLog().level, wantLevel))

ev = &TestEvent{priority: syslog.LOG_EMERG, message: wantMsg}
event.Dispatch(ev)
if !strings.Contains(tl.getLog().level, wantLevel) {
t.Errorf("error log level [%s], want level [%s]", tl.getLog().level, wantLevel)
}
assert.True(t, strings.Contains(tl.getLog().level, wantLevel))

wantLevel = "WARNING"
ev = &TestEvent{priority: syslog.LOG_WARNING, message: wantMsg}
event.Dispatch(ev)
if !strings.Contains(tl.getLog().level, wantLevel) {
t.Errorf("error log level [%s], want level [%s]", tl.getLog().level, wantLevel)
}
assert.True(t, strings.Contains(tl.getLog().level, wantLevel))

wantLevel = "INFO"
ev = &TestEvent{priority: syslog.LOG_INFO, message: wantMsg}
event.Dispatch(ev)
if !strings.Contains(tl.getLog().level, wantLevel) {
t.Errorf("error log level [%s], want level [%s]", tl.getLog().level, wantLevel)
}
assert.True(t, strings.Contains(tl.getLog().level, wantLevel))

ev = &TestEvent{priority: syslog.LOG_NOTICE, message: wantMsg}
event.Dispatch(ev)
if !strings.Contains(tl.getLog().level, wantLevel) {
t.Errorf("error log level [%s], want level [%s]", tl.getLog().level, wantLevel)
}
assert.True(t, strings.Contains(tl.getLog().level, wantLevel))

ev = &TestEvent{priority: syslog.LOG_DEBUG, message: wantMsg}
event.Dispatch(ev)
if !strings.Contains(tl.getLog().level, wantLevel) {
t.Errorf("error log level [%s], want level [%s]", tl.getLog().level, wantLevel)
}
assert.True(t, strings.Contains(tl.getLog().level, wantLevel))
assert.True(t, ev.triggered)

if !ev.triggered {
t.Errorf("passed nil writer to client")
}
}

// TestWriteError checks that we don't panic on a write error.
Expand All @@ -150,24 +135,18 @@ func TestInvalidSeverity(t *testing.T) {
writer = fw

event.Dispatch(&TestEvent{priority: syslog.Priority(123), message: "log me"})
assert.NotEqual(t, "log me", fw.message)

if fw.message == "log me" {
t.Errorf("message was logged despite invalid severity")
}
}

func testSeverity(sev syslog.Priority, t *testing.T) {
fw := &fakeWriter{}
writer = fw

event.Dispatch(&TestEvent{priority: sev, message: "log me"})
assert.Equal(t, sev, fw.priority)
assert.Equal(t, "log me", fw.message)

if fw.priority != sev {
t.Errorf("wrong priority: got %v, want %v", fw.priority, sev)
}
if fw.message != "log me" {
t.Errorf(`wrong message: got "%v", want "%v"`, fw.message, "log me")
}
}

func TestEmerg(t *testing.T) {
Expand Down
10 changes: 4 additions & 6 deletions go/flagutil/flagutil_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"testing"

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

Expand All @@ -38,12 +39,9 @@ func TestStringList(t *testing.T) {
t.Errorf("v.Set(%v): %v", in, err)
continue
}
if strings.Join(p, ".") != out {
t.Errorf("want %#v, got %#v", strings.Split(out, "."), p)
}
if p.String() != in {
t.Errorf("v.String(): want %#v, got %#v", in, p.String())
}
assert.Equal(t, out, strings.Join(p, "."))
assert.Equal(t, in, p.String())

}
}

Expand Down
Loading
Loading