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

proposal: cmd/go: go test should always fuzz a bit even without -fuzz #50969

Closed
eliasnaur opened this issue Feb 2, 2022 · 29 comments
Closed
Labels
FrozenDueToAge fuzz Issues related to native fuzzing support Proposal
Milestone

Comments

@eliasnaur
Copy link
Contributor

eliasnaur commented Feb 2, 2022

It's mentioned in the documentation that go test without the -fuzz flag only tests the seed corpus for Fuzz* functions. I'd like to convert tests that use testing/quick.Check to Fuzz* functions, but I'm too lazy to come up with a seed corpus for otherwise succeeding tests. I propose that go test always fuzz for a bit, even without the -fuzz flag, similar to using testing/quick.

@eliasnaur eliasnaur added the fuzz Issues related to native fuzzing support label Feb 2, 2022
@bcmills bcmills changed the title cmd/go: go test should always fuzz a bit even without -fuzz proposal: cmd/go: go test should always fuzz a bit even without -fuzz Feb 2, 2022
@gopherbot gopherbot added this to the Proposal milestone Feb 2, 2022
@beoran
Copy link

beoran commented Feb 3, 2022

Sorry, but i don't want to have to wait and use resources for something I didn't ask for. If -fuzz should be on by default we should add an environment variable that enables this. Something like GOTESTFLAGS.

@eliasnaur
Copy link
Contributor Author

I don't want -fuzz enabled by default, not least because it makes go test run until Ctrl-C. I also don't want to waste significant resources, just fuzz for as long as testing/quick would.

@mvdan
Copy link
Member

mvdan commented Feb 3, 2022

It's worth noting that fuzz tests already run as part of a plain go test, so there is some precedent here:

Fuzz tests are run much like a unit test by default. Each seed corpus entry will be tested against the fuzz target, reporting any failures before exiting.

And, historically, go test has also tried to strike a balance with running extra checks by default even when the user can run them manually; for instance, see the -vet flag. I think that's been a success.

On one hand, I think this would be a good idea for the sake of good defaults, and for fuzzing to be a better drop-in replacement for testing/quick. On the other, I can't help but feel like you can already get this behavior via go test -fuzz=. -fuzztime=1s. Overall, I lean slightly in favor of this proposal.

fuzz for as long as testing/quick would.

Do you know how long this would be? testing/quick seems to default to 100 checks, so we'd have to somehow convert that to a reasonable time unit.

@rogpeppe
Copy link
Contributor

rogpeppe commented Feb 3, 2022

I think this is a good idea. If I've got a fuzz test, I think it's reasonable to run it for at least a few milliseconds rather than just once.

@eliasnaur
Copy link
Contributor Author

fuzz for as long as testing/quick would.

Do you know how long this would be? testing/quick seems to default to 100 checks, so we'd have to somehow convert that to a reasonable time unit.

I'm happy with 100 iterations if X milliseconds is deemed too long or too difficult to estimate.

@mvdan
Copy link
Member

mvdan commented Feb 3, 2022

Just thinking outloud: the cost of a fuzz function could vary wildly. I have one that takes in the order of tens of milliseconds per input, so 100 iterations would easily get you to a couple of seconds of wall time. I admit that I don't know how -fuzztime works, but presumably we could rely on it to run the fuzz tests for e.g. 100ms each.

@timothy-king
Copy link
Contributor

IMO running a small # of deterministic tests fits in well with existing unit testing expectations. Imagine running go test ./... as a part of CI for your module and some internal/ package has a fuzzing crash, but it does not reproduce locally. Not great as a user experience. This suggests wanting fuzz tests to be deterministic when running go test. Other contexts have found that ~10 fixed inputs + the corpus suffice for this. And if it does crash on this very small set, it suggests a problem with how the fuzz test is written. (Like a unit test for the fuzzer.) Whether the magic # of deterministic iterations is closer to 10 or 100 is up for debate.

Resolving whether this fuzz test does a good job at fuzzing or testing SoT are already covered well by go test -fuzz=. -fuzztime=Xs.

Aside about how to determine whether this fuzz test do a "good" job at fuzzing. This is covered well by fuzzing for a short duration (10-60s) and characterizing its behavior. If this is gaining no coverage, maybe it needs to be rewritten? If it is saturating coverage already, maybe it is not worth running? (If it is crashing, it either needs to be rewritten or bugs need to be fixed.) Does each iteration take too long (>100ms)? Does it stay in your memory budget? You can even automate the decision for whether this is a "healthy" fuzzer as a [roughly] pre/post submit time CI check.

@eliasnaur
Copy link
Contributor Author

