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.

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

support register teardown function in VerifyTestMain #63

Closed
tisonkun opened this issue Aug 14, 2021 · 6 comments · Fixed by #78
Closed

support register teardown function in VerifyTestMain #63

tisonkun opened this issue Aug 14, 2021 · 6 comments · Fixed by #78

Comments

@tisonkun
Copy link

TestMain can be used for setup and teardown logic. However, VerifyTestMain calls os.Exit which disables to register teardown function by defer. cc @prashantv

@abhinav
Copy link
Collaborator

abhinav commented Sep 20, 2021

Thanks for the report and the PR @tisonkun.
We're not certain that that's the best way to accomplish this,
especially because VerifyTestMain and VerifyNone already accept functional options.

One solution is a new option that applies only to VerifyNone and VerifyTestMain.

func Cleanup(func()) Option

I don't know if this is the best solution to this problem, but it's a possibility.

Ref internal issue: GO-888

@tisonkun
Copy link
Author

@abhinav thanks for your suggestion, I'll think of it. The solution in #65 is just what I implement for pingcap/tidb for the same purpose, it is finished out of the library.

@tisonkun
Copy link
Author

tisonkun commented Sep 21, 2021

GO-888

Is this a ticket number of uber internal issue tracker? I guess it is a golang discussion but fail to find it.

@abhinav
Copy link
Collaborator

abhinav commented Sep 21, 2021

GO-888

Is this a ticket number of uber internal issue tracker? I guess it is a golang discussion but fail to find it.

Yeah, sorry it's an internal issue tracker link.
There's no discussion on it;
it's just a tracking task for this issue in our internal issue tracker.

@tisonkun
Copy link
Author

tisonkun commented Sep 24, 2021

@abhinav between these two suggestion, I prefer #65 .

It accepts the return code so that we may skip callbacks or customize logics.

Besides, Option is used already to ignore goroutines (I think most of our users assume this). Overriding the concept and making it a bit different may cause unnecessary misleading. Also highly possibly increase cyclomatic complexity where Option in use.

@tisonkun
Copy link
Author

tisonkun commented Jan 26, 2022

@abhinav today I find a time to attempt the idea you proposed above about a new Option. It can be defined as:

type opts struct {
	filters    []func(stack.Stack) bool
	maxRetries int
	maxSleep   time.Duration
	cleanup    func(int) int
}

func Cleanup(f func(int) int) Option {
	return optionFunc(func(opts *opts) {
		opts.cleanup = f
	})
}

However, I noticed that VerifyNone doesn't need an extra cleanup function as it can be done by defer or any suite's teardown defers. The reason we need a new feature is that: in VerifyTestMain, we call os.Exit which causes defers doesn't have a chance to run. So it's a dedicate wrapper for VerifyTestMain.

So, I still prefer the solution in #65. If we add an option, it only takes effect for VerifyTestMain but not VerifyNone (or we should not), then it looks weird. What do you think?

sywhang added a commit that referenced this issue Sep 6, 2022
This adds Cleanup option which can be passed to VerifyTestMain and
VerifyNone. This takes in a function that will be executed at the
end of the leak verification.

Internal Ref: GO-888
Fix #63

Co-authored-by: Abhinav Gupta <mail@abhinavg.net>
Co-authored-by: Abhinav Gupta <abg@uber.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants