util/encrypt: migrate test-infra to testify

[karuppiah7890]

What problem does this PR solve?

Issue Number: close #27816

Problem Summary: migrate test-infra to testify for util/encrypt package

What is changed and how it works?

What's Changed:

Migrated tests to use testify package instead of github.com/pingcap/check package for assertions and any test utils and matchers that github.com/pingcap/check provided

Check List

Tests

• Unit test

Release note

None

[ti-chi-bot]

[REVIEW NOTIFICATION]

This pull request has been approved by:

• mmyj
• morgo

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.

[karuppiah7890]

/cc @tisonkun

[karuppiah7890]

Question: Do we need the extra options for the goleak? To ignore some top level functions. But I didn't get any leak error while running

{ make failpoint-enable; go test github.com/pingcap/tidb/util/encrypt; make failpoint-disable; }

So I'm guessing there's nothing to ignore in this case, unlike here https://github.com/pingcap/tidb/blob/master/ddl/failtest/main_test.go#L37-L42

[karuppiah7890]

By the way, I noticed some unrelated errors while running all the tests -

tidb $ make gotest
Running in native mode.
gsttimeout-check
grep 'PASS:' gotest.log | go run tools/check/check-timeout.go || { $(find $PWD/ -type d | grep -vE "(\.git|tools)" | xargs tools/bin/failpoint-ctl disable); exit 1; }
^R
The following test cases take too long to finish:
PASS: builtin_time_vec_generated_test.go:6727: testVectorizeSuite1.TestVectorizedBuiltinTimeEvalOneVecGenerated	6.754s
PASS: builtin_time_vec_test.go:575: testVectorizeSuite2.TestVectorizedBuiltinTimeEvalOneVec	7.341s
PASS: builtin_time_vec_test.go:579: testVectorizeSuite2.TestVectorizedBuiltinTimeFunc	5.569s
--- PASS: TestClusterConfigInfoschema (5.71s)
--- PASS: TestCTEPreviewAndReport (7.12s)
exit status 252
make: *** [gotest] Error 1
tidb $ 

[karuppiah7890]

There seems to be an unrelated error in pipeline too over here https://ci.pingcap.net/blue/organizations/jenkins/tidb_ghpr_check_2/detail/tidb_ghpr_check_2/33109/pipeline

[2021-09-13T15:40:22.324Z] FAIL
[2021-09-13T15:40:22.324Z] + cat test.log
[2021-09-13T15:40:22.324Z] + grep -Ev '^\[[[:digit:]]{4}(/[[:digit:]]{2}){2}'
[2021-09-13T15:40:22.324Z] + grep -A 30 '\-------'
[2021-09-13T15:40:22.324Z] + grep -A 29 FAIL
[2021-09-13T15:40:22.324Z] FAIL: attributes_sql_test.go:237: testDBSuite8.TestFlashbackTable
[2021-09-13T15:40:22.324Z] 
[2021-09-13T15:40:22.324Z] attributes_sql_test.go:301:
[2021-09-13T15:40:22.324Z]     c.Assert(rows2[0][3], Equals, rows[0][3])
[2021-09-13T15:40:22.324Z] ... obtained string = "7480000000000021ff4e5f720000000000fa"
[2021-09-13T15:40:22.324Z] ... expected string = "7480000000000000ff375f720000000000fa"

[karuppiah7890]

Oops. Sorry, I just did rebase and force pushed and noticed that there was a merge being done

[tisonkun]

Hi @karuppiah7890 ! Thanks for your contribution and sorry for our unstable tests in such a terrible situation.

For any failing tests you found, you can try to find an issue on #25899:

• If there is already one, report another instance you meet so that we may increase its priority.
• If there is none, create an issue and ask for link in the tracking issue for unstable tests.

Thanks for your understanding.

[unconsolable]

Question: Do we need the extra options for the goleak? To ignore some top level functions. But I didn't get any leak error while running

{ make failpoint-enable; go test github.com/pingcap/tidb/util/encrypt; make failpoint-disable; }

So I'm guessing there's nothing to ignore in this case, unlike here https://github.com/pingcap/tidb/blob/master/ddl/failtest/main_test.go#L37-L42

For some test cases, goroutine leaks are known issues, we should add these goroutines to ignore list, such as

tidb/ddl/failtest/main_test.go

Lines 37 to 40 in 9d3b67f

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

But some goroutine leaks are caused by test code, we should try to fix them.