IMO running a small # of deterministic tests fits in well with existing unit testing expectations. Imagine running go test ./... as a part of CI for your module and some internal/ package has a fuzzing crash, but it does not reproduce locally. Not great as a user experience. This suggests wanting fuzz tests to be deterministic when running go test. Other contexts have found that ~10 fixed inputs + the corpus suffice for this. And if it does crash on this very small set, it suggests a problem with how the fuzz test is written. (Like a unit test for the fuzzer.) Whether the magic # of deterministic iterations is closer to 10 or 100 is up for debate.

Resolving whether this fuzz test does a good job at fuzzing or testing SoT are already covered well by go test -fuzz=. -fuzztime=Xs.

I don't think anybody questions that -fuzz -fuzztime=Xs is a great way of shaking out bugs from fuzzing tests. This issue is about making a bit of fuzzing the default so that there's less need to remember to run go test twice, first for regular tests, then for fuzzing.

In other words, if you agree that go test -fuzz -fuzztime=Xs is a good idea to run after fuzz test or code changes, why not make it the default (albeit with a smaller -fuzztime)?

@beoran
Copy link

beoran commented Feb 4, 2022

I expect to run tests without fuzz testing a lot more often than fuzz tests. I like saving electricity on my old refurbished computers as I talked about before on this issue tracker. Fuzz testing is something I plan on doing on "finished" software only, while normal go testing I do all the time during development.

@mvdan
Copy link
Member

mvdan commented Feb 4, 2022

@beoran if that is the case in general, I imagine you could use -fuzz=- just like you are probably using -vet=off today. Is there a reason why fuzzing for a very short time should be any different? In my experience, vet already does take hundreds of milliseconds for any non-trivial package, as it does its own loading and type-checking separately from the compiler.

@beoran
Copy link

beoran commented Feb 4, 2022

I'd rather not have another thing to turn off, really. And go vet is rather predictable in resource usage, while fuzz testing can be less so.

By the way, should it not also be -fuzz=off for consistency, or has that ship sailed already?

@timothy-king
Copy link
Contributor

In other words, if you agree that go test -fuzz -fuzztime=Xs is a good idea to run after fuzz test or code changes, why not make it the default (albeit with a smaller -fuzztime)?

I think this level of unpredictability is not in line with what people expect between runs of go test. Flakiness should not be the default for the testing framework.

OTOH I would also encourage folks to rerun their fuzz tests as code changes. I would be very happy to have go test -fuzz -fuzztime=Xs run as a part of a CI system for my code. Do I want this each time I run a package's unit tests? Not sure. And I suspect others will be more hesitant than I am.

My current opinion is that turning fuzzing on should be "opt in" instead of "opt out".

@eliasnaur
Copy link
Contributor Author

In other words, if you agree that go test -fuzz -fuzztime=Xs is a good idea to run after fuzz test or code changes, why not make it the default (albeit with a smaller -fuzztime)?

I think this level of unpredictability is not in line with what people expect between runs of go test. Flakiness should not be the default for the testing framework.

This is misleading. go test -fuzz doesn't introduce flakes, it exposes existing flakiness is in your code or Fuzz* tests. I run go test to see new bugs exposed, not to see it complete successfully.

OTOH I would also encourage folks to rerun their fuzz tests as code changes. I would be very happy to have go test -fuzz -fuzztime=Xs run as a part of a CI system for my code. Do I want this each time I run a package's unit tests? Not sure. And I suspect others will be more hesitant than I am.

I don't understand why you're happy to have fuzzing in CI where there's a risk of losing the reproducer, but not during manual go test runs where you're almost sure to recover the reproducer (as dumped by the Go fuzzer).

@timothy-king
Copy link
Contributor

This is misleading. go test -fuzz doesn't introduce flakes, it exposes existing flakiness is in your code or Fuzz* tests.

I think where we are disagreeing is what the defaults for go test (no flag) should be for Fuzz* functions. I support running the fuzz target a very small and deterministic number of times during go test. If 100 iterations is too high, I think ~10 + the corpus should be fine. IMO this is effectively still a unit test (of the fuzzer). So I think this fits in well with what people already expect from go test.

Running for a fixed time duration on randomly generated tests could make a test change whether it passes from one run to the next due to differences in timing, machine, load, or seed choice. Repeated executions would almost certainly find more crashes than a single fixed sequence of testing inputs, but it would do so non-deterministically.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Feb 16, 2022
@bcmills
Copy link
Contributor

bcmills commented Feb 16, 2022

go test -fuzz doesn't introduce flakes, it exposes existing flakiness is in your code or Fuzz* tests.

go test -fuzz=. intentionally and nondeterministically widens the space of behaviors covered by tests. That's usually a good thing (because it finds bugs), but can also be intrusive (because the bugs it finds may or may not be important to fix right away).

If a deterministic test finds a low-priority bug, that part of the test can normally be skipped to avoid masking new failures with known-but-unimportant existing failure modes. However, there isn't a straightforward way to tell the fuzzer to skip a known-bad codepath: restructuring a fuzz test to avoid a bad tree of inputs can sometimes be even more work than fixing that codepath!

