expression: migrate test-infra to testify for expression/constant_test.go (#28854) #28895

agneum · 2021-10-17T17:35:25Z

What problem does this PR solve?

Issue Number: close #28854, #29140, #29138

Problem Summary:

What is changed and how it works?

What's Changed:

Migrate test-infra to testify for:

Tests

Unit test

Release note

None

…stant_test.go (pingcap#28854)

ti-chi-bot · 2021-10-17T17:35:26Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

tisonkun
xiongjiwei

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.

ti-chi-bot · 2021-10-17T17:35:36Z

Welcome @agneum!

It looks like this is your first PR to pingcap/tidb 🎉.

I'm the bot to help you request reviewers, add labels and more, See available commands.

We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to pingcap/tidb. 😃

agneum · 2021-10-17T17:43:55Z

/cc @Reviewer

ti-chi-bot · 2021-10-17T17:43:56Z

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

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

In response to this:

/cc @Reviewer

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.

agneum · 2021-10-20T02:33:48Z

/cc @tisonkun

agneum · 2021-10-25T16:10:15Z

@tisonkun Could you please review the PR?

tisonkun · 2021-10-25T16:57:06Z

expression/constant_test.go

 	"time"

-	. "github.com/pingcap/check"
+	"github.com/stretchr/testify/assert"


please try to use require consistently

tisonkun · 2021-10-25T16:57:39Z

expression/constant_test.go

@@ -56,7 +55,7 @@ func newFunction(funcName string, args ...Expression) Expression {
 	return NewFunctionInternal(mock.NewContext(), funcName, typeLong, args...)
 }

-func (*testExpressionSuite) TestConstantPropagation(c *C) {
+func TestConstantPropagation(t *testing.T) {
 	tests := []struct {
 		solver     []PropagateConstantSolver
 		conditions []Expression


please verify running these tests in parallel with t.Parallel(). Otherwise rename the file as xxx_serial_test.go

ditto others

tisonkun · 2021-10-25T16:57:59Z

expression/constant_test.go

@@ -177,12 +176,12 @@ func (*testExpressionSuite) TestConstantPropagation(c *C) {
 				result = append(result, v.String())
 			}
 			sort.Strings(result)
-			c.Assert(strings.Join(result, ", "), Equals, tt.result, Commentf("different for expr %s", tt.conditions))
+			assert.Equal(t, tt.result, strings.Join(result, ", "), fmt.Sprintf("different for expr %s", tt.conditions))


require.Equalf will help.

tisonkun · 2021-10-25T16:58:36Z

expression/constant_test.go


 	d, _, err := cstInt.EvalInt(ctx, chunk.Row{})
-	c.Assert(err, IsNil)
-	c.Assert(d, Equals, int64(1))
+	require.Nil(t, err)


require.NoError(t, err)

ditto others.

tisonkun · 2021-10-25T16:59:10Z

expression/constant_test.go

 	m := &MockExpr{}
 	ctx := mock.NewContext()
 	cst := &Constant{DeferredExpr: m, RetType: newIntFieldType()}
 	m.i, m.err = nil, fmt.Errorf("ERROR")
 	_, _, err := cst.EvalInt(ctx, chunk.Row{})
-	c.Assert(err, NotNil)
+	assert.NotNil(t, err)


require.Error(t, err), ditto others

tisonkun · 2021-10-25T17:00:23Z

Comments inline. Generally three suggestions:

use require
try to run in parallel
precise assertions

agneum · 2021-10-26T16:29:04Z

Comments inline. Generally three suggestions:
1. use `require`

2. try to run in parallel

3. precise assertions

@tisonkun Thanks for the review. I've fixed these comments

tisonkun

LGTM. Please also assign #29140 #29138 to yourself and update the PR description to close these two issues.

tisonkun · 2021-10-27T00:37:56Z

/cc @xiongjiwei

agneum · 2021-10-27T01:48:24Z

LGTM. Please also assign #29140 #29138 to yourself and update the PR description to close these two issues.

Thanks, done

xiongjiwei · 2021-10-27T02:07:16Z

/merge

ti-chi-bot · 2021-10-27T02:07:19Z

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

Commit hash: 1c6941a

ti-chi-bot · 2021-10-27T02:14:58Z

@agneum: 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.

expression/constant: migrate test-infra to testify for expression/con…

3fa1cde

…stant_test.go (pingcap#28854)

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 17, 2021

tisonkun self-requested a review October 25, 2021 16:56

tisonkun reviewed Oct 25, 2021

View reviewed changes

tisonkun mentioned this pull request Oct 25, 2021

expression: migrate test-infra to testify for expression/collation_test.go (#28849) #28965

Merged

1 task

agneum added 3 commits October 26, 2021 22:55

fix after review: use require, run tests in parallel, precise assertions

f49a848

use require consistently

af19da6

use require in helper and rand tests

1c6941a

tisonkun approved these changes Oct 27, 2021

View reviewed changes

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

ti-chi-bot requested a review from xiongjiwei October 27, 2021 00:37

xiongjiwei approved these changes Oct 27, 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 Oct 27, 2021

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

ti-chi-bot added 2 commits October 27, 2021 10:07

Merge branch 'master' into 28854-migrate-constant-tests-to-testify

db72222

Merge branch 'master' into 28854-migrate-constant-tests-to-testify

32d504c

ti-chi-bot merged commit bfe344c into pingcap:master Oct 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expression: migrate test-infra to testify for expression/constant_test.go (#28854) #28895

expression: migrate test-infra to testify for expression/constant_test.go (#28854) #28895

agneum commented Oct 17, 2021 •

edited by tisonkun

Loading

ti-chi-bot commented Oct 17, 2021 •

edited

Loading

ti-chi-bot commented Oct 17, 2021

agneum commented Oct 17, 2021

ti-chi-bot commented Oct 17, 2021

agneum commented Oct 20, 2021

agneum commented Oct 25, 2021

tisonkun Oct 25, 2021

tisonkun Oct 25, 2021

tisonkun Oct 25, 2021

tisonkun Oct 25, 2021

tisonkun Oct 25, 2021

tisonkun Oct 25, 2021

tisonkun commented Oct 25, 2021

agneum commented Oct 26, 2021

tisonkun left a comment

tisonkun commented Oct 27, 2021

agneum commented Oct 27, 2021

xiongjiwei commented Oct 27, 2021

ti-chi-bot commented Oct 27, 2021

ti-chi-bot commented Oct 27, 2021

expression: migrate test-infra to testify for expression/constant_test.go (#28854) #28895

expression: migrate test-infra to testify for expression/constant_test.go (#28854) #28895

Conversation

agneum commented Oct 17, 2021 • edited by tisonkun Loading

What problem does this PR solve?

What is changed and how it works?

Release note

ti-chi-bot commented Oct 17, 2021 • edited Loading

ti-chi-bot commented Oct 17, 2021

agneum commented Oct 17, 2021

ti-chi-bot commented Oct 17, 2021

agneum commented Oct 20, 2021

agneum commented Oct 25, 2021

tisonkun Oct 25, 2021

Choose a reason for hiding this comment

tisonkun Oct 25, 2021

Choose a reason for hiding this comment

tisonkun Oct 25, 2021

Choose a reason for hiding this comment

tisonkun Oct 25, 2021

Choose a reason for hiding this comment

tisonkun Oct 25, 2021

Choose a reason for hiding this comment

tisonkun Oct 25, 2021

Choose a reason for hiding this comment

tisonkun commented Oct 25, 2021

agneum commented Oct 26, 2021

tisonkun left a comment

Choose a reason for hiding this comment

tisonkun commented Oct 27, 2021

agneum commented Oct 27, 2021

xiongjiwei commented Oct 27, 2021

ti-chi-bot commented Oct 27, 2021

ti-chi-bot commented Oct 27, 2021

agneum commented Oct 17, 2021 •

edited by tisonkun

Loading

ti-chi-bot commented Oct 17, 2021 •

edited

Loading