From 2c3e813a316598dc22c00b916634278f7ef56f45 Mon Sep 17 00:00:00 2001 From: Varun Gandhi Date: Mon, 3 Jun 2024 19:46:53 +0800 Subject: [PATCH] chore: Add tests documenting error invariants (#62992) Based on the [discussion in Slack](https://sourcegraph.slack.com/archives/C02UC4WUX1Q/p1717067410264679), thought it would be useful to solidify the invariants that hold between errors.As, errors.Is and errors.HasType (and provide counter-examples for things which you think ought to hold but don't actually hold) in code itself, instead of just via docs. So this PR adds property-based tests for quickly generating different kinds of error shapes, and check which invariants hold and which ones don't. It also adds better documentation for errors.Is and errors.As --- deps.bzl | 7 + go.sum | 2 + lib/errors/.gitignore | 10 + lib/errors/BUILD.bazel | 2 + lib/errors/cockroach.go | 27 ++- lib/errors/invariants_test.go | 297 +++++++++++++++++++++++++++++ lib/go.mod | 1 + lib/go.sum | 2 + lib/managedservicesplatform/go.sum | 2 + monitoring/go.sum | 2 + 10 files changed, 350 insertions(+), 2 deletions(-) create mode 100644 lib/errors/.gitignore create mode 100644 lib/errors/invariants_test.go diff --git a/deps.bzl b/deps.bzl index 556457b4797..fa2477bbe8d 100644 --- a/deps.bzl +++ b/deps.bzl @@ -7958,6 +7958,13 @@ def go_dependencies(): sum = "h1:7uVkIFmeBqHfdjD+gZwtXXI+RODJ2Wc4O7MPEh/QiW4=", version = "v1.3.0", ) + go_repository( + name = "net_pgregory_rapid", + build_file_proto_mode = "disable_global", + importpath = "pgregory.net/rapid", + sum = "h1:CMa0sjHSru3puNx+J0MIAuiiEV4N0qj8/cMWGBBCsjw=", + version = "v1.1.0", + ) go_repository( name = "net_starlark_go", build_file_proto_mode = "disable_global", diff --git a/go.sum b/go.sum index 27b4eeb4550..b44f56fa44d 100644 --- a/go.sum +++ b/go.sum @@ -2702,6 +2702,8 @@ oss.terrastruct.com/d2 v0.6.5 h1:VgZgiwtMhh3uVR2mm7e0bdh25f1px3ZCPM/la5GKfMc= oss.terrastruct.com/d2 v0.6.5/go.mod h1:WUTwQN18MM0MWbDgFo7pmm2ousV6N6jUwg8MgVdT6I0= oss.terrastruct.com/util-go v0.0.0-20231101220827-55b3812542c2 h1:n6y6RoZCgZDchN4gLGlzNRO1Jdf9xOGGqohDBph5BG8= oss.terrastruct.com/util-go v0.0.0-20231101220827-55b3812542c2/go.mod h1:eMWv0sOtD9T2RUl90DLWfuShZCYp4NrsqNpI8eqO6U4= +pgregory.net/rapid v1.1.0 h1:CMa0sjHSru3puNx+J0MIAuiiEV4N0qj8/cMWGBBCsjw= +pgregory.net/rapid v1.1.0/go.mod h1:PY5XlDGj0+V1FCq0o192FdRhpKHGTRIWBgqjDBTrq04= rsc.io/binaryregexp v0.2.0/go.mod h1:qTv7/COck+e2FymRvadv62gMdZztPaShugOCi3I+8D8= rsc.io/pdf v0.1.1 h1:k1MczvYDUvJBe93bYd7wrZLLUEcLZAuF824/I4e5Xr4= rsc.io/pdf v0.1.1/go.mod h1:n8OzWcQ6Sp37PL01nO98y4iUCRdTGarVfzxY20ICaU4= diff --git a/lib/errors/.gitignore b/lib/errors/.gitignore new file mode 100644 index 00000000000..e507ce1a657 --- /dev/null +++ b/lib/errors/.gitignore @@ -0,0 +1,10 @@ +# Generated by rapid after finding counter-examples. +# +# When re-running tests locally during development, +# this enables faster failure finding. +# https://github.com/flyingmutant/rapid/commit/4db076b4aa4d41036416da11abd5bbfc228a324e +# +# During development, if a check fails, it makes sense +# to disable or refine the check, and add specific, +# minimal tests using the counter-examples found by rapid. +testdata/rapid/ diff --git a/lib/errors/BUILD.bazel b/lib/errors/BUILD.bazel index 59ffc851521..46d59bb5b62 100644 --- a/lib/errors/BUILD.bazel +++ b/lib/errors/BUILD.bazel @@ -27,6 +27,7 @@ go_test( srcs = [ "errors_test.go", "filter_test.go", + "invariants_test.go", "warning_test.go", ], embed = [":errors"], @@ -34,5 +35,6 @@ go_test( deps = [ "@com_github_stretchr_testify//assert", "@com_github_stretchr_testify//require", + "@net_pgregory_rapid//:rapid", ], ) diff --git a/lib/errors/cockroach.go b/lib/errors/cockroach.go index 321a683c2b9..3b703613ab6 100644 --- a/lib/errors/cockroach.go +++ b/lib/errors/cockroach.go @@ -52,8 +52,31 @@ var ( // - in Sentry reports. WithSafeDetails = errors.WithSafeDetails - Is = errors.Is - IsAny = errors.IsAny + // Is checks if the error tree err is equal to the value target. + // + // For error types which do not contain any data, Is is equivalent to As. + // + // For error types which contain data, it's possible that Is + // returns true for a value other than the one returned by As, + // since an error tree can contain multiple errors of the same + // concrete type but with different data. + Is = errors.Is + IsAny = errors.IsAny + // As checks if the error tree err is of type target, and if so, + // sets target to the value of the error. This can be used in two ways: + // + // 1. If looking for an error of concrete type T, then the second + // argument must be a non-nil pointer of type *T. This implies that + // if the error interface is implemented with a pointer receiver, + // then target must be of type **MyConcreteType. + // 2. If looking for an error satisfying an interface I (with a value + // or pointer receiver), then the second argument must be of type I. + // + // For error types which do not contain any data, As is equivalent to Is. + // + // For error types which contain data, As will return an arbitrary + // error of the target type, in case there are multiple errors of the + // same concrete type in the error tree. As = errors.As HasType = errors.HasType Cause = errors.Cause diff --git a/lib/errors/invariants_test.go b/lib/errors/invariants_test.go new file mode 100644 index 00000000000..a91b25b647c --- /dev/null +++ b/lib/errors/invariants_test.go @@ -0,0 +1,297 @@ +package errors + +// This file contains tests which check the relationships between +// the various error-checking functions such as Is, As and HasType. + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/require" + "pgregory.net/rapid" +) + +// errorTree generates a tree of errors, where the leaves +// are generated using leafError. +func errorTree(leafError *rapid.Generator[error]) *rapid.Generator[error] { + errorSliceGen := rapid.SliceOfN(rapid.Deferred(func() *rapid.Generator[error] { + return errorTree(leafError) + }), 1, 3) + return rapid.OneOf( + leafError, + rapid.Map[[]error, error](errorSliceGen, func(errs []error) error { + return Append(nil, errs...) + }), + ) +} + +func TestInvariants(t *testing.T) { + // Check the behavior of the various error-checking + // functions for different kinds of error types - + // value receiver vs pointer receiver, + // and errors without data vs with data. + + t.Run("payloadLessStruct", func(t *testing.T) { + rapid.Check(t, func(t *rapid.T) { + err := errorTree(rapid.OneOf( + rapid.Just(error(¬TheErrorOfInterest{})), + rapid.Just(error(payloadLessStructError{})), + )).Draw(t, "err") + // Is implies As for errors without data + if Is(err, payloadLessStructError{}) { + // This can be false, see Counter-example 1 + //require.True(t, HasType(err, payloadLessStructError{})) + var check payloadLessStructError + require.True(t, As(err, &check)) + } + // HasType implies Is and As for errors without data + if HasType(err, payloadLessStructError{}) { + require.True(t, Is(err, payloadLessStructError{})) + var check payloadLessStructError + require.True(t, As(err, &check)) + } + var check payloadLessStructError + // As implies Is for errors without data + if As(err, &check) { + require.True(t, Is(err, payloadLessStructError{})) + // This can be false, see Counter-example 2 + //require.True(t, HasType(err, payloadLessStructError{})) + } + }) + }) + + t.Run("withPayloadStructError", func(t *testing.T) { + rapid.Check(t, func(t *rapid.T) { + errorOfInterest := withPayloadStructError{data: 11} + errorWithOtherData := withPayloadStructError{data: 24} + err := errorTree(rapid.OneOf( + rapid.Just(error(¬TheErrorOfInterest{})), + rapid.Just(error(errorOfInterest)), + rapid.Just(error(errorWithOtherData)), + )).Draw(t, "err") + + // Is implies As for errors with data + if Is(err, errorOfInterest) { + // This is false, see Counter-example 5 + //require.False(t, Is(err, errorWithOtherData)) + require.False(t, Is(err, withPayloadStructError{})) + // These can be false, see Counter-example 1 + //require.True(t, HasType(err, errorOfInterest)) + //require.True(t, HasType(err, errorWithOtherData)) + //require.True(t, HasType(err, withPayloadStructError{})) + var check withPayloadStructError + require.True(t, As(err, &check)) + // This can be false, see Counter-example 6 + //require.Equal(t, errorOfInterest, check) + } + + // HasType implies As for errors with data + if HasType(err, errorOfInterest) { + require.True(t, HasType(err, errorWithOtherData)) + require.True(t, HasType(err, withPayloadStructError{})) + // This can be false, see Counter-example 3 + //require.True(t, Is(err, errorOfInterest)) + var check withPayloadStructError + require.True(t, As(err, &check)) + // This can be false, see Counter-example 4 + //require.Equal(t, errorOfInterest, check) + } + + // As implies a limited form of Is for errors with data + var check withPayloadStructError + if As(err, &check) { + require.True(t, check == errorOfInterest || check == errorWithOtherData) + require.True(t, Is(err, errorOfInterest) || Is(err, errorWithOtherData)) + // These can be false, see Counter-example 2 + //require.True(t, HasType(err, errorOfInterest)) + //require.True(t, HasType(err, errorWithOtherData)) + //require.True(t, HasType(err, withPayloadStructError{})) + } + }) + }) + + t.Run("payloadLessPtrError", func(t *testing.T) { + rapid.Check(t, func(t *rapid.T) { + err := errorTree(rapid.OneOf( + rapid.Just(error(¬TheErrorOfInterest{})), + rapid.Just(error(&payloadLessPtrError{})), + )).Draw(t, "err") + // Is implies As for errors without data + if Is(err, &payloadLessPtrError{}) { + // This can be false, see Counter-example 1 + //require.True(t, HasType(err, &payloadLessPtrError{})) + require.Panics(t, func() { + var check payloadLessPtrError + require.True(t, As(err, &check)) + }) + var check *payloadLessPtrError + require.True(t, As(err, &check)) + } + // HasType implies Is and As for errors without data + if HasType(err, &payloadLessPtrError{}) { + require.True(t, Is(err, &payloadLessPtrError{})) + require.Panics(t, func() { + var check payloadLessPtrError + require.True(t, As(err, &check)) + }) + var check *payloadLessPtrError + require.True(t, As(err, &check)) + } + var check *payloadLessPtrError + // As implies Is for errors without data + if As(err, &check) { + require.True(t, Is(err, &payloadLessPtrError{})) + // This can be false, see Counter-example 2 + //require.True(t, errors.HasType(err, &payloadLessPtrError{})) + } + }) + }) + + t.Run("withPayloadPtrError", func(t *testing.T) { + rapid.Check(t, func(t *rapid.T) { + errorOfInterest := &withPayloadPtrError{data: 11} + errorWithOtherData := &withPayloadPtrError{data: 24} + err := errorTree(rapid.OneOf( + rapid.Just(error(¬TheErrorOfInterest{})), + rapid.Just(error(errorOfInterest)), + rapid.Just(error(errorWithOtherData)), + )).Draw(t, "err") + + // Is implies As for errors with data + if Is(err, errorOfInterest) { + // This is false, see Counter-example 5 + //require.False(t, Is(err, errorWithOtherData)) + require.False(t, Is(err, &withPayloadPtrError{})) + // These can be false, see Counter-example 1 + //require.True(t, HasType(err, errorOfInterest)) + //require.True(t, HasType(err, errorWithOtherData)) + //require.True(t, HasType(err, withPayloadStructError{})) + require.Panics(t, func() { + var check withPayloadPtrError + _ = As(err, &check) + }) + var check *withPayloadPtrError + require.True(t, As(err, &check)) + // This can be false, see Counter-example 6 + //require.Equal(t, *errorOfInterest, *check) + } + + // HasType implies As for errors with data + if HasType(err, errorOfInterest) { + require.True(t, HasType(err, errorWithOtherData)) + require.True(t, HasType(err, &withPayloadPtrError{})) + //This can be false, see Counter-example 3 + //require.True(t, Is(err, errorOfInterest)) + require.Panics(t, func() { + var check withPayloadPtrError + _ = As(err, &check) + }) + var check *withPayloadPtrError + require.True(t, As(err, &check)) + require.True(t, *check == *errorOfInterest || *check == *errorWithOtherData) + } + + // As implies a limited form of Is for errors with data + var check *withPayloadPtrError + if As(err, &check) { + require.True(t, *check == *errorOfInterest || *check == *errorWithOtherData) + require.True(t, Is(err, errorOfInterest) || Is(err, errorWithOtherData)) + // These can be false, see Counter-example 2 + //require.True(t, HasType(err, errorOfInterest)) + //require.True(t, HasType(err, errorWithOtherData)) + } + }) + }) + + t.Run("Counter-examples", func(t *testing.T) { + // Counter-example 1. Is does not imply HasType + { + err := Append(payloadLessStructError{}, ¬TheErrorOfInterest{}) + check := payloadLessStructError{} + require.True(t, Is(err, check)) + require.False(t, HasType(err, check)) + } + // Counter-example 2. As does not imply HasType + { + err := Append(payloadLessStructError{}, ¬TheErrorOfInterest{}) + check := payloadLessStructError{} + require.True(t, As(err, &check)) + require.False(t, HasType(err, payloadLessStructError{})) + } + // Counter-example 3. HasType does not imply Is + { + err := error(withPayloadStructError{data: 3}) + require.True(t, HasType(err, withPayloadStructError{})) + require.False(t, Is(err, withPayloadStructError{data: 1})) + } + // Counter-example 4. HasType does not imply As + { + err := error(withPayloadStructError{data: 3}) + hasTypeCheck := withPayloadStructError{data: 1} + require.True(t, HasType(err, hasTypeCheck)) + var valueFromAs withPayloadStructError + require.True(t, As(err, &valueFromAs)) + require.NotEqual(t, hasTypeCheck, valueFromAs) + } + // Counter-example 5. Is can return true for distinct values + { + err := Append(withPayloadStructError{data: 3}, withPayloadStructError{data: 1}) + require.True(t, Is(err, withPayloadStructError{data: 3})) + require.True(t, Is(err, withPayloadStructError{data: 1})) + } + // Counter-example 6. As can return a different value than the one passed to Is + { + err := Append(withPayloadStructError{data: 3}, withPayloadStructError{data: 1}) + var check withPayloadStructError + require.True(t, Is(err, withPayloadStructError{data: 1})) + require.True(t, As(err, &check)) + // 'As' picks the first value it finds. + require.Equal(t, check, withPayloadStructError{data: 3}) + require.NotEqual(t, check, withPayloadStructError{data: 1}) + } + }) +} + +type payloadLessStructError struct{} + +func (p payloadLessStructError) Error() string { + return "payloadLessStructError{}" +} + +var _ error = payloadLessStructError{} + +type withPayloadStructError struct { + data int +} + +var _ error = withPayloadStructError{} + +func (p withPayloadStructError) Error() string { + return fmt.Sprintf("withPayloadStructError{data: %v}", p.data) +} + +type payloadLessPtrError struct{} + +var _ error = &payloadLessPtrError{} + +func (p *payloadLessPtrError) Error() string { + return "payloadLessPtrError{}" +} + +type withPayloadPtrError struct { + data int +} + +var _ error = &withPayloadPtrError{} + +func (p *withPayloadPtrError) Error() string { + return fmt.Sprintf("withPayloadPtrError{data: %d}", p.data) +} + +type notTheErrorOfInterest struct{} + +var _ error = ¬TheErrorOfInterest{} + +func (p *notTheErrorOfInterest) Error() string { + return "notTheErrorOfInterest{}" +} diff --git a/lib/go.mod b/lib/go.mod index be26c6b4b85..0ef9fee2c64 100644 --- a/lib/go.mod +++ b/lib/go.mod @@ -41,6 +41,7 @@ require ( google.golang.org/protobuf v1.33.0 gopkg.in/yaml.v2 v2.4.0 gopkg.in/yaml.v3 v3.0.1 + pgregory.net/rapid v1.1.0 ) require ( diff --git a/lib/go.sum b/lib/go.sum index 747cbda50a1..4cf17593100 100644 --- a/lib/go.sum +++ b/lib/go.sum @@ -555,3 +555,5 @@ honnef.co/go/tools v0.0.0-20190102054323-c2f93a96b099/go.mod h1:rf3lG4BRIbNafJWh honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4= mvdan.cc/gofumpt v0.4.0 h1:JVf4NN1mIpHogBj7ABpgOyZc65/UUOkKQFkoURsz4MM= mvdan.cc/gofumpt v0.4.0/go.mod h1:PljLOHDeZqgS8opHRKLzp2It2VBuSdteAgqUfzMTxlQ= +pgregory.net/rapid v1.1.0 h1:CMa0sjHSru3puNx+J0MIAuiiEV4N0qj8/cMWGBBCsjw= +pgregory.net/rapid v1.1.0/go.mod h1:PY5XlDGj0+V1FCq0o192FdRhpKHGTRIWBgqjDBTrq04= diff --git a/lib/managedservicesplatform/go.sum b/lib/managedservicesplatform/go.sum index 7e40bb8b71a..7b76e8abb87 100644 --- a/lib/managedservicesplatform/go.sum +++ b/lib/managedservicesplatform/go.sum @@ -542,3 +542,5 @@ honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc/go.mod h1:rf3lG4BRIbNafJWh mvdan.cc/gofumpt v0.4.0/go.mod h1:PljLOHDeZqgS8opHRKLzp2It2VBuSdteAgqUfzMTxlQ= mvdan.cc/gofumpt v0.5.0 h1:0EQ+Z56k8tXjj/6TQD25BFNKQXpCvT0rnansIc7Ug5E= mvdan.cc/gofumpt v0.5.0/go.mod h1:HBeVDtMKRZpXyxFciAirzdKklDlGu8aAy1wEbH5Y9js= +pgregory.net/rapid v1.1.0 h1:CMa0sjHSru3puNx+J0MIAuiiEV4N0qj8/cMWGBBCsjw= +pgregory.net/rapid v1.1.0/go.mod h1:PY5XlDGj0+V1FCq0o192FdRhpKHGTRIWBgqjDBTrq04= diff --git a/monitoring/go.sum b/monitoring/go.sum index 48cd603cf4d..a87873d8f1c 100644 --- a/monitoring/go.sum +++ b/monitoring/go.sum @@ -716,6 +716,8 @@ honnef.co/go/tools v0.0.1-2020.1.3/go.mod h1:X/FiERA/W4tHapMX5mGpAtMSVEeEUOyHaw9 honnef.co/go/tools v0.0.1-2020.1.4/go.mod h1:X/FiERA/W4tHapMX5mGpAtMSVEeEUOyHaw9vFzvIQ3k= mvdan.cc/gofumpt v0.4.0 h1:JVf4NN1mIpHogBj7ABpgOyZc65/UUOkKQFkoURsz4MM= mvdan.cc/gofumpt v0.4.0/go.mod h1:PljLOHDeZqgS8opHRKLzp2It2VBuSdteAgqUfzMTxlQ= +pgregory.net/rapid v1.1.0 h1:CMa0sjHSru3puNx+J0MIAuiiEV4N0qj8/cMWGBBCsjw= +pgregory.net/rapid v1.1.0/go.mod h1:PY5XlDGj0+V1FCq0o192FdRhpKHGTRIWBgqjDBTrq04= rsc.io/binaryregexp v0.2.0/go.mod h1:qTv7/COck+e2FymRvadv62gMdZztPaShugOCi3I+8D8= rsc.io/quote/v3 v3.1.0/go.mod h1:yEA65RcK8LyAZtP9Kv3t0HmxON59tX3rD+tICJqUlj0= rsc.io/sampler v1.3.0/go.mod h1:T1hPZKmBbMNahiBKFy5HrXp6adAjACjK9JXDnKaTXpA=