From 0ba4e3a630ac97e9d7a5b3ae210f8e7504392bef Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Sat, 28 Mar 2020 00:19:44 +0100 Subject: [PATCH] types/errors: make Wrap() work like github.com/pkg/errors.Wrap Wrap(ErrInsufficientFunds, "40 < 500") returns strings like "insufficient funds: 40 < 50". Although he function syntax fits perfectly SDK app developers use case and as such it should not be changed, the interface is still counter-intuitive as most Golang developers may reasonably expect a different result similar to what other popular libraries' (see [1]) would produce (e.g. "40 < 500: insufficient funds"). Follow up PR of #5876. [1] https://godoc.org/github.com/pkg/errors#Wrapf --- types/errors/abci_test.go | 33 +++++++++++---------------------- types/errors/errors.go | 2 +- types/errors/errors_test.go | 25 +++++++++++++++++++++++++ types/errors/stacktrace_test.go | 8 ++++---- 4 files changed, 41 insertions(+), 27 deletions(-) diff --git a/types/errors/abci_test.go b/types/errors/abci_test.go index 9b9e8280a..6e89d83cd 100644 --- a/types/errors/abci_test.go +++ b/types/errors/abci_test.go @@ -28,7 +28,7 @@ func TestABCInfo(t *testing.T) { "wrapped SDK error": { err: Wrap(Wrap(ErrUnauthorized, "foo"), "bar"), debug: false, - wantLog: "unauthorized: foo: bar", + wantLog: "bar: foo: unauthorized", wantCode: ErrUnauthorized.code, wantSpace: RootCodespace, }, @@ -95,15 +95,9 @@ func TestABCInfo(t *testing.T) { tc := tc t.Run(testName, func(t *testing.T) { space, code, log := ABCIInfo(tc.err, tc.debug) - if space != tc.wantSpace { - t.Errorf("want %s space, got %s", tc.wantSpace, space) - } - if code != tc.wantCode { - t.Errorf("want %d code, got %d", tc.wantCode, code) - } - if log != tc.wantLog { - t.Errorf("want %q log, got %q", tc.wantLog, log) - } + require.Equal(t, tc.wantSpace, space) + require.Equal(t, tc.wantCode, code) + require.Equal(t, tc.wantLog, log) }) } } @@ -119,19 +113,19 @@ func TestABCIInfoStacktrace(t *testing.T) { err: Wrap(ErrUnauthorized, "wrapped"), debug: true, wantStacktrace: true, - wantErrMsg: "unauthorized: wrapped", + wantErrMsg: "wrapped: unauthorized", }, "wrapped SDK error in non-debug mode does not have stacktrace": { err: Wrap(ErrUnauthorized, "wrapped"), debug: false, wantStacktrace: false, - wantErrMsg: "unauthorized: wrapped", + wantErrMsg: "wrapped: unauthorized", }, "wrapped stdlib error in debug mode provides stacktrace": { err: Wrap(fmt.Errorf("stdlib"), "wrapped"), debug: true, wantStacktrace: true, - wantErrMsg: "stdlib: wrapped", + wantErrMsg: "wrapped: stdlib", }, "wrapped stdlib error in non-debug mode does not have stacktrace": { err: Wrap(fmt.Errorf("stdlib"), "wrapped"), @@ -165,10 +159,7 @@ func TestABCIInfoStacktrace(t *testing.T) { func TestABCIInfoHidesStacktrace(t *testing.T) { err := Wrap(ErrUnauthorized, "wrapped") _, _, log := ABCIInfo(err, false) - - if log != "unauthorized: wrapped" { - t.Fatalf("unexpected message in non debug mode: %s", log) - } + require.Equal(t, "wrapped: unauthorized", log) } func TestRedact(t *testing.T) { @@ -208,12 +199,12 @@ func TestABCIInfoSerializeErr(t *testing.T) { "single error": { src: myErrDecode, debug: false, - exp: "tx parse error: test", + exp: "test: tx parse error", }, "second error": { src: myErrAddr, debug: false, - exp: "invalid address: tester", + exp: "tester: invalid address", }, "single error with debug": { src: myErrDecode, @@ -260,9 +251,7 @@ func TestABCIInfoSerializeErr(t *testing.T) { spec := spec t.Run(msg, func(t *testing.T) { _, _, log := ABCIInfo(spec.src, spec.debug) - if exp, got := spec.exp, log; exp != got { - t.Errorf("expected %v but got %v", exp, got) - } + require.Equal(t, spec.exp, log) }) } } diff --git a/types/errors/errors.go b/types/errors/errors.go index a781a4cdd..f881a2827 100644 --- a/types/errors/errors.go +++ b/types/errors/errors.go @@ -262,7 +262,7 @@ type wrappedError struct { } func (e *wrappedError) Error() string { - return fmt.Sprintf("%s: %s", e.parent.Error(), e.msg) + return fmt.Sprintf("%s: %s", e.msg, e.parent.Error()) } func (e *wrappedError) Cause() error { diff --git a/types/errors/errors_test.go b/types/errors/errors_test.go index f6e6e8cd7..d618902c5 100644 --- a/types/errors/errors_test.go +++ b/types/errors/errors_test.go @@ -208,3 +208,28 @@ func TestWrappedUnwrapFail(t *testing.T) { err := Wrap(errTest2, Wrap(errTest, "some random description").Error()) require.NotEqual(t, errTest, stdlib.Unwrap(err)) } + +func TestABCIError(t *testing.T) { + require.Equal(t, "custom: tx parse error", ABCIError(RootCodespace, 2, "custom").Error()) + require.Equal(t, "custom: unknown", ABCIError("unknown", 1, "custom").Error()) +} + +func ExampleWrap() { + err1 := Wrap(ErrInsufficientFunds, "90 is smaller than 100") + err2 := errors.Wrap(ErrInsufficientFunds, "90 is smaller than 100") + fmt.Println(err1.Error()) + fmt.Println(err2.Error()) + // Output: + // 90 is smaller than 100: insufficient funds + // 90 is smaller than 100: insufficient funds +} + +func ExampleWrapf() { + err1 := Wrap(ErrInsufficientFunds, "90 is smaller than 100") + err2 := errors.Wrap(ErrInsufficientFunds, "90 is smaller than 100") + fmt.Println(err1.Error()) + fmt.Println(err2.Error()) + // Output: + // 90 is smaller than 100: insufficient funds + // 90 is smaller than 100: insufficient funds +} diff --git a/types/errors/stacktrace_test.go b/types/errors/stacktrace_test.go index 042edc0b6..bef4bccb7 100644 --- a/types/errors/stacktrace_test.go +++ b/types/errors/stacktrace_test.go @@ -15,19 +15,19 @@ func TestStackTrace(t *testing.T) { }{ "New gives us a stacktrace": { err: Wrap(ErrNoSignatures, "name"), - wantError: "no signatures supplied: name", + wantError: "name: no signatures supplied", }, "Wrapping stderr gives us a stacktrace": { err: Wrap(fmt.Errorf("foo"), "standard"), - wantError: "foo: standard", + wantError: "standard: foo", }, "Wrapping pkg/errors gives us clean stacktrace": { err: Wrap(errors.New("bar"), "pkg"), - wantError: "bar: pkg", + wantError: "pkg: bar", }, "Wrapping inside another function is still clean": { err: Wrap(fmt.Errorf("indirect"), "do the do"), - wantError: "indirect: do the do", + wantError: "do the do: indirect", }, }