Skip to content

Commit

Permalink
add option to detect global Expect in async assertions
Browse files Browse the repository at this point in the history
  • Loading branch information
nunnatsa committed Dec 2, 2024
1 parent 3ad0777 commit 1d8e9f8
Show file tree
Hide file tree
Showing 16 changed files with 482 additions and 82 deletions.
20 changes: 11 additions & 9 deletions analyzer.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@ package ginkgolinter
import (
"flag"
"fmt"
"github.com/nunnatsa/ginkgolinter/gomegaanalyzer"

"golang.org/x/tools/go/analysis"

"github.com/nunnatsa/ginkgolinter/gomegaanalyzer"
"github.com/nunnatsa/ginkgolinter/linter"
"github.com/nunnatsa/ginkgolinter/types"
"github.com/nunnatsa/ginkgolinter/version"
Expand All @@ -28,14 +28,15 @@ func NewAnalyzerWithConfig(config *types.Config) *analysis.Analyzer {
// NewAnalyzer returns an Analyzer - the package interface with nogo
func NewAnalyzer() *analysis.Analyzer {
config := &types.Config{
SuppressLen: false,
SuppressNil: false,
SuppressErr: false,
SuppressCompare: false,
ForbidFocus: false,
AllowHaveLen0: false,
ForceExpectTo: false,
ForceSucceedForFuncs: false,
SuppressLen: false,
SuppressNil: false,
SuppressErr: false,
SuppressCompare: false,
ForbidFocus: false,
AllowHaveLen0: false,
ForceExpectTo: false,
ForceSucceedForFuncs: false,
ForbidAsyncGlobalAssertions: false,
}

a := NewAnalyzerWithConfig(config)
Expand All @@ -53,6 +54,7 @@ func NewAnalyzer() *analysis.Analyzer {
a.Flags.BoolVar(&config.ForbidFocus, "forbid-focus-container", config.ForbidFocus, "trigger a warning for ginkgo focus containers like FDescribe, FContext, FWhen or FIt; default = false.")
a.Flags.BoolVar(&config.ForbidSpecPollution, "forbid-spec-pollution", config.ForbidSpecPollution, "trigger a warning for variable assignments in ginkgo containers like Describe, Context and When, instead of in BeforeEach(); default = false.")
a.Flags.BoolVar(&config.ForceSucceedForFuncs, "force-succeed", config.ForceSucceedForFuncs, "force using the Succeed matcher for error functions, and the HaveOccurred matcher for non-function error values")
a.Flags.BoolVar(&config.ForbidAsyncGlobalAssertions, "forbid-global-assertion", config.ForbidAsyncGlobalAssertions, "don't allow global Expect inside async assertions")

return a
}
Expand Down
7 changes: 7 additions & 0 deletions analyzer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,13 @@ func TestFlags(t *testing.T) {
"force-succeed": "true",
},
},
{
testName: "forbid global async assertions",
testData: []string{"a/asyncglobal"},
flags: map[string]string{
"forbid-global-assertion": "true",
},
},
} {
t.Run(tc.testName, func(tt *testing.T) {
analyzer := ginkgolinter.NewAnalyzer()
Expand Down
65 changes: 56 additions & 9 deletions gomegaanalyzer/analyzer.go
Original file line number Diff line number Diff line change
@@ -1,34 +1,71 @@
package gomegaanalyzer

import (
"github.com/nunnatsa/ginkgolinter/internal/expression"
"github.com/nunnatsa/ginkgolinter/internal/gomegahandler"
"go/ast"
"golang.org/x/tools/go/analysis"
"go/token"
"reflect"
"sort"

"github.com/nunnatsa/ginkgolinter/internal/asyncfuncpos"

"golang.org/x/tools/go/analysis"

"github.com/nunnatsa/ginkgolinter/internal/expression"
"github.com/nunnatsa/ginkgolinter/internal/gomegahandler"
)

type Result map[ast.Expr]*expression.GomegaExpression
type FileResult map[ast.Expr]*expression.GomegaExpression

type Result struct {
expressions map[*ast.File]FileResult
asyncScope asyncfuncpos.Poses
}

func (r Result) GetFileResults() map[*ast.File]FileResult {
return r.expressions
}

func (r Result) GetFileResult(file *ast.File) (FileResult, bool) {
fr, ok := r.expressions[file]
return fr, ok
}

func (r Result) InAsyncScope(pos token.Pos) (token.Pos, bool) {
if fncPos, found := r.asyncScope.Contains(pos); found {
return fncPos.CallPos, true
}

return token.NoPos, false
}

func New() *analysis.Analyzer {
return &analysis.Analyzer{
Name: "gomegaAnalyzer",
Doc: "gather all gomega expressions",
Run: run,
ResultType: reflect.TypeOf(Result{}),
ResultType: reflect.TypeOf(&Result{}),
}
}

func run(pass *analysis.Pass) (any, error) {
expressions := make(Result)
expressions := make(map[*ast.File]FileResult)
var funcPoss asyncfuncpos.Poses

for _, file := range pass.Files {
gomegaHndlr := gomegahandler.GetGomegaHandler(file, pass)
if gomegaHndlr == nil {
continue
}

fileResult := make(FileResult)

ast.Inspect(file, func(n ast.Node) bool {
call, ok := n.(*ast.CallExpr)
stmt, ok := n.(*ast.ExprStmt)
if !ok {
return true
}

call, ok := stmt.X.(*ast.CallExpr)
if !ok {
return true
}
Expand All @@ -38,13 +75,23 @@ func run(pass *analysis.Pass) (any, error) {
return true
}

expressions[call] = expr
fileResult[call] = expr

if expr.IsAsync() {
if arg := expr.GetAsyncActualArg(); arg != nil && arg.HasAsyncFuncPos() {
funcPoss = append(funcPoss, arg.AsyncFucPos())
}
}

return true
})

expressions[file] = fileResult
}

return expressions, nil
sort.Sort(funcPoss)

return &Result{expressions: expressions, asyncScope: funcPoss}, nil
}

func getTimePkg(file *ast.File) string {
Expand Down
48 changes: 48 additions & 0 deletions internal/asyncfuncpos/asyncfuncpos.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
package asyncfuncpos

import (
"go/token"
gotypes "go/types"
)

type AsyncFuncPos struct {
Scope *gotypes.Scope
CallPos token.Pos
}

type Poses []*AsyncFuncPos

func (p Poses) Len() int {
return len(p)
}

func (p Poses) Less(i, j int) bool {
return p[i].Scope.Pos() < p[j].Scope.Pos()
}

func (p Poses) Swap(i, j int) {
p[i], p[j] = p[j], p[i]
}

func (p Poses) Contains(pos token.Pos) (*AsyncFuncPos, bool) {
if len(p) == 0 {
return nil, false
}
mid := len(p) / 2

if p[mid].Scope.Contains(pos) {
return p[0], true
}

left := p[:mid]
if l := len(left); l > 0 && left[0].Scope.Pos() <= pos && left[l-1].Scope.End() >= pos {
return left.Contains(pos)
} else {

Check failure on line 40 in internal/asyncfuncpos/asyncfuncpos.go

View workflow job for this annotation

GitHub Actions / build

indent-error-flow: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary) (revive)
right := p[mid+1:]
if l := len(right); l > 0 && right[0].Scope.Pos() <= pos && right[l-1].Scope.End() >= pos {
return right.Contains(pos)
}
}

return nil, false
}
52 changes: 50 additions & 2 deletions internal/expression/actual/asyncactual.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (

"golang.org/x/tools/go/analysis"

"github.com/nunnatsa/ginkgolinter/internal/asyncfuncpos"
"github.com/nunnatsa/ginkgolinter/internal/intervals"
)

Expand All @@ -17,6 +18,7 @@ type AsyncArg struct {
pollingInterval intervals.DurationValue
tooManyTimeouts bool
tooManyPolling bool
asyncPos *asyncfuncpos.AsyncFuncPos
}

func newAsyncArg(origExpr, cloneExpr, orig, clone *ast.CallExpr, argType gotypes.Type, pass *analysis.Pass, actualOffset int, timePkg string) *AsyncArg {
Expand All @@ -25,15 +27,52 @@ func newAsyncArg(origExpr, cloneExpr, orig, clone *ast.CallExpr, argType gotypes
valid = true
timeout intervals.DurationValue
polling intervals.DurationValue
funcPos *asyncfuncpos.AsyncFuncPos
)

if _, isActualFuncCall := orig.Args[actualOffset].(*ast.CallExpr); isActualFuncCall {
switch arg := orig.Args[actualOffset].(type) {
case *ast.CallExpr:
fun = clone.Args[actualOffset].(*ast.CallExpr)
valid = isValidAsyncValueType(argType)

case *ast.FuncLit:
var scope *gotypes.Scope
s, ok := pass.TypesInfo.Scopes[arg]
if ok {
scope = s
} else {
scope = gotypes.NewScope(nil, arg.Pos(), arg.End(), "")
}

funcPos = &asyncfuncpos.AsyncFuncPos{
Scope: scope,
CallPos: arg.Pos(),
}

case *ast.Ident:
if _, ok := pass.TypesInfo.TypeOf(arg).(*gotypes.Signature); ok {
if s, ok := pass.TypesInfo.Uses[arg]; ok {
if theFunc, ok := s.(*gotypes.Func); ok {
funcPos = &asyncfuncpos.AsyncFuncPos{
Scope: theFunc.Scope(),
CallPos: arg.Pos(),
}
}
}
}

case *ast.SelectorExpr:
if sel, ok := pass.TypesInfo.Uses[arg.Sel]; ok {
if theFunc, ok := sel.(*gotypes.Func); ok {
funcPos = &asyncfuncpos.AsyncFuncPos{
Scope: theFunc.Scope(),
CallPos: arg.Pos(),
}
}
}
}

timeoutOffset := actualOffset + 1
//var err error
tooManyTimeouts := false
tooManyPolling := false

Expand Down Expand Up @@ -87,6 +126,7 @@ func newAsyncArg(origExpr, cloneExpr, orig, clone *ast.CallExpr, argType gotypes
pollingInterval: polling,
tooManyTimeouts: tooManyTimeouts,
tooManyPolling: tooManyPolling,
asyncPos: funcPos,
}
}

Expand All @@ -110,6 +150,14 @@ func (a *AsyncArg) TooManyPolling() bool {
return a.tooManyPolling
}

func (a *AsyncArg) HasAsyncFuncPos() bool {
return a.asyncPos != nil
}

func (a *AsyncArg) AsyncFucPos() *asyncfuncpos.AsyncFuncPos {
return a.asyncPos
}

func isValidAsyncValueType(t gotypes.Type) bool {
switch t.(type) {
// allow functions that return function or channel.
Expand Down
6 changes: 6 additions & 0 deletions internal/expression/expression.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
type GomegaExpression struct {
orig *ast.CallExpr
clone *ast.CallExpr
pos token.Pos

assertionFuncName string
origAssertionFuncName string
Expand Down Expand Up @@ -85,6 +86,7 @@ func New(origExpr *ast.CallExpr, pass *analysis.Pass, handler gomegahandler.Hand
gexp := &GomegaExpression{
orig: origExpr,
clone: exprClone,
pos: origExpr.Pos(),

assertionFuncName: origSel.Sel.Name,
origAssertionFuncName: origSel.Sel.Name,
Expand All @@ -106,6 +108,10 @@ func New(origExpr *ast.CallExpr, pass *analysis.Pass, handler gomegahandler.Hand
return gexp, true
}

func (e *GomegaExpression) Pos() token.Pos {
return e.pos
}

func (e *GomegaExpression) IsMissingAssertion() bool {
return e.matcher == nil
}
Expand Down
4 changes: 4 additions & 0 deletions internal/formatter/formatter.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,7 @@ func (f GoFmtFormatter) Format(exp ast.Expr) string {
_ = printer.Fprint(&buf, f.fset, exp)
return buf.String()
}

func (f GoFmtFormatter) FormatPosition(pos token.Pos) string {
return f.fset.Position(pos).String()
}
5 changes: 5 additions & 0 deletions internal/reports/report-builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ func (b *Builder) OldExp() string {
}

func (b *Builder) AddIssue(suggestFix bool, issue string, args ...any) {
for i, arg := range args {
if p, ok := arg.(token.Pos); ok {
args[i] = b.formatter.FormatPosition(p)
}
}
if len(args) > 0 {
issue = fmt.Sprintf(issue, args...)
}
Expand Down
28 changes: 28 additions & 0 deletions internal/rules/asyncglobalexpect.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package rules

import (
"github.com/nunnatsa/ginkgolinter/gomegaanalyzer"
"github.com/nunnatsa/ginkgolinter/internal/expression"
"github.com/nunnatsa/ginkgolinter/internal/reports"
"github.com/nunnatsa/ginkgolinter/types"
)

type AsyncGlobalExpect struct {
res *gomegaanalyzer.Result
}

func NewAsyncGlobalExpect(res *gomegaanalyzer.Result) *AsyncGlobalExpect {
return &AsyncGlobalExpect{res: res}
}

func (r AsyncGlobalExpect) Apply(gexp *expression.GomegaExpression, config types.Config, reportBuilder *reports.Builder) bool {
if !bool(config.ForbidAsyncGlobalAssertions) || gexp.IsAsync() || gexp.IsUsingGomegaVar() {
return false
}

if pos, inScope := r.res.InAsyncScope(gexp.Pos()); inScope {
reportBuilder.AddIssue(false, "using a global %s in an async assertion at %v; either use a Gomega variable, or StopTrying(...).Now()", gexp.GetActualFuncName(), pos)
}

return false
}
Loading

0 comments on commit 1d8e9f8

Please sign in to comment.