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.