Skip to content

[dev.fuzz] testing: errors in (*testing.F).Cleanup functions do not always report the failure #46993

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

Closed
katiehockman opened this issue Jun 30, 2021 · 3 comments
Labels
FrozenDueToAge fuzz Issues related to native fuzzing support NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Milestone

Comments

@katiehockman
Copy link
Contributor

katiehockman commented Jun 30, 2021

When running go test -fuzz with -run=None and without setting -v, any errors/panics/failures inside of an f.Cleanup function are not reported.

This is likely related to #46631, as I discovered this issue while debugging that one. It appears we are handling f.Cleanup functions incorrectly when fuzzing right now. Possibly also related to changes from #46632.

Update: From trying to repro locally, sometimes it does the right thing, sometimes it doesn't, so there's probably a race somewhere.

Reproducer target:

func FuzzCleanupError(f *testing.F) {
	f.Cleanup(func() {
		f.Errorf("bad thing here")
	})
	f.Fuzz(func(t *testing.T, data []byte) {
		time.Sleep(time.Second)
	})
}

If run with gotip test -run=None -fuzz=FuzzCleanupError for a few seconds, then pressing Ctrl+C, the console doesn't print anything at all (neither an "ok" or a failure).

If the target is executed with -run, then it does print the failure correctly:

~ gotip test -run=FuzzCleanupError           
--- FAIL: FuzzCleanupError (0.00s)
    foo_test.go:46: bad thing here
FAIL
FAIL    foo   0.138s
FAIL

The same correctly reported failure occurs if run with gotip test -run=None -fuzz=FuzzCleanupError -v

/cc @golang/fuzzing

@katiehockman katiehockman added NeedsFix The path to resolution is known, but the work has not been done. fuzz Issues related to native fuzzing support labels Jun 30, 2021
@katiehockman katiehockman self-assigned this Jun 30, 2021
@jayconrod
Copy link
Contributor

I wonder if the race here is happening because SIGINT is delivered to the coordinator and all the workers concurrently? So the coordinator could receive that signal first and shut down all the workers gracefully. Or a worker could receive it first and hit this error.

We could potentially make this more predictable by putting the coordinator and all workers in a new progress group. Then only the go test parent process would get SIGINT, and it would have to tell the coordinator to stop, and the coordinator would tell all the workers. I think that's roughly how it works on Windows; the shell sends CTRL_CLOSE_EVENT or something similar only to go test. I'm not sure we're handling that correctly either though.

@katiehockman
Copy link
Contributor Author

This will likely be a release blocker for Go 1.18

@katiehockman katiehockman added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 14, 2021
@gopherbot gopherbot removed the NeedsFix The path to resolution is known, but the work has not been done. label Sep 14, 2021
@katiehockman katiehockman added this to the Go1.18 milestone Sep 14, 2021
@katiehockman
Copy link
Contributor Author

I'm not able to reproduce this, anymore. I wouldn't be surprised if it got fixed along the way in our several refactors. I'm going to close this issue, but we can re-open it if users run into it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge fuzz Issues related to native fuzzing support NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Projects
Status: No status
Development

No branches or pull requests

3 participants