diff --git a/README.md b/README.md index e622e386..3414b79c 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,26 @@ 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, "456")) +assert.True(t, strings.Contains(string(b), "abc123")) +assert.False(t, strings.Contains(string(b), "456")) + +✅ +assert.Contains(t, s, "abc123") +assert.NotContains(t, s, "456") +``` + +**Autofix**: true.
+**Enabled by default**: true.
+**Reason**: 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..f20f6333 --- /dev/null +++ b/analyzer/testdata/src/checkers-default/contains/contains_test.go @@ -0,0 +1,55 @@ +// Code generated by testifylint/internal/testgen. DO NOT EDIT. + +package contains + +import ( + "errors" + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestContainsChecker(t *testing.T) { + var ( + a = "abc123" + b = []byte(a) + errSentinel = errors.New("user not found") + ) + + // Invalid. + { + assert.True(t, strings.Contains(a, "abc123")) // want "contains: use assert\\.Contains" + assert.Truef(t, strings.Contains(a, "abc123"), "msg with args %d %s", 42, "42") // want "contains: use assert\\.Containsf" + assert.False(t, strings.Contains(a, "456")) // want "contains: use assert\\.NotContains" + assert.Falsef(t, strings.Contains(a, "456"), "msg with args %d %s", 42, "42") // want "contains: use assert\\.NotContainsf" + 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(string(b), "456")) // want "contains: use assert\\.NotContains" + assert.Falsef(t, strings.Contains(string(b), "456"), "msg with args %d %s", 42, "42") // want "contains: use assert\\.NotContainsf" + } + + // Valid. + { + assert.Contains(t, a, "abc123") + assert.Containsf(t, a, "abc123", "msg with args %d %s", 42, "42") + assert.NotContains(t, a, "456") + assert.NotContainsf(t, a, "456", "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, string(b), "456") + assert.NotContainsf(t, string(b), "456", "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.Equal(t, strings.Contains(a, "abc123"), true) + assert.Equalf(t, strings.Contains(a, "abc123"), true, "msg with args %d %s", 42, "42") + assert.False(t, !strings.Contains(a, "abc123")) + assert.Falsef(t, !strings.Contains(a, "abc123"), "msg with args %d %s", 42, "42") + assert.True(t, !strings.Contains(a, "456")) + assert.Truef(t, !strings.Contains(a, "456"), "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..d07f2556 --- /dev/null +++ b/analyzer/testdata/src/checkers-default/contains/contains_test.go.golden @@ -0,0 +1,55 @@ +// Code generated by testifylint/internal/testgen. DO NOT EDIT. + +package contains + +import ( + "errors" + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestContainsChecker(t *testing.T) { + var ( + a = "abc123" + b = []byte(a) + errSentinel = errors.New("user not found") + ) + + // Invalid. + { + assert.Contains(t, a, "abc123") // want "contains: use assert\\.Contains" + assert.Containsf(t, a, "abc123", "msg with args %d %s", 42, "42") // want "contains: use assert\\.Containsf" + assert.NotContains(t, a, "456") // want "contains: use assert\\.NotContains" + assert.NotContainsf(t, a, "456", "msg with args %d %s", 42, "42") // want "contains: use assert\\.NotContainsf" + 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, string(b), "456") // want "contains: use assert\\.NotContains" + assert.NotContainsf(t, string(b), "456", "msg with args %d %s", 42, "42") // want "contains: use assert\\.NotContainsf" + } + + // Valid. + { + assert.Contains(t, a, "abc123") + assert.Containsf(t, a, "abc123", "msg with args %d %s", 42, "42") + assert.NotContains(t, a, "456") + assert.NotContainsf(t, a, "456", "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, string(b), "456") + assert.NotContainsf(t, string(b), "456", "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.Equal(t, strings.Contains(a, "abc123"), true) + assert.Equalf(t, strings.Contains(a, "abc123"), true, "msg with args %d %s", 42, "42") + assert.False(t, !strings.Contains(a, "abc123")) + assert.Falsef(t, !strings.Contains(a, "abc123"), "msg with args %d %s", 42, "42") + assert.True(t, !strings.Contains(a, "456")) + assert.Truef(t, !strings.Contains(a, "456"), "msg with args %d %s", 42, "42") + } +} 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..120e58d9 --- /dev/null +++ b/internal/checkers/contains.go @@ -0,0 +1,79 @@ +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, "456")) +// assert.True(t, strings.Contains(string(b), "abc123")) +// assert.False(t, strings.Contains(string(b), "456")) +// +// and requires +// +// assert.Contains(t, a, "abc123") +// assert.NotContains(t, a, "456") +// assert.Contains(t, string(b), "abc123") +// assert.NotContains(t, string(b), "456") +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 { + switch call.Fn.NameFTrimmed { + case "True": + if len(call.Args) < 1 { + return nil + } + + ce, ok := call.Args[0].(*ast.CallExpr) + if !ok { + return nil + } + if len(ce.Args) != 2 { + return nil + } + + if isStringsContainsCall(pass, ce) { + const proposed = "Contains" + return newUseFunctionDiagnostic(checker.Name(), call, proposed, + newSuggestedFuncReplacement(call, proposed, analysis.TextEdit{ + Pos: ce.Pos(), + End: ce.End(), + NewText: formatAsCallArgs(pass, ce.Args[0], ce.Args[1]), + }), + ) + } + + case "False": + if len(call.Args) < 1 { + return nil + } + + ce, ok := call.Args[0].(*ast.CallExpr) + if !ok { + return nil + } + if len(ce.Args) != 2 { + return nil + } + + if isStringsContainsCall(pass, ce) { + const proposed = "NotContains" + return newUseFunctionDiagnostic(checker.Name(), call, proposed, + newSuggestedFuncReplacement(call, proposed, analysis.TextEdit{ + Pos: ce.Pos(), + End: ce.End(), + NewText: formatAsCallArgs(pass, ce.Args[0], ce.Args[1]), + }), + ) + } + } + return nil +} 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..45e8f409 --- /dev/null +++ b/internal/testgen/gen_contains.go @@ -0,0 +1,122 @@ +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 { + checker := g.Checker().Name() + + return struct { + CheckerName CheckerName + InvalidAssertions []Assertion + ValidAssertions []Assertion + IgnoredAssertions []Assertion + }{ + CheckerName: CheckerName(checker), + InvalidAssertions: []Assertion{ + { + Fn: "True", + Argsf: `strings.Contains(a, "abc123")`, + ReportMsgf: checker + ": use %s.%s", + ProposedFn: "Contains", + ProposedArgsf: `a, "abc123"`, + }, { + Fn: "False", + Argsf: `strings.Contains(a, "456")`, + ReportMsgf: checker + ": use %s.%s", + ProposedFn: "NotContains", + ProposedArgsf: `a, "456"`, + }, + + { + Fn: "True", + Argsf: `strings.Contains(string(b), "abc123")`, + ReportMsgf: checker + ": use %s.%s", + ProposedFn: "Contains", + ProposedArgsf: `string(b), "abc123"`, + }, { + Fn: "False", + Argsf: `strings.Contains(string(b), "456")`, + ReportMsgf: checker + ": use %s.%s", + ProposedFn: "NotContains", + ProposedArgsf: `string(b), "456"`, + }, + }, + ValidAssertions: []Assertion{ + {Fn: "Contains", Argsf: `a, "abc123"`}, + {Fn: "NotContains", Argsf: `a, "456"`}, + {Fn: "Contains", Argsf: `string(b), "abc123"`}, + {Fn: "NotContains", Argsf: `string(b), "456"`}, + }, + IgnoredAssertions: []Assertion{ + {Fn: "Contains", Argsf: `errSentinel.Error(), "user"`}, // Requested by https://github.com/Antonboom/testifylint/issues/47 + {Fn: "Equal", Argsf: `strings.Contains(a, "abc123"), true`}, // Handled by bool-compare + {Fn: "False", Argsf: `!strings.Contains(a, "abc123")`}, // Handled by bool-compare + {Fn: "True", Argsf: `!strings.Contains(a, "456")`}, // Handled by bool-compare + }, + } +} + +func (ContainsTestsGenerator) ErroredTemplate() Executor { + return template.Must(template.New("ContainsTestsGenerator.ErroredTemplate"). + Funcs(fm). + Parse(constainsTestTmpl)) +} + +func (ContainsTestsGenerator) GoldenTemplate() Executor { + return template.Must(template.New("ContainsTestsGenerator.GoldenTemplate"). + Funcs(fm). + Parse(strings.ReplaceAll(constainsTestTmpl, "NewAssertionExpander", "NewAssertionExpander.AsGolden"))) +} + +const constainsTestTmpl = header + ` + +package {{ .CheckerName.AsPkgName }} + +import ( + "errors" + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +func {{ .CheckerName.AsTestName }}(t *testing.T) { + var ( + a = "abc123" + b = []byte(a) + errSentinel = errors.New("user not found") + ) + + // Invalid. + { + {{- range $ai, $assrn := $.InvalidAssertions }} + {{ NewAssertionExpander.Expand $assrn "assert" "t" nil }} + {{- end }} + } + + // Valid. + { + {{- range $ai, $assrn := $.ValidAssertions }} + {{ NewAssertionExpander.Expand $assrn "assert" "t" nil }} + {{- 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{},