[tisonkun]

Oops. Sorry, I just did rebase and force pushed and noticed that there was a merge being done

Never mind. If my action causes any inconvenient, please let me know.

In our dev guide it says:

Review and address comments on your pull request. If your pull request becomes outdated with the target branch, you need to rebase your pull request to keep it up to date. Since TiDB uses squash and merge, simply merging master to catch up the change is also acceptable.

Since the catching up so frequently I move to use the merge button gradually.

[karuppiah7890]

Makes sense 👍

[tisonkun]

/cc @mmyj @morgo

[karuppiah7890]

@unconsolable how do I know if my test has any known goroutine leaks and that I need to ignore them? Similar to https://github.com/pingcap/tidb/blob/master/ddl/failtest/main_test.go#L37-L42

How was it found that https://github.com/pingcap/tidb/blob/master/ddl/failtest/main_test.go#L37-L42 has known goroutine leaks - does it always show as leak error or only sometimes?

[unconsolable]

@unconsolable how do I know if my test has any known goroutine leaks and that I need to ignore them? Similar to https://github.com/pingcap/tidb/blob/master/ddl/failtest/main_test.go#L37-L42

How was it found that https://github.com/pingcap/tidb/blob/master/ddl/failtest/main_test.go#L37-L42 has known goroutine leaks - does it always show as leak error or only sometimes?

I usually add main_test.go first and run locally, then when I encounter goroutine leak, I will refer the list here

tidb/util/testleak/leaktest.go

Lines 55 to 69 in 9d3b67f

	// these go routines are async terminated, so they may still alive after test end, thus cause
	// false positive leak failures
	"google.golang.org/grpc.(*addrConn).resetTransport",
	"google.golang.org/grpc.(*ccBalancerWrapper).watcher",
	"github.com/pingcap/goleveldb/leveldb/util.(*BufferPool).drain",
	"github.com/pingcap/goleveldb/leveldb.(*DB).compactionError",
	"github.com/pingcap/goleveldb/leveldb.(*DB).mpoolDrain",
	"go.etcd.io/etcd/pkg/logutil.(*MergeLogger).outputLoop",
	"go.etcd.io/etcd/v3/pkg/logutil.(*MergeLogger).outputLoop",
	"oracles.(*pdOracle).updateTS",
	"tikv.(*KVStore).runSafePointChecker",
	"tikv.(*RegionCache).asyncCheckAndResolveLoop",
	"github.com/pingcap/badger",
	"github.com/ngaut/unistore/tikv.(*MVCCStore).runUpdateSafePointLoop",
	"github.com/tikv/client-go/v2/tikv.(*ttlManager).keepAlive", // See https://github.com/tikv/client-go/issues/174

However, there is a exception. #27704 (comment)

Solution

tidb/store/mockstore/mockcopr/main_test.go

Lines 32 to 37 in 9d3b67f

	callback := func(i int) int {
	// wait for leveldb to close, leveldb will be closed in one second
	time.Sleep(time.Second)
	return i
	}
	goleak.VerifyTestMain(testmain.WrapTestingM(m, callback), opts...)

[karuppiah7890]

Cool ! So, when I ran in my local, I noticed no issues / errors related to goroutine leaks, let me also check the exception now

[karuppiah7890]

Looks like after rebase there are no unstable errors in my local and in CI, interesting

[tisonkun]

@karuppiah7890 the most unstable timeout checker is temporarily disabled by #27980

[karuppiah7890]

Ah okay!

[mmyj]

lgtm

[mmyj]

/merge

[ti-chi-bot]

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

Commit hash: 5551888

[tisonkun]

@karuppiah7890 I think almost these test cases can be run in parallel.

[karuppiah7890]

@tisonkun I just saw this comment. Should I raise another PR? With t.Parallel() in all these tests. That's what you mean right @tisonkun ?

[tisonkun]

@tisonkun I just saw this comment. Should I raise another PR? With t.Parallel() in all these tests. That's what you mean right @tisonkun ?

I think it is ok to leave as is since it is almost lightweight test. But if you can investigate the possibility for running in parallel, I'm glad to give it a review and moderate the contribution. You can create an issue for the follow up and then a PR, if you'd like to do it.

[karuppiah7890]

Yes, the tests are lightweight indeed, all running within less than 1 second. But yes, parallelization would be cool. I will raise an issue and a PR for it sometime 👍

[karuppiah7890]

Created the issue for now - #28034