Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: prep for errors v2 #20539

Merged
merged 14 commits into from
Jun 11, 2024
+42 −730
Merged

feat: prep for errors v2 #20539

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
6795837
remove all deps from errors
tac0turtle Jun 4, 2024
4cbf254
more changes
tac0turtle Jun 5, 2024
72e4bd8
remove wrap errors in favour of fmt.Errorf
tac0turtle Jun 5, 2024
6e065b3
minor cleanup
tac0turtle Jun 5, 2024
afbc2b2
fix test
tac0turtle Jun 5, 2024
e64794a
remove invalid test cases
Jun 5, 2024
27f6378
Merge branch 'main' into marko/errorsv2
tac0turtle Jun 5, 2024
f5290fd
Merge branch 'main' into marko/errorsv2
tac0turtle Jun 6, 2024
4e69931
remove unused interface
tac0turtle Jun 6, 2024
d3bf0d2
remove unused error
tac0turtle Jun 6, 2024
a065237
address comments
tac0turtle Jun 6, 2024
98eef6d
address comments
tac0turtle Jun 6, 2024
5afda00
changelog entry
tac0turtle Jun 11, 2024
9fdbb30
remove handle.go
tac0turtle Jun 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 13 additions & 20 deletions errors/abci.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package errors

import (
"errors"
"fmt"
"reflect"
)
Expand Down Expand Up @@ -46,10 +47,6 @@ func defaultErrEncoder(err error) string {
return err.Error()
}

type coder interface {
ABCICode() uint32
}

// abciCode tests if given error contains an ABCI code and returns the value of
// it if available. This function is testing for the causer interface as well
// and unwraps the error.
Expand All @@ -59,22 +56,15 @@ func abciCode(err error) uint32 {
}

