Skip to content

Commit c325ce8

Browse files
pohlynunnatsa
authored andcommitted
accept async functions with testing.TB variant instead of Gomega
In Kubernetes' ktesting package the following assertion is valid: tCtx.Eventually(func(tCtx TContext) { /* maybe fail via tCtx */ }).Should(Succeed()) That works because tCtx.Eventually wraps the function such that failures reported via tCtx.Error/Errorf/Fatal/Fatalf are visible to the matcher. Without this change, the "Success matcher only support a single error value, or function with Gomega as its first parameter" warning is triggered. Instead of building in specific support for ktesting, the approach is to check for a first parameter which has any (not necessarily all!) of the testing.TB methods for reporting failures. That is considered a valid alternative to a Gomega instance. There's now a potential false negative in the linter when passing such a function directly to gomega.Eventually/Consistently. That'll fail at runtime because gomega cannot call it, so such a false negative isn't too bad (fails reliably and obviously). The warning message doesn't get changed because it would be confusing for most users to call out some vague alternative (vague because the linter has no details) that most users won't be able to use.
1 parent c5a6b5f commit c325ce8

8 files changed

Lines changed: 206 additions & 4 deletions

File tree

internal/expression/actual/actualarg.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ const (
2929
FuncSigArgType
3030
ErrFuncActualArgType
3131
GomegaParamArgType
32+
TBParamArgType
3233
MultiRetsArgType
3334
ErrorMethodArgType
3435

internal/expression/actual/asyncfuncarg.go

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,13 @@ func getAsyncFuncArg(sig *gotypes.Signature) ArgPayload {
1717

1818
if sig.Params().Len() > 0 {
1919
arg := sig.Params().At(0).Type()
20-
if gomegainfo.IsGomegaType(arg) && sig.Results().Len() == 0 {
21-
argType |= FuncSigArgType | GomegaParamArgType
20+
if sig.Results().Len() == 0 {
21+
if gomegainfo.IsGomegaType(arg) {
22+
argType |= FuncSigArgType | GomegaParamArgType
23+
}
24+
if typecheck.ImplementsTB(arg) {
25+
argType |= FuncSigArgType | TBParamArgType
26+
}
2227
}
2328
}
2429

internal/rules/asyncsucceedrule.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,14 +17,17 @@ func (AsyncSucceedRule) isApply(gexp *expression.GomegaExpression) bool {
1717
return gexp.IsAsync() &&
1818
gexp.MatcherTypeIs(matcher.SucceedMatcherType) &&
1919
gexp.ActualArgTypeIs(actual.FuncSigArgType) &&
20-
!gexp.ActualArgTypeIs(actual.ErrorTypeArgType|actual.GomegaParamArgType)
20+
!gexp.ActualArgTypeIs(actual.ErrorTypeArgType|actual.GomegaParamArgType|actual.TBParamArgType)
2121
}
2222

2323
func (r AsyncSucceedRule) Apply(gexp *expression.GomegaExpression, _ config.Config, reportBuilder *reports.Builder) bool {
2424
if r.isApply(gexp) {
2525
if gexp.ActualArgTypeIs(actual.MultiRetsArgType) {
2626
reportBuilder.AddIssue(false, "Success matcher does not support multiple values")
2727
} else {
28+
// The message intentionally does not call out "function with a TB implementation" as another alternative because
29+
// that alternative is not valid for generic Gomega - it would be confusing for many users. Users
30+
// of a Gomega wrapper which supports such functions must figure that out themselves.
2831
reportBuilder.AddIssue(false, "Success matcher only support a single error value, or function with Gomega as its first parameter")
2932
}
3033
}

internal/typecheck/interfaces.go

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,16 @@ import (
88
var (
99
errorType *gotypes.Interface
1010
gomegaMatcherType *gotypes.Interface
11+
tbTypes = []*gotypes.Interface{
12+
// In practice, interfaces which mimick testing.TB probably implement
13+
// more than one of these at the same time. But for ImplementsTB
14+
// it's sufficient to have just one method which can be used
15+
// to report a test failure.
16+
tbInterface("Error", false),
17+
tbInterface("Errorf", true),
18+
tbInterface("Fatal", false),
19+
tbInterface("Fatalf", true),
20+
}
1121
)
1222

1323
func init() {
@@ -76,3 +86,32 @@ func ImplementsError(t gotypes.Type) bool {
7686
func ImplementsGomegaMatcher(t gotypes.Type) bool {
7787
return t != nil && gotypes.Implements(t, gomegaMatcherType)
7888
}
89+
90+
// ImplementsTB checks if the argument type implements any of the methods in testing.TB which
91+
// can be used to report test failures. Such a type is a potential alternative to a Gomega
92+
// parameter in some Gomega wrappers.
93+
func ImplementsTB(t gotypes.Type) bool {
94+
for _, tbType := range tbTypes {
95+
if gotypes.Implements(t, tbType) {
96+
return true
97+
}
98+
}
99+
return false
100+
}
101+
102+
// tbInterface generates an interface type with exactly one method
103+
// which has the given name and Printf or Println signature.
104+
func tbInterface(name string, printf bool) *gotypes.Interface {
105+
var params []*gotypes.Var
106+
if printf {
107+
params = append(params, gotypes.NewVar(0, nil, "", gotypes.Typ[gotypes.String]))
108+
}
109+
params = append(params, gotypes.NewVar(0, nil, "", gotypes.NewSlice(gotypes.Universe.Lookup("any").Type())))
110+
signature := gotypes.NewSignatureType(nil, nil, nil,
111+
gotypes.NewTuple(params...),
112+
gotypes.NewTuple(),
113+
true,
114+
)
115+
method := gotypes.NewFunc(0, nil, name, signature)
116+
return gotypes.NewInterfaceType([]*gotypes.Func{method}, nil)
117+
}
Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,129 @@
1+
package typecheck
2+
3+
import (
4+
"go/ast"
5+
"go/importer"
6+
"go/parser"
7+
"go/token"
8+
gotypes "go/types"
9+
"runtime"
10+
"strings"
11+
"testing"
12+
)
13+
14+
func TestImplementsTB(t *testing.T) {
15+
src := `
16+
package main
17+
18+
var (
19+
NilInstance = any(nil)
20+
)
21+
22+
type Error interface {
23+
Error(args ...any)
24+
}
25+
26+
type Errorf interface {
27+
Errorf(fmt string, args ...any)
28+
}
29+
30+
type Fatal interface {
31+
Fatal(args ...any)
32+
}
33+
34+
type Fatalf interface {
35+
Fatalf(fmt string, args ...any)
36+
}
37+
38+
type ErrorWrongArgs interface {
39+
Error(fmt string, args ...any)
40+
}
41+
42+
type ErrorfWrongArgs interface {
43+
ErrorF(args ...any)
44+
}
45+
46+
type FatalWrongArgs interface {
47+
Fatal(fmt string, args ...any)
48+
}
49+
50+
type FatalfWrongArgs interface {
51+
Fatalf(args ...any)
52+
}
53+
`
54+
pkg := mustTypecheck(src)
55+
56+
for name, expectResult := range map[string]bool{
57+
"NilInstance": false,
58+
"Error": true,
59+
"Errorf": true,
60+
"Fatal": true,
61+
"Fatalf": true,
62+
"ErrorWrongArgs": false,
63+
"ErrorfWrongArgs": false,
64+
"FatalWrongArgs": false,
65+
"FatalfWrongArgs": false,
66+
} {
67+
t.Run(name, func(t *testing.T) {
68+
instance := pkg.Scope().Lookup(name)
69+
if instance == nil {
70+
t.Fatalf("%s not defined in source code", name)
71+
}
72+
actualResult := ImplementsTB(instance.Type())
73+
if actualResult != expectResult {
74+
t.Errorf("expected %v, got %v", expectResult, actualResult)
75+
}
76+
})
77+
}
78+
}
79+
80+
// The following code was copied from https://cs.opensource.google/go/go/+/refs/tags/go1.25.6:src/go/types/api_test.go;l=29-74;drc=34b70684ba2fc8c5cba900e9abdfb874c1bd8c0e
81+
// and slightly simplified.
82+
//
83+
// Copyright 2013 The Go Authors. All rights reserved.
84+
// Use of this source code is governed by a BSD-style
85+
// license that can be found in the https://cs.opensource.google/go/go/+/refs/tags/go1.25.6:LICENSE
86+
87+
func defaultImporter(fset *token.FileSet) gotypes.Importer {
88+
return importer.ForCompiler(fset, runtime.Compiler, nil)
89+
}
90+
91+
func mustParse(fset *token.FileSet, src string) *ast.File {
92+
f, err := parser.ParseFile(fset, pkgName(src), src, parser.ParseComments)
93+
if err != nil {
94+
panic(err) // so we don't need to pass *testing.T
95+
}
96+
return f
97+
}
98+
99+
func typecheck(src string) (*gotypes.Package, error) {
100+
fset := token.NewFileSet()
101+
f := mustParse(fset, src)
102+
conf := &gotypes.Config{
103+
Error: func(error) {}, // collect all errors
104+
Importer: defaultImporter(fset),
105+
}
106+
return conf.Check(f.Name.Name, fset, []*ast.File{f}, nil)
107+
}
108+
109+
func mustTypecheck(src string) *gotypes.Package {
110+
pkg, err := typecheck(src)
111+
if err != nil {
112+
panic(err) // so we don't need to pass *testing.T
113+
}
114+
return pkg
115+
}
116+
117+
// pkgName extracts the package name from src, which must contain a package header.
118+
func pkgName(src string) string {
119+
const kw = "package "
120+
if i := strings.Index(src, kw); i >= 0 {
121+
after := src[i+len(kw):]
122+
n := len(after)
123+
if i := strings.IndexAny(after, "\n\t ;/"); i >= 0 {
124+
n = i
125+
}
126+
return after[:n]
127+
}
128+
panic("missing package header: " + src)
129+
}

testdata/src/a/eventually/eventuallyFunc.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,12 @@ var _ = Describe("Eventually with function", func() {
7878
Eventually(os.Getwd).Should(Equal("something")) // valid
7979
Eventually(os.Getwd()).Should(Equal("something")) // want `ginkgo-linter: use a function call in Eventually\. This actually checks nothing, because Eventually receives the function returned value, instead of function itself, and this value is never changed\. Consider using .Eventually\(os\.Getwd\)\.Should\(Equal\("something"\)\). instead`
8080

81+
Eventually(func(g Gomega) {}).Should(Succeed())
82+
wrappers.MyEventually(func(tb wrappers.MyTB) {}).Should(Succeed())
83+
Eventually(func() {}).Should(Succeed()) // want `ginkgo-linter: Success matcher only support a single error value, or function with Gomega as its first parameter`
84+
wrappers.MyEventually(func() {}).Should(Succeed()) // want `ginkgo-linter: Success matcher only support a single error value, or function with Gomega as its first parameter`
85+
wrappers.MyEventually(func(tb wrappers.MyTB) {}).Should(Succeed())
86+
8187
tst := test{}
8288
Eventually(tst.slowInt).Should(Equal(42)) // valid
8389
Eventually(retMethod(tst)).Should(Equal(42)) // valid. The function returns a method

testdata/src/a/eventually/eventuallyFunc.go.golden

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,13 @@ var _ = Describe("Eventually with function", func() {
7878
Eventually(os.Getwd).Should(Equal("something")) // valid
7979
Eventually(os.Getwd).Should(Equal("something")) // want `ginkgo-linter: use a function call in Eventually\. This actually checks nothing, because Eventually receives the function returned value, instead of function itself, and this value is never changed\. Consider using .Eventually\(os\.Getwd\)\.Should\(Equal\("something"\)\). instead`
8080

81-
tst := test{}
81+
Eventually(func(g Gomega) {}).Should(Succeed())
82+
wrappers.MyEventually(func(tb wrappers.MyTB) {}).Should(Succeed())
83+
Eventually(func() {}).Should(Succeed()) // want `ginkgo-linter: Success matcher only support a single error value, or function with Gomega as its first parameter`
84+
wrappers.MyEventually(func() {}).Should(Succeed()) // want `ginkgo-linter: Success matcher only support a single error value, or function with Gomega as its first parameter`
85+
wrappers.MyEventually(func(tb wrappers.MyTB) {}).Should(Succeed())
86+
87+
tst := test{}
8288
Eventually(tst.slowInt).Should(Equal(42)) // valid
8389
Eventually(retMethod(tst)).Should(Equal(42)) // valid. The function returns a method
8490
Eventually(tst.slowInt).Should(Equal(42)) // want `ginkgo-linter: use a function call in Eventually\. This actually checks nothing, because Eventually receives the function returned value, instead of function itself, and this value is never changed\. Consider using .Eventually\(tst\.slowInt\)\.Should\(Equal\(42\)\). instead`

testdata/src/a/wrappers/wrappers.go

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,30 @@
11
package wrappers
22

33
import (
4+
"testing"
5+
46
gtypes "github.com/onsi/gomega/types"
57

68
g "github.com/onsi/gomega"
79
)
810

11+
type MyTB interface {
12+
Error(args ...any)
13+
Errorf(format string, args ...any)
14+
Fatal(args ...any)
15+
Fatalf(format string, args ...any)
16+
}
17+
18+
var _ MyTB = &testing.T{}
19+
var _ MyTB = &testing.B{}
20+
921
var Expect = g.Expect
1022

1123
func MyExpect(value any) g.Assertion {
1224
return g.Expect(value)
1325
}
1426

27+
// value = func(tb MyTB) without return value is okay, failures can be reported via tb.Error/Fatal/Errorf/Fatalf.
1528
func MyEventually(value any) g.AsyncAssertion {
1629
return g.Eventually(value)
1730
}

0 commit comments

Comments
 (0)