The difficulty of skipping known-bad inputs is a problem that fuzzing shares in common with other nondeterministic tests, but with one important caveat: fuzz tests also have a deterministic component. The behaviors for the inputs added by calls to Add and/or files checked in to testdata are typically very deterministic — otherwise the fuzzer would be unable to minimize inputs, and would be much more difficult to use.

So, to me, it is important that the “run deterministic fuzzer inputs” step be separate from the “run nondeterministic fuzz inputs” step, because that allows the existing inputs to continue to function as regression tests even in the presence of other known bugs.

@rsc
Copy link
Contributor

rsc commented Feb 16, 2022

I'm confused about the proposal. How much additional time should every invocation of 'go test' take?

@timothy-king
Copy link
Contributor

@bcmills Being able to skip tests is a valid concern I had not really considered. I still think there is value in each fuzz test being run deterministically >0 times by default on go test. This tests the fuzz test itself. My preference is for this to happen even without a call to Add. (Not all users will Add something or have testdata inputs.) I am not sure how to balance this with allowing test cases to be skipped though.

@rsc rsc moved this from Incoming to Active in Proposals (old) Feb 16, 2022
@rsc
Copy link
Contributor

rsc commented Feb 16, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@bcmills
Copy link
Contributor

bcmills commented Feb 16, 2022

@timothy-king

I still think there is value in each fuzz test being run deterministically >0 times by default on go test.

It does seem like we could at least invoke the Fuzz target with all zero-values just to demonstrate that the function is of a supported type. That would be completely deterministic, and also trivial enough that it shouldn't uncover anything particularly unexpected!

@prattmic
Copy link
Member

prattmic commented Feb 16, 2022

That would be completely deterministic, and also trivial enough that it shouldn't uncover anything particularly unexpected!

Even if this does uncover something "unexpected", I think that is totally fine. The input is deterministic, so this is effectively the same as a unit test and should be easy to reproduce. And if it does need to be skipped, the fuzz function could skip on this (known) input.

@timothy-king
Copy link
Contributor

@bcmills For other contexts that use essentially only []byte based fuzzers, they have found 9 reasonable deterministic inputs are nil, {0x0}, ..., {0x8} for unit test mode IIRC. Running all the zero value input is very much in this spirit. I also do not have a problem with running ~10 deterministic instances (seed using hash of test name or something similar). Both schemes are >0.

@nightlyone
Copy link
Contributor

How much longer would the typical test on file save and go test ./... given single file changes e.g. in text/template with hot caches, but on a repository like the Go projects take in that case?

@eliasnaur
Copy link
Contributor Author

I also do not have a problem with running ~10 deterministic instances (seed using hash of test name or something similar). Both schemes are >0.

I like this idea of fuzzing a bit with deterministic input. If you make the count 100, it matches testing.Quick :)

I'm confused about the proposal. How much additional time should every invocation of 'go test' take?

Given the objections to unexpected non-deterministic input, I now believe changing my proposal to do 100 fuzz calls with deterministic input is the best option.

@rsc
Copy link
Contributor

rsc commented Feb 23, 2022

Fuzzing is not terribly cheap. Doing it always is almost certainly not a good idea.

@rsc rsc moved this from Active to Likely Decline in Proposals (old) Mar 2, 2022
@rsc
Copy link
Contributor

rsc commented Mar 2, 2022

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@rogpeppe
Copy link
Contributor

rogpeppe commented Mar 3, 2022

Fuzzing is not terribly cheap. Doing it always is almost certainly not a good idea.

Isn't doing just a few rounds of fuzzing cheap? In general a single fuzzing iteration tends to be quite cheap, otherwise there's not much point in fuzzing because there wouldn't be much hope of getting good code coverage. Most of the functionality that I've seen fuzzed runs in the order of a millisecond or so per iteration at most.

So I'd support running a few rounds of fuzzing (maybe even just one) just to make sure the code works in principle, much as we do with examples.

@eliasnaur
Copy link
Contributor Author

Fuzzing is not terribly cheap. Doing it always is almost certainly not a good idea.

Even if this proposal is eventually declined, please expand this comment to make the expense clearer. What do you mean by not terribly cheap? Compared to what? And is it relevant in the presence of test result caching?

@rsc
Copy link
Contributor

rsc commented Mar 9, 2022

Fuzzing requires creating a subprocess, setting up shared memory, feeding data, and just tons more machinery than running a simple test binary. And then there are the fuzzing functions themselves, which may also not be very cheap.

@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Mar 9, 2022
@rsc
Copy link
Contributor

rsc commented Mar 9, 2022

No change in consensus, so declined.
— rsc for the proposal review group

@rsc rsc closed this as completed Mar 9, 2022
@golang golang locked and limited conversation to collaborators Mar 9, 2023
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 Proposal
Projects
Status: No status
Development

No branches or pull requests

10 participants