[dev.fuzz] testing: F.Setenv plus various fixes and revisions

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

@@ -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"

src/cmd/go/testdata/script/test_fuzz_setenv.txt

@@ -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)
+}

src/testing/fuzz.go

@@ -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
+	// inFuzzFn is true when the fuzz function is running. Most F methods cannot
-	result      FuzzResult    // result is the result of running the fuzz target
+	// be called when inFuzzFn is true.
-	fuzzCalled  bool
+	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
+// Cleanup registers a function to be called after the fuzz function has been
-// subtests complete. Cleanup functions will be called in last added,
+// called on all seed corpus entries, and after fuzzing completes (if enabled).
-// first called order.
+// 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
+	// Load the testdata seed corpus. Check types of entries in the testdata
-	if f.fuzzContext.runFuzzWorker == nil {
+	// corpus and entries declared with F.Add.
-		// Check the corpus provided by 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 {
+			if err := f.fuzzContext.deps.CheckCorpus(c.Values, types); err != nil {
-				// TODO: Is there a way to save which line number is associated
+				// TODO(#48302): Report the source location of the F.Add call.
-				// with the f.Add call that failed?
 				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
+		// If this is the coordinator process, zero the values, since we don't need
-		// onto them.
+		// to hold onto them.
-		if f.fuzzContext.coordinateFuzzing != nil {
+		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()
+			defer f.fuzzContext.deps.SnapshotCoverage()
-			f.fuzzContext.resetCoverage()
+			f.fuzzContext.deps.ResetCoverage()
 			fn.Call(args)
 		})
 		<-t.signal
@@ -396,14 +418,14 @@ func (f *F) Fuzz(ff interface{}) {
 		return nil
 	}
 
-	switch {
+	switch f.fuzzContext.mode {
-	case f.fuzzContext.coordinateFuzzing != nil:
+	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
+	deps testDeps
-	coordinateFuzzing func(time.Duration, int64, time.Duration, int64, int, []corpusEntry, []reflect.Type, string, string) error
+	mode fuzzMode
-	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()
 }
 
+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{
+	fctx := &fuzzContext{deps: deps, mode: seedCorpusOnly}
-		importPath:       deps.ImportPath,
-		readCorpus:       deps.ReadCorpus,
-		checkCorpus:      deps.CheckCorpus,
-		resetCoverage:    deps.ResetCoverage,
-		snapshotCoverage: deps.SnapshotCoverage,
-	}
 	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,
+		deps: deps,
-		readCorpus:       deps.ReadCorpus,
-		checkCorpus:      deps.CheckCorpus,
-		resetCoverage:    deps.ResetCoverage,
-		snapshotCoverage: deps.SnapshotCoverage,
 	}
 	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
+		// calling F.Fuzz, F.Fail, or F.Skip. If it did, panic (possibly replacing a
-		// a nil panic value). Nothing should recover after fRunner unwinds,
+		// nil panic value). Nothing should recover after fRunner unwinds, so this
-		// so this should crash the process with a stack. Unfortunately, recovering
+		// should crash the process and print stack. Unfortunately, recovering here
-		// here adds stack frames, but the location of the original panic should
+		// adds stack frames, but the location of the original panic should still be
-		// still be clear.
+		// 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.
+			// Unblock inputs that called T.Parallel while running the seed corpus.
-			// Release the parallel subtests.
+			// 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 {

src/testing/testing.go

@@ -143,8 +143,11 @@
 // 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 {
+	for com := c; com != nil; com = com.parent {
-		return true
+		if com.fuzzing {
-	}
-	for parent := c.parent; parent != nil; parent = parent.parent {
-		if parent.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 {
-			if didPanic && !isFuzzing {
 				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)