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

testing: run Cleanup if SIGINT occurs #41891

Open
katiehockman opened this issue Oct 9, 2020 · 6 comments
Open

testing: run Cleanup if SIGINT occurs #41891

katiehockman opened this issue Oct 9, 2020 · 6 comments
Milestone

Comments

@katiehockman
Copy link
Member

@katiehockman katiehockman commented Oct 9, 2020

Currently, testing.T.Cleanup and testing.B.Cleanup are only run if the test and all of its subtests have finished running. This means it won't run if the test is stopped with a SIGINT (e.g. Ctrl+C). There are several things that may be in such a Cleanup function that would be helpful to run regardless of whether or not the test fully completed.

This will be especially unfortunate in the case of fuzzing (http://golang.org/s/draft-fuzzing-design). Take for example:

func FuzzFoo(f *testing.F) {
	f.Cleanup(func() {
		// Check for things like stray go routines, etc
		// Delete tmp resources that may be left behind
	})
	f.Fuzz(func(t *testing.T, b []byte) {
		// some code being fuzzed
	})
}

It may be a very common use case that someone could run go test -fuzz FuzzFoo and give a SIGINT after a few hours. If no crash occurs, there would be no way to run any cleanup code or do any checks after fuzzing completed since the fuzzing runs in an infinite loop.

I would like to expand the behavior of Cleanup to not only run if tests finished, but also run in the case of a SIGINT. This could of course be special cased to fuzzing, but I can see good reasons why it should be a part of unit and benchmark testing as well (e.g. if the test hangs for some reason).

/cc @jayconrod @bcmills @FiloSottile

@katiehockman katiehockman added this to the Go1.16 milestone Oct 9, 2020
@katiehockman
Copy link
Member Author

@katiehockman katiehockman commented Oct 9, 2020

A note on possible complexity from @jayconrod: "One problem is that we can't handle SIGINT on Windows. It looks like there a couple similar things (CTRL_CLOSE_EVENT). Not sure if we can hook into that without cgo."

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Oct 9, 2020

Not sure whether we should handle SIGINT in all tests and benchmarks. User code may install signal handlers (for example, in an initializer in a cgo library), and we might accidentally replace those.

It makes more sense for fuzzing, specifically when running the fuzzing engine in worker processes, since 1) we don't need to be compatible with existing fuzz targets because there are none, and 2) it will be common for users to stop fuzzing with ^C.

@bcmills
Copy link
Member

@bcmills bcmills commented Oct 9, 2020

Can the test function itself register the SIGINT handler, rather than having the testing package do that?

For example, something like: https://play.golang.org/p/ZsRDGgDMgGz

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Oct 9, 2020

We were looking for something more general purpose with fuzzing. The fuzzing coordinator process (started by go test) should handle SIGINT and should flush pending updates to the corpus. It should also stop its worker processes.

@bcmills
Copy link
Member

@bcmills bcmills commented Oct 9, 2020

I don't think Cleanup is appropriate for that without coordination from the test itself, though.

The Cleanup function can normally assume that it does not race with any subtests, and can also assume that it is only called after the subtests are complete (compare #40908). So it seems to me that if you want to run Cleanup functions on SIGINT, you fundamentally have to thread that signal down to the test functions and have them cooperatively complete.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 10, 2020

Note that the last time we tried to handle SIGINT in the testing package, for #19397, we had to roll back the change because it broke tests that were testing code that itself handled SIGINT. See #21000.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.