util/stmtsummary: migrate test-infra to testify (part 0) #26869

feitian124 · 2021-08-03T13:40:01Z

What problem does this PR solve?

Issue Number: close #26420
split from #26771, [0/2]

Release note

None

ti-chi-bot · 2021-08-03T13:40:03Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

mmyj
xhebox

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

feitian124 · 2021-08-04T15:07:05Z

/cc @tisonkun
hi, please help review, thanks

tisonkun

LGTM

ti-chi-bot · 2021-08-05T05:07:52Z

@tisonkun: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

LGTM

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.

tisonkun · 2021-08-05T05:08:12Z

/cc @xhebox @djshow832

PTAL. Thanks!

xhebox · 2021-08-05T05:21:17Z

util/stmtsummary/variables_test.go


 err := sv.setVariable(typeMaxStmtCount, "10", false)
- c.Assert(err, IsNil)
+ require.Nil(t, err)


You can use NoError.

fixed, thanks

tisonkun · 2021-08-06T09:10:28Z

/cc @mmyj

ti-chi-bot · 2021-08-06T09:10:29Z

@tisonkun: GitHub didn't allow me to request PR reviews from the following users: mmyj.

Note that only pingcap members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @mmyj

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 kubernetes/test-infra repository.

mmyj · 2021-08-06T11:09:09Z

util/stmtsummary/variables_test.go

 en = sv.getVariable(typeEnable)
- c.Assert(en > 0, Equals, false)
+ require.Equal(t, false, en > 0)


require.LessOrEqual(t, en, 0) is better?

hi， all this kind is fixed, thanks.

hi @mmyj , unit test failed due to Elements type not match, one is int64 and the other is int.
i am search testify api to see if there are some method can do the compare but ignore the int type, otherwise seems a convert is required.
if so, they orginal one seems more simple.
what your idea?

I think convert the int to int64 can resolve this issue.

ok, fixed, please check

mmyj · 2021-08-06T11:11:01Z

util/stmtsummary/variables_test.go

 en = sv.getVariable(typeEnable)
- c.Assert(en > 0, Equals, true)
+ require.Equal(t, true, en > 0)


require.Greater(t, en, 0) is better?

mmyj · 2021-08-06T11:17:32Z

util/stmtsummary/variables_test.go

 en = sv.getVariable(typeEnable)
- c.Assert(en > 0, Equals, false)
+ require.Equal(t, false, en > 0)


mmyj · 2021-08-06T11:17:40Z

util/stmtsummary/variables_test.go

 en = sv.getVariable(typeEnable)
- c.Assert(en > 0, Equals, true)
+ require.Equal(t, true, en > 0)


mmyj · 2021-08-06T11:17:59Z

util/stmtsummary/variables_test.go

 en = sv.getVariable(typeEnable)
- c.Assert(en > 0, Equals, true)
+ require.Equal(t, true, en > 0)


mmyj · 2021-08-06T11:18:05Z

util/stmtsummary/variables_test.go

 en = sv.getVariable(typeEnable)
- c.Assert(en > 0, Equals, false)
+ require.Equal(t, false, en > 0)


mmyj · 2021-08-06T11:18:13Z

util/stmtsummary/variables_test.go

 en = sv.getVariable(typeEnable)
- c.Assert(en > 0, Equals, true)
+ require.Equal(t, true, en > 0)


mmyj · 2021-08-06T11:22:32Z

util/stmtsummary/variables_test.go


 err := sv.setVariable(typeMaxStmtCount, "10", false)
- c.Assert(err, IsNil)
+ require.NoError(t, err)
 st = sv.getVariable(typeMaxStmtCount)


It seems that we don't need the var st. How about the following code?

require.Equal(t, int64(10), sv.getVariable(typeMaxStmtCount))

thank you for your suggestion.
please forgive me，but i think keep the var st there is more clear and more consistent with the err part

err = sv.setVariable(typeEnable, "OFF", false) require.NoError(t, err)

another reason is that this is a migrate, less change may let the whole job faster done and less mistake injected.

mmyj · 2021-08-06T11:22:42Z

util/stmtsummary/variables_test.go


 err := sv.setVariable(typeEnable, "OFF", false)
- c.Assert(err, IsNil)
+ require.NoError(t, err)
 en = sv.getVariable(typeEnable)


mmyj · 2021-08-06T11:22:59Z

util/stmtsummary/variables_test.go

 sv := newSysVars()
 err := sv.setVariable(typeMaxStmtCount, "0", false)
- c.Assert(err, IsNil)
+ require.NoError(t, err)
 v := sv.getVariable(typeMaxStmtCount)


mmyj · 2021-08-06T13:35:33Z

util/stmtsummary/variables_test.go

@@ -65,31 +65,31 @@ func TestSetBoolVariable(t *testing.T) {
 err := sv.setVariable(typeEnable, "OFF", false)
 require.NoError(t, err)
 en = sv.getVariable(typeEnable)
- require.Equal(t, false, en > 0)
+ require.LessOrEqual(t, en, 0 )


mmyj · 2021-08-06T13:35:42Z

util/stmtsummary/variables_test.go

 err = sv.setVariable(typeEnable, "OFF", true)
 require.NoError(t, err)
 en = sv.getVariable(typeEnable)
- require.Equal(t, false, en > 0)
+ require.LessOrEqual(t, en, 0 )


mmyj · 2021-08-06T13:35:48Z

util/stmtsummary/variables_test.go

 err = sv.setVariable(typeEnable, "", true)
 require.NoError(t, err)
 en = sv.getVariable(typeEnable)
- require.Equal(t, false, en > 0)
+ require.LessOrEqual(t, en, 0 )


mmyj

lgtm

mmyj · 2021-08-06T14:22:54Z

/merge

ti-chi-bot · 2021-08-06T14:22:57Z

This pull request has been accepted and is ready to merge.

Commit hash: 2be4a6f

ti-chi-bot · 2021-08-06T14:23:07Z

@feitian124: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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.

feitian124 · 2021-08-06T14:55:42Z

/run-check_dev_2

util/stmtsummary: migrate test-infra to testify (part 0)

a31346d

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 3, 2021

feitian124 mentioned this pull request Aug 3, 2021

util/stmtsummary: migrate test-infra to testify (part 1) #26870

Merged

ti-chi-bot requested a review from tisonkun August 4, 2021 15:07

tisonkun approved these changes Aug 5, 2021

View reviewed changes

ti-chi-bot requested review from djshow832 and xhebox August 5, 2021 05:08

xhebox reviewed Aug 5, 2021

View reviewed changes

use NoError replace Nil(t, err)

65fb97a

xhebox approved these changes Aug 6, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 6, 2021

mmyj reviewed Aug 6, 2021

View reviewed changes

use more straight checker: greater and less

fc74f19

mmyj reviewed Aug 6, 2021

View reviewed changes

feitian124 added 2 commits August 6, 2021 22:06

go fmt

24892e3

fix type mismatch issue

2be4a6f

mmyj approved these changes Aug 6, 2021

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 6, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Aug 6, 2021

Merge branch 'master' into issue-26420-part0

4434dae

ti-chi-bot merged commit b8107d7 into pingcap:master Aug 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

util/stmtsummary: migrate test-infra to testify (part 0) #26869

util/stmtsummary: migrate test-infra to testify (part 0) #26869

feitian124 commented Aug 3, 2021

ti-chi-bot commented Aug 3, 2021 •

edited

Loading

feitian124 commented Aug 4, 2021

tisonkun left a comment

ti-chi-bot commented Aug 5, 2021

tisonkun commented Aug 5, 2021

xhebox Aug 5, 2021

feitian124 Aug 5, 2021

tisonkun commented Aug 6, 2021

ti-chi-bot commented Aug 6, 2021

mmyj Aug 6, 2021

feitian124 Aug 6, 2021

feitian124 Aug 6, 2021

mmyj Aug 6, 2021

feitian124 Aug 6, 2021

mmyj Aug 6, 2021

mmyj Aug 6, 2021

mmyj Aug 6, 2021

mmyj Aug 6, 2021

mmyj Aug 6, 2021

mmyj Aug 6, 2021

mmyj Aug 6, 2021

feitian124 Aug 6, 2021

mmyj Aug 6, 2021

mmyj Aug 6, 2021

mmyj Aug 6, 2021

mmyj Aug 6, 2021

mmyj Aug 6, 2021

mmyj Aug 6, 2021

mmyj left a comment

mmyj commented Aug 6, 2021

ti-chi-bot commented Aug 6, 2021

ti-chi-bot commented Aug 6, 2021

feitian124 commented Aug 6, 2021

util/stmtsummary: migrate test-infra to testify (part 0) #26869

util/stmtsummary: migrate test-infra to testify (part 0) #26869

Conversation

feitian124 commented Aug 3, 2021

What problem does this PR solve?

Release note

ti-chi-bot commented Aug 3, 2021 • edited Loading

feitian124 commented Aug 4, 2021

tisonkun left a comment

Choose a reason for hiding this comment

ti-chi-bot commented Aug 5, 2021

tisonkun commented Aug 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tisonkun commented Aug 6, 2021

ti-chi-bot commented Aug 6, 2021

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

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

mmyj left a comment

Choose a reason for hiding this comment

mmyj commented Aug 6, 2021

ti-chi-bot commented Aug 6, 2021

ti-chi-bot commented Aug 6, 2021

feitian124 commented Aug 6, 2021

ti-chi-bot commented Aug 3, 2021 •

edited

Loading