[go/dev.fuzz] [dev.fuzz] testing: support T.Parallel in fuzz functions
63 views
Skip to first unread message
Jay Conrod (Gerrit)
Mar 10, 2021, 11:17:20 AM3/10/21
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Jay Conrod has uploaded this change for review.
[dev.fuzz] testing: support T.Parallel in fuzz functions
While running the seed corpus, T.Parallel acts like it does in
subtests started with T.Run: it blocks until all other non-parallel
subtests have finished, then unblocks when the barrier chan is
closed. A semaphore (t.context.waitParallel) limits the number of
tests that run concurrently (determined by -test.parallel).
While fuzzing, T.Parallel has no effect, other than asserting that it
can't be called multiple times. We already run different inputs in
concurrent processes, but we can't run inputs concurrently in the same
process if we want to attribute crashes to specific inputs.
Change-Id: I2bac08e647e1d92ea410c83c3f3558a033fe3dd1
---
A src/cmd/go/testdata/script/test_fuzz_parallel.txt
M src/internal/fuzz/worker.go
M src/testing/fuzz.go
M src/testing/testing.go
4 files changed, 135 insertions(+), 24 deletions(-)
diff --git a/src/cmd/go/testdata/script/test_fuzz_parallel.txt b/src/cmd/go/testdata/script/test_fuzz_parallel.txt
new file mode 100644
index 0000000..d9f6cc7
--- /dev/null
+++ b/src/cmd/go/testdata/script/test_fuzz_parallel.txt
@@ -0,0 +1,61 @@
+# TODO(jayconrod): support shared memory on more platforms.
+[!darwin] [!linux] [!windows] skip
+
+[short] skip
+
+# When running seed inputs, T.Parallel should let multiple inputs run in
+# parallel.
+go test -run=FuzzSeed
+
+# When fuzzing, T.Parallel should be safe to call, but it should have no effect.
+# We just check that it doesn't hang, which would be the most obvious
+# failure mode.
+# TODO(jayconrod): check for the string "after T.Parallel". It's not printed
+# by 'go test', so we can't distinguish that crasher from some other panic.
+! go test -run=FuzzMutate -fuzz=FuzzMutate
+exists testdata/corpus/FuzzMutate
+
+-- go.mod --
+module fuzz_parallel
+
+go 1.17
+-- fuzz_parallel_test.go --
+package fuzz_parallel
+
+import (
+ "sort"
+ "sync"
+ "testing"
+)
+
+func FuzzSeed(f *testing.F) {
+ for _, v := range [][]byte{{'a'}, {'b'}, {'c'}} {
+ f.Add(v)
+ }
+
+ var mu sync.Mutex
+ var before, after []byte
+ f.Cleanup(func() {
+ sort.Slice(after, func(i, j int) bool { return after[i] < after[j] })
+ got := string(before) + string(after)
+ want := "abcabc"
+ if got != want {
+ f.Fatalf("got %q; want %q", got, want)
+ }
+ })
+
+ f.Fuzz(func(t *testing.T, b []byte) {
+ before = append(before, b...)
+ t.Parallel()
+ mu.Lock()
+ after = append(after, b...)
+ mu.Unlock()
+ })
+}
+
+func FuzzMutate(f *testing.F) {
+ f.Fuzz(func(t *testing.T, _ []byte) {
+ t.Parallel()
+ t.Error("after T.Parallel")
+ })
+}
diff --git a/src/internal/fuzz/worker.go b/src/internal/fuzz/worker.go
index d42044b..752e0cb 100644
--- a/src/internal/fuzz/worker.go
+++ b/src/internal/fuzz/worker.go
@@ -279,18 +279,20 @@
// stop returns the error the process terminated with, if any (same as
// w.waitErr).
//
-// stop must be called once after start returns successfully, even if the
-// worker process terminates unexpectedly.
+// stop must be called at least once after start returns successfully, even if
+// the worker process terminates unexpectedly.
func (w *worker) stop() error {
if w.termC == nil {
panic("worker was not started successfully")
}
select {
case <-w.termC:
- // Worker already terminated, perhaps unexpectedly.
+ // Worker already terminated.
if w.client == nil {
- panic("worker already stopped")
+ // stop already called.
+ return w.waitErr
}
+ // Possible unexpected termination.
w.client.Close()
w.cmd = nil
w.client = nil
diff --git a/src/testing/fuzz.go b/src/testing/fuzz.go
index c855379..28d741a 100644
--- a/src/testing/fuzz.go
+++ b/src/testing/fuzz.go
@@ -298,7 +298,6 @@
// fn is called in its own goroutine.
//
// TODO(jayconrod,katiehockman): dedupe testdata corpus with entries from f.Add
- // TODO(jayconrod,katiehockman): handle T.Parallel calls within fuzz function.
run := func(e corpusEntry) error {
if e.Values == nil {
// Every code path should have already unmarshaled Data into Values.
@@ -478,11 +477,12 @@
}
f := &F{
common: common{
- signal: make(chan bool),
- name: testName,
- parent: &root,
- level: root.level + 1,
- chatty: root.chatty,
+ signal: make(chan bool),
+ barrier: make(chan bool),
+ name: testName,
+ parent: &root,
+ level: root.level + 1,
+ chatty: root.chatty,
},
testContext: tctx,
fuzzContext: fctx,
@@ -538,11 +538,12 @@
target = ft
f = &F{
common: common{
- signal: make(chan bool),
- name: testName,
- parent: &root,
- level: root.level + 1,
- chatty: root.chatty,
+ signal: make(chan bool),
+ barrier: nil, // T.Parallel has no effect when fuzzing.
+ name: testName,
+ parent: &root,
+ level: root.level + 1,
+ chatty: root.chatty,
},
fuzzContext: fctx,
testContext: tctx,
@@ -566,7 +567,10 @@
//
// fRunner is analogous with tRunner, which wraps subtests started with T.Run.
// Tests and fuzz targets work a little differently, so for now, these functions
-// aren't consoldiated.
+// 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
+// simplifications are made. We also require that F.Fuzz, F.Skip, or F.Fail is
+// called.
func fRunner(f *F, fn func(*F)) {
// When this goroutine is done, either because runtime.Goexit was called,
// a panic started, or fn returned normally, record the duration and send
@@ -589,10 +593,29 @@
err = errNilPanicOrGoexit
}
+ // 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() {
+ if didPanic {
+ return
+ }
+ if err != nil {
+ panic(err)
+ }
+ // Only report that the test is complete if it doesn't panic,
+ // as otherwise the test binary can exit before the panic is
+ // reported to the user. See issue 41479.
+ f.signal <- true
+ }()
+
// If we recovered a panic or inappropriate runtime.Goexit, fail the test,
// flush the output log up to the root, then panic.
- if err != nil {
+ doPanic := func(err interface{}) {
f.Fail()
+ if r := f.runCleanup(recoverAndReturnPanic); r != nil {
+ f.Logf("cleanup panicked with %v", r)
+ }
for root := &f.common; root.parent != nil; root = root.parent {
root.mu.Lock()
root.duration += time.Since(root.start)
@@ -600,22 +623,41 @@
root.mu.Unlock()
root.flushToParent(root.name, "--- FAIL: %s (%s)\n", root.name, fmtDuration(d))
}
+ didPanic = true
panic(err)
}
+ if err != nil {
+ doPanic(err)
+ }
- // No panic or inappropriate Goexit. Record duration and report the result.
+ // No panic or inappropriate Goexit.
f.duration += time.Since(f.start)
+
+ if len(f.sub) > 0 {
+ // Run parallel inputs.
+ // Release the parallel subtests.
+ close(f.barrier)
+ // Wait for the subtests to complete.
+ for _, sub := range f.sub {
+ <-sub.signal
+ }
+ cleanupStart := time.Now()
+ err := f.runCleanup(recoverAndReturnPanic)
+ f.duration += time.Since(cleanupStart)
+ if err != nil {
+ doPanic(err)
+ }
+ }
+
+ // Report after all subtests have finished.
f.report()
f.done = true
f.setRan()
-
- // Only report that the test is complete if it doesn't panic,
- // as otherwise the test binary can exit before the panic is
- // reported to the user. See issue 41479.
- f.signal <- true
}()
defer func() {
- f.runCleanup(normalPanic)
+ if len(f.sub) == 0 {
+ f.runCleanup(normalPanic)
+ }
}()
f.start = time.Now()
diff --git a/src/testing/testing.go b/src/testing/testing.go
index 2ad39f7..2ca411f 100644
--- a/src/testing/testing.go
+++ b/src/testing/testing.go
@@ -1038,6 +1038,12 @@
panic("testing: t.Parallel called multiple times")
}
t.isParallel = true
+ if t.parent.barrier == nil {
+ // T.Parallel has no effect when fuzzing.
+ // Multiple processes may run in parallel, but only one input can run at a
+ // time per process so we can attribute crashes to specific inputs.
+ return
+ }
// We don't want to include the time we spend waiting for serial tests
// in the test duration. Record the elapsed time thus far and reset the
To view, visit change 300449. To unsubscribe, or for help writing mail filters, visit settings.
Jay Conrod (Gerrit)
Mar 10, 2021, 11:17:35 AM3/10/21
to goph...@pubsubhelper.golang.org, Katie Hockman, golang-co...@googlegroups.com
Attention is currently required from: Katie Hockman.
Patch set 1:Run-TryBot +1Trust +1
1 comment:
Patchset:
TRY=linux-amd64-longtest
To view, visit change 300449. To unsubscribe, or for help writing mail filters, visit settings.
Jay Conrod (Gerrit)
Mar 11, 2021, 1:05:19 PM3/11/21
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention is currently required from: Jay Conrod, Katie Hockman.
Jay Conrod uploaded patch set #2 to this change.
[dev.fuzz] testing: support T.Parallel in fuzz functions
While running the seed corpus, T.Parallel acts like it does in
subtests started with T.Run: it blocks until all other non-parallel
subtests have finished, then unblocks when the barrier chan is
closed. A semaphore (t.context.waitParallel) limits the number of
tests that run concurrently (determined by -test.parallel).
While fuzzing, T.Parallel has no effect, other than asserting that it
can't be called multiple times. We already run different inputs in
concurrent processes, but we can't run inputs concurrently in the same
process if we want to attribute crashes to specific inputs.
Change-Id: I2bac08e647e1d92ea410c83c3f3558a033fe3dd1
---
A src/cmd/go/testdata/script/test_fuzz_parallel.txt
M src/internal/fuzz/worker.go
M src/testing/fuzz.go
M src/testing/sub_test.go
M src/testing/testing.go
5 files changed, 139 insertions(+), 27 deletions(-)
To view, visit change 300449. To unsubscribe, or for help writing mail filters, visit settings.
Jay Conrod (Gerrit)
Mar 11, 2021, 1:06:12 PM3/11/21
to goph...@pubsubhelper.golang.org, Go Bot, Katie Hockman, golang-co...@googlegroups.com
Attention is currently required from: Katie Hockman.
Patch set 2:Run-TryBot +1Trust +1
To view, visit change 300449. To unsubscribe, or for help writing mail filters, visit settings.
Katie Hockman (Gerrit)
Mar 15, 2021, 4:48:47 PM3/15/21
to Jay Conrod, goph...@pubsubhelper.golang.org, Go Bot, golang-co...@googlegroups.com
Attention is currently required from: Jay Conrod.
Patch set 2:Code-Review +2
Jay Conrod (Gerrit)
Mar 24, 2021, 12:36:00 PM3/24/21
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention is currently required from: Jay Conrod.
Jay Conrod uploaded patch set #3 to this change.
[dev.fuzz] testing: support T.Parallel in fuzz functions
While running the seed corpus, T.Parallel acts like it does in
subtests started with T.Run: it blocks until all other non-parallel
subtests have finished, then unblocks when the barrier chan is
closed. A semaphore (t.context.waitParallel) limits the number of
tests that run concurrently (determined by -test.parallel).
While fuzzing, T.Parallel has no effect, other than asserting that it
can't be called multiple times. We already run different inputs in
concurrent processes, but we can't run inputs concurrently in the same
process if we want to attribute crashes to specific inputs.
Change-Id: I2bac08e647e1d92ea410c83c3f3558a033fe3dd1
---
A src/cmd/go/testdata/script/test_fuzz_parallel.txt
M src/internal/fuzz/worker.go
M src/testing/fuzz.go
M src/testing/sub_test.go
M src/testing/testing.go
5 files changed, 139 insertions(+), 27 deletions(-)
To view, visit change 300449. To unsubscribe, or for help writing mail filters, visit settings.
Jay Conrod (Gerrit)
Mar 24, 2021, 12:36:13 PM3/24/21
to goph...@pubsubhelper.golang.org, Katie Hockman, Go Bot, golang-co...@googlegroups.com
Patch set 3:Run-TryBot +1Trust +1
To view, visit change 300449. To unsubscribe, or for help writing mail filters, visit settings.
Jay Conrod (Gerrit)
Apr 2, 2021, 5:00:49 PM4/2/21
to goph...@pubsubhelper.golang.org, Go Bot, Katie Hockman, golang-co...@googlegroups.com
Patch set 4:Run-TryBot +1Trust +1
1 comment:
Patchset:
TRY=linux-amd64-longtest,windows-amd64-longtest
To view, visit change 300449. To unsubscribe, or for help writing mail filters, visit settings.
Jay Conrod (Gerrit)
Apr 5, 2021, 11:47:23 AM4/5/21
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Jay Conrod uploaded patch set #5 to this change.
[dev.fuzz] testing: support T.Parallel in fuzz functions
While running the seed corpus, T.Parallel acts like it does in
subtests started with T.Run: it blocks until all other non-parallel
subtests have finished, then unblocks when the barrier chan is
closed. A semaphore (t.context.waitParallel) limits the number of
tests that run concurrently (determined by -test.parallel).
While fuzzing, T.Parallel has no effect, other than asserting that it
can't be called multiple times. We already run different inputs in
concurrent processes, but we can't run inputs concurrently in the same
process if we want to attribute crashes to specific inputs.
Change-Id: I2bac08e647e1d92ea410c83c3f3558a033fe3dd1
---
A src/cmd/go/testdata/script/test_fuzz_parallel.txt
M src/internal/fuzz/worker.go
M src/testing/fuzz.go
M src/testing/sub_test.go
M src/testing/testing.go
5 files changed, 139 insertions(+), 27 deletions(-)
To view, visit change 300449. To unsubscribe, or for help writing mail filters, visit settings.
Jay Conrod (Gerrit)
Apr 5, 2021, 12:05:00 PM4/5/21
to goph...@pubsubhelper.golang.org, Go Bot, Katie Hockman, golang-co...@googlegroups.com
Patch set 6:Run-TryBot +1Trust +1
To view, visit change 300449. To unsubscribe, or for help writing mail filters, visit settings.
Jay Conrod (Gerrit)
Apr 8, 2021, 11:10:14 AM4/8/21
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Jay Conrod has uploaded this change for review.
[dev.fuzz] testing: support T.Parallel in fuzz functions
While running the seed corpus, T.Parallel acts like it does in
subtests started with T.Run: it blocks until all other non-parallel
subtests have finished, then unblocks when the barrier chan is
closed. A semaphore (t.context.waitParallel) limits the number of
tests that run concurrently (determined by -test.parallel).
While fuzzing, T.Parallel has no effect, other than asserting that it
can't be called multiple times. We already run different inputs in
concurrent processes, but we can't run inputs concurrently in the same
process if we want to attribute crashes to specific inputs.
Change-Id: I2bac08e647e1d92ea410c83c3f3558a033fe3dd1
---
A src/cmd/go/testdata/script/test_fuzz_parallel.txt
M src/internal/fuzz/worker.go
M src/testing/fuzz.go
M src/testing/sub_test.go
M src/testing/testing.go
5 files changed, 139 insertions(+), 27 deletions(-)
index 9766ab1..64aac73 100644
--- a/src/internal/fuzz/worker.go
+++ b/src/internal/fuzz/worker.go
@@ -274,18 +274,20 @@
// stop returns the error the process terminated with, if any (same as
// w.waitErr).
//
-// stop must be called once after start returns successfully, even if the
-// worker process terminates unexpectedly.
+// stop must be called at least once after start returns successfully, even if
+// the worker process terminates unexpectedly.
func (w *worker) stop() error {
if w.termC == nil {
panic("worker was not started successfully")
}
select {
case <-w.termC:
- // Worker already terminated, perhaps unexpectedly.
+ // Worker already terminated.
if w.client == nil {
- panic("worker already stopped")
+ // stop already called.
+ return w.waitErr
}
+ // Possible unexpected termination.
w.client.Close()
w.cmd = nil
w.client = nil
diff --git a/src/testing/fuzz.go b/src/testing/fuzz.go
index 0c1280c..70e1b41 100644
--- a/src/testing/fuzz.go
+++ b/src/testing/fuzz.go
@@ -298,7 +298,6 @@
// fn is called in its own goroutine.
//
// TODO(jayconrod,katiehockman): dedupe testdata corpus with entries from f.Add
- // TODO(jayconrod,katiehockman): handle T.Parallel calls within fuzz function.
// TODO(jayconrod,katiehockman): improve output when running the subtest.
// e.g. instead of
// --- FAIL: FuzzSomethingError/#00 (0.00s)
@@ -485,11 +484,12 @@
}
f := &F{
common: common{
- signal: make(chan bool),
- name: testName,
- parent: &root,
- level: root.level + 1,
- chatty: root.chatty,
+ signal: make(chan bool),
+ barrier: make(chan bool),
+ name: testName,
+ parent: &root,
+ level: root.level + 1,
+ chatty: root.chatty,
},
testContext: tctx,
fuzzContext: fctx,
@@ -548,11 +548,12 @@
target = ft
f = &F{
common: common{
- signal: make(chan bool),
- name: testName,
- parent: &root,
- level: root.level + 1,
- chatty: root.chatty,
+ signal: make(chan bool),
+ barrier: nil, // T.Parallel has no effect when fuzzing.
+ name: testName,
+ parent: &root,
+ level: root.level + 1,
+ chatty: root.chatty,
},
fuzzContext: fctx,
testContext: tctx,
@@ -576,7 +577,10 @@
//
// fRunner is analogous with tRunner, which wraps subtests started with T.Run.
// Tests and fuzz targets work a little differently, so for now, these functions
-// aren't consoldiated.
+// 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
+// simplifications are made. We also require that F.Fuzz, F.Skip, or F.Fail is
+// called.
func fRunner(f *F, fn func(*F)) {
// When this goroutine is done, either because runtime.Goexit was called,
// a panic started, or fn returned normally, record the duration and send
@@ -599,10 +603,29 @@@@ -610,22 +633,41 @@diff --git a/src/testing/sub_test.go b/src/testing/sub_test.go
index d2b966d..2d9e145 100644
--- a/src/testing/sub_test.go
+++ b/src/testing/sub_test.go
@@ -480,9 +480,10 @@
buf := &bytes.Buffer{}
root := &T{
common: common{
- signal: make(chan bool),
- name: "Test",
- w: buf,
+ signal: make(chan bool),
+ barrier: make(chan bool),
+ name: "Test",
+ w: buf,
},
context: ctx,
}
diff --git a/src/testing/testing.go b/src/testing/testing.go
index 48e9ee0..da26dec 100644
--- a/src/testing/testing.go
+++ b/src/testing/testing.go
@@ -1039,6 +1039,12 @@
panic("testing: t.Parallel called multiple times")
}
t.isParallel = true
+ if t.parent.barrier == nil {
+ // T.Parallel has no effect when fuzzing.
+ // Multiple processes may run in parallel, but only one input can run at a
+ // time per process so we can attribute crashes to specific inputs.
+ return
+ }
// We don't want to include the time we spend waiting for serial tests
// in the test duration. Record the elapsed time thus far and reset the
To view, visit change 308513. To unsubscribe, or for help writing mail filters, visit settings.
Jay Conrod (Gerrit)
Apr 8, 2021, 11:11:53 AM4/8/21
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Jay Conrod abandoned this change.
Abandoned
Duplicate created by 'git mail'
To view, visit change 308513. To unsubscribe, or for help writing mail filters, visit settings.
Jay Conrod (Gerrit)
Apr 8, 2021, 11:15:39 AM4/8/21
to goph...@pubsubhelper.golang.org, Go Bot, Katie Hockman, golang-co...@googlegroups.com
Patch set 7:Run-TryBot +1Trust +1
To view, visit change 300449. To unsubscribe, or for help writing mail filters, visit settings.
Jay Conrod (Gerrit)
Apr 8, 2021, 11:19:48 AM4/8/21
to goph...@pubsubhelper.golang.org, Go Bot, Katie Hockman, golang-co...@googlegroups.com
Patch set 8:Run-TryBot +1Trust +1
Jay Conrod (Gerrit)
Apr 9, 2021, 2:16:39 PM4/9/21
to goph...@pubsubhelper.golang.org, Go Bot, Katie Hockman, golang-co...@googlegroups.com
Patch set 9:Run-TryBot +1Trust +1
Jay Conrod (Gerrit)
Apr 9, 2021, 3:51:39 PM4/9/21
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Jay Conrod uploaded patch set #10 to this change.
[dev.fuzz] testing: support T.Parallel in fuzz functions
While running the seed corpus, T.Parallel acts like it does in
subtests started with T.Run: it blocks until all other non-parallel
subtests have finished, then unblocks when the barrier chan is
closed. A semaphore (t.context.waitParallel) limits the number of
tests that run concurrently (determined by -test.parallel).
While fuzzing, T.Parallel has no effect, other than asserting that it
can't be called multiple times. We already run different inputs in
concurrent processes, but we can't run inputs concurrently in the same
process if we want to attribute crashes to specific inputs.
Change-Id: I2bac08e647e1d92ea410c83c3f3558a033fe3dd1
---
A src/cmd/go/testdata/script/test_fuzz_parallel.txt
M src/internal/fuzz/worker.go
M src/testing/fuzz.go
M src/testing/sub_test.go
M src/testing/testing.go
5 files changed, 139 insertions(+), 27 deletions(-)
To view, visit change 300449. To unsubscribe, or for help writing mail filters, visit settings.
Jay Conrod (Gerrit)
Apr 9, 2021, 3:52:11 PM4/9/21
to goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, Go Bot, Katie Hockman, golang-co...@googlegroups.com
Jay Conrod submitted this change.
Approvals:
Katie Hockman: Looks good to me, approved
Jay Conrod: Trusted; Run TryBots
Go Bot: TryBots succeeded
[dev.fuzz] testing: support T.Parallel in fuzz functions
While running the seed corpus, T.Parallel acts like it does in
subtests started with T.Run: it blocks until all other non-parallel
subtests have finished, then unblocks when the barrier chan is
closed. A semaphore (t.context.waitParallel) limits the number of
tests that run concurrently (determined by -test.parallel).
While fuzzing, T.Parallel has no effect, other than asserting that it
can't be called multiple times. We already run different inputs in
concurrent processes, but we can't run inputs concurrently in the same
process if we want to attribute crashes to specific inputs.
Change-Id: I2bac08e647e1d92ea410c83c3f3558a033fe3dd1
Reviewed-on: https://go-review.googlesource.com/c/go/+/300449
Trust: Jay Conrod <jayc...@google.com>
Run-TryBot: Jay Conrod <jayc...@google.com>
TryBot-Result: Go Bot <go...@golang.org>
Reviewed-by: Katie Hockman <ka...@golang.org>
---
A src/cmd/go/testdata/script/test_fuzz_parallel.txt
M src/internal/fuzz/worker.go
M src/testing/fuzz.go
M src/testing/sub_test.go
M src/testing/testing.go
5 files changed, 139 insertions(+), 27 deletions(-)
index f784a04..22c8561 100644
2 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
To view, visit change 300449. To unsubscribe, or for help writing mail filters, visit settings.
Reply all
Reply to author
Forward
0 new messages