proposal: testing/quick: deprecate package

[seankhliao]

Proposal Details

testing/quick has been frozen since #15557, CL 31910. It has, accordingly, not received much new development (primarily repo-wide updates).

Compared to other frozen packages, there aren't that many users, pkg.go.dev only lists 335 importers https://pkg.go.dev/testing/quick There's also a clear replacement we can point to, testing's built in fuzz support introduced in Go 1.18.

I propose we mark the package as deprecated, and point users towards fuzzing.

[qiulaidongfeng]

A question : How can these fuzzy tests from the sync package of the standard library be replace to use fuzzing from the testing package?

https://github.com/golang/go/blob/master/src/sync/map_test.go#L136C1-L146C2

[seankhliao]

Right now quick.CheckEqual takes wrapper funcs that generate input values from a rand.Rand. This could be changed to be a wrapper func that takes a []byte generated by the fuzzer and uses encoding/binary parse it into the structures it needs.

[seankhliao]

e.g. something like:

func fuzzArgs(b []byte) []mapCall {
    var calls []mapCall
    // 11 bytes per mapCall:
    // 1 for op
    // 1 + 4 for key
    // 1 + 4 for optional value
    for i := 0; i < len(b)/11; i++ {
        offset := i * 11
        var call mapCall
        call.op = mapOps[int(b[offset])%len(mapOps)]
        call.k = string(b[offset+2 : offset+2+(int(b[offset+1])%4)])
        call.v = string(b[offset+7 : offset+7+(int(b[offset+6])%4)])
        calls = append(calls, call)
    }
    return calls
}

func FuzzMapMatchesRWMutex(f *testing.F) {
    f.Fuzz(func(t *testing.T, b []byte) {
        calls := fuzzArgs(b)
        mapRes, finalMap := applyMap(calls)
        rwRes, finalRW := applyRWMutexMap(calls)
        if !slices.Equal(mapRes, rwRes) {
            t.Errorf("mapResults not equal")
        }
        if !maps.Equal(finalMap, finalRW) {
            t.Errorf("final maps not equal")
        }
    })
}

[qiulaidongfeng]

In this case, it seems more appropriate to wait for fuzzing to be more powerful and not need write such complex code before deprecate the quick package.

[seankhliao]

The quick boilerplate was arguably longer: https://github.com/golang/go/blob/master/src/sync/map_test.go#L92-L107

[qiulaidongfeng]

The problem is not the length of the code, but whether it is simpler, easier to understand, and relatively less error-prone, and now that sync has this code, it is easy to read. For code like this, I don't think it would be simpler, easier to read, or relatively less error-prone.

    var calls []mapCall
    // 11 bytes per mapCall:
    // 1 for op
    // 1 + 4 for key
    // 1 + 4 for optional value
    for i := 0; i < len(b)/11; i++ {
        offset := i * 11
        var call mapCall
        call.op = mapOps[int(b[offset])%len(mapOps)]
        call.k = string(b[offset+2 : offset+2+(int(b[offset+1])%4)])
        call.v = string(b[offset+7 : offset+7+(int(b[offset+6])%4)])
        calls = append(calls, call)
    }
    return calls

[seankhliao]

You could make it look nicer by having the arguments implement encoding.BinaryUnmarshaler, but I don't think we should block deprecation on things like #48815

Deprecation means existing code will still work, but is a much stronger discouragement for any new code to use this library. New code really shouldn't be using this, given how it is strictly worse (generate a rng seed vs coverage guided mutator), existing code can update the next time someone goes to update it.