Skip to content

Commit

Permalink
Improve focus rule
Browse files Browse the repository at this point in the history
1. Forbid also the usage of the `Focus` individual spec, e.g.
    It("test something", Focus, func(){
        Expect(...)...
    })
2. Add `FDescribeTable` and `FEntry` to the forbiden containers.
  • Loading branch information
nunnatsa committed Aug 14, 2023
1 parent 469bb35 commit 7fd98db
Show file tree
Hide file tree
Showing 10 changed files with 209 additions and 17 deletions.
29 changes: 20 additions & 9 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -153,24 +153,35 @@ The linter will not suggest a fix for this warning.
This rule cannot be suppressed.
### Focus Container Found [BUG]
This rule finds ginkgo focus containers in the code.
### Focus Container / Focus individual spec found [BUG]
This rule finds ginkgo focus containers, or the `Focus` individual spec in the code.
ginkgo supports the `FDescribe`, `FContext`, `FWhen` and `FIt` containers to allow the developer to focus
ginkgo supports the `FDescribe`, `FContext`, `FWhen`, `FIt`, `FDescribeTable` and `FEntry`
containers to allow the developer to focus
on a specific test or set of tests during test development or debug.
***This rule is disabled by default***. Use the `--forbid-focus-container=true` command line flag to enable it.
For example:
```go
var _ = Describe("checking something", func() {
FIt("this test is the only one that will run", func(){
...
})
FIt("this test is the only one that will run", func(){
...
})
})
```
Alternatively, the `Focus` individual spec may be used for the same purpose, e.g.
```go
var _ = Describe("checking something", Focus, func() {
It("this test is the only one that will run", func(){
...
})
})
```
These container must not be part of the final source code, and should only be used locally by the developer.
These container, or the `Focus` spec, must not be part of the final source code, and should only be used locally by the developer.
***This rule is disabled by default***. Use the `--forbid-focus-container=true` command line flag to enable it.
### Wrong Length Assertion [STYLE]
The linter finds assertion of the golang built-in `len` function, with all kind of matchers, while there are already gomega matchers for these usecases; We want to assert the item, rather than its length.
Expand Down
20 changes: 18 additions & 2 deletions ginkgo_linter.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ const (
missingAssertionMessage = linterName + `: %q: missing assertion method. Expected "Should()", "To()", "ShouldNot()", "ToNot()" or "NotTo()"`
missingAsyncAssertionMessage = linterName + `: %q: missing assertion method. Expected "Should()" or "ShouldNot()"`
focusContainerFound = linterName + ": Focus container found. This is used only for local debug and should not be part of the actual source code, consider to replace with %q"
focusSpecFound = linterName + ": Focus spec found. This is used only for local debug and should not be part of the actual source code, consider to remove it"
)
const ( // gomega matchers
beEmpty = "BeEmpty"
Expand Down Expand Up @@ -232,12 +233,27 @@ func (l *ginkgoLinter) run(pass *analysis.Pass) (interface{}, error) {
}

func checkFocusContainer(pass *analysis.Pass, ginkgoHndlr ginkgohandler.Handler, exp *ast.CallExpr) bool {
foundFocus := false
isFocus, id := ginkgoHndlr.GetFocusContainerName(exp)
if isFocus {
reportNewName(pass, id, id.Name[1:], focusContainerFound, id.Name)
return true
foundFocus = true
}
return false

if id != nil && ginkgohandler.IsContainer(id) {
for _, arg := range exp.Args {
if ginkgoHndlr.IsFocusSpec(arg) {
reportNoFix(pass, arg.Pos(), focusSpecFound)
foundFocus = true
} else if callExp, ok := arg.(*ast.CallExpr); ok {
if checkFocusContainer(pass, ginkgoHndlr, callExp) { // handle table entries
foundFocus = true
}
}
}
}

return foundFocus
}

func checkExpression(pass *analysis.Pass, config types.Config, assertionExp *ast.CallExpr, actualExpr *ast.CallExpr, handler gomegahandler.Handler) bool {
Expand Down
35 changes: 33 additions & 2 deletions ginkgohandler/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,24 @@ import (
"go/ast"
)

const (
importPath = `"github.com/onsi/ginkgo"`
importPathV2 = `"github.com/onsi/ginkgo/v2"`

focusSpec = "Focus"
)

// Handler provide different handling, depend on the way ginkgo was imported, whether
// in imported with "." name, custom name or without any name.
type Handler interface {
GetFocusContainerName(*ast.CallExpr) (bool, *ast.Ident)
IsFocusSpec(ident ast.Expr) bool
}

// GetGinkgoHandler returns a ginkgor handler according to the way ginkgo was imported in the specific file
func GetGinkgoHandler(file *ast.File) Handler {
for _, imp := range file.Imports {
if imp.Path.Value != `"github.com/onsi/ginkgo"` && imp.Path.Value != `"github.com/onsi/ginkgo/v2"` {
if imp.Path.Value != importPath && imp.Path.Value != importPathV2 {
continue
}

Expand Down Expand Up @@ -41,6 +49,11 @@ func (h dotHandler) GetFocusContainerName(exp *ast.CallExpr) (bool, *ast.Ident)
return false, nil
}

func (h dotHandler) IsFocusSpec(exp ast.Expr) bool {
id, ok := exp.(*ast.Ident)
return ok && id.Name == focusSpec
}

// nameHandler is used when importing ginkgo without name; i.e.
// import "github.com/onsi/ginkgo"
//
Expand All @@ -57,10 +70,28 @@ func (h nameHandler) GetFocusContainerName(exp *ast.CallExpr) (bool, *ast.Ident)
return false, nil
}

func (h nameHandler) IsFocusSpec(exp ast.Expr) bool {
if selExp, ok := exp.(*ast.SelectorExpr); ok {
if x, ok := selExp.X.(*ast.Ident); ok && x.Name == string(h) {
return selExp.Sel.Name == focusSpec
}
}

return false
}

func isFocusContainer(name string) bool {
switch name {
case "FDescribe", "FContext", "FWhen", "FIt":
case "FDescribe", "FContext", "FWhen", "FIt", "FDescribeTable", "FEntry":
return true
}
return false
}

func IsContainer(id *ast.Ident) bool {
switch id.Name {
case "It", "When", "Context", "Describe", "DescribeTable", "Entry":
return true
}
return isFocusContainer(id.Name)
}
16 changes: 16 additions & 0 deletions testdata/src/a/focus/focus_key_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package focus

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

var _ = Describe("focused Describe", Focus, func() {
When("focused when", Focus, func() {
Context("focused Context", Focus, func() {
It("focused It", Focus, func() {
Expect(len("1234")).Should(Equal(4)) // want `ginkgo-linter: wrong length assertion; consider using .Expect\("1234"\)\.Should\(HaveLen\(4\)\). instead`
})
})
})
})
22 changes: 22 additions & 0 deletions testdata/src/a/focus/table_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package focus

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

var _ = Describe("test focused tables", func() {
FDescribeTable("focused table", func(s []int, l int) {
Expect(len(s)).Should(Equal(l)) // want `ginkgo-linter: wrong length assertion; consider using .Expect\(s\)\.Should\(HaveLen\(l\)\). instead`
},
Entry([]int{1, 2, 3, 4}, 4),
FEntry([]int{1, 2, 3, 4, 5}, 5),
)

DescribeTable("non-focused table", func(s []int, l int) {
Expect(s).Should(HaveLen(l))
},
Entry([]int{1, 2, 3, 4}, 4),
FEntry([]int{1, 2, 3, 4, 5}, 5),
)
})
16 changes: 16 additions & 0 deletions testdata/src/a/focusconfig/focus_key_name_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package focus

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

var _ = ginkgo.Describe("focused Describe", ginkgo.Focus, func() { // want `ginkgo-linter: Focus spec found. This is used only for local debug and should not be part of the actual source code, consider to remove it`
ginkgo.When("focused when", ginkgo.Focus, func() { // want `ginkgo-linter: Focus spec found. This is used only for local debug and should not be part of the actual source code, consider to remove it`
ginkgo.Context("focused Context", ginkgo.Focus, func() { // want `ginkgo-linter: Focus spec found. This is used only for local debug and should not be part of the actual source code, consider to remove it`
ginkgo.It("focused It", ginkgo.Focus, func() { // want `ginkgo-linter: Focus spec found. This is used only for local debug and should not be part of the actual source code, consider to remove it`
Expect(len("1234")).Should(Equal(4)) // want `ginkgo-linter: wrong length assertion; consider using .Expect\("1234"\)\.Should\(HaveLen\(4\)\). instead`
})
})
})
})
16 changes: 16 additions & 0 deletions testdata/src/a/focusconfig/focus_key_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package focus

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

var _ = Describe("focused Describe", Focus, func() { // want `ginkgo-linter: Focus spec found. This is used only for local debug and should not be part of the actual source code, consider to remove it`
When("focused when", Focus, func() { // want `ginkgo-linter: Focus spec found. This is used only for local debug and should not be part of the actual source code, consider to remove it`
Context("focused Context", Focus, func() { // want `ginkgo-linter: Focus spec found. This is used only for local debug and should not be part of the actual source code, consider to remove it`
It("focused It", Focus, func() { // want `ginkgo-linter: Focus spec found. This is used only for local debug and should not be part of the actual source code, consider to remove it`
Expect(len("1234")).Should(Equal(4)) // want `ginkgo-linter: wrong length assertion; consider using .Expect\("1234"\)\.Should\(HaveLen\(4\)\). instead`
})
})
})
})
32 changes: 32 additions & 0 deletions testdata/src/a/focusconfig/table_name_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package focus

import (
gnk "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
)

var _ = gnk.Describe("test focused tables", func() {
gnk.FDescribeTable("focused table", func(s []int, l int) { // want `ginkgo-linter: Focus container found. This is used only for local debug and should not be part of the actual source code, consider to replace with "DescribeTable"`
Expect(len(s)).Should(Equal(l)) // want `ginkgo-linter: wrong length assertion; consider using .Expect\(s\)\.Should\(HaveLen\(l\)\). instead`
},
gnk.Entry("check slice not focused", []int{1, 2, 3, 4}, 4),
gnk.Entry("check slice focus spec", gnk.Focus, []int{1, 2, 3, 4}, 4), // want `ginkgo-linter: Focus spec found. This is used only for local debug and should not be part of the actual source code, consider to remove it`
gnk.FEntry("check slice focused", []int{1, 2, 3, 4, 5}, 5), // want `ginkgo-linter: Focus container found. This is used only for local debug and should not be part of the actual source code, consider to replace with "Entry"`
)

gnk.DescribeTable("non-focused table", func(s []int, l int) {
Expect(s).Should(HaveLen(l))
},
gnk.Entry("check slice not focused", []int{1, 2, 3, 4}, 4),
gnk.Entry("check slice focus spec", gnk.Focus, []int{1, 2, 3, 4}, 4), // want `ginkgo-linter: Focus spec found. This is used only for local debug and should not be part of the actual source code, consider to remove it`
gnk.FEntry("check slice focused", []int{1, 2, 3, 4, 5}, 5), // want `ginkgo-linter: Focus container found. This is used only for local debug and should not be part of the actual source code, consider to replace with "Entry"`
)

gnk.DescribeTable("spec focused table", gnk.Focus, func(s []int, l int) { // want `ginkgo-linter: Focus spec found. This is used only for local debug and should not be part of the actual source code, consider to remove it`
Expect(s).Should(HaveLen(l))
},
gnk.Entry("check slice not focused", []int{1, 2, 3, 4}, 4),
gnk.Entry("check slice focus spec", gnk.Focus, []int{1, 2, 3, 4}, 4), // want `ginkgo-linter: Focus spec found. This is used only for local debug and should not be part of the actual source code, consider to remove it`
gnk.FEntry("check slice focused", []int{1, 2, 3, 4, 5}, 5), // want `ginkgo-linter: Focus container found. This is used only for local debug and should not be part of the actual source code, consider to replace with "Entry"`
)
})
32 changes: 32 additions & 0 deletions testdata/src/a/focusconfig/table_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package focus

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

var _ = Describe("test focused tables", func() {
FDescribeTable("focused table", func(s []int, l int) { // want `ginkgo-linter: Focus container found. This is used only for local debug and should not be part of the actual source code, consider to replace with "DescribeTable"`
Expect(len(s)).Should(Equal(l)) // want `ginkgo-linter: wrong length assertion; consider using .Expect\(s\)\.Should\(HaveLen\(l\)\). instead`
},
Entry("check slice not focused", []int{1, 2, 3, 4}, 4),
Entry("check slice focus spec", Focus, []int{1, 2, 3, 4}, 4), // want `ginkgo-linter: Focus spec found. This is used only for local debug and should not be part of the actual source code, consider to remove it`
FEntry("check slice focused", []int{1, 2, 3, 4, 5}, 5), // want `ginkgo-linter: Focus container found. This is used only for local debug and should not be part of the actual source code, consider to replace with "Entry"`
)

DescribeTable("non-focused table", func(s []int, l int) {
Expect(s).Should(HaveLen(l))
},
Entry("check slice not focused", []int{1, 2, 3, 4}, 4),
Entry("check slice focus spec", Focus, []int{1, 2, 3, 4}, 4), // want `ginkgo-linter: Focus spec found. This is used only for local debug and should not be part of the actual source code, consider to remove it`
FEntry("check slice focused", []int{1, 2, 3, 4, 5}, 5), // want `ginkgo-linter: Focus container found. This is used only for local debug and should not be part of the actual source code, consider to replace with "Entry"`
)

DescribeTable("spec focused table", Focus, func(s []int, l int) { // want `ginkgo-linter: Focus spec found. This is used only for local debug and should not be part of the actual source code, consider to remove it`
Expect(s).Should(HaveLen(l))
},
Entry("check slice not focused", []int{1, 2, 3, 4}, 4),
Entry("check slice focus spec", Focus, []int{1, 2, 3, 4}, 4), // want `ginkgo-linter: Focus spec found. This is used only for local debug and should not be part of the actual source code, consider to remove it`
FEntry("check slice focused", []int{1, 2, 3, 4, 5}, 5), // want `ginkgo-linter: Focus container found. This is used only for local debug and should not be part of the actual source code, consider to replace with "Entry"`
)
})
8 changes: 4 additions & 4 deletions tests/e2e.sh
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,20 @@ cp ginkgolinter testdata/src/a
cd testdata/src/a

# no suppress
[[ $(./ginkgolinter a/... 2>&1 | wc -l) == 2374 ]]
[[ $(./ginkgolinter a/... 2>&1 | wc -l) == 2380 ]]
# suppress all but nil
[[ $(./ginkgolinter --suppress-len-assertion=true --suppress-err-assertion=true --suppress-compare-assertion=true --suppress-async-assertion=true a/... 2>&1 | wc -l) == 1452 ]]
# suppress all but len
[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-err-assertion=true --suppress-compare-assertion=true --suppress-async-assertion=true a/... 2>&1 | wc -l) == 744 ]]
[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-err-assertion=true --suppress-compare-assertion=true --suppress-async-assertion=true a/... 2>&1 | wc -l) == 750 ]]
# suppress all but err
[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-len-assertion=true --suppress-compare-assertion=true --suppress-async-assertion=true a/... 2>&1 | wc -l) == 212 ]]
# suppress all but compare
[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-err-assertion=true --suppress-len-assertion=true --suppress-async-assertion=true a/... 2>&1 | wc -l) == 222 ]]
# suppress all but async
[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-err-assertion=true --suppress-len-assertion=true --suppress-compare-assertion=true a/... 2>&1 | wc -l) == 119 ]]
# suppress all but focus
[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-err-assertion=true --suppress-len-assertion=true --suppress-compare-assertion=true --suppress-async-assertion=true --forbid-focus-container=true a/... 2>&1 | wc -l) == 112 ]]
[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-err-assertion=true --suppress-len-assertion=true --suppress-compare-assertion=true --suppress-async-assertion=true --forbid-focus-container=true a/... 2>&1 | wc -l) == 143 ]]
# allow HaveLen(0)
[[ $(./ginkgolinter --allow-havelen-0=true a/... 2>&1 | wc -l) == 2361 ]]
[[ $(./ginkgolinter --allow-havelen-0=true a/... 2>&1 | wc -l) == 2367 ]]
# suppress all - should only return the few non-suppressble
[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-len-assertion=true --suppress-err-assertion=true --suppress-compare-assertion=true --suppress-async-assertion=true --forbid-focus-container=false a/... 2>&1 | wc -l) == 88 ]]

0 comments on commit 7fd98db

Please sign in to comment.