testing: support subtests in fuzzer

[AlekSi]

Consider the following test using testing.F:

func FuzzAtoi(f *testing.F) {
    for _, tc := range []struct {
        s        string
        expected int
    }{
        {s: "1", expected: 1},
        {s: "0", expected: 1},
    } {
        actual, err := strconv.Atoi(tc.s)
        if err != nil {
            f.Fatal(err)
        }
        if tc.expected != actual {
            f.Error(actual)
        }
    }
}

It fails with an unhelpful error message:

=== RUN  FuzzAtoi
    atoi_test.go:77: 0
--- FAIL: FuzzAtoi (0.00s)
FAIL

It may be made better by adding additional context into f.Error line. But there is another way for that available to testing.T, but not testing.F - subtests:

func TestAtoi(t *testing.T) {
    for _, tc := range []struct {
        s        string
        expected int
    }{
        {s: "1", expected: 1},
        {s: "0", expected: 1},
    } {
        tc := tc
        t.Run(tc.s, func(t *testing.T) {
            actual, err := strconv.Atoi(tc.s)
            if err != nil {
                t.Fatal(err)
            }
            if tc.expected != actual {
                t.Error(actual)
            }
        })
    }
}

This includes subtest name in the error message automatically, which is very helpful:

=== RUN   TestAtoi
=== RUN   TestAtoi/1
=== RUN   TestAtoi/0
    atoi_test.go:77: 0
--- FAIL: TestAtoi (0.00s)
    --- PASS: TestAtoi/1 (0.00s)
    --- FAIL: TestAtoi/0 (0.00s)
FAIL

In the end, I want to be able to do something like that:

func FuzzAtoi(f *testing.F) {
    for _, tc := range []struct {
        s        string
        expected int
    }{
        {s: "1", expected: 1},
        {s: "0", expected: 1},
    } {
        tc := tc
        f.Run(tc.s, func(t *testing.T) { // note 1
            t.Parallel()

            actual, err := strconv.Atoi(tc.s)
            if err != nil {
                f.Fatal(err)
            }
            if tc.expected != actual {
                f.Error(actual)
            }

            f.Add(tc.s) // note 2
        })

        f.Add(tc.s) // note 3
    }

    f.Fuzz(func(t *testing.T, s string) {
        t.Parallel()

        // ...
    })
}

Note that f.Run accepts a function accepting testing.T instead of testing.F. That allows one to run subtests in parallel and also removes ambiguity about the fuzz corpus - this way, there is only one for the whole FuzzAtoi function.
f.Add could be called in both places - inside and outside of a subtest. So it should be made thread-safe.

This issue is different from #46780 as it requests subtests support for testing.F instead of more complicated subfuzz targets.

[AlekSi]

@gopherbot fuzz

[jayconrod]

cc @golang/fuzzing

Sorry, I'm still not sure I understand this.

• In your last example, you're calling F.Add twice with each input. Why? If the extra name given to F.Run is useful in the output, why isn't it useful in both places?
• Why test seed input values before calling F.Add? F.Fuzz could check them instead? I suppose you'd only have expected values for the seed inputs, but then why not just have a separate unit test?
• Not sure I see the parallelism advantage here at all. The top-level fuzz target just needs to call F.Add for each input, then F.Fuzz. It probably shouldn't do much computation other than that. The fuzz function passed to F.Fuzz may call T.Parallel, and that will run the seed inputs in parallel. It has no effect when fuzzing though; multiple inputs will run in parallel in different processes regardless of whether T.Parallel was called.

I'd expect this to be written as:

package atoi

import (
	"strconv"
	"testing"
)

var atoiTests = []struct {
	name, s  string
	expected int
}{
	{"zero", "0", 0},
	{"one", "1", 1},
}

func TestAtoi(t *testing.T) {
	for _, test := range atoiTests {
		test := test
		t.Run(test.name, func(t *testing.T) {
			t.Parallel()
			n, err := strconv.Atoi(test.s)
			if err != nil {
				t.Fatal(err)
			}
			if n != test.expected {
				t.Errorf("got %d; want %d", n, test.expected)
			}
		})
	}
}

func FuzzAtoi(f *testing.F) {
	for _, test := range atoiTests {
		f.Add(test.s)
	}
	f.Fuzz(func(t *testing.T, s string) {
		t.Parallel()
		n, err := strconv.Atoi(s)
		if err != nil {
			return
		}
		got := strconv.Itoa(n)
		if s != got {
			t.Fatalf("converting %q to integer and back, got %q", s, got)
		}
	})
}

[AlekSi]

In your last example, you're calling F.Add twice with each input. Why?

I tried to explain that in the text: "f.Add could be called in both places - inside and outside of a subtest. So it should be made thread-safe." It should not be, of course, but the idea was to show it either place can be picked by a user, and any of them will not be a mistake.

Why test seed input values before calling F.Add? F.Fuzz could check them instead? I suppose you'd only have expected values for the seed inputs, but then why not just have a separate unit test?

I guess that's the core of it – I don't want a separate unit test. The real package I'm working on is a data marshaller / unmarshaller. The unit tests for correct data looked almost identical to fuzz tests (unmarshal bytes into an object, re-marshal this object, compare bytes), so I merged them, as this example tried to show. Fuzzing draft proposal says talks about it: "In the long term, this design could start to replace existing table tests, seamlessly integrating into the existing Go testing ecosystem." Maybe I'm just living in the future.

Not sure I see the parallelism advantage here at all.

If you think about that part as a unit test, then parallelism advantage is clear.

I'd expect this to be written as

Note that your example:

• contains similar code in TestAtoi and FuzzAtoi (and the amount of shared code is larger in real case);
• contains a shared package-level variable.

My proposal allows one to write a single test function that both tests happy cases and feeds them to the fuzzer. Then the only tests that should be written separately are tests for returned error values.

[jayconrod]

Thanks for explaining, I think I understand better what you're getting at.

This would be a pretty big design change, so I'd like to make sure @katiehockman and @rolandshoemaker have a voice in this, too. Katie's on vacation for a few more weeks, so we won't be able to make a decision right away.

My API design intuition is that it's better to keep testing.F simpler and focused on fuzzing, even if it could be adapted to do most of what testing.T can do in non-fuzz tests. There's definitely some overlap though.