util/decoder: migrate test-infra to testify #26629

feitian124 · 2021-07-27T04:37:30Z

What problem does this PR solve?

Issue Number: close #26414

Release note

None

ti-chi-bot · 2021-07-27T04:37:31Z

[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-07-27T05:27:41Z

/cc @tisonkun
please help review, thanks

tisonkun

Generally looks good. Comment inline that you can extract the method in this pass. Ask me for help if you need it.

tisonkun · 2021-07-27T08:00:50Z

util/rowDecoder/decoder_test.go

 		}
 	}
 }
+
+// TODO: local copy of testutil.MustNewCommonHandle，should use function in testutil after it migrate to testify
+func MustNewCommonHandle(t *testing.T, values ...interface{}) kv.Handle {


You can just create a file named, for example, "handle.go" under "testkit" and do the todo a favor.

sure, a little busy now hope finish before bed

Thanks ❤️

sorry, i met some problems and not sure how to resolve.

decoder_test.go unit test failed for goleak

goleak: Errors on successful test run: found unexpected goroutines: [Goroutine 21 in state select, with go.opencensus.io/stats/view.(*worker).start on top of the stack: goroutine 21 [select]: go.opencensus.io/stats/view.(*worker).start(0xc000162410) /home/ming/go/pkg/mod/[email protected]/stats/view/worker.go:154 +0xcd created by go.opencensus.io/stats/view.init.0 /home/ming/go/pkg/mod/[email protected]/stats/view/worker.go:32 +0x57

key_test.go and keydecoder_test.go have cycle import with util/testkit/handle.go

# github.com/pingcap/tidb/kv FAIL github.com/pingcap/tidb/kv [setup failed] package github.com/pingcap/tidb/kv (test) imports github.com/pingcap/tidb/util/testkit imports github.com/pingcap/tidb/domain FAIL imports github.com/pingcap/tidb/bindinfo imports github.com/pingcap/tidb/sessionctx imports github.com/pingcap/tidb/kv: import cycle not allowed in test

i need some dig to resolve the 2 problems, any suggestion is appreciated

For goleak, it is a known goroutine that doesn't exit, you can ignore by

opts := []goleak.Option{ goleak.IgnoreTopFunction("go.etcd.io/etcd/pkg/logutil.(*MergeLogger).outputLoop"), goleak.IgnoreTopFunction("go.opencensus.io/stats/view.(*worker).start"), } goleak.VerifyTestMain(m, opts...)

for cycle import, you can try to change package keydecoder to package keydecoder_test

thanks, helps a lot

updated, please review again, thanks

ti-chi-bot · 2021-07-27T08:01:39Z

@tisonkun: Request changes is only allowed for the reviewers in list.

In response to this:

Generally looks good. Comment inline that you can extract the method in this pass. Ask me for help if you need it.

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.

util/testkit/handle.go

Signed-off-by: tison <[email protected]>

tisonkun

LGTM

ti-chi-bot · 2021-07-27T23:17:40Z

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

Signed-off-by: tison <[email protected]>

feitian124 · 2021-07-29T03:08:13Z

hello @tisonkun , anything else need me to do to merge this pr?

tisonkun · 2021-07-29T03:12:43Z

@feitian124 thanks for ping me. I'll ping other reviewers for you.

/cc @xhebox @tiancaiamao @mmyj

mmyj

lgtm

tisonkun · 2021-07-29T05:13:47Z

@mmyj may you help on merging as there are 2 approvals?

mmyj · 2021-07-29T05:14:36Z

/merge

ti-chi-bot · 2021-07-29T05:14:40Z

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

Commit hash: 8b1e4a0

feitian124 · 2021-07-29T06:06:09Z

ci failed, i checked a few latest pr merged to this branch and try run unit test(i did not check all the prs):

statistics/handle/update_test.go statistics: trigger auto-analyze based on histogram row count #24382

----------------------------------------------------------------------
FAIL: handle_test.go:2984: testStatsSuite.TestIncrementalModifyCountUpdate

handle_test.go:3024:
    // Check the count / modify_count changes during the analyze are not lost.
    tk.MustQuery(fmt.Sprintf("select count, modify_count from mysql.stats_meta where table_id = %d", tid)).Check(testkit.Rows(
        "6 3",
    ))
/home/ming/learn/go/pingcap_projects/tidb/util/testkit/testkit.go:63:
    res.c.Assert(resBuff.String(), check.Equals, needBuff.String(), res.comment)
... obtained string = "[6 0]\n"
... expected string = "[6 3]\n"
... sql:select count, modify_count from mysql.stats_meta where table_id = 386, args:[]

OOPS: 109 passed, 1 skipped, 3 FAILED, 1 PANICKED
--- FAIL: TestT (24.31s)

statistics/selectivity_test.go statistics: not to convert small range into points when estimate rowcount using v2 stats #26695

/home/ming/learn/go/pingcap_projects/tidb/util/testkit/testkit.go:63:
    res.c.Assert(resBuff.String(), check.Equals, needBuff.String(), res.comment)
... obtained string = "" +
...     "[IndexReader_6 1.36 root  index:IndexRangeScan_5]\n" +
...     "[└─IndexRangeScan_5 1.36 cop[tikv] table:exp_backoff, index:idx(a, b, c, d) range:[1 1 1 3,1 1 1 5], keep order:false]\n"
... expected string = "" +
...     "[IndexReader_6 0.00 root  index:IndexRangeScan_5]\n" +
...     "[└─IndexRangeScan_5 0.00 cop[tikv] table:exp_backoff, index:idx(a, b, c, d) range:[1 1 1 3,1 1 1 5], keep order:false]\n"
... sql:explain select * from exp_backoff where a = 1 and b = 1 and c = 1 and d >= 3 and d<= 5, args:[]

OOPS: 58 passed, 1 FAILED
--- FAIL: TestT (7.62s)

feitian124 · 2021-07-29T06:10:13Z

my change should have no impact on other package, not sure why they are fail.
any idea?
@tisonkun @mmyj

tisonkun · 2021-07-29T06:15:43Z

/run-check_dev_2

tisonkun · 2021-07-29T06:16:19Z

@feitian124 you can check #25899 to see if there is a ticket for this test.

tisonkun · 2021-07-29T06:34:05Z

/merge

ti-chi-bot · 2021-07-29T06:34:07Z

@tisonkun: /merge is only allowed for the committers, you can assign this pull request to the committer in list by filling /assign @committer in the comment to help merge this pull request.

In response to this:

/merge

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-07-29T06:34:48Z

@feitian124 you can check #25899 to see if there is a ticket for this test.

not found, may be need add the above 2.
it's surprising to find there are so many unstable testing, no wonder i run the unit twice, the failed test count is not equal..

tisonkun · 2021-07-29T06:47:27Z

@feitian124 you can check #25899 to see if there is a ticket for this test.

not found, may be need add the above 2.
it's surprising to find there are so many unstable testing, no wonder i run the unit twice, the failed test count is not equal..

Yes. It is one thing our community struggle for...hopefully we converge with the subtasks of tracking issue done one by one and you can digest some of them when migrate tests.

tisonkun · 2021-07-29T06:47:39Z

/run-unit-test

ti-chi-bot · 2021-07-29T06:58:56Z

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

util/decoder: migrate test-infra to testify

b0df9a5

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 Jul 27, 2021

fix ci error

ddc2809

ti-chi-bot requested a review from tisonkun July 27, 2021 05:27

add t.Parallel() in test

51b1a1f

tisonkun requested changes Jul 27, 2021

View reviewed changes

feitian124 added 9 commits July 27, 2021 20:19

Merge branch 'master' into issue-26414

5ac8899

extract MustNewCommonHandle to testkit/handle.go

6ddef4e

migrate 2 file to use testkit.MustNewCommonHandle

abbdffe

update comment

8977d31

ignore goleak and fix cycle import

7a4f7f2

Merge branch 'master' into issue-26414

fbc3957

fix ci error

a1def0e

fix ci error

0a08466

fix ci error

53853a4

tisonkun reviewed Jul 27, 2021

View reviewed changes

util/testkit/handle.go Outdated Show resolved Hide resolved

update utils

7c32835

Signed-off-by: tison <[email protected]>

tisonkun approved these changes Jul 27, 2021

View reviewed changes

tisonkun and others added 2 commits July 28, 2021 07:24

conditional build

777b890

Signed-off-by: tison <[email protected]>

Merge branch 'master' into issue-26414

8b1e4a0

ti-chi-bot requested review from tiancaiamao and xhebox July 29, 2021 03:12

xhebox approved these changes Jul 29, 2021

View reviewed changes

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

mmyj approved these changes Jul 29, 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 Jul 29, 2021

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

Merge branch 'master' into issue-26414

8c79b8d

Merge branch 'master' into issue-26414

2adab08

ti-chi-bot merged commit 7d45b32 into pingcap:master Jul 29, 2021

tisonkun mentioned this pull request Aug 7, 2021

migrate test-infra to testify for util/keydecoder pkg #26414

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

util/decoder: migrate test-infra to testify #26629

util/decoder: migrate test-infra to testify #26629

feitian124 commented Jul 27, 2021

ti-chi-bot commented Jul 27, 2021 •

edited

Loading

feitian124 commented Jul 27, 2021

tisonkun left a comment

tisonkun Jul 27, 2021

feitian124 Jul 27, 2021

tisonkun Jul 27, 2021

feitian124 Jul 27, 2021

feitian124 Jul 27, 2021

tisonkun Jul 27, 2021

feitian124 Jul 27, 2021

feitian124 Jul 27, 2021

ti-chi-bot commented Jul 27, 2021

tisonkun left a comment

ti-chi-bot commented Jul 27, 2021

feitian124 commented Jul 29, 2021

tisonkun commented Jul 29, 2021 •

edited

Loading

mmyj left a comment

tisonkun commented Jul 29, 2021 •

edited

Loading

mmyj commented Jul 29, 2021

ti-chi-bot commented Jul 29, 2021

feitian124 commented Jul 29, 2021 •

edited

Loading

feitian124 commented Jul 29, 2021 •

edited

Loading

tisonkun commented Jul 29, 2021

tisonkun commented Jul 29, 2021 •

edited

Loading

tisonkun commented Jul 29, 2021

ti-chi-bot commented Jul 29, 2021

feitian124 commented Jul 29, 2021

tisonkun commented Jul 29, 2021

tisonkun commented Jul 29, 2021

ti-chi-bot commented Jul 29, 2021

util/decoder: migrate test-infra to testify #26629

util/decoder: migrate test-infra to testify #26629

Conversation

feitian124 commented Jul 27, 2021

What problem does this PR solve?

Release note

ti-chi-bot commented Jul 27, 2021 • edited Loading

feitian124 commented Jul 27, 2021

tisonkun 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

ti-chi-bot commented Jul 27, 2021

tisonkun left a comment

Choose a reason for hiding this comment

ti-chi-bot commented Jul 27, 2021

feitian124 commented Jul 29, 2021

tisonkun commented Jul 29, 2021 • edited Loading

mmyj left a comment

Choose a reason for hiding this comment

tisonkun commented Jul 29, 2021 • edited Loading

mmyj commented Jul 29, 2021

ti-chi-bot commented Jul 29, 2021

feitian124 commented Jul 29, 2021 • edited Loading

feitian124 commented Jul 29, 2021 • edited Loading

tisonkun commented Jul 29, 2021

tisonkun commented Jul 29, 2021 • edited Loading

tisonkun commented Jul 29, 2021

ti-chi-bot commented Jul 29, 2021

feitian124 commented Jul 29, 2021

tisonkun commented Jul 29, 2021

tisonkun commented Jul 29, 2021

ti-chi-bot commented Jul 29, 2021

ti-chi-bot commented Jul 27, 2021 •

edited

Loading

tisonkun commented Jul 29, 2021 •

edited

Loading

tisonkun commented Jul 29, 2021 •

edited

Loading

feitian124 commented Jul 29, 2021 •

edited

Loading

feitian124 commented Jul 29, 2021 •

edited

Loading

tisonkun commented Jul 29, 2021 •

edited

Loading