Skip to content

Commit

Permalink
Fix bug: ginkgolinter ignores the Error() method
Browse files Browse the repository at this point in the history
as in:

```go
Expect(func() (int, error) {return 42, nil}()).Error().ToNot(HaveOccurred())
```
  • Loading branch information
nunnatsa committed Nov 11, 2024
1 parent be3cbdb commit 608f078
Show file tree
Hide file tree
Showing 8 changed files with 58 additions and 11 deletions.
4 changes: 4 additions & 0 deletions analyzer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,10 @@ func TestAllUseCases(t *testing.T) {
testName: "nil error-func variable",
testData: "a/issue-171",
},
{
testName: "respect the Error() method",
testData: "a/issue-173",
},
{
testName: "matchError with func return error-func",
testData: "a/issue-174",
Expand Down
4 changes: 2 additions & 2 deletions internal/expression/actual/actual.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,13 @@ type Actual struct {
actualOffset int
}

func New(origExpr, cloneExpr *ast.CallExpr, orig *ast.CallExpr, clone *ast.CallExpr, pass *analysis.Pass, handler gomegahandler.Handler, timePkg string) (*Actual, bool) {
func New(origExpr, cloneExpr *ast.CallExpr, orig *ast.CallExpr, clone *ast.CallExpr, pass *analysis.Pass, handler gomegahandler.Handler, timePkg string, errMethodExists bool) (*Actual, bool) {
funcName, ok := handler.GetActualFuncName(orig)
if !ok {
return nil, false
}

arg, actualOffset := getActualArgPayload(orig, clone, pass, funcName)
arg, actualOffset := getActualArgPayload(orig, clone, pass, funcName, errMethodExists)
if arg == nil {
return nil, false
}
Expand Down
13 changes: 11 additions & 2 deletions internal/expression/actual/actualarg.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ const (
ErrFuncActualArgType
GomegaParamArgType
MultiRetsArgType
ErrorMethodArgType

ErrorTypeArgType

Expand All @@ -39,15 +40,17 @@ func (a ArgType) Is(val ArgType) bool {
return a&val != 0
}

func getActualArgPayload(origActualExpr, actualExprClone *ast.CallExpr, pass *analysis.Pass, actualMethodName string) (ArgPayload, int) {
func getActualArgPayload(origActualExpr, actualExprClone *ast.CallExpr, pass *analysis.Pass, actualMethodName string, errMethodExists bool) (ArgPayload, int) {
origArgExpr, argExprClone, actualOffset, isGomegaExpr := getActualArg(origActualExpr, actualExprClone, actualMethodName, pass)
if !isGomegaExpr {
return nil, 0
}

var arg ArgPayload

if value.IsExprError(pass, origArgExpr) {
if errMethodExists {
arg = &ErrorMethodPayload{}
} else if value.IsExprError(pass, origArgExpr) {
arg = newErrPayload(origArgExpr, argExprClone, pass)
} else {
switch expr := origArgExpr.(type) {
Expand Down Expand Up @@ -183,6 +186,12 @@ func (*ErrPayload) ArgType() ArgType {
return ErrActualArgType | ErrorTypeArgType
}

type ErrorMethodPayload struct{}

func (ErrorMethodPayload) ArgType() ArgType {
return ErrorMethodArgType | ErrorTypeArgType
}

func parseBinaryExpr(origActualExpr, argExprClone *ast.BinaryExpr, pass *analysis.Pass) ArgPayload {
left, right, op := origActualExpr.X, origActualExpr.Y, origActualExpr.Op
replace := false
Expand Down
6 changes: 4 additions & 2 deletions internal/expression/expression.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,9 @@ func New(origExpr *ast.CallExpr, pass *analysis.Pass, handler gomegahandler.Hand
exprClone := astcopy.CallExpr(origExpr)
selClone := exprClone.Fun.(*ast.SelectorExpr)

origActual := handler.GetActualExpr(origSel)
errMethodExists := false

origActual := handler.GetActualExpr(origSel, &errMethodExists)
if origActual == nil {
return nil, false
}
Expand All @@ -62,7 +64,7 @@ func New(origExpr *ast.CallExpr, pass *analysis.Pass, handler gomegahandler.Hand
return nil, false
}

actl, ok := actual.New(origExpr, exprClone, origActual, actualClone, pass, handler, timePkg)
actl, ok := actual.New(origExpr, exprClone, origActual, actualClone, pass, handler, timePkg, errMethodExists)
if !ok {
return nil, false
}
Expand Down
8 changes: 6 additions & 2 deletions internal/gomegahandler/dothandler.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func (dotHandler) GetNewWrapperMatcher(name string, existing *ast.CallExpr) *ast
}
}

func (h dotHandler) GetActualExpr(assertionFunc *ast.SelectorExpr) *ast.CallExpr {
func (h dotHandler) GetActualExpr(assertionFunc *ast.SelectorExpr, errMethodExists *bool) *ast.CallExpr {
actualExpr, ok := assertionFunc.X.(*ast.CallExpr)
if !ok {
return nil
Expand All @@ -66,7 +66,11 @@ func (h dotHandler) GetActualExpr(assertionFunc *ast.SelectorExpr) *ast.CallExpr
return actualExpr
}
} else {
return h.GetActualExpr(fun)
if fun.Sel.Name == "Error" {
*errMethodExists = true
}

return h.GetActualExpr(fun, errMethodExists)
}
}
return nil
Expand Down
2 changes: 1 addition & 1 deletion internal/gomegahandler/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ type Handler interface {
// ReplaceFunction replaces the function with another one, for fix suggestions
ReplaceFunction(*ast.CallExpr, *ast.Ident)

GetActualExpr(assertionFunc *ast.SelectorExpr) *ast.CallExpr
GetActualExpr(assertionFunc *ast.SelectorExpr, errMethodExists *bool) *ast.CallExpr

GetActualExprClone(origFunc, funcClone *ast.SelectorExpr) *ast.CallExpr

Expand Down
7 changes: 5 additions & 2 deletions internal/gomegahandler/namedhandler.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func (g nameHandler) isGomegaVar(x ast.Expr) bool {
return gomegainfo.IsGomegaVar(x, g.pass)
}

func (g nameHandler) GetActualExpr(assertionFunc *ast.SelectorExpr) *ast.CallExpr {
func (g nameHandler) GetActualExpr(assertionFunc *ast.SelectorExpr, errMethodExists *bool) *ast.CallExpr {
actualExpr, ok := assertionFunc.X.(*ast.CallExpr)
if !ok {
return nil
Expand All @@ -69,7 +69,10 @@ func (g nameHandler) GetActualExpr(assertionFunc *ast.SelectorExpr) *ast.CallExp
return actualExpr
}
} else {
return g.GetActualExpr(fun)
if fun.Sel.Name == "Error" {
*errMethodExists = true
}
return g.GetActualExpr(fun, errMethodExists)
}
}
return nil
Expand Down
25 changes: 25 additions & 0 deletions testdata/src/a/issue-173/issue173.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package issue_173

import (
"errors"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
)

func intErrFunc() (int, error) {
return 42, nil
}

func strErr() (string, error) {
return "", errors.New("error")
}

var _ = Describe("test if issue 173 was solved", func() {
It("should respect the Error() method", func() {
Expect(intErrFunc()).Error().To(BeNil()) // want `ginkgo-linter: wrong error assertion. Consider using .Expect\(intErrFunc\(\)\)\.Error\(\)\.ToNot\(HaveOccurred\(\)\). instead`
Expect(intErrFunc()).Error().ToNot(HaveOccurred())
Expect(intErrFunc()).Error().ToNot(Succeed())
Expect(strErr()).Error().To(MatchError(ContainSubstring("error")))
})
})

0 comments on commit 608f078

Please sign in to comment.