util/filesort: migrate test-infra to testify #26946
Conversation
|
[REVIEW NOTIFICATION] This pull request has not been approved. To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
|
Welcome @kevgeo! |
| @@ -0,0 +1,13 @@ | |||
| package filesort | |||
There was a problem hiding this comment.
We need a license header here. See other main_test.go file as an example.
There was a problem hiding this comment.
Thank you for your kind feedback. I will update with the license header.
| fsBuilder.SetSchema(keySize, valSize) | ||
| _, err = fsBuilder.Build() | ||
| c.Assert(err.Error(), Equals, "buffer size is not positive") | ||
| require.Equal(t, "buffer size is not positive", err.Error()) |
There was a problem hiding this comment.
You can use EqualError here also.
There was a problem hiding this comment.
EqualError expects the second argument to be of error type and third argument as string.
err.Error() is of string type. So since both second argument i.e "buffer size is not positive" and third argument, err.Error() are of string type, I'm using Equal.
There was a problem hiding this comment.
If you take a look at how require.EqualError implements, you will found
require.Equal(t, "buffer size is not positive", err.Error())
// equals to
require.EqualError(t, err, "buffer size is not positive")... with extra check err is not nil so you won't panic and IDE like GoLand won't complain about nil pointer potential.
| ret, err := lessThan(sc, t.Arg1, t.Arg2, t.Arg3) | ||
| c.Assert(err, IsNil) | ||
| c.Assert(ret, Equals, t.Ret) | ||
| for _, T := range tblTwoColumns { |
There was a problem hiding this comment.
You may try to name the struct as testCases, iteratee as testCase, for expressive naming.
testCases := []struct {
datum0 []types.Datum
datum1 []types.Datum
byDesc []bool
res bool
}There was a problem hiding this comment.
ditto above, you can join these two cases.
There was a problem hiding this comment.
Should I try to differentiate the two different struct testCases by naming the first and second as testCasesTblOneColumn and testCasesTblTwoColumns respectively?
There was a problem hiding this comment.
You may think of it. My opinion is that they can be the same testCases array.
Preciously, I mean something like
func TestLessThan(t *testing.T) {
t.Parallel()
sc := new(stmtctx.StatementContext)
d0 := types.NewDatum(0)
d1 := types.NewDatum(1)
tests := []struct {
i []types.Datum
j []types.Datum
byDesc []bool
res bool
}{
// one column
{[]types.Datum{d0}, []types.Datum{d0}, []bool{false}, false},
{[]types.Datum{d0}, []types.Datum{d1}, []bool{false}, true},
{[]types.Datum{d1}, []types.Datum{d0}, []bool{false}, false},
{[]types.Datum{d0}, []types.Datum{d0}, []bool{true}, false},
{[]types.Datum{d0}, []types.Datum{d1}, []bool{true}, false},
{[]types.Datum{d1}, []types.Datum{d0}, []bool{true}, true},
// two columns
{[]types.Datum{d0, d0}, []types.Datum{d1, d1}, []bool{false, false}, true},
{[]types.Datum{d0, d1}, []types.Datum{d1, d1}, []bool{false, false}, true},
{[]types.Datum{d0, d0}, []types.Datum{d1, d1}, []bool{false, false}, true},
{[]types.Datum{d0, d0}, []types.Datum{d0, d1}, []bool{false, false}, true},
{[]types.Datum{d0, d1}, []types.Datum{d0, d1}, []bool{false, false}, false},
{[]types.Datum{d0, d1}, []types.Datum{d0, d0}, []bool{false, false}, false},
{[]types.Datum{d1, d0}, []types.Datum{d0, d1}, []bool{false, false}, false},
{[]types.Datum{d1, d1}, []types.Datum{d0, d1}, []bool{false, false}, false},
{[]types.Datum{d1, d1}, []types.Datum{d0, d0}, []bool{false, false}, false},
}
for _, test := range tests {
res, err := lessThan(sc, test.i, test.j, test.byDesc)
require.NoError(t, err)
require.Equal(t, test.res, res)
}
}|
@tisonkun: Request changes is only allowed for the reviewers in list. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
|
|
||
| fsBuilder := new(Builder) | ||
| fs, err = fsBuilder.SetSC(sc).SetSchema(keySize, valSize).SetBuf(bufSize).SetWorkers(1).SetDesc(byDesc).SetDir(tmpDir).Build() | ||
| c.Assert(err, IsNil) | ||
| require.NoError(t, err) | ||
| defer fs.Close() |
There was a problem hiding this comment.
try to handle the error by
defer func() {
err := fs.Close()
require.NoError(t, err)
}()|
As @kevgeo inactive for a while, I'll take this PR over. |
What problem does this PR solve?
Issue Number: close #26178
Problem Summary: refer to #26022
Release note