From 8afa7312516d0c308e6b352d33d2a65866d0b39a Mon Sep 17 00:00:00 2001 From: Matthieu MOREL Date: Wed, 26 Jun 2024 05:25:21 +0200 Subject: [PATCH] new checker `contains` (#152) * new checker `contains` Co-authored-by: ccoVeille <3875889+ccoVeille@users.noreply.github.com> * contains: maintainer review --------- Co-authored-by: ccoVeille <3875889+ccoVeille@users.noreply.github.com> Co-authored-by: Anton Telyshev --- README.md | 22 +++ analyzer/checkers_factory_test.go | 2 + .../contains/contains_test.go | 66 +++++++++ .../contains/contains_test.go.golden | 66 +++++++++ .../src/debug/contains_replacements_test.go | 70 +++++++++ internal/checkers/checkers_registry.go | 1 + internal/checkers/checkers_registry_test.go | 2 + internal/checkers/contains.go | 72 ++++++++++ internal/checkers/helpers_pkg_func.go | 4 + internal/testgen/gen_contains.go | 135 ++++++++++++++++++ internal/testgen/main.go | 1 + 11 files changed, 441 insertions(+) create mode 100644 analyzer/testdata/src/checkers-default/contains/contains_test.go create mode 100644 analyzer/testdata/src/checkers-default/contains/contains_test.go.golden create mode 100644 analyzer/testdata/src/debug/contains_replacements_test.go create mode 100644 internal/checkers/contains.go create mode 100644 internal/testgen/gen_contains.go diff --git a/README.md b/README.md index e622e386..7fe6314f 100644 --- a/README.md +++ b/README.md @@ -94,6 +94,7 @@ https://golangci-lint.run/usage/linters/#testifylint | [blank-import](#blank-import) | ✅ | ❌ | | [bool-compare](#bool-compare) | ✅ | ✅ | | [compares](#compares) | ✅ | ✅ | +| [contains](#contains) | ✅ | ✅ | | [empty](#empty) | ✅ | ✅ | | [error-is-as](#error-is-as) | ✅ | 🤏 | | [error-nil](#error-nil) | ✅ | ✅ | @@ -216,6 +217,27 @@ due to the inappropriate recursive nature of `assert.Equal` (based on --- +### contains + +```go +❌ +assert.True(t, strings.Contains(a, "abc123")) +assert.False(t, !strings.Contains(a, "abc123")) + +assert.False(t, strings.Contains(a, "abc123")) +assert.True(t, !strings.Contains(a, "abc123")) + +✅ +assert.Contains(t, a, "abc123") +assert.NotContains(t, a, "abc123") +``` + +**Autofix**: true.
+**Enabled by default**: true.
+**Reason**: Code simplification and more appropriate `testify` API with clearer failure message. + +--- + ### empty ```go diff --git a/analyzer/checkers_factory_test.go b/analyzer/checkers_factory_test.go index 68b1573c..6533494a 100644 --- a/analyzer/checkers_factory_test.go +++ b/analyzer/checkers_factory_test.go @@ -21,6 +21,7 @@ func Test_newCheckers(t *testing.T) { checkers.NewLen(), checkers.NewNegativePositive(), checkers.NewCompares(), + checkers.NewContains(), checkers.NewErrorNil(), checkers.NewNilCompare(), checkers.NewErrorIsAs(), @@ -38,6 +39,7 @@ func Test_newCheckers(t *testing.T) { checkers.NewLen(), checkers.NewNegativePositive(), checkers.NewCompares(), + checkers.NewContains(), checkers.NewErrorNil(), checkers.NewNilCompare(), checkers.NewErrorIsAs(), diff --git a/analyzer/testdata/src/checkers-default/contains/contains_test.go b/analyzer/testdata/src/checkers-default/contains/contains_test.go new file mode 100644 index 00000000..a5cc4823 --- /dev/null +++ b/analyzer/testdata/src/checkers-default/contains/contains_test.go @@ -0,0 +1,66 @@ +// Code generated by testifylint/internal/testgen. DO NOT EDIT. + +package contains + +import ( + "bytes" + "errors" + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestContainsChecker(t *testing.T) { + var ( + s = "abc123" + b = []byte(s) + errSentinel = errors.New("user not found") + ) + + // Invalid. + { + assert.True(t, strings.Contains(s, "abc123")) // want "contains: use assert\\.Contains" + assert.Truef(t, strings.Contains(s, "abc123"), "msg with args %d %s", 42, "42") // want "contains: use assert\\.Containsf" + assert.True(t, strings.Contains(string(b), "abc123")) // want "contains: use assert\\.Contains" + assert.Truef(t, strings.Contains(string(b), "abc123"), "msg with args %d %s", 42, "42") // want "contains: use assert\\.Containsf" + assert.False(t, !strings.Contains(s, "abc123")) // want "contains: use assert\\.Contains" + assert.Falsef(t, !strings.Contains(s, "abc123"), "msg with args %d %s", 42, "42") // want "contains: use assert\\.Containsf" + assert.False(t, !strings.Contains(string(b), "abc123")) // want "contains: use assert\\.Contains" + assert.Falsef(t, !strings.Contains(string(b), "abc123"), "msg with args %d %s", 42, "42") // want "contains: use assert\\.Containsf" + assert.False(t, strings.Contains(s, "abc123")) // want "contains: use assert\\.NotContains" + assert.Falsef(t, strings.Contains(s, "abc123"), "msg with args %d %s", 42, "42") // want "contains: use assert\\.NotContainsf" + assert.False(t, strings.Contains(string(b), "abc123")) // want "contains: use assert\\.NotContains" + assert.Falsef(t, strings.Contains(string(b), "abc123"), "msg with args %d %s", 42, "42") // want "contains: use assert\\.NotContainsf" + assert.True(t, !strings.Contains(s, "abc123")) // want "contains: use assert\\.NotContains" + assert.Truef(t, !strings.Contains(s, "abc123"), "msg with args %d %s", 42, "42") // want "contains: use assert\\.NotContainsf" + assert.True(t, !strings.Contains(string(b), "abc123")) // want "contains: use assert\\.NotContains" + assert.Truef(t, !strings.Contains(string(b), "abc123"), "msg with args %d %s", 42, "42") // want "contains: use assert\\.NotContainsf" + } + + // Valid. + { + assert.Contains(t, s, "abc123") + assert.Containsf(t, s, "abc123", "msg with args %d %s", 42, "42") + assert.Contains(t, string(b), "abc123") + assert.Containsf(t, string(b), "abc123", "msg with args %d %s", 42, "42") + assert.NotContains(t, s, "abc123") + assert.NotContainsf(t, s, "abc123", "msg with args %d %s", 42, "42") + assert.NotContains(t, string(b), "abc123") + assert.NotContainsf(t, string(b), "abc123", "msg with args %d %s", 42, "42") + } + + // Ignored. + { + assert.Contains(t, errSentinel.Error(), "user") + assert.Containsf(t, errSentinel.Error(), "user", "msg with args %d %s", 42, "42") + assert.True(t, bytes.Contains(b, []byte("a"))) + assert.Truef(t, bytes.Contains(b, []byte("a")), "msg with args %d %s", 42, "42") + assert.False(t, !bytes.Contains(b, []byte("a"))) + assert.Falsef(t, !bytes.Contains(b, []byte("a")), "msg with args %d %s", 42, "42") + assert.False(t, bytes.Contains(b, []byte("a"))) + assert.Falsef(t, bytes.Contains(b, []byte("a")), "msg with args %d %s", 42, "42") + assert.True(t, !bytes.Contains(b, []byte("a"))) + assert.Truef(t, !bytes.Contains(b, []byte("a")), "msg with args %d %s", 42, "42") + } +} diff --git a/analyzer/testdata/src/checkers-default/contains/contains_test.go.golden b/analyzer/testdata/src/checkers-default/contains/contains_test.go.golden new file mode 100644 index 00000000..d56c60f8 --- /dev/null +++ b/analyzer/testdata/src/checkers-default/contains/contains_test.go.golden @@ -0,0 +1,66 @@ +// Code generated by testifylint/internal/testgen. DO NOT EDIT. + +package contains + +import ( + "bytes" + "errors" + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestContainsChecker(t *testing.T) { + var ( + s = "abc123" + b = []byte(s) + errSentinel = errors.New("user not found") + ) + + // Invalid. + { + assert.Contains(t, s, "abc123") // want "contains: use assert\\.Contains" + assert.Containsf(t, s, "abc123", "msg with args %d %s", 42, "42") // want "contains: use assert\\.Containsf" + assert.Contains(t, string(b), "abc123") // want "contains: use assert\\.Contains" + assert.Containsf(t, string(b), "abc123", "msg with args %d %s", 42, "42") // want "contains: use assert\\.Containsf" + assert.Contains(t, s, "abc123") // want "contains: use assert\\.Contains" + assert.Containsf(t, s, "abc123", "msg with args %d %s", 42, "42") // want "contains: use assert\\.Containsf" + assert.Contains(t, string(b), "abc123") // want "contains: use assert\\.Contains" + assert.Containsf(t, string(b), "abc123", "msg with args %d %s", 42, "42") // want "contains: use assert\\.Containsf" + assert.NotContains(t, s, "abc123") // want "contains: use assert\\.NotContains" + assert.NotContainsf(t, s, "abc123", "msg with args %d %s", 42, "42") // want "contains: use assert\\.NotContainsf" + assert.NotContains(t, string(b), "abc123") // want "contains: use assert\\.NotContains" + assert.NotContainsf(t, string(b), "abc123", "msg with args %d %s", 42, "42") // want "contains: use assert\\.NotContainsf" + assert.NotContains(t, s, "abc123") // want "contains: use assert\\.NotContains" + assert.NotContainsf(t, s, "abc123", "msg with args %d %s", 42, "42") // want "contains: use assert\\.NotContainsf" + assert.NotContains(t, string(b), "abc123") // want "contains: use assert\\.NotContains" + assert.NotContainsf(t, string(b), "abc123", "msg with args %d %s", 42, "42") // want "contains: use assert\\.NotContainsf" + } + + // Valid. + { + assert.Contains(t, s, "abc123") + assert.Containsf(t, s, "abc123", "msg with args %d %s", 42, "42") + assert.Contains(t, string(b), "abc123") + assert.Containsf(t, string(b), "abc123", "msg with args %d %s", 42, "42") + assert.NotContains(t, s, "abc123") + assert.NotContainsf(t, s, "abc123", "msg with args %d %s", 42, "42") + assert.NotContains(t, string(b), "abc123") + assert.NotContainsf(t, string(b), "abc123", "msg with args %d %s", 42, "42") + } + + // Ignored. + { + assert.Contains(t, errSentinel.Error(), "user") + assert.Containsf(t, errSentinel.Error(), "user", "msg with args %d %s", 42, "42") + assert.True(t, bytes.Contains(b, []byte("a"))) + assert.Truef(t, bytes.Contains(b, []byte("a")), "msg with args %d %s", 42, "42") + assert.False(t, !bytes.Contains(b, []byte("a"))) + assert.Falsef(t, !bytes.Contains(b, []byte("a")), "msg with args %d %s", 42, "42") + assert.False(t, bytes.Contains(b, []byte("a"))) + assert.Falsef(t, bytes.Contains(b, []byte("a")), "msg with args %d %s", 42, "42") + assert.True(t, !bytes.Contains(b, []byte("a"))) + assert.Truef(t, !bytes.Contains(b, []byte("a")), "msg with args %d %s", 42, "42") + } +} diff --git a/analyzer/testdata/src/debug/contains_replacements_test.go b/analyzer/testdata/src/debug/contains_replacements_test.go new file mode 100644 index 00000000..ecea217c --- /dev/null +++ b/analyzer/testdata/src/debug/contains_replacements_test.go @@ -0,0 +1,70 @@ +package debug + +import ( + "bytes" + "fmt" + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestContainsNotContainsReplacements_String(t *testing.T) { + var tm tMock + + const substr = "abc123" + + for _, s := range []string{"", "random", substr + substr} { + t.Run(s, func(t *testing.T) { + assert.Equal(t, + assert.True(tm, strings.Contains(s, substr)), + assert.Contains(tm, s, substr), + ) + + assert.Equal(t, + assert.False(tm, !strings.Contains(s, substr)), + assert.Contains(tm, s, substr), + ) + + assert.Equal(t, + assert.False(tm, strings.Contains(s, substr)), + assert.NotContains(tm, s, substr), + ) + + assert.Equal(t, + assert.True(tm, !strings.Contains(s, substr)), + assert.NotContains(tm, s, substr), + ) + }) + } +} + +func TestContainsNotContainsReplacements_Bytes(t *testing.T) { + var tm tMock + + subbytes := []byte("abc123") + + for _, s := range [][]byte{nil, []byte("random"), append(subbytes, subbytes...)} { + t.Run(fmt.Sprintf("[]byte(%s)", s), func(t *testing.T) { + assert.Equal(t, + assert.True(tm, bytes.Contains(s, subbytes)), + assert.Contains(tm, s, subbytes), + ) + + assert.Equal(t, + assert.False(tm, !bytes.Contains(s, subbytes)), + assert.Contains(tm, s, subbytes), + ) + + assert.Equal(t, + assert.False(tm, bytes.Contains(s, subbytes)), + assert.NotContains(tm, s, subbytes), + ) + + assert.Equal(t, + assert.True(tm, !bytes.Contains(s, subbytes)), + assert.NotContains(tm, s, subbytes), + ) + }) + } +} diff --git a/internal/checkers/checkers_registry.go b/internal/checkers/checkers_registry.go index d52bbe49..993c42ff 100644 --- a/internal/checkers/checkers_registry.go +++ b/internal/checkers/checkers_registry.go @@ -13,6 +13,7 @@ var registry = checkersRegistry{ {factory: asCheckerFactory(NewLen), enabledByDefault: true}, {factory: asCheckerFactory(NewNegativePositive), enabledByDefault: true}, {factory: asCheckerFactory(NewCompares), enabledByDefault: true}, + {factory: asCheckerFactory(NewContains), enabledByDefault: true}, {factory: asCheckerFactory(NewErrorNil), enabledByDefault: true}, {factory: asCheckerFactory(NewNilCompare), enabledByDefault: true}, {factory: asCheckerFactory(NewErrorIsAs), enabledByDefault: true}, diff --git a/internal/checkers/checkers_registry_test.go b/internal/checkers/checkers_registry_test.go index 73c3725d..ecdbefdb 100644 --- a/internal/checkers/checkers_registry_test.go +++ b/internal/checkers/checkers_registry_test.go @@ -41,6 +41,7 @@ func TestAll(t *testing.T) { "len", "negative-positive", "compares", + "contains", "error-nil", "nil-compare", "error-is-as", @@ -76,6 +77,7 @@ func TestEnabledByDefault(t *testing.T) { "len", "negative-positive", "compares", + "contains", "error-nil", "nil-compare", "error-is-as", diff --git a/internal/checkers/contains.go b/internal/checkers/contains.go new file mode 100644 index 00000000..afbb0d26 --- /dev/null +++ b/internal/checkers/contains.go @@ -0,0 +1,72 @@ +package checkers + +import ( + "go/ast" + + "golang.org/x/tools/go/analysis" +) + +// Contains detects situations like +// +// assert.True(t, strings.Contains(a, "abc123")) +// assert.False(t, !strings.Contains(a, "abc123")) +// +// assert.False(t, strings.Contains(a, "abc123")) +// assert.True(t, !strings.Contains(a, "abc123")) +// +// and requires +// +// assert.Contains(t, a, "abc123") +// assert.NotContains(t, a, "abc123") +type Contains struct{} + +// NewContains constructs Contains checker. +func NewContains() Contains { return Contains{} } +func (Contains) Name() string { return "contains" } + +func (checker Contains) Check(pass *analysis.Pass, call *CallMeta) *analysis.Diagnostic { + if len(call.Args) < 1 { + return nil + } + + expr := call.Args[0] + unpacked, isNeg := isNegation(expr) + if isNeg { + expr = unpacked + } + + ce, ok := expr.(*ast.CallExpr) + if !ok || len(ce.Args) != 2 { + return nil + } + + if !isStringsContainsCall(pass, ce) { + return nil + } + + var proposed string + switch call.Fn.NameFTrimmed { + default: + return nil + + case "True": + proposed = "Contains" + if isNeg { + proposed = "NotContains" + } + + case "False": + proposed = "NotContains" + if isNeg { + proposed = "Contains" + } + } + + return newUseFunctionDiagnostic(checker.Name(), call, proposed, + newSuggestedFuncReplacement(call, proposed, analysis.TextEdit{ + Pos: call.Args[0].Pos(), + End: call.Args[0].End(), + NewText: formatAsCallArgs(pass, ce.Args[0], ce.Args[1]), + }), + ) +} diff --git a/internal/checkers/helpers_pkg_func.go b/internal/checkers/helpers_pkg_func.go index 13668028..a81cbc65 100644 --- a/internal/checkers/helpers_pkg_func.go +++ b/internal/checkers/helpers_pkg_func.go @@ -12,6 +12,10 @@ func isRegexpMustCompileCall(pass *analysis.Pass, ce *ast.CallExpr) bool { return isPkgFnCall(pass, ce, "regexp", "MustCompile") } +func isStringsContainsCall(pass *analysis.Pass, ce *ast.CallExpr) bool { + return isPkgFnCall(pass, ce, "strings", "Contains") +} + func isPkgFnCall(pass *analysis.Pass, ce *ast.CallExpr, pkg, fn string) bool { se, ok := ce.Fun.(*ast.SelectorExpr) if !ok { diff --git a/internal/testgen/gen_contains.go b/internal/testgen/gen_contains.go new file mode 100644 index 00000000..4afb0948 --- /dev/null +++ b/internal/testgen/gen_contains.go @@ -0,0 +1,135 @@ +package main + +import ( + "strings" + "text/template" + + "github.com/Antonboom/testifylint/internal/checkers" +) + +type ContainsTestsGenerator struct{} + +func (ContainsTestsGenerator) Checker() checkers.Checker { + return checkers.NewContains() +} + +func (g ContainsTestsGenerator) TemplateData() any { + var ( + checker = g.Checker().Name() + report = checker + ": use %s.%s" + ) + + return struct { + CheckerName CheckerName + Vars []string + InvalidAssertions []Assertion + ValidAssertions []Assertion + IgnoredAssertions []Assertion + }{ + CheckerName: CheckerName(checker), + Vars: []string{"s", "string(b)"}, + InvalidAssertions: []Assertion{ + { + Fn: "True", + Argsf: `strings.Contains(%s, "abc123")`, + ReportMsgf: report, + ProposedFn: "Contains", + ProposedArgsf: `%s, "abc123"`, + }, + { + Fn: "False", + Argsf: `!strings.Contains(%s, "abc123")`, + ReportMsgf: report, + ProposedFn: "Contains", + ProposedArgsf: `%s, "abc123"`, + }, + + { + Fn: "False", + Argsf: `strings.Contains(%s, "abc123")`, + ReportMsgf: report, + ProposedFn: "NotContains", + ProposedArgsf: `%s, "abc123"`, + }, + { + Fn: "True", + Argsf: `!strings.Contains(%s, "abc123")`, + ReportMsgf: report, + ProposedFn: "NotContains", + ProposedArgsf: `%s, "abc123"`, + }, + }, + ValidAssertions: []Assertion{ + {Fn: "Contains", Argsf: `%s, "abc123"`}, + {Fn: "NotContains", Argsf: `%s, "abc123"`}, + }, + IgnoredAssertions: []Assertion{ + {Fn: "Contains", Argsf: `errSentinel.Error(), "user"`}, // error-compare case. + + // https://github.com/Antonboom/testifylint/issues/154 + {Fn: "True", Argsf: `bytes.Contains(b, []byte("a"))`}, + {Fn: "False", Argsf: `!bytes.Contains(b, []byte("a"))`}, + {Fn: "False", Argsf: `bytes.Contains(b, []byte("a"))`}, + {Fn: "True", Argsf: `!bytes.Contains(b, []byte("a"))`}, + }, + } +} + +func (ContainsTestsGenerator) ErroredTemplate() Executor { + return template.Must(template.New("ContainsTestsGenerator.ErroredTemplate"). + Funcs(fm). + Parse(containsTestTmpl)) +} + +func (ContainsTestsGenerator) GoldenTemplate() Executor { + return template.Must(template.New("ContainsTestsGenerator.GoldenTemplate"). + Funcs(fm). + Parse(strings.ReplaceAll(containsTestTmpl, "NewAssertionExpander", "NewAssertionExpander.AsGolden"))) +} + +const containsTestTmpl = header + ` + +package {{ .CheckerName.AsPkgName }} + +import ( + "bytes" + "errors" + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +func {{ .CheckerName.AsTestName }}(t *testing.T) { + var ( + s = "abc123" + b = []byte(s) + errSentinel = errors.New("user not found") + ) + + // Invalid. + { + {{- range $ai, $assrn := $.InvalidAssertions }} + {{- range $vi, $var := $.Vars }} + {{ NewAssertionExpander.Expand $assrn "assert" "t" (arr $var) }} + {{- end }} + {{- end }} + } + + // Valid. + { + {{- range $ai, $assrn := $.ValidAssertions }} + {{- range $vi, $var := $.Vars }} + {{ NewAssertionExpander.Expand $assrn "assert" "t" (arr $var) }} + {{- end }} + {{- end }} + } + + // Ignored. + { + {{- range $ai, $assrn := $.IgnoredAssertions }} + {{ NewAssertionExpander.Expand $assrn "assert" "t" nil }} + {{- end }} + } +} +` diff --git a/internal/testgen/main.go b/internal/testgen/main.go index f714d020..c3984b6c 100644 --- a/internal/testgen/main.go +++ b/internal/testgen/main.go @@ -27,6 +27,7 @@ var checkerTestsGenerators = []CheckerTestsGenerator{ BlankImportTestsGenerator{}, BoolCompareTestsGenerator{}, ComparesTestsGenerator{}, + ContainsTestsGenerator{}, EmptyTestsGenerator{}, ErrorNilTestsGenerator{}, ErrorIsAsTestsGenerator{},