]> Cypherpunks repositories - gostls13.git/commitdiff
[dev.fuzz] testing: F.Setenv plus various fixes and revisions
authorJay Conrod <jayconrod@google.com>
Fri, 10 Sep 2021 17:45:47 +0000 (10:45 -0700)
committerJay Conrod <jayconrod@google.com>
Fri, 10 Sep 2021 21:28:48 +0000 (21:28 +0000)
I spent some time looking through all the changes we've made to
testing and cmd/go/... on the dev.fuzz branch. CL 348469 shows those
differences. This CL fixes comments, TODOs, and simplifies code in a
few places. It also implements F.Setenv.

Change-Id: I6fd7ef5fbd0bb6055e38d56cb42bddcf6f4ffdaf
Reviewed-on: https://go-review.googlesource.com/c/go/+/349109
Trust: Jay Conrod <jayconrod@google.com>
Trust: Katie Hockman <katie@golang.org>
Run-TryBot: Jay Conrod <jayconrod@google.com>
Run-TryBot: Katie Hockman <katie@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
src/cmd/go/internal/cfg/cfg.go
src/cmd/go/testdata/script/test_fuzz_setenv.txt [new file with mode: 0644]
src/testing/fuzz.go
src/testing/testing.go

index 7b6c429f4253375a9c0f68a98f6f1c9adfe13db2..ac2c70fa60028768f66fb743e38bcc1fedbb22eb 100644 (file)
@@ -61,7 +61,7 @@ var (
 func defaultContext() build.Context {
        ctxt := build.Default
 
-       // TODO(b/187972950): remove this tag before merging to master.
+       // TODO(#47037): remove this tag before merging to master.
        ctxt.BuildTags = []string{"gofuzzbeta"}
 
        ctxt.JoinPath = filepath.Join // back door to say "do not use go command"
diff --git a/src/cmd/go/testdata/script/test_fuzz_setenv.txt b/src/cmd/go/testdata/script/test_fuzz_setenv.txt
new file mode 100644 (file)
index 0000000..9738697
--- /dev/null
@@ -0,0 +1,45 @@
+[short] skip
+[!darwin] [!linux] [!windows] skip
+
+go test -fuzz=FuzzA -fuzztime=100x fuzz_setenv_test.go
+
+-- fuzz_setenv_test.go --
+package fuzz
+
+import (
+  "flag"
+  "os"
+  "testing"
+)
+
+func FuzzA(f *testing.F) {
+  if s := os.Getenv("TEST_FUZZ_SETENV_A"); isWorker() && s == "" {
+    f.Fatal("environment variable not set")
+  } else if !isWorker() && s != "" {
+    f.Fatal("environment variable already set")
+  }
+  f.Setenv("TEST_FUZZ_SETENV_A", "A")
+  if os.Getenv("TEST_FUZZ_SETENV_A") == "" {
+    f.Fatal("Setenv did not set environment variable")
+  }
+  f.Fuzz(func(*testing.T, []byte) {})
+}
+
+func FuzzB(f *testing.F) {
+  if os.Getenv("TEST_FUZZ_SETENV_A") != "" {
+    f.Fatal("environment variable not cleared after FuzzA")
+  }
+  f.Skip()
+}
+
+func isWorker() bool {
+       f := flag.Lookup("test.fuzzworker")
+       if f == nil {
+               return false
+       }
+       get, ok := f.Value.(flag.Getter)
+       if !ok {
+               return false
+       }
+       return get.Get() == interface{}(true)
+}
index d94ec35dc744a9c7f70102f43487a27325d2b15f..65c3437ed4fe1e268011e32da98c79c1867202d1 100644 (file)
@@ -49,22 +49,35 @@ type InternalFuzzTarget struct {
        Fn   func(f *F)
 }
 
-// F is a type passed to fuzz targets for fuzz testing.
+// F is a type passed to fuzz targets.
+//
+// A fuzz target may add seed corpus entries using F.Add or by storing files in
+// the testdata/fuzz/<FuzzTargetName> directory. The fuzz target must then
+// call F.Fuzz once to provide a fuzz function. See the testing package
+// documentation for an example, and see the F.Fuzz and F.Add method
+// documentation for details.
 type F struct {
        common
        fuzzContext *fuzzContext
        testContext *testContext
-       inFuzzFn    bool          // set to true when fuzz function is running
-       corpus      []corpusEntry // corpus is the in-memory corpus
-       result      FuzzResult    // result is the result of running the fuzz target
-       fuzzCalled  bool
+
+       // inFuzzFn is true when the fuzz function is running. Most F methods cannot
+       // be called when inFuzzFn is true.
+       inFuzzFn bool
+
+       // corpus is a set of seed corpus entries, added with F.Add and loaded
+       // from testdata.
+       corpus []corpusEntry
+
+       result     FuzzResult
+       fuzzCalled bool
 }
 
 var _ TB = (*F)(nil)
 
 // corpusEntry is an alias to the same type as internal/fuzz.CorpusEntry.
 // We use a type alias because we don't want to export this type, and we can't
-// importing internal/fuzz from testing.
+// import internal/fuzz from testing.
 type corpusEntry = struct {
        Parent     string
        Name       string
@@ -73,9 +86,9 @@ type corpusEntry = struct {
        Generation int
 }
 
-// Cleanup registers a function to be called when the test and all its
-// subtests complete. Cleanup functions will be called in last added,
-// first called order.
+// Cleanup registers a function to be called after the fuzz function has been
+// called on all seed corpus entries, and after fuzzing completes (if enabled).
+// Cleanup functions will be called in last added, first called order.
 func (f *F) Cleanup(fn func()) {
        if f.inFuzzFn {
                panic("testing: f.Cleanup was called inside the f.Fuzz function, use t.Cleanup instead")
@@ -114,9 +127,9 @@ func (f *F) Fail() {
 // FailNow marks the function as having failed and stops its execution
 // by calling runtime.Goexit (which then runs all deferred calls in the
 // current goroutine).
-// Execution will continue at the next test or benchmark.
+// Execution will continue at the next test, benchmark, or fuzz function.
 // FailNow must be called from the goroutine running the
-// test or benchmark function, not from other goroutines
+// fuzz target, not from other goroutines
 // created during the test. Calling FailNow does not stop
 // those other goroutines.
 func (f *F) FailNow() {
@@ -173,9 +186,18 @@ func (f *F) Helper() {
        }
 }
 
-// Setenv is not supported since fuzzing runs in parallel.
+// Setenv calls os.Setenv(key, value) and uses Cleanup to restore the
+// environment variable to its original value after the test.
+//
+// When fuzzing is enabled, the fuzzing engine spawns worker processes running
+// the test binary. Each worker process inherits the environment of the parent
+// process, including environment variables set with F.Setenv.
 func (f *F) Setenv(key, value string) {
-       panic("testing: f.Setenv is not supported")
+       if f.inFuzzFn {
+               panic("testing: f.Setenv was called inside the f.Fuzz function, use t.Setenv instead")
+       }
+       f.common.Helper()
+       f.common.Setenv(key, value)
 }
 
 // Skip is equivalent to Log followed by SkipNow.
@@ -305,26 +327,27 @@ func (f *F) Fuzz(ff interface{}) {
                types = append(types, t)
        }
 
-       // Only load the corpus if we need it
-       if f.fuzzContext.runFuzzWorker == nil {
-               // Check the corpus provided by f.Add
+       // Load the testdata seed corpus. Check types of entries in the testdata
+       // corpus and entries declared with F.Add.
+       //
+       // Don't load the seed corpus if this is a worker process; we won't use it.
+       if f.fuzzContext.mode != fuzzWorker {
                for _, c := range f.corpus {
-                       if err := f.fuzzContext.checkCorpus(c.Values, types); err != nil {
-                               // TODO: Is there a way to save which line number is associated
-                               // with the f.Add call that failed?
+                       if err := f.fuzzContext.deps.CheckCorpus(c.Values, types); err != nil {
+                               // TODO(#48302): Report the source location of the F.Add call.
                                f.Fatal(err)
                        }
                }
 
                // Load seed corpus
-               c, err := f.fuzzContext.readCorpus(filepath.Join(corpusDir, f.name), types)
+               c, err := f.fuzzContext.deps.ReadCorpus(filepath.Join(corpusDir, f.name), types)
                if err != nil {
                        f.Fatal(err)
                }
 
-               // If this is the coordinator process, zero the values, since we don't need to hold
-               // onto them.
-               if f.fuzzContext.coordinateFuzzing != nil {
+               // If this is the coordinator process, zero the values, since we don't need
+               // to hold onto them.
+               if f.fuzzContext.mode == fuzzCoordinator {
                        for i := range c {
                                c[i].Values = nil
                        }
@@ -336,8 +359,6 @@ func (f *F) Fuzz(ff interface{}) {
        // run calls fn on a given input, as a subtest with its own T.
        // run is analogous to T.Run. The test filtering and cleanup works similarly.
        // fn is called in its own goroutine.
-       //
-       // TODO(jayconrod,katiehockman): dedupe testdata corpus with entries from f.Add
        run := func(e corpusEntry) error {
                if e.Values == nil {
                        // Every code path should have already unmarshaled Data into Values.
@@ -372,7 +393,8 @@ func (f *F) Fuzz(ff interface{}) {
                }
                t.w = indenter{&t.common}
                if t.chatty != nil {
-                       t.chatty.Updatef(t.name, "=== RUN  %s\n", t.name)
+                       // TODO(#48132): adjust this to work with test2json.
+                       t.chatty.Updatef(t.name, "=== RUN   %s\n", t.name)
                }
                f.inFuzzFn = true
                go tRunner(t, func(t *T) {
@@ -384,8 +406,8 @@ func (f *F) Fuzz(ff interface{}) {
                        // make sure it is called right before the tRunner function exits,
                        // regardless of whether it was executed cleanly, panicked, or if the
                        // fuzzFn called t.Fatal.
-                       defer f.fuzzContext.snapshotCoverage()
-                       f.fuzzContext.resetCoverage()
+                       defer f.fuzzContext.deps.SnapshotCoverage()
+                       f.fuzzContext.deps.ResetCoverage()
                        fn.Call(args)
                })
                <-t.signal
@@ -396,14 +418,14 @@ func (f *F) Fuzz(ff interface{}) {
                return nil
        }
 
-       switch {
-       case f.fuzzContext.coordinateFuzzing != nil:
+       switch f.fuzzContext.mode {
+       case fuzzCoordinator:
                // Fuzzing is enabled, and this is the test process started by 'go test'.
                // Act as the coordinator process, and coordinate workers to perform the
                // actual fuzzing.
                corpusTargetDir := filepath.Join(corpusDir, f.name)
                cacheTargetDir := filepath.Join(*fuzzCacheDir, f.name)
-               err := f.fuzzContext.coordinateFuzzing(
+               err := f.fuzzContext.deps.CoordinateFuzzing(
                        fuzzDuration.d,
                        int64(fuzzDuration.n),
                        minimizeDuration.d,
@@ -420,16 +442,16 @@ func (f *F) Fuzz(ff interface{}) {
                        if crashErr, ok := err.(fuzzCrashError); ok {
                                crashName := crashErr.CrashName()
                                fmt.Fprintf(f.w, "Crash written to %s\n", filepath.Join(corpusDir, f.name, crashName))
-                               fmt.Fprintf(f.w, "To re-run:\ngo test %s -run=%s/%s\n", f.fuzzContext.importPath(), f.name, crashName)
+                               fmt.Fprintf(f.w, "To re-run:\ngo test %s -run=%s/%s\n", f.fuzzContext.deps.ImportPath(), f.name, crashName)
                        }
                }
                // TODO(jayconrod,katiehockman): Aggregate statistics across workers
                // and add to FuzzResult (ie. time taken, num iterations)
 
-       case f.fuzzContext.runFuzzWorker != nil:
+       case fuzzWorker:
                // Fuzzing is enabled, and this is a worker process. Follow instructions
                // from the coordinator.
-               if err := f.fuzzContext.runFuzzWorker(run); err != nil {
+               if err := f.fuzzContext.deps.RunFuzzWorker(run); err != nil {
                        // Internal errors are marked with f.Fail; user code may call this too, before F.Fuzz.
                        // The worker will exit with fuzzWorkerExitCode, indicating this is a failure
                        // (and 'go test' should exit non-zero) but a crasher should not be recorded.
@@ -503,17 +525,20 @@ type fuzzCrashError interface {
        CrashName() string
 }
 
-// fuzzContext holds all fields that are common to all fuzz targets.
+// fuzzContext holds fields common to all fuzz targets.
 type fuzzContext struct {
-       importPath        func() string
-       coordinateFuzzing func(time.Duration, int64, time.Duration, int64, int, []corpusEntry, []reflect.Type, string, string) error
-       runFuzzWorker     func(func(corpusEntry) error) error
-       readCorpus        func(string, []reflect.Type) ([]corpusEntry, error)
-       checkCorpus       func(vals []interface{}, types []reflect.Type) error
-       resetCoverage     func()
-       snapshotCoverage  func()
+       deps testDeps
+       mode fuzzMode
 }
 
+type fuzzMode uint8
+
+const (
+       seedCorpusOnly fuzzMode = iota
+       fuzzCoordinator
+       fuzzWorker
+)
+
 // runFuzzTargets runs the fuzz targets matching the pattern for -run. This will
 // only run the f.Fuzz function for each seed corpus without using the fuzzing
 // engine to generate or mutate inputs.
@@ -525,13 +550,7 @@ func runFuzzTargets(deps testDeps, fuzzTargets []InternalFuzzTarget, deadline ti
        m := newMatcher(deps.MatchString, *match, "-test.run")
        tctx := newTestContext(*parallel, m)
        tctx.deadline = deadline
-       fctx := &fuzzContext{
-               importPath:       deps.ImportPath,
-               readCorpus:       deps.ReadCorpus,
-               checkCorpus:      deps.CheckCorpus,
-               resetCoverage:    deps.ResetCoverage,
-               snapshotCoverage: deps.SnapshotCoverage,
-       }
+       fctx := &fuzzContext{deps: deps, mode: seedCorpusOnly}
        root := common{w: os.Stdout} // gather output in one place
        if Verbose() {
                root.chatty = newChattyPrinter(root.w)
@@ -558,7 +577,8 @@ func runFuzzTargets(deps testDeps, fuzzTargets []InternalFuzzTarget, deadline ti
                }
                f.w = indenter{&f.common}
                if f.chatty != nil {
-                       f.chatty.Updatef(f.name, "=== RUN  %s\n", f.name)
+                       // TODO(#48132): adjust this to work with test2json.
+                       f.chatty.Updatef(f.name, "=== RUN   %s\n", f.name)
                }
 
                go fRunner(f, ft.Fn)
@@ -583,18 +603,14 @@ func runFuzzing(deps testDeps, fuzzTargets []InternalFuzzTarget) (ran bool, matc
        m := newMatcher(deps.MatchString, *matchFuzz, "-test.fuzz")
        tctx := newTestContext(1, m)
        fctx := &fuzzContext{
-               importPath:       deps.ImportPath,
-               readCorpus:       deps.ReadCorpus,
-               checkCorpus:      deps.CheckCorpus,
-               resetCoverage:    deps.ResetCoverage,
-               snapshotCoverage: deps.SnapshotCoverage,
+               deps: deps,
        }
        root := common{w: os.Stdout}
        if *isFuzzWorker {
                root.w = io.Discard
-               fctx.runFuzzWorker = deps.RunFuzzWorker
+               fctx.mode = fuzzWorker
        } else {
-               fctx.coordinateFuzzing = deps.CoordinateFuzzing
+               fctx.mode = fuzzCoordinator
        }
        if Verbose() && !*isFuzzWorker {
                root.chatty = newChattyPrinter(root.w)
@@ -628,6 +644,7 @@ func runFuzzing(deps testDeps, fuzzTargets []InternalFuzzTarget) (ran bool, matc
        }
        f.w = indenter{&f.common}
        if f.chatty != nil {
+               // TODO(#48132): adjust this to work with test2json.
                f.chatty.Updatef(f.name, "=== FUZZ  %s\n", f.name)
        }
        go fRunner(f, target.Fn)
@@ -637,9 +654,9 @@ func runFuzzing(deps testDeps, fuzzTargets []InternalFuzzTarget) (ran bool, matc
 
 // fRunner wraps a call to a fuzz target and ensures that cleanup functions are
 // called and status flags are set. fRunner should be called in its own
-// goroutine. To wait for its completion, receive f.signal.
+// goroutine. To wait for its completion, receive from f.signal.
 //
-// fRunner is analogous with tRunner, which wraps subtests started with T.Run.
+// fRunner is analogous to tRunner, which wraps subtests started with T.Run.
 // Tests and fuzz targets work a little differently, so for now, these functions
 // aren't consolidated. In particular, because there are no F.Run and F.Parallel
 // methods, i.e., no fuzz sub-targets or parallel fuzz targets, a few
@@ -651,11 +668,11 @@ func fRunner(f *F, fn func(*F)) {
        // t.signal, indicating the fuzz target is done.
        defer func() {
                // Detect whether the fuzz target panicked or called runtime.Goexit without
-               // calling F.Fuzz, F.Fail, or F.Skip. If it did, panic (possibly replacing
-               // a nil panic value). Nothing should recover after fRunner unwinds,
-               // so this should crash the process with a stack. Unfortunately, recovering
-               // here adds stack frames, but the location of the original panic should
-               // still be clear.
+               // calling F.Fuzz, F.Fail, or F.Skip. If it did, panic (possibly replacing a
+               // nil panic value). Nothing should recover after fRunner unwinds, so this
+               // should crash the process and print stack. Unfortunately, recovering here
+               // adds stack frames, but the location of the original panic should still be
+               // clear.
                if f.Failed() {
                        atomic.AddUint32(&numFailed, 1)
                }
@@ -708,8 +725,9 @@ func fRunner(f *F, fn func(*F)) {
                f.duration += time.Since(f.start)
 
                if len(f.sub) > 0 {
-                       // Run parallel inputs.
-                       // Release the parallel subtests.
+                       // Unblock inputs that called T.Parallel while running the seed corpus.
+                       // T.Parallel has no effect while fuzzing, so this only affects fuzz
+                       // targets run as normal tests.
                        close(f.barrier)
                        // Wait for the subtests to complete.
                        for _, sub := range f.sub {
index be21d643fd249ca2e33a121e2120567c1bb55559..5e66a0610bd20907aa5cc11c48985f204365a0b9 100644 (file)
 // https://golang.org/cmd/go/#hdr-Testing_flags.
 //
 // For a description of fuzzing, see golang.org/s/draft-fuzzing-design.
+// TODO(#48255): write and link to documentation that will be helpful to users
+// who are unfamiliar with fuzzing.
 //
 // A sample fuzz target looks like this:
+//
 //     func FuzzBytesCmp(f *testing.F) {
 //         f.Fuzz(func(t *testing.T, a, b []byte) {
 //             if bytes.HasPrefix(a, b) && !bytes.Contains(a, b) {
@@ -582,8 +585,6 @@ func (c *common) frameSkip(skip int) runtime.Frame {
 // and inserts the final newline if needed and indentation spaces for formatting.
 // This function must be called with c.mu held.
 func (c *common) decorate(s string, skip int) string {
-       // TODO(jayconrod,katiehockman): Consider refactoring the logging logic.
-       // If more helper PCs have been added since we last did the conversion
        if c.helperNames == nil {
                c.helperNames = make(map[string]struct{})
                for pc := range c.helperPCs {
@@ -663,11 +664,8 @@ func (c *common) flushToParent(testName, format string, args ...interface{}) {
 // isFuzzing returns whether the current context, or any of the parent contexts,
 // are a fuzzing target
 func (c *common) isFuzzing() bool {
-       if c.fuzzing {
-               return true
-       }
-       for parent := c.parent; parent != nil; parent = parent.parent {
-               if parent.fuzzing {
+       for com := c; com != nil; com = com.parent {
+               if com.fuzzing {
                        return true
                }
        }
@@ -1228,10 +1226,25 @@ func tRunner(t *T, fn func(t *T)) {
                        t.Errorf("race detected during execution of test")
                }
 
-               // If the test panicked, print any test output before dying.
+               // Check if the test panicked or Goexited inappropriately.
+               //
+               // If this happens in a normal test, print output but continue panicking.
+               // tRunner is called in its own goroutine, so this terminates the process.
+               //
+               // If this happens while fuzzing, recover from the panic and treat it like a
+               // normal failure. It's important that the process keeps running in order to
+               // find short inputs that cause panics.
                err := recover()
                signal := true
 
+               if err != nil && t.isFuzzing() {
+                       t.Errorf("panic: %s\n%s\n", err, string(debug.Stack()))
+                       t.mu.Lock()
+                       t.finished = true
+                       t.mu.Unlock()
+                       err = nil
+               }
+
                t.mu.RLock()
                finished := t.finished
                t.mu.RUnlock()
@@ -1249,15 +1262,15 @@ func tRunner(t *T, fn func(t *T)) {
                                }
                        }
                }
+
                // Use a deferred call to ensure that we report that the test is
                // complete even if a cleanup function calls t.FailNow. See issue 41355.
                didPanic := false
                defer func() {
-                       isFuzzing := t.common.isFuzzing()
-                       if didPanic && !isFuzzing {
+                       if didPanic {
                                return
                        }
-                       if err != nil && !isFuzzing {
+                       if err != nil {
                                panic(err)
                        }
                        // Only report that the test is complete if it doesn't panic,
@@ -1283,12 +1296,6 @@ func tRunner(t *T, fn func(t *T)) {
                                }
                        }
                        didPanic = true
-                       if t.common.fuzzing {
-                               for root := &t.common; root.parent != nil; root = root.parent {
-                                       fmt.Fprintf(root.parent.w, "panic: %s\n%s\n", err, string(debug.Stack()))
-                               }
-                               return
-                       }
                        panic(err)
                }
                if err != nil {
@@ -1325,7 +1332,7 @@ func tRunner(t *T, fn func(t *T)) {
                t.report() // Report after all subtests have finished.
 
                // Do not lock t.done to allow race detector to detect race in case
-               // the user does not appropriately synchronizes a goroutine.
+               // the user does not appropriately synchronize a goroutine.
                t.done = true
                if t.parent != nil && atomic.LoadInt32(&t.hasSub) == 0 {
                        t.setRan()
@@ -1571,7 +1578,7 @@ func (m *M) Run() (code int) {
                return
        }
        if *matchFuzz != "" && *fuzzCacheDir == "" {
-               fmt.Fprintln(os.Stderr, "testing: internal error: -test.fuzzcachedir must be set if -test.fuzz is set")
+               fmt.Fprintln(os.Stderr, "testing: -test.fuzzcachedir must be set if -test.fuzz is set")
                flag.Usage()
                m.exitCode = 2
                return
@@ -1606,9 +1613,11 @@ func (m *M) Run() (code int) {
 
        m.before()
        defer m.after()
+
+       // Run tests, examples, and benchmarks unless this is a fuzz worker process.
+       // Workers start after this is done by their parent process, and they should
+       // not repeat this work.
        if !*isFuzzWorker {
-               // The fuzzing coordinator will already run all tests, examples,
-               // and benchmarks. Don't make the workers do redundant work.
                deadline := m.startAlarm()
                haveExamples = len(m.examples) > 0
                testRan, testOk := runTests(m.deps.MatchString, m.tests, deadline)