for {
if c, ok := err.(coder); ok {
return c.ABCICode()
}

if c, ok := err.(causer); ok {
err = c.Cause()
var customErr *Error
tac0turtle marked this conversation as resolved.
Show resolved Hide resolved
if errors.As(err, &customErr) {
return customErr.ABCICode()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Error type is undefined in this context. Ensure it is imported or defined within the package.

+ type Error struct {
+     codespace string
+     code uint32
+     desc string
+ }

Also applies to: 77-79

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
var customErr *Error
if errors.As(err, &customErr) {
return customErr.ABCICode()
var customErr *Error
if errors.As(err, &customErr) {
return customErr.ABCICode()
type Error struct {
codespace string
code uint32
desc string
}
Tools
golangci-lint

59-59: undefined: Error (typecheck)

} else {
return internalABCICode
}
}
}

type codespacer interface {
Codespace() string
}

// abciCodespace tests if given error contains a codespace and returns the value of
// it if available. This function is testing for the causer interface as well
// and unwraps the error.
Expand All @@ -84,12 +74,9 @@ func abciCodespace(err error) string {
}

for {
if c, ok := err.(codespacer); ok {
return c.Codespace()
}

if c, ok := err.(causer); ok {
err = c.Cause()
var customErr *Error
tac0turtle marked this conversation as resolved.
Show resolved Hide resolved
if errors.As(err, &customErr) {
return customErr.Codespace()
tac0turtle marked this conversation as resolved.
Show resolved Hide resolved
} else {
return internalABCICodespace
}
Expand All @@ -110,3 +97,9 @@ func errIsNil(err error) bool {
}
return false
}

// causer is an interface implemented by an error that supports wrapping. Use
// it to test if an error wraps another error instance.
type causer interface {
Cause() error
}
113 changes: 22 additions & 91 deletions errors/abci_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,25 +3,10 @@ package errors
import (
"fmt"
"io"
"strings"
"testing"

"github.com/stretchr/testify/suite"
)

type abciTestSuite struct {
suite.Suite
}

func TestABCITestSuite(t *testing.T) {
suite.Run(t, new(abciTestSuite))
}

func (s *abciTestSuite) SetupSuite() {
s.T().Parallel()
}

func (s *abciTestSuite) TestABCInfo() {
func TestABCInfo(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

Codebase Verification

The search results indicate that there are no direct references to the additional error handling functions (debugErrEncoder, defaultErrEncoder, abciCode, abciCodespace, errIsNil) in the errors/abci_test.go file. This suggests that these functions might not be explicitly tested.

To ensure comprehensive test coverage, it is important to add test cases that directly test these functions. Here are the specific locations in the codebase that need attention:

  • errors/abci.go: Functions debugErrEncoder, defaultErrEncoder, abciCode, abciCodespace, errIsNil need direct test cases.
  • errors/abci_test.go: Add new test functions to cover the above-mentioned functions.

Please add the necessary test cases to ensure all error handling logic is thoroughly tested.

Analysis chain

Ensure comprehensive test coverage for new error handling logic.

The output shows that the errors/abci.go file contains several functions related to error handling, such as ABCIInfo, debugErrEncoder, defaultErrEncoder, abciCode, abciCodespace, and errIsNil. The errors/abci_test.go file includes two test functions: TestABCInfo and TestABCIInfoSerializeErr.

Both test functions seem to cover various scenarios for the ABCIInfo function, including different types of errors and debug modes. However, there is no explicit indication that the other error handling functions (debugErrEncoder, defaultErrEncoder, abciCode, abciCodespace, errIsNil) are directly tested.

To ensure comprehensive test coverage, we need to verify if these additional functions are tested either directly or indirectly through the existing test cases. Let's generate a script to search for references to these functions within the test file.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that all new error handling logic in `errors/abci.go` is covered by tests in `errors/abci_test.go`.

# Test: Search for test cases covering new logic. Expect: Test cases found.
rg --type go 'Test' errors/abci_test.go

Length of output: 118

Script:

#!/bin/bash
# Description: Identify error handling logic in `errors/abci.go` and check if the test functions in `errors/abci_test.go` cover them.

# Step 1: Search for error handling logic in `errors/abci.go`.
rg --type go 'error' errors/abci.go

# Step 2: Display the content of `errors/abci_test.go` to cross-reference the test functions.
cat errors/abci_test.go

Length of output: 3763

Script:

#!/bin/bash
# Description: Verify if additional error handling functions in `errors/abci.go` are covered by tests in `errors/abci_test.go`.

# Step 1: Search for references to `debugErrEncoder` in `errors/abci_test.go`.
rg --type go 'debugErrEncoder' errors/abci_test.go

# Step 2: Search for references to `defaultErrEncoder` in `errors/abci_test.go`.
rg --type go 'defaultErrEncoder' errors/abci_test.go

# Step 3: Search for references to `abciCode` in `errors/abci_test.go`.
rg --type go 'abciCode' errors/abci_test.go

# Step 4: Search for references to `abciCodespace` in `errors/abci_test.go`.
rg --type go 'abciCodespace' errors/abci_test.go

# Step 5: Search for references to `errIsNil` in `errors/abci_test.go`.
rg --type go 'errIsNil' errors/abci_test.go

Length of output: 240

cases := map[string]struct {
err error
debug bool
Expand All @@ -37,7 +22,7 @@ func (s *abciTestSuite) TestABCInfo() {
wantSpace: testCodespace,
},
"wrapped SDK error": {
err: Wrap(Wrap(ErrUnauthorized, "foo"), "bar"),
err: fmt.Errorf("bar: %w", fmt.Errorf("foo: %w", ErrUnauthorized)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Undefined error constant ErrUnauthorized needs to be defined or imported.

+ import (
+     "path/to/errors/constants" // Adjust the import path as necessary
+ )
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
err: fmt.Errorf("bar: %w", fmt.Errorf("foo: %w", ErrUnauthorized)),
import (
"path/to/errors/constants" // Adjust the import path as necessary
)
func TestErrors(t *testing.T) {
tests := []struct {
name string
err error
}{
{
name: "unauthorized error",
err: fmt.Errorf("bar: %w", fmt.Errorf("foo: %w", ErrUnauthorized)),
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if tt.err == nil {
t.Errorf("expected error, got nil")
}
})
}
}
Tools
golangci-lint

25-25: undefined: ErrUnauthorized (typecheck)

debug: false,
wantLog: "bar: foo: unauthorized",
wantCode: ErrUnauthorized.code,
Expand All @@ -64,93 +49,37 @@ func (s *abciTestSuite) TestABCInfo() {
wantCode: 1,
wantSpace: UndefinedCodespace,
},
// This is hard to test because of attached stacktrace. This
// case is tested in an another test.
// "wrapped stdlib is a full message in debug mode": {
// err: Wrap(io.EOF, "cannot read file"),
// debug: true,
// wantLog: "cannot read file: EOF",
// wantCode: 1,
// },
"custom error": {
err: customErr{},
debug: false,
wantLog: "custom",
wantCode: 999,
wantSpace: "extern",
},
"custom error in debug mode": {
err: customErr{},
debug: true,
wantLog: "custom",
wantCode: 999,
wantSpace: "extern",
},
}

for testName, tc := range cases {
s.T().Run(testName, func(t *testing.T) {
t.Run(testName, func(t *testing.T) {
space, code, log := ABCIInfo(tc.err, tc.debug)
s.Require().Equal(tc.wantSpace, space, testName)
s.Require().Equal(tc.wantCode, code, testName)
s.Require().Equal(tc.wantLog, log, testName)
})
}
}

func (s *abciTestSuite) TestABCIInfoStacktrace() {
cases := map[string]struct {
err error
debug bool
wantStacktrace bool
wantErrMsg string
}{
"wrapped SDK error in debug mode provides stacktrace": {
err: Wrap(ErrUnauthorized, "wrapped"),
debug: true,
wantStacktrace: true,
wantErrMsg: "wrapped: unauthorized",
},
"wrapped SDK error in non-debug mode does not have stacktrace": {
err: Wrap(ErrUnauthorized, "wrapped"),
debug: false,
wantStacktrace: false,
wantErrMsg: "wrapped: unauthorized",
},
"wrapped stdlib error in debug mode provides stacktrace": {
err: Wrap(fmt.Errorf("stdlib"), "wrapped"),
debug: true,
wantStacktrace: true,
wantErrMsg: "wrapped: stdlib",
},
}

const thisTestSrc = "cosmossdk.io/errors.(*abciTestSuite).TestABCIInfoStacktrace"

for testName, tc := range cases {
s.T().Run(testName, func(t *testing.T) {
_, _, log := ABCIInfo(tc.err, tc.debug)
if !tc.wantStacktrace {
s.Require().Equal(tc.wantErrMsg, log, testName)
} else {
s.Require().True(strings.Contains(log, thisTestSrc), testName)
s.Require().True(strings.Contains(log, tc.wantErrMsg), testName)
if space != tc.wantSpace {
tac0turtle marked this conversation as resolved.
Show resolved Hide resolved
t.Errorf("%s: expected space %s, got %s", testName, tc.wantSpace, space)
}
if code != tc.wantCode {
t.Errorf("%s: expected code %d, got %d", testName, tc.wantCode, code)
}
if log != tc.wantLog {
t.Errorf("%s: expected log %s, got %s", testName, tc.wantLog, log)
Comment on lines 56 to +64
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

Codebase Verification

The ABCIInfo function is not accessible in the test context because the package containing it is not imported in errors/abci_test.go. Please add the necessary import statement to ensure ABCIInfo is available for testing.

  • errors/abci_test.go does not import the package containing ABCIInfo.
Analysis chain

Ensure ABCIInfo is defined and accessible in the test context.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the definition and accessibility of `ABCIInfo` in the test context.

# Test: Search for the definition of `ABCIInfo`. Expect: Definition found.
ast-grep --lang go --pattern $'func ABCIInfo($_, $_) ($_,$_,$_){ $$$ }'

Length of output: 581

Script:

#!/bin/bash
# Description: Verify if `errors/abci_test.go` imports the package containing `ABCIInfo`.

# Test: Search for import statements in `errors/abci_test.go`. Expect: Import statement for `errors/abci.go` found.
rg --lang go --pattern 'import' errors/abci_test.go

Length of output: 460

Script:

#!/bin/bash
# Description: Verify if `errors/abci_test.go` imports the package containing `ABCIInfo`.

# Test: Search for import statements in `errors/abci_test.go`. Expect: Import statement for `errors/abci.go` found.
rg 'import' errors/abci_test.go

Length of output: 40

Script:

#!/bin/bash
# Description: List all import statements in `errors/abci_test.go` to verify if the package containing `ABCIInfo` is imported.

# Test: List all import statements in `errors/abci_test.go`. Expect: Import statement for `errors/abci.go` or its package found.
rg '^import' -A 10 errors/abci_test.go

Length of output: 193

Tools
golangci-lint

56-56: undefined: ABCIInfo (typecheck)

Comment on lines +55 to +64
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor test cases to use assertions for improved readability and maintainability.

- if space != tc.wantSpace {
-   t.Errorf("%s: expected space %s, got %s", testName, tc.wantSpace, space)
- }
- if code != tc.wantCode {
-   t.Errorf("%s: expected code %d, got %d", testName, tc.wantCode, code)
- }
- if log != tc.wantLog {
-   t.Errorf("%s: expected log %s, got %s", testName, tc.wantLog, log)
- }
+ assert.Equal(t, tc.wantSpace, space, "%s: expected space %s, got %s", testName, tc.wantSpace, space)
+ assert.Equal(t, tc.wantCode, code, "%s: expected code %d, got %d", testName, tc.wantCode, code)
+ assert.Equal(t, tc.wantLog, log, "%s: expected log %s, got %s", testName, tc.wantLog, log)

Committable suggestion was skipped due to low confidence.

Tools
golangci-lint

56-56: undefined: ABCIInfo (typecheck)

}
})
}
}

func (s *abciTestSuite) TestABCIInfoHidesStacktrace() {
err := Wrap(ErrUnauthorized, "wrapped")
func TestABCIInfoHidesStacktrace(t *testing.T) {
tac0turtle marked this conversation as resolved.
Show resolved Hide resolved
err := fmt.Errorf("wrapped: %w", ErrUnauthorized)
_, _, log := ABCIInfo(err, false)
s.Require().Equal("wrapped: unauthorized", log)
if log != "wrapped: unauthorized" {
t.Errorf("expected log %s, got %s", "wrapped: unauthorized", log)
}
}

func (s *abciTestSuite) TestABCIInfoSerializeErr() {
func TestABCIInfoSerializeErr(t *testing.T) {
var (
// Create errors with stacktrace for equal comparison.
myErrDecode = Wrap(ErrTxDecode, "test")
myErrAddr = Wrap(ErrInvalidAddress, "tester")
myErrDecode = fmt.Errorf("test: %w", ErrTxDecode)
tac0turtle marked this conversation as resolved.
Show resolved Hide resolved
myErrAddr = fmt.Errorf("tester: %w", ErrInvalidAddress)
myPanic = ErrPanic
Comment on lines +73 to 75
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure error constants like ErrTxDecode, ErrInvalidAddress, and ErrPanic are defined or imported.

+ import (
+     "path/to/errors/constants" // Adjust the import path as necessary
+ )

Committable suggestion was skipped due to low confidence.

Tools
golangci-lint

73-73: undefined: ErrTxDecode (typecheck)

74-74: undefined: ErrInvalidAddress (typecheck)

75-75: undefined: ErrPanic (typecheck)

)

Expand Down Expand Up @@ -183,7 +112,9 @@ func (s *abciTestSuite) TestABCIInfoSerializeErr() {
for msg, spec := range specs {
spec := spec
_, _, log := ABCIInfo(spec.src, spec.debug)
s.Require().Equal(spec.exp, log, msg)
if log != spec.exp {
t.Errorf("%s: expected log %s, got %s", msg, spec.exp, log)
}
}
}

Expand Down
Loading
Loading