Skip to content

Commit

Permalink
assert: improve failure message for ErrorIS
Browse files Browse the repository at this point in the history
Removes the stdlib error types from the failure message
(*errors.errorString, and fmt.wrapError). These types are not relevant because
they come from using fmt.Errorf or errors.New. The type of the error is only
relevant when the error is a user defined type.
  • Loading branch information
dnephin committed Apr 15, 2022
1 parent f3b9c7b commit c563078
Show file tree
Hide file tree
Showing 5 changed files with 60 additions and 27 deletions.
2 changes: 1 addition & 1 deletion assert/assert.go
Expand Up @@ -39,7 +39,7 @@ messages it produces.
assert.ErrorContains(t, err, "includes this")
// assertion failed: expected error to contain "includes this", got "oops"
assert.ErrorIs(t, err, os.ErrNotExist)
// assertion failed: error is "oops" (err *errors.errorString), not "file does not exist" (os.ErrNotExist *errors.errorString)
// assertion failed: error is "oops", not "file does not exist" (os.ErrNotExist)
// complex types
assert.DeepEqual(t, result, myStruct{Name: "title"})
Expand Down
4 changes: 2 additions & 2 deletions assert/assert_test.go
Expand Up @@ -450,15 +450,15 @@ func TestErrorIs(t *testing.T) {

var err error
ErrorIs(fakeT, err, os.ErrNotExist)
expected := `assertion failed: error is nil, not "file does not exist" (os.ErrNotExist *errors.errorString)`
expected := `assertion failed: error is nil, not "file does not exist" (os.ErrNotExist)`
expectFailNowed(t, fakeT, expected)
})
t.Run("different error", func(t *testing.T) {
fakeT := &fakeTestingT{}

err := fmt.Errorf("the actual error")
ErrorIs(fakeT, err, os.ErrNotExist)
expected := `assertion failed: error is "the actual error" (err *errors.errorString), not "file does not exist" (os.ErrNotExist *errors.errorString)`
expected := `assertion failed: error is "the actual error", not "file does not exist" (os.ErrNotExist)`
expectFailNowed(t, fakeT, expected)
})
t.Run("same error", func(t *testing.T) {
Expand Down
23 changes: 14 additions & 9 deletions assert/cmp/compare.go
Expand Up @@ -2,13 +2,13 @@
package cmp // import "gotest.tools/v3/assert/cmp"

import (
"errors"
"fmt"
"reflect"
"regexp"
"strings"

"github.com/google/go-cmp/cmp"
"github.com/pkg/errors"
"gotest.tools/v3/internal/format"
)

Expand Down Expand Up @@ -365,6 +365,11 @@ func isPtrToStruct(typ reflect.Type) bool {
return typ.Kind() == reflect.Ptr && typ.Elem().Kind() == reflect.Struct
}

var (
stdlibErrorNewType = reflect.TypeOf(errors.New(""))
stdlibFmtErrorType = reflect.TypeOf(fmt.Errorf("%w", fmt.Errorf("")))
)

// ErrorIs succeeds if errors.Is(actual, expected) returns true. See
// https://golang.org/pkg/errors/#Is for accepted argument values.
func ErrorIs(actual error, expected error) Comparison {
Expand All @@ -373,16 +378,16 @@ func ErrorIs(actual error, expected error) Comparison {
return ResultSuccess
}

// The type of stdlib errors is excluded because the type is not relevant
// in those cases. The type is only important when it is a user defined
// custom error type.
return ResultFailureTemplate(`error is
{{- if not .Data.a }} nil,{{ else }}
{{- printf " \"%v\"" .Data.a}} (
{{- with callArg 0 }}{{ formatNode . }} {{end -}}
{{- printf "%T" .Data.a -}}
),
{{- end }} not {{ printf "\"%v\"" .Data.x}} (
{{- with callArg 1 }}{{ formatNode . }} {{end -}}
{{- printf "%T" .Data.x -}}
)`,
{{- printf " \"%v\"" .Data.a }}
{{- if notStdlibErrorType .Data.a }} ({{ printf "%T" .Data.a }}){{ end }},
{{- end }} not {{ printf "\"%v\"" .Data.x }} (
{{- with callArg 1 }}{{ formatNode . }}{{ end }}
{{- if notStdlibErrorType .Data.x }}{{ printf " %T" .Data.x }}{{ end }})`,
map[string]interface{}{"a": actual, "x": expected})
}
}
52 changes: 37 additions & 15 deletions assert/cmp/compare_test.go
Expand Up @@ -600,30 +600,52 @@ func TestErrorIs(t *testing.T) {
result := ErrorIs(stubError{}, stubError{})()
assertSuccess(t, result)
})
t.Run("actual is nil", func(t *testing.T) {
t.Run("actual is nil, not stdlib error", func(t *testing.T) {
result := ErrorIs(nil, stubError{})()
args := []ast.Expr{
&ast.Ident{Name: "err"},
&ast.StarExpr{
X: &ast.SelectorExpr{
X: &ast.Ident{Name: "errors"},
Sel: &ast.Ident{Name: "errorString"},
}},
&ast.SelectorExpr{
X: &ast.Ident{Name: "mypkg"},
Sel: &ast.Ident{Name: "StubError"},
},
}
expected := `error is nil, not "stub error" (*errors.errorString cmp.stubError)`
expected := `error is nil, not "stub error" (mypkg.StubError cmp.stubError)`
assertFailureTemplate(t, result, args, expected)
})
t.Run("not equal", func(t *testing.T) {
result := ErrorIs(os.ErrClosed, stubError{})()
t.Run("not equal, not stdlib error", func(t *testing.T) {
result := ErrorIs(notStubError{}, stubError{})()
args := []ast.Expr{
&ast.Ident{Name: "err"},
&ast.StarExpr{
X: &ast.SelectorExpr{
X: &ast.Ident{Name: "errors"},
Sel: &ast.Ident{Name: "errorString"},
}},
&ast.SelectorExpr{
X: &ast.Ident{Name: "mypkg"},
Sel: &ast.Ident{Name: "StubError"},
},
}
expected := `error is "file already closed" (err *errors.errorString), not "stub error" (*errors.errorString cmp.stubError)`
expected := `error is "not stub error" (cmp.notStubError), not "stub error" (mypkg.StubError cmp.stubError)`
assertFailureTemplate(t, result, args, expected)
})
t.Run("actual is nil, stdlib error", func(t *testing.T) {
result := ErrorIs(nil, os.ErrClosed)()
args := []ast.Expr{
&ast.Ident{Name: "err"},
&ast.SelectorExpr{
X: &ast.Ident{Name: "os"},
Sel: &ast.Ident{Name: "ErrClosed"},
},
}
expected := `error is nil, not "file already closed" (os.ErrClosed)`
assertFailureTemplate(t, result, args, expected)
})
t.Run("not equal, stdlib error", func(t *testing.T) {
result := ErrorIs(fmt.Errorf("foo"), os.ErrClosed)()
args := []ast.Expr{
&ast.Ident{Name: "err"},
&ast.SelectorExpr{
X: &ast.Ident{Name: "os"},
Sel: &ast.Ident{Name: "ErrClosed"},
},
}
expected := `error is "foo", not "file already closed" (os.ErrClosed)`
assertFailureTemplate(t, result, args, expected)
})
}
6 changes: 6 additions & 0 deletions assert/cmp/result.go
Expand Up @@ -4,6 +4,7 @@ import (
"bytes"
"fmt"
"go/ast"
"reflect"
"text/template"

"gotest.tools/v3/internal/source"
Expand Down Expand Up @@ -85,6 +86,11 @@ func renderMessage(result templatedResult, args []ast.Expr) (string, error) {
}
return args[index]
},
// TODO: any way to include this from ErrorIS instead of here?
"notStdlibErrorType": func(typ interface{}) bool {
r := reflect.TypeOf(typ)
return r != stdlibFmtErrorType && r != stdlibErrorNewType
},
})
var err error
tmpl, err = tmpl.Parse(result.template)
Expand Down

0 comments on commit c563078

Please sign in to comment.