diff --git a/README.md b/README.md index 3832f61..8e2e9ed 100644 --- a/README.md +++ b/README.md @@ -356,6 +356,17 @@ Expect(x1 == c1).Should(BeTrue()) // ==> Expect(x1).Should(Equal(c1)) Expect(c1 == x1).Should(BeTrue()) // ==> Expect(x1).Should(Equal(c1)) ``` +### Don't Allow Using `Expect` with `Should` or `ShouldNot` [STYLE] +This optional rule forces the usage of the `Expect` method only with the `To`, `ToNot` or `NotTo` +assertion methods; e.g. +```go +Expect("abc").Should(HaveLen(3)) // => Expect("abc").To(HaveLen(3)) +Expect("abc").ShouldNot(BeEmpty()) // => Expect("abc").ToNot(BeEmpty()) +``` +This rule support auto fixing. + +***This rule is disabled by default***. Use the `--force-expect-to=true` command line flag to enable it. + ## Suppress the linter ### Suppress warning from command line * Use the `--suppress-len-assertion=true` flag to suppress the wrong length assertion warning diff --git a/doc.go b/doc.go new file mode 100644 index 0000000..b94e37c --- /dev/null +++ b/doc.go @@ -0,0 +1,54 @@ +package ginkgolinter + +const doc = `enforces standards of using ginkgo and gomega + +or + ginkgolinter version + +version: %s + +currently, the linter searches for following: +* trigger a warning when using Eventually or Constantly with a function call. This is in order to prevent the case when + using a function call instead of a function. Function call returns a value only once, and so the original value + is tested again and again and is never changed. [Bug] + +* trigger a warning when comparing a pointer to a value. [Bug] + +* trigger a warning for missing assertion method: [Bug] + Eventually(checkSomething) + +* trigger a warning when a ginkgo focus container (FDescribe, FContext, FWhen or FIt) is found. [Bug] + +* validate the MatchError gomega matcher [Bug] + +* trigger a warning when using the Equal or the BeIdentical matcher with two different types, as these matchers will + fail in runtime. + +* wrong length assertions. We want to assert the item rather than its length. [Style] +For example: + Expect(len(x)).Should(Equal(1)) +This should be replaced with: + Expect(x)).Should(HavelLen(1)) + +* wrong nil assertions. We want to assert the item rather than a comparison result. [Style] +For example: + Expect(x == nil).Should(BeTrue()) +This should be replaced with: + Expect(x).Should(BeNil()) + +* wrong error assertions. For example: [Style] + Expect(err == nil).Should(BeTrue()) +This should be replaced with: + Expect(err).ShouldNot(HaveOccurred()) + +* wrong boolean comparison, for example: [Style] + Expect(x == 8).Should(BeTrue()) +This should be replaced with: + Expect(x).Should(BeEqual(8)) + +* replaces Equal(true/false) with BeTrue()/BeFalse() [Style] + +* replaces HaveLen(0) with BeEmpty() [Style] + +* replaces Expect(...).Should(...) with Expect(...).To() [stype] +` diff --git a/ginkgo_linter.go b/ginkgo_linter.go index 632ad62..1ad30ef 100644 --- a/ginkgo_linter.go +++ b/ginkgo_linter.go @@ -46,6 +46,7 @@ const ( matchErrorMissingDescription = linterName + ": missing function description as second parameter of MatchError" matchErrorRedundantArg = linterName + ": redundant MatchError arguments; consider removing them; consider using `%s` instead" matchErrorNoFuncDescription = linterName + ": The second parameter of MatchError must be the function description (string)" + forceExpectToTemplate = linterName + ": must not use Expect with %s" ) const ( // gomega matchers @@ -96,6 +97,7 @@ func NewAnalyzer() *analysis.Analyzer { SuppressCompare: false, ForbidFocus: false, AllowHaveLen0: false, + ForceExpectTo: false, }, } @@ -114,64 +116,13 @@ func NewAnalyzer() *analysis.Analyzer { a.Flags.Var(&linter.config.SuppressAsync, "suppress-async-assertion", "Suppress warning for function call in async assertion, like Eventually") a.Flags.Var(&linter.config.SuppressTypeCompare, "suppress-type-compare-assertion", "Suppress warning for comparing values from different types, like int32 and uint32") a.Flags.Var(&linter.config.AllowHaveLen0, "allow-havelen-0", "Do not warn for HaveLen(0); default = false") - + a.Flags.Var(&linter.config.ForceExpectTo, "force-expect-to", "force using `Expect` with `To`, `ToNot` or `NotTo`. reject using `Expect` with `Should` or `ShouldNot`; default = false (not forced)") a.Flags.BoolVar(&ignored, "suppress-focus-container", true, "Suppress warning for ginkgo focus containers like FDescribe, FContext, FWhen or FIt. Deprecated and ignored: use --forbid-focus-container instead") a.Flags.Var(&linter.config.ForbidFocus, "forbid-focus-container", "trigger a warning for ginkgo focus containers like FDescribe, FContext, FWhen or FIt; default = false.") return a } -const doc = `enforces standards of using ginkgo and gomega - -or - ginkgolinter version - -version: %s - -currently, the linter searches for following: -* trigger a warning when using Eventually or Constantly with a function call. This is in order to prevent the case when - using a function call instead of a function. Function call returns a value only once, and so the original value - is tested again and again and is never changed. [Bug] - -* trigger a warning when comparing a pointer to a value. [Bug] - -* trigger a warning for missing assertion method: [Bug] - Eventually(checkSomething) - -* trigger a warning when a ginkgo focus container (FDescribe, FContext, FWhen or FIt) is found. [Bug] - -* validate the MatchError gomega matcher [Bug] - -* trigger a warning when using the Equal or the BeIdentical matcher with two different types, as these matchers will - fail in runtime. - -* wrong length assertions. We want to assert the item rather than its length. [Style] -For example: - Expect(len(x)).Should(Equal(1)) -This should be replaced with: - Expect(x)).Should(HavelLen(1)) - -* wrong nil assertions. We want to assert the item rather than a comparison result. [Style] -For example: - Expect(x == nil).Should(BeTrue()) -This should be replaced with: - Expect(x).Should(BeNil()) - -* wrong error assertions. For example: [Style] - Expect(err == nil).Should(BeTrue()) -This should be replaced with: - Expect(err).ShouldNot(HaveOccurred()) - -* wrong boolean comparison, for example: [Style] - Expect(x == 8).Should(BeTrue()) -This should be replaced with: - Expect(x).Should(BeEqual(8)) - -* replaces Equal(true/false) with BeTrue()/BeFalse() [Style] - -* replaces HaveLen(0) with BeEmpty() [Style] -` - // main assertion function func (l *ginkgoLinter) run(pass *analysis.Pass) (interface{}, error) { for _, file := range pass.Files { @@ -295,14 +246,46 @@ func checkExpression(pass *analysis.Pass, config types.Config, assertionExp *ast return true } - goNested, diags := doCheckExpression(pass, config, assertionExp, actualArg, expr, handler, oldExpr) - if len(diags) > 0 { - report(pass, diags) + goNested, diagnostics := doCheckExpression(pass, config, assertionExp, actualArg, expr, handler, oldExpr) + + if config.ForceExpectTo { + if valid, diags := forceExpectTo(pass, expr, handler, oldExpr); !valid { + diagnostics = append(diagnostics, diags...) + } + } + + if len(diagnostics) > 0 { + report(pass, diagnostics) } return goNested } +func forceExpectTo(pass *analysis.Pass, expr *ast.CallExpr, handler gomegahandler.Handler, oldExpr string) (bool, []analysis.Diagnostic) { + if asrtFun, ok := expr.Fun.(*ast.SelectorExpr); ok { + if actualFuncName, ok := handler.GetActualFuncName(expr); ok && actualFuncName == expect { + var ( + name string + newIdent *ast.Ident + ) + + switch name = asrtFun.Sel.Name; name { + case "Should": + newIdent = ast.NewIdent("To") + case "ShouldNot": + newIdent = ast.NewIdent("ToNot") + default: + return true, nil + } + + handler.ReplaceFunction(expr, newIdent) + return false, prepareReport(pass, expr, fmt.Sprintf(forceExpectToTemplate, name)+"; consider using `%s` instead", oldExpr) + } + } + + return false, nil +} + func doCheckExpression(pass *analysis.Pass, config types.Config, assertionExp *ast.CallExpr, actualArg ast.Expr, expr *ast.CallExpr, handler gomegahandler.Handler, oldExpr string) (bool, []analysis.Diagnostic) { if !bool(config.SuppressLen) && isActualIsLenFunc(actualArg) { return checkLengthMatcher(expr, pass, handler, oldExpr) @@ -1359,17 +1342,18 @@ func report(pass *analysis.Pass, diagnostics []analysis.Diagnostic) { pass.Report(diagnostics[0]) default: + // use only the latest SuggestedFix, to avoid conflicts var fixes []analysis.SuggestedFix for i := range diagnostics[:len(diagnostics)-1] { if len(diagnostics[i].SuggestedFixes) > 0 { - fixes = append(fixes, diagnostics[i].SuggestedFixes...) + fixes = diagnostics[i].SuggestedFixes diagnostics[i].SuggestedFixes = nil } pass.Report(diagnostics[i]) } lastDiag := diagnostics[len(diagnostics)-1] - if len(fixes) > 0 { - lastDiag.SuggestedFixes = append(lastDiag.SuggestedFixes, fixes...) + if len(lastDiag.SuggestedFixes) == 0 && len(fixes) > 0 { + lastDiag.SuggestedFixes = fixes } pass.Report(lastDiag) } diff --git a/ginkgo_linter_test.go b/ginkgo_linter_test.go index f44fd99..bfc88b9 100644 --- a/ginkgo_linter_test.go +++ b/ginkgo_linter_test.go @@ -142,6 +142,11 @@ func TestFlags(t *testing.T) { testData: []string{"a/comparetypesconfig"}, flags: map[string]string{"suppress-type-compare-assertion": "true"}, }, + { + testName: "test the force-expect-to flag", + testData: []string{"a/forceExpectTo"}, + flags: map[string]string{"force-expect-to": "true"}, + }, } { t.Run(tc.testName, func(tt *testing.T) { analyzer := ginkgolinter.NewAnalyzer() diff --git a/gomegahandler/handler.go b/gomegahandler/handler.go index 419145b..4290e73 100644 --- a/gomegahandler/handler.go +++ b/gomegahandler/handler.go @@ -70,7 +70,12 @@ func (h dotHandler) GetActualFuncName(expr *ast.CallExpr) (string, bool) { // ReplaceFunction replaces the function with another one, for fix suggestions func (dotHandler) ReplaceFunction(caller *ast.CallExpr, newExpr *ast.Ident) { - caller.Fun = newExpr + switch f := caller.Fun.(type) { + case *ast.Ident: + caller.Fun = newExpr + case *ast.SelectorExpr: + f.Sel = newExpr + } } func (dotHandler) getDefFuncName(expr *ast.CallExpr) string { diff --git a/testdata/src/a/forceExpectTo/gomegavar.go b/testdata/src/a/forceExpectTo/gomegavar.go new file mode 100644 index 0000000..cb3828b --- /dev/null +++ b/testdata/src/a/forceExpectTo/gomegavar.go @@ -0,0 +1,22 @@ +package forceExpectTo + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("gomega var", func() { + It("in a valid Eventually", func() { + Eventually(func(g Gomega) { + g.Expect("a").Should(HaveLen(1)) // want `ginkgo-linter: must not use Expect with Should; consider using .g\.Expect\("a"\)\.To\(HaveLen\(1\)\). instead` + g.Expect(len("a")).Should(Equal(1)) // want `ginkgo-linter: wrong length assertion; consider using .g\.Expect\("a"\)\.Should\(HaveLen\(1\)\). instead` `ginkgo-linter: must not use Expect with Should; consider using .g\.Expect\("a"\)\.To\(HaveLen\(1\)\). instead` + }).Should(Succeed()) + }) + + It("in an invalid Eventually", func() { + Eventually(func(g Gomega) { // want `ginkgo-linter: "Eventually": missing assertion method\. Expected "Should\(\)" or "ShouldNot\(\)"` + g.Expect("a").Should(HaveLen(1)) // want `ginkgo-linter: must not use Expect with Should; consider using .g\.Expect\("a"\)\.To\(HaveLen\(1\)\). instead` + g.Expect(len("a")).Should(Equal(1)) // want `ginkgo-linter: wrong length assertion; consider using .g\.Expect\("a"\)\.Should\(HaveLen\(1\)\). instead` `ginkgo-linter: must not use Expect with Should; consider using .g\.Expect\("a"\)\.To\(HaveLen\(1\)\). instead` + }) + }) +}) diff --git a/testdata/src/a/forceExpectTo/nodotimport.go b/testdata/src/a/forceExpectTo/nodotimport.go new file mode 100644 index 0000000..6914a2b --- /dev/null +++ b/testdata/src/a/forceExpectTo/nodotimport.go @@ -0,0 +1,19 @@ +package forceExpectTo + +import ( + . "github.com/onsi/ginkgo/v2" + "github.com/onsi/gomega" +) + +var _ = Describe("simple case", func() { + It("just replace assertion method", func() { + gomega.Expect("a").Should(gomega.HaveLen(1)) // want `ginkgo-linter: must not use Expect with Should; consider using .gomega\.Expect\("a"\)\.To\(gomega\.HaveLen\(1\)\). instead` + gomega.Expect("a").ShouldNot(gomega.BeEmpty()) // want `ginkgo-linter: must not use Expect with ShouldNot; consider using .gomega\.Expect\("a"\)\.ToNot\(gomega\.BeEmpty\(\)\). instead` + gomega.Expect("a").Should(gomega.Not(gomega.BeEmpty())) // want `ginkgo-linter: must not use Expect with ShouldNot; consider using .gomega\.Expect\("a"\)\.ToNot\(gomega\.BeEmpty\(\)\). instead` + }) + + It("mix with len assertion", func() { + gomega.Expect(len("a")).Should(gomega.Equal(1)) // want `ginkgo-linter: wrong length assertion; consider using .gomega\.Expect\("a"\)\.Should\(gomega\.HaveLen\(1\)\). instead` `ginkgo-linter: must not use Expect with Should; consider using .gomega\.Expect\("a"\)\.To\(gomega\.HaveLen\(1\)\). instead` + gomega.Expect(len("a")).ShouldNot(gomega.Equal(0)) // want `ginkgo-linter: wrong length assertion; consider using .gomega\.Expect\("a"\)\.ShouldNot\(gomega\.BeEmpty\(\)\). instead` `ginkgo-linter: must not use Expect with ShouldNot; consider using .gomega\.Expect\("a"\)\.ToNot\(gomega\.BeEmpty\(\)\). instead` + }) +}) diff --git a/testdata/src/a/forceExpectTo/simple.go b/testdata/src/a/forceExpectTo/simple.go new file mode 100644 index 0000000..6140b11 --- /dev/null +++ b/testdata/src/a/forceExpectTo/simple.go @@ -0,0 +1,21 @@ +package forceExpectTo + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("simple case", func() { + It("just replace assertion method", func() { + Expect("a").Should(HaveLen(1)) // want `ginkgo-linter: must not use Expect with Should; consider using .Expect\("a"\)\.To\(HaveLen\(1\)\). instead` + Expect("a").ShouldNot(BeEmpty()) // want `ginkgo-linter: must not use Expect with ShouldNot; consider using .Expect\("a"\)\.ToNot\(BeEmpty\(\)\). instead` + Expect("a").Should(Not(BeEmpty())) // want `ginkgo-linter: must not use Expect with ShouldNot; consider using .Expect\("a"\)\.ToNot\(BeEmpty\(\)\). instead` + Ω("a").Should(HaveLen(1)) + Ω("a").ShouldNot(BeEmpty()) + }) + + It("mix with len assertion", func() { + Expect(len("a")).Should(Equal(1)) // want `ginkgo-linter: wrong length assertion; consider using .Expect\("a"\)\.Should\(HaveLen\(1\)\). instead` `ginkgo-linter: must not use Expect with Should; consider using .Expect\("a"\)\.To\(HaveLen\(1\)\). instead` + Expect(len("a")).ShouldNot(Equal(0)) // want `ginkgo-linter: wrong length assertion; consider using .Expect\("a"\)\.ShouldNot\(BeEmpty\(\)\). instead` `ginkgo-linter: must not use Expect with ShouldNot; consider using .Expect\("a"\)\.ToNot\(BeEmpty\(\)\). instead` + }) +}) diff --git a/tests/e2e.sh b/tests/e2e.sh index f9273ed..1a9182b 100755 --- a/tests/e2e.sh +++ b/tests/e2e.sh @@ -6,22 +6,26 @@ cp ginkgolinter testdata/src/a cd testdata/src/a # no suppress -[[ $(./ginkgolinter a/... 2>&1 | wc -l) == 2540 ]] +[[ $(./ginkgolinter a/... 2>&1 | wc -l) == 2547 ]] # suppress all but nil -[[ $(./ginkgolinter --suppress-len-assertion=true --suppress-err-assertion=true --suppress-compare-assertion=true --suppress-async-assertion=true --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 1518 ]] +[[ $(./ginkgolinter --suppress-len-assertion=true --suppress-err-assertion=true --suppress-compare-assertion=true --suppress-async-assertion=true --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 1519 ]] # suppress all but len -[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-err-assertion=true --suppress-compare-assertion=true --suppress-async-assertion=true --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 816 ]] +[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-err-assertion=true --suppress-compare-assertion=true --suppress-async-assertion=true --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 823 ]] # suppress all but err -[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-len-assertion=true --suppress-compare-assertion=true --suppress-async-assertion=true --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 278 ]] +[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-len-assertion=true --suppress-compare-assertion=true --suppress-async-assertion=true --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 279 ]] # suppress all but compare -[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-err-assertion=true --suppress-len-assertion=true --suppress-async-assertion=true --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 288 ]] +[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-err-assertion=true --suppress-len-assertion=true --suppress-async-assertion=true --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 289 ]] # suppress all but async -[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-err-assertion=true --suppress-len-assertion=true --suppress-compare-assertion=true --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 191 ]] +[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-err-assertion=true --suppress-len-assertion=true --suppress-compare-assertion=true --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 192 ]] # 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 --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 209 ]] +[[ $(./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 --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 210 ]] # suppress all but compare different types -[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-err-assertion=true --suppress-len-assertion=true --suppress-compare-assertion=true --suppress-compare-assertion=true a/... 2>&1 | wc -l) == 287 ]] +[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-err-assertion=true --suppress-len-assertion=true --suppress-compare-assertion=true --suppress-compare-assertion=true a/... 2>&1 | wc -l) == 288 ]] # allow HaveLen(0) -[[ $(./ginkgolinter --allow-havelen-0=true a/... 2>&1 | wc -l) == 2526 ]] +[[ $(./ginkgolinter --allow-havelen-0=true a/... 2>&1 | wc -l) == 2533 ]] +# force Expect with To +[[ $(./ginkgolinter --force-expect-to=true a/... 2>&1 | wc -l) == 3209 ]] # 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 --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 154 ]] +[[ $(./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 --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 155 ]] +# suppress all, force Expect with To +[[ $(./ginkgolinter --force-expect-to=true --suppress-nil-assertion=true --suppress-len-assertion=true --suppress-err-assertion=true --suppress-compare-assertion=true --suppress-async-assertion=true --forbid-focus-container=false --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 817 ]] diff --git a/types/config.go b/types/config.go index 6de2273..c6ff7b5 100644 --- a/types/config.go +++ b/types/config.go @@ -26,6 +26,7 @@ type Config struct { ForbidFocus Boolean SuppressTypeCompare Boolean AllowHaveLen0 Boolean + ForceExpectTo Boolean } func (s *Config) AllTrue() bool { @@ -42,6 +43,7 @@ func (s *Config) Clone() Config { ForbidFocus: s.ForbidFocus, SuppressTypeCompare: s.SuppressTypeCompare, AllowHaveLen0: s.AllowHaveLen0, + ForceExpectTo: s.ForceExpectTo, } }