chore: modernize tests #15244

Maniktherana · 2024-02-15T08:28:05Z

Description

Used test writer to convert t.errorf to assert

Related Issue(s)

addresses #15183
partially covers #15182

Checklist

"Backport to:" labels have been added if this change should be back-ported to release branches
If this change is to be back-ported to previous releases, a justification is included in the PR description
Tests were added or are not required
Did the new or modified tests pass consistently locally and on CI?
Documentation was added or is not required

Deployment Notes

Signed-off-by: Manik Rana <manikrana54@gmail.com>

vitess-bot · 2024-02-15T08:28:07Z

codecov · 2024-02-15T08:47:27Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (de3c9c5) 67.46% compared to head (faadfce) 67.54%.
Report is 19 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #15244      +/-   ##
==========================================
+ Coverage   67.46%   67.54%   +0.07%     
==========================================
  Files        1560     1561       +1     
  Lines      193186   193387     +201     
==========================================
+ Hits       130333   130618     +285     
+ Misses      62853    62769      -84

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Manik Rana <manikrana54@gmail.com>

Maniktherana · 2024-02-15T11:38:00Z

hey @ajm188 I tried to add the new range in some of the testfiles but on commiting, the linter's complaining with this output:

git commit -m "chore: use new range for loops"
2024/02/15 17:04:03 5 files OK
Linting go/bucketpool go/cache/theine go/mysql go/mysql/endtoend go/pools go/pools/smartconnpool
go/bucketpool/bucketpool_test.go:181:12: cannot range over 20000 (untyped int constant) (typecheck)
        for range 20000 {
                  ^

not sure if the issue's with CI or with my machine as I've been having some issues setting up go1.22

ajm188 · 2024-02-15T12:52:14Z

not sure if the issue's with CI or with my machine as I've been having some issues setting up go1.22

if you aren't on at least 1.22 this syntax is invalid, so that is probably the issue. what is the issue you are having setting up on your machine? i'm happy to help!

Maniktherana · 2024-02-15T12:55:02Z

I did install go1.22 and it's now the default go version on my machine. These errors don't show up in vscode. Only shows up when I try to commit via my terminal 🤔

As for my setup I'm having issues with the Go lsp on vscode but I think that's unrelated.

ajm188 · 2024-02-15T13:00:08Z

I just tried applying that change and didn't have a problem:

$ git commit -sS -m 'test commit'
2024/02/15 07:58:42 5 files OK
Linting go/bucketpool
[andrew/delete-vtctl-commands 12a6beabaa] test commit
 1 file changed, 1 insertion(+), 1 deletion(-)
$ git show HEAD -p
commit 12a6beabaa1dfdff4e2470871d4725fcea8c9203
Author: Andrew Mason <andrew@planetscale.com>
Date:   Thu Feb 15 07:58:39 2024 -0500

    test commit
    
    Signed-off-by: Andrew Mason <andrew@planetscale.com>

diff --git a/go/bucketpool/bucketpool_test.go b/go/bucketpool/bucketpool_test.go
index cc8bcff0ee..6f088caddc 100644
--- a/go/bucketpool/bucketpool_test.go
+++ b/go/bucketpool/bucketpool_test.go
@@ -178,7 +178,7 @@ func TestPoolWeirdMaxSize(t *testing.T) {
 
 func TestFuzz(t *testing.T) {
 	maxTestSize := 16384
-	for i := 0; i < 20000; i++ {
+	for range 20000 {
 		minSize := rand.Intn(maxTestSize)
 		if minSize == 0 {
 			minSize = 1

maybe you need to update golangci-lint after the go upgrade?

sync with main

ajm188 · 2024-02-15T13:28:19Z

@Maniktherana i think you've overlapped with some work i did here: #15194 you might want to rebase on that

Signed-off-by: Manik Rana <manikrana54@gmail.com>

Maniktherana · 2024-02-15T14:00:28Z

@Maniktherana i think you've overlapped with some work i did here: #15194 you might want to rebase on that

done

Signed-off-by: Manik Rana <Manikrana54@gmail.com>

frouioui · 2024-02-15T15:02:38Z

go/cache/lru_cache_test.go

+	assert.Equal(t, 0, l, "length = %v, want 0", l)
+	assert.Equal(t, int64(0), sz, "size = %v, want 0", sz)
+	assert.Equal(t, int64(5), c, "capacity = %v, want 5", c)


When using the asset I don't think it is useful to add the expectations in the message field. The method already prints the wanted value and the received value:

Error: Not equal: expected: 1 actual : 0 Test: TestInitialState Messages: size = 0, want 1

This apply to all the files you modified in this PR

done, I'll wait on the the CI checks tho

Signed-off-by: Manik Rana <Manikrana54@gmail.com>

Signed-off-by: Manik Rana <manikrana54@gmail.com>

ajm188

looking good, pending previous review

ajm188 · 2024-02-15T22:05:55Z

go/cache/theine/singleflight_test.go

@@ -45,6 +45,7 @@ func TestDo(t *testing.T) {

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


on your next pass you should ditch this extra line

Signed-off-by: Manik Rana <manikrana54@gmail.com>

ajm188 · 2024-02-16T16:37:44Z

go/cache/theine/singleflight_test.go

@@ -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)


why did we change from ErrorIs to Equal here?

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

ajm188 · 2024-02-16T16:37:55Z

go/cache/theine/singleflight_test.go

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


why not assert.Empty?

I'll revert this one too

ajm188 · 2024-02-16T16:38:37Z

go/cache/theine/singleflight_test.go

@@ -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)


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

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

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

ajm188 · 2024-02-16T16:40:01Z

go/event/event_test.go

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


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")

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

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.

deepthi

Nice work. I can approve once the current set of comments are addressed. I didn't see anything else that needs to change.

deepthi · 2024-02-20T05:10:29Z

go/event/event_test.go

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


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.

deepthi · 2024-02-20T05:11:01Z

go/streamlog/streamlog_test.go

@@ -315,7 +315,6 @@ func TestShouldEmitLog(t *testing.T) {
 	}

 	for _, tt := range tests {
-		tt := tt


Signed-off-by: Manik Rana <manikrana54@gmail.com>

Maniktherana · 2024-02-20T06:36:59Z

I've refactored the tests @deepthi @ajm188
this should cover it

deepthi

I found a couple of small things that still need to fixed. Will give final approval after that.

deepthi · 2024-02-21T03:52:26Z

go/event/event_test.go

@@ -245,10 +227,8 @@ func TestDispatchUpdate(t *testing.T) {

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


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

deepthi · 2024-02-21T03:53:24Z

go/mysql/decimal/decimal_test.go

+		assert.
+			// rule 1: d = d2*q +r
+			True(t, d.Equal(d2.mul(q).Add(r)))


Should fix the formatting. Comment should go on the line above the assertion.

Signed-off-by: Manik Rana <manikrana54@gmail.com>

systay · 2024-02-21T07:20:22Z

go/cache/lru_cache_test.go

-		t.Errorf("misses = %v, want 0", c)
-	}
+	assert.Equal(t, 0, l)
+	assert.Equal(t, int64(0), sz)


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

systay · 2024-02-21T07:21:46Z

go/mysql/decimal/decimal_test.go

-	if Zero.Sign() != 0 {
-		t.Errorf("%q should have sign 0", Zero)
-	}
+	assert.Equal(t, 0, Zero.Sign())


nitpick: assert.Zero

systay · 2024-02-21T07:22:47Z

go/mysql/decimal/decimal_test.go

-	if c.Cmp(a) == 0 {
-		t.Errorf("error")
-	}
+	assert.NotEqual(t, 0, c.Cmp(a))


nitpick: assert.NotZero

Signed-off-by: Manik Rana <manikrana54@gmail.com>

ajm188

i'm on-board with the assert.True messages and the Greater/Less, but i have questions about the Zero and EqualValues

ajm188 · 2024-02-21T13:03:25Z

go/cache/lru_cache_test.go

+	assert.Zero(t, l)
+	assert.EqualValues(t, 0, sz)
+	assert.EqualValues(t, 5, c)
+	assert.EqualValues(t, 0, e)
+	assert.EqualValues(t, 0, h)
+	assert.EqualValues(t, 0, m)


why did we make these changes?

I received a review earlier to use EqualValues to avoid typecasting and Zero/NotZero. I thought it would apply to other portions of the PR such as here

Sorry @Maniktherana I completely missed those review comments. I'm good to merge!

chore: remove loopclosure captures from tests

06ba65a

Signed-off-by: Manik Rana <manikrana54@gmail.com>

Maniktherana requested a review from deepthi as a code owner February 15, 2024 08:28

Maniktherana marked this pull request as draft February 15, 2024 08:28

github-actions bot added this to the v20.0.0 milestone Feb 15, 2024

chore: convert errorf to assert

3b4b428

Signed-off-by: Manik Rana <manikrana54@gmail.com>

Maniktherana changed the title ~~chore: remove loopclosure captures from tests~~ chore: modernize tests Feb 15, 2024

Merge pull request #3 from vitessio/main

ebcd8b9

sync with main

Maniktherana added 3 commits February 15, 2024 19:20

chore: remove loopclosure captures from tests

56e4628

Signed-off-by: Manik Rana <manikrana54@gmail.com>

chore: convert errorf to assert

790243f

Signed-off-by: Manik Rana <manikrana54@gmail.com>

Merge changes from origin/modernize-tests

292e9c5

Signed-off-by: Manik Rana <manikrana54@gmail.com>

chore: replace errof with assert using test-rewrite tool

aff9e49

Signed-off-by: Manik Rana <Manikrana54@gmail.com>

Maniktherana marked this pull request as ready for review February 15, 2024 14:31

Maniktherana requested review from harshit-gangal, systay, mattlord and vmg as code owners February 15, 2024 14:31

frouioui reviewed Feb 15, 2024

View reviewed changes

frouioui removed the NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work label Feb 15, 2024

Maniktherana and others added 2 commits February 15, 2024 23:09

Merge branch 'main' into modernize-tests

3ccab1e

Signed-off-by: Manik Rana <Manikrana54@gmail.com>

chore: replace with assert using test-rewriter + fix failing test

fec218e

Signed-off-by: Manik Rana <manikrana54@gmail.com>

ajm188 reviewed Feb 15, 2024

View reviewed changes

Maniktherana added 3 commits February 16, 2024 09:48

chore: remove unneeded assert messages

8f9426a

Signed-off-by: Manik Rana <manikrana54@gmail.com>

chore: continue remove unneeded assert messages

b8f5b3e

Signed-off-by: Manik Rana <manikrana54@gmail.com>

lint

fed3dbc

Signed-off-by: Manik Rana <manikrana54@gmail.com>

ajm188 reviewed Feb 16, 2024

View reviewed changes

deepthi reviewed Feb 20, 2024

View reviewed changes

Maniktherana added 2 commits February 20, 2024 11:58

chore: readd messages on test failure

c7de6a8

Signed-off-by: Manik Rana <manikrana54@gmail.com>

refac: use proper assertion methods

d9d6bda

Signed-off-by: Manik Rana <manikrana54@gmail.com>

ajm188 approved these changes Feb 20, 2024

View reviewed changes

deepthi reviewed Feb 21, 2024

View reviewed changes

chore: format tests

bc8b48c

Signed-off-by: Manik Rana <manikrana54@gmail.com>

systay reviewed Feb 21, 2024

View reviewed changes

Maniktherana added 3 commits February 21, 2024 12:59

refac: use assert.EqualValues instead of equal

3e9e55a

Signed-off-by: Manik Rana <manikrana54@gmail.com>

refac: use assert.Zero/NotZero

303c47e

Signed-off-by: Manik Rana <manikrana54@gmail.com>

refac: replace assert.equal with appropriate assertions

faadfce

Signed-off-by: Manik Rana <manikrana54@gmail.com>

ajm188 reviewed Feb 21, 2024

View reviewed changes

deepthi approved these changes Feb 22, 2024

View reviewed changes

ajm188 merged commit 6fec119 into vitessio:main Feb 22, 2024
102 checks passed

		@@ -45,6 +45,7 @@ func TestDo(t *testing.T) {

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

chore: modernize tests #15244

chore: modernize tests #15244

Conversation

Maniktherana commented Feb 15, 2024 • edited Loading

Description

Related Issue(s)

Checklist

Deployment Notes

vitess-bot bot commented Feb 15, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

codecov bot commented Feb 15, 2024 • edited Loading

Codecov Report

Maniktherana commented Feb 15, 2024 • edited Loading

ajm188 commented Feb 15, 2024

Maniktherana commented Feb 15, 2024

ajm188 commented Feb 15, 2024

ajm188 commented Feb 15, 2024

Maniktherana commented Feb 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajm188 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepthi Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

deepthi left a comment

Choose a reason for hiding this comment

deepthi Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Maniktherana commented Feb 20, 2024

deepthi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Maniktherana Feb 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajm188 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Maniktherana commented Feb 15, 2024 •

edited

Loading

codecov bot commented Feb 15, 2024 •

edited

Loading

Maniktherana commented Feb 15, 2024 •

edited

Loading

deepthi Feb 20, 2024 •

edited

Loading

deepthi Feb 20, 2024 •

edited

Loading

Maniktherana Feb 21, 2024 •

edited

Loading