Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

migrate test-infra to testify for executor #26854

Closed
75 tasks done
Tracked by #26022
tisonkun opened this issue Aug 3, 2021 · 13 comments
Closed
75 tasks done
Tracked by #26022

migrate test-infra to testify for executor #26854

tisonkun opened this issue Aug 3, 2021 · 13 comments
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. type/enhancement The issue or PR belongs to an enhancement.

Comments

@tisonkun
Copy link
Contributor

tisonkun commented Aug 3, 2021 •
edited
Loading

@tisonkun tisonkun mentioned this issue Aug 3, 2021
Closed
53 tasks
@tisonkun
Copy link
Contributor Author

tisonkun commented Aug 3, 2021

This package is too complex to have its own issue and we'd migrate then in multiple batches. Please comment the filename you want to do and I will update the checklist.

@tisonkun tisonkun self-assigned this Aug 3, 2021
@tisonkun tisonkun added good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Aug 3, 2021
@tisonkun tisonkun mentioned this issue Aug 4, 2021
Merged
@tisonkun tisonkun removed their assignment Aug 8, 2021
@unconsolable
Copy link
Contributor

Some tests has both serial suite and suite, for example cte_test.go has both CTETestSuite and CTESerialTestSuite. Serial suite in pingcap/check can be replaced by suite in testify, while suite can not because parallel execution is not supported in testify.

IMO a parent test may be built and acted as SetUpSuite and TearDownSuite, and other tests are introduced by sub tests. Also stretchr/testify#1109 are working on parellel execution, which is waiting to be reviewed.

So should we wait for the upstream PR? Or propose other alternatives?

@tisonkun
Copy link
Contributor Author

@unconsolable almost we use testify checkers only and it is viable to setup/teardown tests with go testing.

See also https://pkg.go.dev/testing#hdr-Subtests_and_Sub_benchmarks.

testify is already aware of this issue stretchr/testify#187 years ago and the author of stretchr/testify#1109 isn't one of the maintainer. I don't think they will enable this feature soon.

You can see how we migrate suite to go testing + testify previously.

@unconsolable unconsolable mentioned this issue Aug 11, 2021
Closed
@unconsolable
Copy link
Contributor

Please add #27097 as a sub task

@tisonkun
Copy link
Contributor Author

@unconsolable done. Thank you!

@unconsolable unconsolable mentioned this issue Aug 19, 2021
Merged
@unconsolable
Copy link
Contributor

When migrating the tests in executor, please check the unexpected goroutine leak issues manually. See also #27103 (comment)

We don't have to add it now since there are multiple tests leak on never closing domain, etc. We will perform another pass to fix the leak problem but I'd prefer it soon after we migrate the tests. And thus during the migration, I hope the author manually check whether or not the tests leak.

etcd related leaks is somehow "expected", domain's, badger's are unexpected.

@tisonkun
Copy link
Contributor Author

@unconsolable I hope we can enable goleak by #27405.

@tisonkun tisonkun mentioned this issue Sep 2, 2021
Open
@unconsolable
Copy link
Contributor

unconsolable commented Sep 27, 2021 •
edited
Loading

@tisonkun
As currently tests under executor packages are using both pingcap/check and testify, shall we move the code in TestT to main_test.go to set up all of the tests?

tidb/executor/executor_test.go

Lines 89 to 111 in 76267b2

func TestT(t *testing.T) {
CustomVerboseFlag = true
*CustomParallelSuiteFlag = true
logLevel := os.Getenv("log_level")
err := logutil.InitLogger(logutil.NewLogConfig(logLevel, logutil.DefaultLogFormat, "", logutil.EmptyFileLogConfig, false))
if err != nil {
t.Fatal(err)
}
autoid.SetStep(5000)
config.UpdateGlobal(func(conf *config.Config) {
conf.Log.SlowThreshold = 30000 // 30s
conf.TiKVClient.AsyncCommit.SafeWindow = 0
conf.TiKVClient.AsyncCommit.AllowedClockDrift = 0
})
tikv.EnableFailpoints()
tmpDir := config.GetGlobalConfig().TempStoragePath
_ = os.RemoveAll(tmpDir) // clean the uncleared temp file during the last run.
_ = os.MkdirAll(tmpDir, 0755)
testleak.BeforeTest()
TestingT(t)
testleak.AfterTestT(t)()
}

From my point of view, code related to pingcap/check can still in TestT, while code related to testleak may be replaced by goleak enabled in TestMain. All the other code, may be moved to TestMain. WDYT

@tisonkun
Copy link
Contributor Author

@unconsolable reasonable. I'd suggest you create an issue and start to work on it if you want to. Ask me to linked at this issue.

@unconsolable
Copy link
Contributor

@tisonkun Created #28440

@karuppiah7890
Copy link
Contributor

Let's create issues for each of the files like other package issues?

@tisonkun
Copy link
Contributor Author

tisonkun commented Oct 8, 2021

@karuppiah7890 yes. Creating...

@tisonkun tisonkun added the type/enhancement The issue or PR belongs to an enhancement. label Feb 17, 2022
@tisonkun
Copy link
Contributor Author

Closed as all subtasks done. Finally!

@tisonkun tisonkun mentioned this issue May 17, 2022
Merged
ti-chi-bot pushed a commit that referenced this issue May 18, 2022
@tisonkun
executor: remove testify suite (#34756)
d40b5e1 
ref #26854
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@unconsolable @karuppiah7890 @tisonkun