From 8bba3e86d2cd04ac87378cca92c6d2ef7452d035 Mon Sep 17 00:00:00 2001 From: Matthieu MOREL Date: Sun, 23 Jun 2024 10:29:02 +0200 Subject: [PATCH] new checker `contains` --- README.md | 26 ++++++ analyzer/checkers_factory_test.go | 2 + .../contains/contains_test.go | 30 +++++++ .../contains/contains_test.go.golden | 30 +++++++ internal/checkers/checkers_registry.go | 1 + internal/checkers/checkers_registry_test.go | 2 + internal/checkers/contains.go | 75 ++++++++++++++++ internal/checkers/helpers_string.go | 11 +++ internal/testgen/gen_contains.go | 88 +++++++++++++++++++ internal/testgen/main.go | 1 + 10 files changed, 266 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 internal/checkers/contains.go create mode 100644 internal/checkers/helpers_string.go create mode 100644 internal/testgen/gen_contains.go diff --git a/README.md b/README.md index e622e386..45e7113c 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,31 @@ due to the inappropriate recursive nature of `assert.Equal` (based on --- +### contains + +```go +❌ +assert.True(t, strings.Contains(s, "abc123")) +assert.False(t, strings.Contains(s, "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. + +
+Related issues... + +- https://github.com/Antonboom/testifylint/issues/54 + +
+ +--- + ### empty ```go diff --git a/analyzer/checkers_factory_test.go b/analyzer/checkers_factory_test.go index 68b1573c..14134912 100644 --- a/analyzer/checkers_factory_test.go +++ b/analyzer/checkers_factory_test.go @@ -17,6 +17,7 @@ func Test_newCheckers(t *testing.T) { enabledByDefaultRegularCheckers := []checkers.RegularChecker{ checkers.NewFloatCompare(), checkers.NewBoolCompare(), + checkers.NewContains(), checkers.NewEmpty(), checkers.NewLen(), checkers.NewNegativePositive(), @@ -34,6 +35,7 @@ func Test_newCheckers(t *testing.T) { allRegularCheckers := []checkers.RegularChecker{ checkers.NewFloatCompare(), checkers.NewBoolCompare(), + checkers.NewContains(), checkers.NewEmpty(), checkers.NewLen(), checkers.NewNegativePositive(), 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..f617c4bf --- /dev/null +++ b/analyzer/testdata/src/checkers-default/contains/contains_test.go @@ -0,0 +1,30 @@ +// Code generated by testifylint/internal/testgen. DO NOT EDIT. + +package contains + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestContainsChecker(t *testing.T) { + var s = "abc123" + + // Invalid. + { + assert.False(t, strings.Contains(s, "456")) // want "contains: use assert\\.NotContains" + assert.Falsef(t, strings.Contains(s, "456"), "msg with args %d %s", 42, "42") // want "contains: use assert\\.NotContainsf" + 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" + } + + // Valid. + { + assert.Contains(t, s, "abc123") + assert.Containsf(t, s, "abc123", "msg with args %d %s", 42, "42") + assert.NotContains(t, s, "456") + assert.NotContainsf(t, s, "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..ec51a97e --- /dev/null +++ b/analyzer/testdata/src/checkers-default/contains/contains_test.go.golden @@ -0,0 +1,30 @@ +// Code generated by testifylint/internal/testgen. DO NOT EDIT. + +package contains + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestContainsChecker(t *testing.T) { + var s = "abc123" + + // Invalid. + { + assert.NotContains(t, s, "456") // want "contains: use assert\\.NotContains" + assert.NotContainsf(t, s, "456", "msg with args %d %s", 42, "42") // want "contains: use assert\\.NotContainsf" + 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" + } + + // Valid. + { + assert.Contains(t, s, "abc123") + assert.Containsf(t, s, "abc123", "msg with args %d %s", 42, "42") + assert.NotContains(t, s, "456") + assert.NotContainsf(t, s, "456", "msg with args %d %s", 42, "42") + } +} diff --git a/internal/checkers/checkers_registry.go b/internal/checkers/checkers_registry.go index d52bbe49..1cfe51d5 100644 --- a/internal/checkers/checkers_registry.go +++ b/internal/checkers/checkers_registry.go @@ -9,6 +9,7 @@ var registry = checkersRegistry{ // Regular checkers. {factory: asCheckerFactory(NewFloatCompare), enabledByDefault: true}, {factory: asCheckerFactory(NewBoolCompare), enabledByDefault: true}, + {factory: asCheckerFactory(NewContains), enabledByDefault: true}, {factory: asCheckerFactory(NewEmpty), enabledByDefault: true}, {factory: asCheckerFactory(NewLen), enabledByDefault: true}, {factory: asCheckerFactory(NewNegativePositive), enabledByDefault: true}, diff --git a/internal/checkers/checkers_registry_test.go b/internal/checkers/checkers_registry_test.go index 73c3725d..8b564e2f 100644 --- a/internal/checkers/checkers_registry_test.go +++ b/internal/checkers/checkers_registry_test.go @@ -37,6 +37,7 @@ func TestAll(t *testing.T) { expected := []string{ "float-compare", "bool-compare", + "contains", "empty", "len", "negative-positive", @@ -72,6 +73,7 @@ func TestEnabledByDefault(t *testing.T) { expected := []string{ "float-compare", "bool-compare", + "contains", "empty", "len", "negative-positive", diff --git a/internal/checkers/contains.go b/internal/checkers/contains.go new file mode 100644 index 00000000..aa34561f --- /dev/null +++ b/internal/checkers/contains.go @@ -0,0 +1,75 @@ +package checkers + +import ( + "go/ast" + + "golang.org/x/tools/go/analysis" +) + +// Contains detects situations like +// +// assert.True(t, strings.Contains(s, "abc123")) +// assert.False(t, strings.Contains(s, "456")) +// +// and requires +// +// assert.Contains(t, s, "abc123") +// assert.NotContains(t, s, "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_string.go b/internal/checkers/helpers_string.go new file mode 100644 index 00000000..4b44a6c0 --- /dev/null +++ b/internal/checkers/helpers_string.go @@ -0,0 +1,11 @@ +package checkers + +import ( + "go/ast" + + "golang.org/x/tools/go/analysis" +) + +func isStringsContainsCall(pass *analysis.Pass, ce *ast.CallExpr) bool { + return isPkgFnCall(pass, ce, "strings", "Contains") +} diff --git a/internal/testgen/gen_contains.go b/internal/testgen/gen_contains.go new file mode 100644 index 00000000..06632469 --- /dev/null +++ b/internal/testgen/gen_contains.go @@ -0,0 +1,88 @@ +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 + }{ + CheckerName: CheckerName(checker), + InvalidAssertions: []Assertion{ + { + Fn: "False", + Argsf: `strings.Contains(s, "456")`, + ReportMsgf: checker + ": use %s.%s", + ProposedFn: "NotContains", + ProposedArgsf: "s, \"456\"", + }, + { + Fn: "True", + Argsf: `strings.Contains(s, "abc123")`, + ReportMsgf: checker + ": use %s.%s", + ProposedFn: "Contains", + ProposedArgsf: "s, \"abc123\"", + }, + }, + ValidAssertions: []Assertion{ + {Fn: "Contains", Argsf: `s, "abc123"`}, + {Fn: "NotContains", Argsf: `s, "456"`}, + }, + } +} + +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 ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +func {{ .CheckerName.AsTestName }}(t *testing.T) { + var s = "abc123" + + // Invalid. + { + {{- range $ai, $assrn := $.InvalidAssertions }} + {{ NewAssertionExpander.Expand $assrn "assert" "t" nil }} + {{- end }} + } + + // Valid. + { + {{- range $ai, $assrn := $.ValidAssertions }} + {{ NewAssertionExpander.Expand $assrn "assert" "t" nil }} + {{- end }} + } +} +` diff --git a/internal/testgen/main.go b/internal/testgen/main.go index f714d020..d95b17d4 100644 --- a/internal/testgen/main.go +++ b/internal/testgen/main.go @@ -26,6 +26,7 @@ var freeTestsGenerators = map[string]TestsGenerator{ // by subdirectory. var checkerTestsGenerators = []CheckerTestsGenerator{ BlankImportTestsGenerator{}, BoolCompareTestsGenerator{}, + ContainsTestsGenerator{}, ComparesTestsGenerator{}, EmptyTestsGenerator{}, ErrorNilTestsGenerator{},