From 8d58ad359476c7140f58d3ef8ee6ec07812718cf Mon Sep 17 00:00:00 2001 From: Matthieu MOREL Date: Fri, 29 May 2026 11:13:40 +0200 Subject: [PATCH 1/3] Add `redundant-assert` checker for implied `assert.Error` assertions Signed-off-by: Matthieu MOREL --- README.md | 30 ++++ analyzer/checkers_factory_test.go | 2 + .../redundant-assert/redundant_assert_test.go | 64 ++++++++ .../redundant_assert_test.go.golden | 60 ++++++++ internal/checkers/checkers_registry.go | 1 + internal/checkers/checkers_registry_test.go | 2 + internal/checkers/redundant_assert.go | 143 ++++++++++++++++++ 7 files changed, 302 insertions(+) create mode 100644 analyzer/testdata/src/checkers-default/redundant-assert/redundant_assert_test.go create mode 100644 analyzer/testdata/src/checkers-default/redundant-assert/redundant_assert_test.go.golden create mode 100644 internal/checkers/redundant_assert.go diff --git a/README.md b/README.md index 74009f0b..b7101fc1 100644 --- a/README.md +++ b/README.md @@ -107,6 +107,7 @@ https://golangci-lint.run/docs/linters/configuration/#testifylint | [negative-positive](#negative-positive) | ✅ | ✅ | | [nil-compare](#nil-compare) | ✅ | ✅ | | [regexp](#regexp) | ✅ | ✅ | +| [redundant-assert](#redundant-assert) | ✅ | ✅ | | [require-error](#require-error) | ✅ | ❌ | | [suite-broken-parallel](#suite-broken-parallel) | ✅ | ✅ | | [suite-dont-use-pkg](#suite-dont-use-pkg) | ✅ | ✅ | @@ -997,6 +998,35 @@ assert.NotRegexp(t, `\[.*\] TRACE message`, out) --- +### redundant-assert + +```go +❌ +assert.Error(t, err) +assert.ErrorContains(t, err, "not found") + +assert.Error(t, err) +assert.ErrorIs(t, err, io.EOF) + +assert.Error(t, err) +assert.ErrorAs(t, err, &target) + +assert.Error(t, err) +assert.EqualError(t, err, "end of file") + +✅ +assert.ErrorContains(t, err, "not found") +assert.ErrorIs(t, err, io.EOF) +assert.ErrorAs(t, err, &target) +assert.EqualError(t, err, "end of file") +``` + +**Autofix**: true.
+**Enabled by default**: true.
+**Reason**: Code simplification. + +--- + ### require-error ```go diff --git a/analyzer/checkers_factory_test.go b/analyzer/checkers_factory_test.go index ef45c4c9..f9d29341 100644 --- a/analyzer/checkers_factory_test.go +++ b/analyzer/checkers_factory_test.go @@ -61,6 +61,7 @@ func Test_newCheckers(t *testing.T) { } enabledByDefaultAdvancedCheckers := []checkers.AdvancedChecker{ + checkers.NewRedundantAssert(), checkers.NewBlankImport(), checkers.NewGoRequire(), checkers.NewMockExpect(), @@ -70,6 +71,7 @@ func Test_newCheckers(t *testing.T) { checkers.NewSuiteSubtestRun(), } allAdvancedCheckers := []checkers.AdvancedChecker{ + checkers.NewRedundantAssert(), checkers.NewBlankImport(), checkers.NewGoRequire(), checkers.NewMockExpect(), diff --git a/analyzer/testdata/src/checkers-default/redundant-assert/redundant_assert_test.go b/analyzer/testdata/src/checkers-default/redundant-assert/redundant_assert_test.go new file mode 100644 index 00000000..a794fa4c --- /dev/null +++ b/analyzer/testdata/src/checkers-default/redundant-assert/redundant_assert_test.go @@ -0,0 +1,64 @@ +package redundantassert + +import ( + "errors" + "io" + "os" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRedundantAssert(t *testing.T) { + t.Parallel() + + var ( + errA = errors.New("err A") + errB = errors.New("err B") + target *os.PathError + ) + + assObj := assert.New(t) + + // Invalid. + { + assert.Error(t, errA) // want "redundant-assert: remove assert\\.Error: assert\\.ErrorContains already asserts that the error is not nil" + assert.ErrorContains(t, errA, "err") + + assert.EqualError(t, errA, "err A") + assert.Error(t, errA) // want "redundant-assert: remove assert\\.Error: assert\\.EqualError already asserts that the error is not nil" + + assObj.Error(errA) // want "redundant-assert: remove assert\\.Error: assObj\\.ErrorAs already asserts that the error is not nil" + assObj.ErrorAs(errA, &target) + + assert.Errorf(t, errA, "msg") // want "redundant-assert: remove assert\\.Errorf: assObj\\.ErrorAs already asserts that the error is not nil" + require.ErrorIs(t, errA, io.EOF) + } + + // Valid. + { + { + assert.Error(t, errA) + assert.ErrorContains(t, errB, "err") + } + + { + assert.Error(t, errA) + } + + if assert.Error(t, errA) { + assert.ErrorContains(t, errA, "err") + } + + { + { + assert.Error(t, errA) + } + assert.ErrorContains(t, errA, "err") + } + + require.Error(t, errA) + require.ErrorContains(t, errA, "err") + } +} diff --git a/analyzer/testdata/src/checkers-default/redundant-assert/redundant_assert_test.go.golden b/analyzer/testdata/src/checkers-default/redundant-assert/redundant_assert_test.go.golden new file mode 100644 index 00000000..482d5f79 --- /dev/null +++ b/analyzer/testdata/src/checkers-default/redundant-assert/redundant_assert_test.go.golden @@ -0,0 +1,60 @@ +package redundantassert + +import ( + "errors" + "io" + "os" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRedundantAssert(t *testing.T) { + t.Parallel() + + var ( + errA = errors.New("err A") + errB = errors.New("err B") + target *os.PathError + ) + + assObj := assert.New(t) + + // Invalid. + { + assert.ErrorContains(t, errA, "err") + + assert.EqualError(t, errA, "err A") + + assObj.ErrorAs(errA, &target) + + require.ErrorIs(t, errA, io.EOF) + } + + // Valid. + { + { + assert.Error(t, errA) + assert.ErrorContains(t, errB, "err") + } + + { + assert.Error(t, errA) + } + + if assert.Error(t, errA) { + assert.ErrorContains(t, errA, "err") + } + + { + { + assert.Error(t, errA) + } + assert.ErrorContains(t, errA, "err") + } + + require.Error(t, errA) + require.ErrorContains(t, errA, "err") + } +} diff --git a/internal/checkers/checkers_registry.go b/internal/checkers/checkers_registry.go index f942754d..eaaaa00d 100644 --- a/internal/checkers/checkers_registry.go +++ b/internal/checkers/checkers_registry.go @@ -26,6 +26,7 @@ var registry = checkersRegistry{ {factory: asCheckerFactory(NewSuiteExtraAssertCall), enabledByDefault: true}, {factory: asCheckerFactory(NewSuiteDontUsePkg), enabledByDefault: true}, {factory: asCheckerFactory(NewUselessAssert), enabledByDefault: true}, + {factory: asCheckerFactory(NewRedundantAssert), enabledByDefault: true}, {factory: asCheckerFactory(NewFormatter), enabledByDefault: true}, // Advanced checkers. {factory: asCheckerFactory(NewBlankImport), enabledByDefault: true}, diff --git a/internal/checkers/checkers_registry_test.go b/internal/checkers/checkers_registry_test.go index 527a7114..e2a850de 100644 --- a/internal/checkers/checkers_registry_test.go +++ b/internal/checkers/checkers_registry_test.go @@ -54,6 +54,7 @@ func TestAll(t *testing.T) { "suite-extra-assert-call", "suite-dont-use-pkg", "useless-assert", + "redundant-assert", "formatter", "blank-import", "go-require", @@ -96,6 +97,7 @@ func TestEnabledByDefault(t *testing.T) { "suite-extra-assert-call", "suite-dont-use-pkg", "useless-assert", + "redundant-assert", "formatter", "blank-import", "go-require", diff --git a/internal/checkers/redundant_assert.go b/internal/checkers/redundant_assert.go new file mode 100644 index 00000000..ed2971c2 --- /dev/null +++ b/internal/checkers/redundant_assert.go @@ -0,0 +1,143 @@ +package checkers + +import ( + "fmt" + "go/ast" + "go/token" + "math" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/ast/inspector" + + "github.com/Antonboom/testifylint/internal/analysisutil" +) + +// RedundantAssert detects redundant assert.Error calls when a stronger error assertion +// on the same error value is present in the same block. +type RedundantAssert struct{} + +// NewRedundantAssert constructs RedundantAssert checker. +func NewRedundantAssert() RedundantAssert { return RedundantAssert{} } +func (RedundantAssert) Name() string { return "redundant-assert" } + +func (checker RedundantAssert) Check(pass *analysis.Pass, insp *inspector.Inspector) []analysis.Diagnostic { + callsByBlock := make(map[*ast.BlockStmt][]redundantAssertCallMeta) + + insp.WithStack([]ast.Node{(*ast.CallExpr)(nil)}, func(node ast.Node, push bool, stack []ast.Node) bool { + if !push { + return false + } + + callExpr := node.(*ast.CallExpr) + testifyCall := NewCallMeta(pass, callExpr) + if testifyCall == nil { + return true + } + + parentBlock := findNearestNode[*ast.BlockStmt](stack) + if parentBlock == nil { + return true + } + + parentStmt := findNearestNode[*ast.ExprStmt](stack) + if parentStmt == nil || parentStmt.X != callExpr { + return true + } + + callsByBlock[parentBlock] = append(callsByBlock[parentBlock], redundantAssertCallMeta{ + call: callExpr, + testifyCall: testifyCall, + parentStmt: parentStmt, + }) + return true + }) + + diagnostics := make([]analysis.Diagnostic, 0) + + for _, calls := range callsByBlock { + for currIdx, curr := range calls { + if !curr.testifyCall.IsAssert || curr.testifyCall.Fn.NameFTrimmed != "Error" { + continue + } + if len(curr.testifyCall.Args) < 1 { + continue + } + + currErr := analysisutil.NodeString(pass.Fset, curr.testifyCall.Args[0]) + if currErr == "" { + continue + } + + closestStrongerFn := "" + closestDistance := math.MaxInt + + for otherIdx, other := range calls { + if curr.call == other.call || len(other.testifyCall.Args) < 1 { + continue + } + + otherFn := other.testifyCall.Fn.NameFTrimmed + switch otherFn { + default: + continue + case "ErrorContains", "ErrorIs", "ErrorAs", "EqualError": + } + + otherErr := analysisutil.NodeString(pass.Fset, other.testifyCall.Args[0]) + if currErr != otherErr { + continue + } + distance := absInt(currIdx - otherIdx) + if distance >= closestDistance { + continue + } + closestDistance = distance + closestStrongerFn = other.testifyCall.String() + } + + if closestStrongerFn != "" { + diagnostics = append(diagnostics, *newDiagnostic( + checker.Name(), + curr.testifyCall, + fmt.Sprintf("remove assert.%s: %s already asserts that the error is not nil", curr.testifyCall.Fn.Name, closestStrongerFn), + analysis.SuggestedFix{ + Message: "Remove redundant assert.Error assertion", + TextEdits: []analysis.TextEdit{{ + Pos: curr.parentStmt.Pos(), + End: toStmtLineEnd(pass, curr.parentStmt.End()), + NewText: []byte(""), + }}, + }, + )) + } + } + } + + return diagnostics +} + +type redundantAssertCallMeta struct { + call *ast.CallExpr + testifyCall *CallMeta + parentStmt *ast.ExprStmt +} + +func absInt(v int) int { + if v < 0 { + return -v + } + return v +} + +func toStmtLineEnd(pass *analysis.Pass, pos token.Pos) token.Pos { + file := pass.Fset.File(pos) + if file == nil { + return pos + } + + line := file.Line(pos) + if line >= file.LineCount() { + return pos + } + return file.LineStart(line + 1) +} From 670de9f6d6585a8ca8cb11437eeb1076ff3792fa Mon Sep 17 00:00:00 2001 From: Matthieu MOREL Date: Fri, 29 May 2026 12:01:57 +0200 Subject: [PATCH 2/3] Handle redundant-assert err reassignment corner case Signed-off-by: Matthieu MOREL --- .../redundant-assert/redundant_assert_test.go | 8 ++ .../redundant_assert_test.go.golden | 8 ++ internal/checkers/redundant_assert.go | 98 ++++++++++++++++++- 3 files changed, 113 insertions(+), 1 deletion(-) diff --git a/analyzer/testdata/src/checkers-default/redundant-assert/redundant_assert_test.go b/analyzer/testdata/src/checkers-default/redundant-assert/redundant_assert_test.go index a794fa4c..202755a2 100644 --- a/analyzer/testdata/src/checkers-default/redundant-assert/redundant_assert_test.go +++ b/analyzer/testdata/src/checkers-default/redundant-assert/redundant_assert_test.go @@ -58,6 +58,14 @@ func TestRedundantAssert(t *testing.T) { assert.ErrorContains(t, errA, "err") } + { + errA = errB + assert.Error(t, errA) + + errA = io.EOF + assert.ErrorContains(t, errA, "EOF") + } + require.Error(t, errA) require.ErrorContains(t, errA, "err") } diff --git a/analyzer/testdata/src/checkers-default/redundant-assert/redundant_assert_test.go.golden b/analyzer/testdata/src/checkers-default/redundant-assert/redundant_assert_test.go.golden index 482d5f79..59c5ffd1 100644 --- a/analyzer/testdata/src/checkers-default/redundant-assert/redundant_assert_test.go.golden +++ b/analyzer/testdata/src/checkers-default/redundant-assert/redundant_assert_test.go.golden @@ -54,6 +54,14 @@ func TestRedundantAssert(t *testing.T) { assert.ErrorContains(t, errA, "err") } + { + errA = errB + assert.Error(t, errA) + + errA = io.EOF + assert.ErrorContains(t, errA, "EOF") + } + require.Error(t, errA) require.ErrorContains(t, errA, "err") } diff --git a/internal/checkers/redundant_assert.go b/internal/checkers/redundant_assert.go index ed2971c2..dbbf1cc0 100644 --- a/internal/checkers/redundant_assert.go +++ b/internal/checkers/redundant_assert.go @@ -4,6 +4,7 @@ import ( "fmt" "go/ast" "go/token" + "go/types" "math" "golang.org/x/tools/go/analysis" @@ -48,13 +49,14 @@ func (checker RedundantAssert) Check(pass *analysis.Pass, insp *inspector.Inspec call: callExpr, testifyCall: testifyCall, parentStmt: parentStmt, + parentBlock: parentBlock, }) return true }) diagnostics := make([]analysis.Diagnostic, 0) - for _, calls := range callsByBlock { + for block, calls := range callsByBlock { for currIdx, curr := range calls { if !curr.testifyCall.IsAssert || curr.testifyCall.Fn.NameFTrimmed != "Error" { continue @@ -87,6 +89,15 @@ func (checker RedundantAssert) Check(pass *analysis.Pass, insp *inspector.Inspec if currErr != otherErr { continue } + + fromStmt, toStmt := curr.parentStmt, other.parentStmt + if fromStmt.Pos() > toStmt.Pos() { + fromStmt, toStmt = toStmt, fromStmt + } + if isReassignedBetween(block, curr.testifyCall.Args[0], fromStmt, toStmt, pass.Fset, pass.TypesInfo) { + continue + } + distance := absInt(currIdx - otherIdx) if distance >= closestDistance { continue @@ -120,6 +131,91 @@ type redundantAssertCallMeta struct { call *ast.CallExpr testifyCall *CallMeta parentStmt *ast.ExprStmt + parentBlock *ast.BlockStmt +} + +func isReassignedBetween( + block *ast.BlockStmt, + errExpr ast.Expr, + fromStmt *ast.ExprStmt, + toStmt *ast.ExprStmt, + fset *token.FileSet, + info *types.Info, +) bool { + if block == nil || errExpr == nil || fromStmt == nil || toStmt == nil { + return false + } + + var ( + errObj = referencedObject(errExpr, info) + errStr string + ) + if errObj == nil { + errStr = analysisutil.NodeString(fset, errExpr) + if errStr == "" { + return false + } + } + + fromPos, toPos := fromStmt.Pos(), toStmt.Pos() + if fromPos > toPos { + fromPos, toPos = toPos, fromPos + } + + for _, stmt := range block.List { + if stmt.Pos() <= fromPos || stmt.Pos() >= toPos { + continue + } + + reassigned := false + ast.Inspect(stmt, func(node ast.Node) bool { + assignStmt, ok := node.(*ast.AssignStmt) + if !ok { + return true + } + + for _, lhs := range assignStmt.Lhs { + if sameErrTarget(lhs, errObj, errStr, fset, info) { + reassigned = true + return false + } + } + return true + }) + if reassigned { + return true + } + } + + return false +} + +func sameErrTarget(lhs ast.Expr, errObj types.Object, errStr string, fset *token.FileSet, info *types.Info) bool { + if errObj != nil { + if lhsObj := referencedObject(lhs, info); lhsObj != nil && lhsObj == errObj { + return true + } + } + + return errStr != "" && analysisutil.NodeString(fset, lhs) == errStr +} + +func referencedObject(expr ast.Expr, info *types.Info) types.Object { + if info == nil { + return nil + } + + switch expr := expr.(type) { + case *ast.Ident: + return info.ObjectOf(expr) + case *ast.SelectorExpr: + if sel, ok := info.Selections[expr]; ok && sel != nil { + return sel.Obj() + } + return info.ObjectOf(expr.Sel) + default: + return nil + } } func absInt(v int) int { From c2da89875d920bf9a52782815b4ab441acfb347b Mon Sep 17 00:00:00 2001 From: Matthieu MOREL Date: Fri, 10 Jul 2026 12:14:00 +0200 Subject: [PATCH 3/3] refactor: rename redundant assert to redundant error assertion and update related tests Signed-off-by: Matthieu MOREL --- README.md | 4 +- analyzer/checkers_factory_test.go | 4 +- .../redundant-assert/redundant_assert_test.go | 72 ----------- .../redundant_error_assertion_test.go | 115 ++++++++++++++++++ .../redundant_error_assertion_test.go.golden} | 48 +++++++- internal/checkers/checkers_registry.go | 2 +- internal/checkers/checkers_registry_test.go | 4 +- internal/checkers/redundant_assert.go | 19 +-- 8 files changed, 174 insertions(+), 94 deletions(-) delete mode 100644 analyzer/testdata/src/checkers-default/redundant-assert/redundant_assert_test.go create mode 100644 analyzer/testdata/src/checkers-default/redundant-error-assertion/redundant_error_assertion_test.go rename analyzer/testdata/src/checkers-default/{redundant-assert/redundant_assert_test.go.golden => redundant-error-assertion/redundant_error_assertion_test.go.golden} (63%) diff --git a/README.md b/README.md index b7101fc1..ab9230b8 100644 --- a/README.md +++ b/README.md @@ -107,7 +107,7 @@ https://golangci-lint.run/docs/linters/configuration/#testifylint | [negative-positive](#negative-positive) | ✅ | ✅ | | [nil-compare](#nil-compare) | ✅ | ✅ | | [regexp](#regexp) | ✅ | ✅ | -| [redundant-assert](#redundant-assert) | ✅ | ✅ | +| [redundant-error-assertion](#redundant-error-assertion) | ✅ | ✅ | | [require-error](#require-error) | ✅ | ❌ | | [suite-broken-parallel](#suite-broken-parallel) | ✅ | ✅ | | [suite-dont-use-pkg](#suite-dont-use-pkg) | ✅ | ✅ | @@ -998,7 +998,7 @@ assert.NotRegexp(t, `\[.*\] TRACE message`, out) --- -### redundant-assert +### redundant-error-assertion ```go ❌ diff --git a/analyzer/checkers_factory_test.go b/analyzer/checkers_factory_test.go index f9d29341..fa17760e 100644 --- a/analyzer/checkers_factory_test.go +++ b/analyzer/checkers_factory_test.go @@ -61,7 +61,7 @@ func Test_newCheckers(t *testing.T) { } enabledByDefaultAdvancedCheckers := []checkers.AdvancedChecker{ - checkers.NewRedundantAssert(), + checkers.NewRedundantErrorAssertion(), checkers.NewBlankImport(), checkers.NewGoRequire(), checkers.NewMockExpect(), @@ -71,7 +71,7 @@ func Test_newCheckers(t *testing.T) { checkers.NewSuiteSubtestRun(), } allAdvancedCheckers := []checkers.AdvancedChecker{ - checkers.NewRedundantAssert(), + checkers.NewRedundantErrorAssertion(), checkers.NewBlankImport(), checkers.NewGoRequire(), checkers.NewMockExpect(), diff --git a/analyzer/testdata/src/checkers-default/redundant-assert/redundant_assert_test.go b/analyzer/testdata/src/checkers-default/redundant-assert/redundant_assert_test.go deleted file mode 100644 index 202755a2..00000000 --- a/analyzer/testdata/src/checkers-default/redundant-assert/redundant_assert_test.go +++ /dev/null @@ -1,72 +0,0 @@ -package redundantassert - -import ( - "errors" - "io" - "os" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestRedundantAssert(t *testing.T) { - t.Parallel() - - var ( - errA = errors.New("err A") - errB = errors.New("err B") - target *os.PathError - ) - - assObj := assert.New(t) - - // Invalid. - { - assert.Error(t, errA) // want "redundant-assert: remove assert\\.Error: assert\\.ErrorContains already asserts that the error is not nil" - assert.ErrorContains(t, errA, "err") - - assert.EqualError(t, errA, "err A") - assert.Error(t, errA) // want "redundant-assert: remove assert\\.Error: assert\\.EqualError already asserts that the error is not nil" - - assObj.Error(errA) // want "redundant-assert: remove assert\\.Error: assObj\\.ErrorAs already asserts that the error is not nil" - assObj.ErrorAs(errA, &target) - - assert.Errorf(t, errA, "msg") // want "redundant-assert: remove assert\\.Errorf: assObj\\.ErrorAs already asserts that the error is not nil" - require.ErrorIs(t, errA, io.EOF) - } - - // Valid. - { - { - assert.Error(t, errA) - assert.ErrorContains(t, errB, "err") - } - - { - assert.Error(t, errA) - } - - if assert.Error(t, errA) { - assert.ErrorContains(t, errA, "err") - } - - { - { - assert.Error(t, errA) - } - assert.ErrorContains(t, errA, "err") - } - - { - errA = errB - assert.Error(t, errA) - - errA = io.EOF - assert.ErrorContains(t, errA, "EOF") - } - - require.Error(t, errA) - require.ErrorContains(t, errA, "err") - } -} diff --git a/analyzer/testdata/src/checkers-default/redundant-error-assertion/redundant_error_assertion_test.go b/analyzer/testdata/src/checkers-default/redundant-error-assertion/redundant_error_assertion_test.go new file mode 100644 index 00000000..b6596bb2 --- /dev/null +++ b/analyzer/testdata/src/checkers-default/redundant-error-assertion/redundant_error_assertion_test.go @@ -0,0 +1,115 @@ +package redundanterrorassertion + +import ( + "errors" + "io" + "os" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRedundantErrorAssertion(t *testing.T) { + t.Parallel() + + var ( + errA = errors.New("err A") + errB = errors.New("err B") + target *os.PathError + ) + + assObj, reqObj := assert.New(t), require.New(t) + + // Invalid. + { + assert.Error(t, errA) // want "redundant-error-assertion: remove assert\\.Error: assert\\.ErrorContains already asserts that the error is not nil" + assert.ErrorContains(t, errA, "err") + } + + { + require.Error(t, errA) // want "redundant-error-assertion: remove require\\.Error: assert\\.ErrorContains already asserts that the error is not nil" + assert.ErrorContains(t, errA, "err") + } + + { + require.Error(t, errA) // want "redundant-error-assertion: remove require\\.Error: require\\.ErrorContains already asserts that the error is not nil" + require.ErrorContains(t, errA, "err") + } + + { + assert.Error(t, errA) // want "redundant-error-assertion: remove assert\\.Error: require\\.ErrorContains already asserts that the error is not nil" + require.ErrorContains(t, errA, "err") + } + + { + assert.EqualError(t, errA, "err A") + assert.Error(t, errA) // want "redundant-error-assertion: remove assert\\.Error: assert\\.EqualError already asserts that the error is not nil" + } + + { + require.EqualError(t, errA, "err A") + require.Error(t, errA) // want "redundant-error-assertion: remove require\\.Error: require\\.EqualError already asserts that the error is not nil" + } + + { + assObj.Error(errA) // want "redundant-error-assertion: remove assObj\\.Error: assObj\\.ErrorAs already asserts that the error is not nil" + assObj.ErrorAs(errA, &target) + } + + { + reqObj.Error(errA) // want "redundant-error-assertion: remove reqObj\\.Error: reqObj\\.ErrorAs already asserts that the error is not nil" + reqObj.ErrorAs(errA, &target) + } + + { + assert.Errorf(t, errA, "msg") // want "redundant-error-assertion: remove assert\\.Errorf: reqObj\\.ErrorAs already asserts that the error is not nil" + reqObj.ErrorAs(errA, &target) + } + + { + require.Errorf(t, errA, "msg") // want "redundant-error-assertion: remove require\\.Errorf: reqObj\\.ErrorAs already asserts that the error is not nil" + reqObj.ErrorAs(errA, &target) + } + + { + require.Errorf(t, errA, "msg") // want "redundant-error-assertion: remove require\\.Errorf: require\\.ErrorIs already asserts that the error is not nil" + require.ErrorIs(t, errA, io.EOF) + } + + // Valid. + { + { + assert.Error(t, errA) + assert.ErrorContains(t, errB, "err") + } + + { + require.Error(t, errA) + assert.ErrorContains(t, errB, "err") + } + + { + assert.Error(t, errA) + } + + if assert.Error(t, errA) { + assert.ErrorContains(t, errA, "err") + } + + { + { + assert.Error(t, errA) + } + assert.ErrorContains(t, errA, "err") + } + + { + errA = errB + assert.Error(t, errA) + + errA = io.EOF + assert.ErrorContains(t, errA, "EOF") + } + } +} diff --git a/analyzer/testdata/src/checkers-default/redundant-assert/redundant_assert_test.go.golden b/analyzer/testdata/src/checkers-default/redundant-error-assertion/redundant_error_assertion_test.go.golden similarity index 63% rename from analyzer/testdata/src/checkers-default/redundant-assert/redundant_assert_test.go.golden rename to analyzer/testdata/src/checkers-default/redundant-error-assertion/redundant_error_assertion_test.go.golden index 59c5ffd1..4fdfa2f1 100644 --- a/analyzer/testdata/src/checkers-default/redundant-assert/redundant_assert_test.go.golden +++ b/analyzer/testdata/src/checkers-default/redundant-error-assertion/redundant_error_assertion_test.go.golden @@ -1,4 +1,4 @@ -package redundantassert +package redundanterrorassertion import ( "errors" @@ -10,7 +10,7 @@ import ( "github.com/stretchr/testify/require" ) -func TestRedundantAssert(t *testing.T) { +func TestRedundantErrorAssertion(t *testing.T) { t.Parallel() var ( @@ -19,16 +19,50 @@ func TestRedundantAssert(t *testing.T) { target *os.PathError ) - assObj := assert.New(t) + assObj, reqObj := assert.New(t), require.New(t) // Invalid. { assert.ErrorContains(t, errA, "err") + } + + { + assert.ErrorContains(t, errA, "err") + } + + { + require.ErrorContains(t, errA, "err") + } + { + require.ErrorContains(t, errA, "err") + } + + { assert.EqualError(t, errA, "err A") + } + + { + require.EqualError(t, errA, "err A") + } + { assObj.ErrorAs(errA, &target) + } + + { + reqObj.ErrorAs(errA, &target) + } + + { + reqObj.ErrorAs(errA, &target) + } + + { + reqObj.ErrorAs(errA, &target) + } + { require.ErrorIs(t, errA, io.EOF) } @@ -39,6 +73,11 @@ func TestRedundantAssert(t *testing.T) { assert.ErrorContains(t, errB, "err") } + { + require.Error(t, errA) + assert.ErrorContains(t, errB, "err") + } + { assert.Error(t, errA) } @@ -61,8 +100,5 @@ func TestRedundantAssert(t *testing.T) { errA = io.EOF assert.ErrorContains(t, errA, "EOF") } - - require.Error(t, errA) - require.ErrorContains(t, errA, "err") } } diff --git a/internal/checkers/checkers_registry.go b/internal/checkers/checkers_registry.go index eaaaa00d..8306d0a1 100644 --- a/internal/checkers/checkers_registry.go +++ b/internal/checkers/checkers_registry.go @@ -26,7 +26,7 @@ var registry = checkersRegistry{ {factory: asCheckerFactory(NewSuiteExtraAssertCall), enabledByDefault: true}, {factory: asCheckerFactory(NewSuiteDontUsePkg), enabledByDefault: true}, {factory: asCheckerFactory(NewUselessAssert), enabledByDefault: true}, - {factory: asCheckerFactory(NewRedundantAssert), enabledByDefault: true}, + {factory: asCheckerFactory(NewRedundantErrorAssertion), enabledByDefault: true}, {factory: asCheckerFactory(NewFormatter), enabledByDefault: true}, // Advanced checkers. {factory: asCheckerFactory(NewBlankImport), enabledByDefault: true}, diff --git a/internal/checkers/checkers_registry_test.go b/internal/checkers/checkers_registry_test.go index e2a850de..621a9ba8 100644 --- a/internal/checkers/checkers_registry_test.go +++ b/internal/checkers/checkers_registry_test.go @@ -54,7 +54,7 @@ func TestAll(t *testing.T) { "suite-extra-assert-call", "suite-dont-use-pkg", "useless-assert", - "redundant-assert", + "redundant-error-assertion", "formatter", "blank-import", "go-require", @@ -97,7 +97,7 @@ func TestEnabledByDefault(t *testing.T) { "suite-extra-assert-call", "suite-dont-use-pkg", "useless-assert", - "redundant-assert", + "redundant-error-assertion", "formatter", "blank-import", "go-require", diff --git a/internal/checkers/redundant_assert.go b/internal/checkers/redundant_assert.go index dbbf1cc0..a2677078 100644 --- a/internal/checkers/redundant_assert.go +++ b/internal/checkers/redundant_assert.go @@ -13,15 +13,15 @@ import ( "github.com/Antonboom/testifylint/internal/analysisutil" ) -// RedundantAssert detects redundant assert.Error calls when a stronger error assertion +// RedundantErrorAssertion detects redundant error assertions when a stronger error assertion // on the same error value is present in the same block. -type RedundantAssert struct{} +type RedundantErrorAssertion struct{} -// NewRedundantAssert constructs RedundantAssert checker. -func NewRedundantAssert() RedundantAssert { return RedundantAssert{} } -func (RedundantAssert) Name() string { return "redundant-assert" } +// NewRedundantErrorAssertion constructs RedundantErrorAssertion checker. +func NewRedundantErrorAssertion() RedundantErrorAssertion { return RedundantErrorAssertion{} } +func (RedundantErrorAssertion) Name() string { return "redundant-error-assertion" } -func (checker RedundantAssert) Check(pass *analysis.Pass, insp *inspector.Inspector) []analysis.Diagnostic { +func (checker RedundantErrorAssertion) Check(pass *analysis.Pass, insp *inspector.Inspector) []analysis.Diagnostic { callsByBlock := make(map[*ast.BlockStmt][]redundantAssertCallMeta) insp.WithStack([]ast.Node{(*ast.CallExpr)(nil)}, func(node ast.Node, push bool, stack []ast.Node) bool { @@ -58,7 +58,7 @@ func (checker RedundantAssert) Check(pass *analysis.Pass, insp *inspector.Inspec for block, calls := range callsByBlock { for currIdx, curr := range calls { - if !curr.testifyCall.IsAssert || curr.testifyCall.Fn.NameFTrimmed != "Error" { + if curr.testifyCall.Fn.NameFTrimmed != "Error" { continue } if len(curr.testifyCall.Args) < 1 { @@ -107,12 +107,13 @@ func (checker RedundantAssert) Check(pass *analysis.Pass, insp *inspector.Inspec } if closestStrongerFn != "" { + removedCall := curr.testifyCall.String() diagnostics = append(diagnostics, *newDiagnostic( checker.Name(), curr.testifyCall, - fmt.Sprintf("remove assert.%s: %s already asserts that the error is not nil", curr.testifyCall.Fn.Name, closestStrongerFn), + fmt.Sprintf("remove %s: %s already asserts that the error is not nil", removedCall, closestStrongerFn), analysis.SuggestedFix{ - Message: "Remove redundant assert.Error assertion", + Message: "Remove redundant error assertion", TextEdits: []analysis.TextEdit{{ Pos: curr.parentStmt.Pos(), End: toStmtLineEnd(pass, curr.parentStmt.End()),