diff --git a/README.md b/README.md index ff40679..e151648 100644 --- a/README.md +++ b/README.md @@ -1,10 +1,34 @@ # testifylint + +[![CI](https://github.com/Antonboom/testifylint/actions/workflows/ci.yml/badge.svg)](https://github.com/Antonboom/testifylint/actions/workflows/ci.yml) +[![Go Report Card](https://goreportcard.com/badge/github.com/Antonboom/testifylint)](https://goreportcard.com/report/github.com/Antonboom/testifylint) +[![Coverage](https://coveralls.io/repos/github/Antonboom/testifylint/badge.svg?branch=master)](https://coveralls.io/github/Antonboom/testifylint?branch=master) +[![MIT License](http://img.shields.io/badge/license-MIT-blue.svg?style=flat)](LICENSE) + Checks usage of [github.com/stretchr/testify](https://github.com/stretchr/testify). +## Problem statement + +Tests are also program code and the requirements for them should not differ much from the requirements +for the code under tests 🙂 + +We should try to maintain the consistency of tests, increase their readability, reduce the chance of bugs +and speed up the search for a problem. + +[testify](https://github.com/stretchr/testify) is the most popular Golang testing framework* in recent years. +But it has a terrible ambiguous API in places, and **the purpose of this linter is to protect you from annoying mistakes**. + +Most checkers are stylistic, but checkers like [error-is](#error-is), [require-error](#require-error), +[expected-actual](#expected-actual), [float-compare](#float-compare) are really helpful. + +_* JetBrains "The State of Go Ecosystem" reports [2021](https://www.jetbrains.com/lp/devecosystem-2021/go/#Go_which-testing-frameworks-do-you-use-regularly-if-any) +and [2022](https://www.jetbrains.com/lp/devecosystem-2022/go/#which-testing-frameworks-do-you-use-regularly-if-any-)._ + ## Installation & usage ``` $ go install github.com/Antonboom/testifylint@latest +$ testifylint -h $ testifylint ./... ``` @@ -33,13 +57,15 @@ $ testifylint --enable=expected-actual -expected-actual.pattern=^wanted$ ./... | [suite-no-extra-assert-call](#suite-no-extra-assert-call) | ❌ | ✅ | | [suite-thelper](#suite-thelper) | ❌ | ✅ | +--- + ### bool-compare ```go ❌ assert.Equal(t, true, result) assert.Equal(t, false, result) assert.NotEqual(t, true, result) assert.False(t, !result) - // And other variations... + // And other variations... ✅ assert.True(t, result) assert.False(t, result) @@ -47,7 +73,10 @@ $ testifylint --enable=expected-actual -expected-actual.pattern=^wanted$ ./... assert.True(t, result) ``` **Autofix**: true.
-**Enabled by default**: true. +**Enabled by default**: true.
+**Reason**: Code simplification. + +--- ### compares ```go @@ -57,7 +86,7 @@ $ testifylint --enable=expected-actual -expected-actual.pattern=^wanted$ ./... assert.True(t, a >= b) assert.True(t, a < b) assert.True(t, a <= b) - // And other variations (assert.False including)... + // And other variations (with assert.False too)... ✅ assert.Equal(t, a, b) assert.NotEqual(t, a, b) @@ -67,19 +96,25 @@ $ testifylint --enable=expected-actual -expected-actual.pattern=^wanted$ ./... assert.LessOrEqual(t, a, b) ``` **Autofix**: true.
-**Enabled by default**: true. +**Enabled by default**: true.
+**Reason**: More appropriate `testify` API with clearer failure message. + +--- ### empty ```go ❌ assert.Len(t, arr, 0) - assert.Equal(t, len(arr), 0) + assert.Equal(t, 0, len(arr)) // And other variations around len(arr)... ✅ assert.Empty(t, arr) assert.Empty(t, arr) ``` **Autofix**: true.
-**Enabled by default**: true. +**Enabled by default**: true.
+**Reason**: More appropriate `testify` API with clearer failure message. + +--- ### error ```go @@ -90,7 +125,10 @@ $ testifylint --enable=expected-actual -expected-actual.pattern=^wanted$ ./... assert.Error(t, err) ``` **Autofix**: true.
-**Enabled by default**: true. +**Enabled by default**: true.
+**Reason**: More appropriate `testify` API with clearer failure message. + +--- ### error-is ```go @@ -101,7 +139,10 @@ $ testifylint --enable=expected-actual -expected-actual.pattern=^wanted$ ./... assert.NotErrorIs(t, err, errSentinel) ``` **Autofix**: true.
-**Enabled by default**: true. +**Enabled by default**: true.
+**Reason**: A common mistake that leads to hiding the incorrect wrapping of sentinel errors. + +--- ### expected-actual ```go @@ -116,7 +157,16 @@ $ testifylint --enable=expected-actual -expected-actual.pattern=^wanted$ ./... assert.YAMLEq(t, "version: '3'", result) ``` **Autofix**: true.
-**Enabled by default**: true. +**Enabled by default**: true.
+**Reason**: A common mistake that makes it harder to understand the reason of failed test. + +The checker considers the expected value to be a basic literal, constant, or variable whose name matches the pattern +(`-expected-actual.pattern` flag). + +It is planned [to change the order of assertion arguments](https://github.com/stretchr/testify/issues/1089#Argument_order) to more natural +(actual, expected) in `v2`. + +--- ### float-compare ```go @@ -124,80 +174,97 @@ $ testifylint --enable=expected-actual -expected-actual.pattern=^wanted$ ./... assert.True(t, a == 42.42) assert.False(t, a != 42.42) -✅ assert.InDelta(t, 42.42, a, 0.0001) +✅ assert.InEpsilon(t, 42.42, a, 0.0001) // Or assert.InDelta assert.InEpsilon(t, 42.42, a, 0.01) - assert.InDelta(t, 42.42, a, 0.001) + assert.InEpsilon(t, 42.42, a, 0.001) ``` **Autofix**: false.
-**Enabled by default**: true. +**Enabled by default**: true.
+**Reason**: Do not forget about [floating point rounding issues](https://floating-point-gui.de/errors/comparison/). + +This checker is similar to the [floatcompare](https://github.com/golangci/golangci-lint/pull/2608) linter. + +--- ### len ```go -❌ assert.Equal(t, len(arr), 3) +❌ assert.Equal(t, 3, len(arr)) assert.True(t, len(arr) == 5) ✅ assert.Len(t, arr, 3) assert.Len(t, arr, 5) ``` **Autofix**: true.
-**Enabled by default**: true. +**Enabled by default**: true.
+**Reason**: More appropriate `testify` API with clearer failure message. + +--- ### require-error ```go -❌ assert.Error(t, err) - assert.ErrorIs(t, err, io.EOF) - assert.ErrorAs(t, err, new(os.PathError)) - assert.NoError(t, err) - assert.NotErrorIs(t, err, io.EOF) - s.Error(err) - s.Assert().ErrorIs(err, io.EOF) - // etc. - -✅ require.Error(t, err) - require.ErrorIs(t, err, io.EOF) - require.ErrorAs(t, err, new(os.PathError)) - require.NoError(t, err) - require.NotErrorIs(t, err, io.EOF) - s.Require().Error(err) +❌ assert.NoError(t, err) + s.ErrorIs(err, io.EOF) + s.Assert().Error(err) + // And other error assertions... + +✅ require.NoError(t, err) s.Require().ErrorIs(err, io.EOF) + s.Require().Error(err) ``` **Autofix**: false.
-**Enabled by default**: true. +**Enabled by default**: true.
+**Reason**: "Ignoring" errors is not the "Go way" and leads to further panics in the test, making it harder to debug. + +`tesitfy/require` allows to stop test execution when a test fails. + +--- ### suite-dont-use-pkg ```go import "github.com/stretchr/testify/assert" func (s *MySuite) TestSomething() { - ❌ assert.Equal(s.T(), 42, value) - ✅ s.Equal(42, value) +❌ assert.Equal(s.T(), 42, value) +✅ s.Equal(42, value) } ``` **Autofix**: true.
-**Enabled by default**: true. +**Enabled by default**: true.
+**Reason**: More simple and uniform code. + +--- ### suite-no-extra-assert-call ```go func (s *MySuite) TestSomething() { - ❌ s.Assert().Equal(42, value) - ✅ s.Equal(42, value) +❌ s.Assert().Equal(42, value) +✅ s.Equal(42, value) } ``` **Autofix**: true.
-**Enabled by default**: false. +**Enabled by default**: false.
+**Reason**: More simple code. + +--- ### suite-thelper ```go ❌ func (s *RoomSuite) assertRoomRound(roundID RoundID) { - s.Equal(roundID, s.getRoom().CurrentRound.ID) +s.Equal(roundID, s.getRoom().CurrentRound.ID) } ✅ func (s *RoomSuite) assertRoomRound(roundID RoundID) { - s.T().Helper() - s.Equal(roundID, s.getRoom().CurrentRound.ID) +s.T().Helper() +s.Equal(roundID, s.getRoom().CurrentRound.ID) } ``` **Autofix**: true.
-**Enabled by default**: false. +**Enabled by default**: false.
+**Reason**: Consistency to non-suite test helpers. Explicit markup of helper methods. + +`s.T().Helper()` call is not important technically because `testify` prints full `Error Trace` +[anyway](https://github.com/stretchr/testify/blob/882382d845cd9780bd93c1acc8e1fa2ffe266ca1/assert/assertions.go#L317). + +The checker rather acts as an example of a [checkers.AdvancedChecker](https://github.com/Antonboom/testifylint/blob/676324836555445fded4e9afc004101ec6f597fe/internal/checkers/checker.go#L56). diff --git a/internal/checkers/bool_compare.go b/internal/checkers/bool_compare.go index f8b482d..ddc024c 100644 --- a/internal/checkers/bool_compare.go +++ b/internal/checkers/bool_compare.go @@ -10,13 +10,17 @@ import ( "github.com/Antonboom/testifylint/internal/analysisutil" ) -// BoolCompare checks situation like +// BoolCompare detects situations like // // assert.Equal(t, false, result) +// assert.False(t, !result) +// ... // -// and requires e.g. +// and requires // // assert.False(t, result) +// assert.True(t, result) +// ... type BoolCompare struct{} // // NewBoolCompare constructs BoolCompare checker. diff --git a/internal/checkers/compares.go b/internal/checkers/compares.go index c9ff6e6..ff9cb00 100644 --- a/internal/checkers/compares.go +++ b/internal/checkers/compares.go @@ -10,13 +10,17 @@ import ( "github.com/Antonboom/testifylint/internal/analysisutil" ) -// Compares checks situation like +// Compares detects situations like // // assert.True(t, a >= b) +// assert.False(t, a != b) +// ... // -// and requires e.g. +// and requires // // assert.GreaterOrEqual(t, a, b) +// assert.Equal(t, a, b) +// ... type Compares struct{} // NewCompares constructs Compares checker. diff --git a/internal/checkers/empty.go b/internal/checkers/empty.go index 040fe8e..9892035 100644 --- a/internal/checkers/empty.go +++ b/internal/checkers/empty.go @@ -11,11 +11,12 @@ import ( "github.com/Antonboom/testifylint/internal/analysisutil" ) -// Empty checks situation like +// Empty detects situations like // -// assert.Equal(t, len(arr), 0) +// assert.Equal(t, 0, len(arr)) +// assert.Len(t, arr, 0) // -// and requires e.g. +// and requires // // assert.Empty(t, arr) type Empty struct{} diff --git a/internal/checkers/error.go b/internal/checkers/error.go index 05d1ca0..c05a3de 100644 --- a/internal/checkers/error.go +++ b/internal/checkers/error.go @@ -7,13 +7,15 @@ import ( "golang.org/x/tools/go/analysis" ) -// Error checks situation like +// Error detects situations like // // assert.Nil(t, err) +// assert.NotNil(t, err) // -// and requires e.g. +// and requires // // assert.NoError(t, err) +// assert.Error(t, err) type Error struct{} // NewError constructs Error checker. diff --git a/internal/checkers/error_is.go b/internal/checkers/error_is.go index 80adb6f..afff380 100644 --- a/internal/checkers/error_is.go +++ b/internal/checkers/error_is.go @@ -6,13 +6,15 @@ import ( "golang.org/x/tools/go/analysis" ) -// ErrorIs checks situation like +// ErrorIs detects situations like // -// assert.Equal(t, len(arr), 3) +// assert.Error(t, err, errSentinel) +// assert.NoError(t, err, errSentinel) // -// and requires e.g. +// and requires // -// assert.Len(t, arr, 3) +// assert.ErrorIs(t, err, errSentinel) +// assert.NotErrorIs(t, err, errSentinel) type ErrorIs struct{} // NewErrorIs constructs ErrorIs checker. @@ -28,14 +30,14 @@ func (checker ErrorIs) Check(pass *analysis.Pass, call *CallMeta) *analysis.Diag switch call.Fn.Name { case "Error", "Errorf": if isError(pass, errArg) { - proposed := "ErrorIs" + const proposed = "ErrorIs" msg := fmt.Sprintf("invalid usage of %[1]s.Error, use %[1]s.%[2]s instead", call.SelectorXStr, proposed) return newDiagnostic(checker.Name(), call, msg, newSuggestedFuncReplacement(call, proposed)) } case "NoError", "NoErrorf": if isError(pass, errArg) { - proposed := "NotErrorIs" + const proposed = "NotErrorIs" msg := fmt.Sprintf("invalid usage of %[1]s.NoError, use %[1]s.%[2]s instead", call.SelectorXStr, proposed) return newDiagnostic(checker.Name(), call, msg, newSuggestedFuncReplacement(call, proposed)) } diff --git a/internal/checkers/expected_actual.go b/internal/checkers/expected_actual.go index ce9192c..c913145 100644 --- a/internal/checkers/expected_actual.go +++ b/internal/checkers/expected_actual.go @@ -14,11 +14,11 @@ import ( var DefaultExpectedVarPattern = regexp.MustCompile( `(^(exp(ected)?|want(ed)?)([A-Z]\w*)?$)|(^(\w*[a-z])?(Exp(ected)?|Want(ed)?)$)`) -// ExpectedActual checks situation like +// ExpectedActual detects situation like // // assert.NotEqual(t, result, "expected value") // -// and requires e.g. +// and requires // // assert.NotEqual(t, "expected value", result) type ExpectedActual struct { diff --git a/internal/checkers/float_compare.go b/internal/checkers/float_compare.go index 7015e5b..8705cf0 100644 --- a/internal/checkers/float_compare.go +++ b/internal/checkers/float_compare.go @@ -7,13 +7,13 @@ import ( "golang.org/x/tools/go/analysis" ) -// FloatCompare checks situation like +// FloatCompare detects situation like // // assert.Equal(t, 42.42, a) // -// and requires e.g. +// and requires // -// assert.InEpsilon(t, 42.42, a, 0.0001) +// assert.InEpsilon(t, 42.42, a, 0.0001) // Or assert.InDelta type FloatCompare struct{} // NewFloatCompare constructs FloatCompare checker. diff --git a/internal/checkers/len.go b/internal/checkers/len.go index 51fa19a..a27067a 100644 --- a/internal/checkers/len.go +++ b/internal/checkers/len.go @@ -7,13 +7,15 @@ import ( "golang.org/x/tools/go/analysis" ) -// Len checks situation like +// Len detects situations like // -// assert.Equal(t, 0, len(arr)) +// assert.Equal(t, 3, len(arr)) +// assert.True(t, len(arr) == 5) // -// and requires e.g. +// and requires // -// assert.Len(t, arr, 0) +// assert.Len(t, arr, 3) +// assert.Len(t, arr, 5) type Len struct{} // NewLen constructs Len checker. diff --git a/internal/checkers/require_error.go b/internal/checkers/require_error.go index ca5e5d8..7fd07dc 100644 --- a/internal/checkers/require_error.go +++ b/internal/checkers/require_error.go @@ -2,13 +2,17 @@ package checkers import "golang.org/x/tools/go/analysis" -// RequireError checks situation like +// RequireError detects situations like // // assert.NoError(t, err) +// s.ErrorIs(err, io.EOF) +// s.Assert().Error(err) // -// and requires e.g. +// and requires // // require.NoError(t, err) +// s.Require().ErrorIs(err, io.EOF) +// s.Require().Error(err) type RequireError struct{} // NewRequireError constructs RequireError checker. diff --git a/internal/checkers/suite_dont_use_pkg.go b/internal/checkers/suite_dont_use_pkg.go index 1351cf7..44a1c4e 100644 --- a/internal/checkers/suite_dont_use_pkg.go +++ b/internal/checkers/suite_dont_use_pkg.go @@ -11,13 +11,13 @@ import ( "github.com/Antonboom/testifylint/internal/testify" ) -// SuiteDontUsePkg checks situation like +// SuiteDontUsePkg detects situation like // // func (s *MySuite) TestSomething() { // assert.Equal(s.T(), 42, value) // } // -// and requires e.g. +// and requires // // func (s *MySuite) TestSomething() { // s.Equal(42, value) diff --git a/internal/checkers/suite_no_extra_assert_call.go b/internal/checkers/suite_no_extra_assert_call.go index b0f407e..dd8aee7 100644 --- a/internal/checkers/suite_no_extra_assert_call.go +++ b/internal/checkers/suite_no_extra_assert_call.go @@ -6,13 +6,13 @@ import ( "golang.org/x/tools/go/analysis" ) -// SuiteNoExtraAssertCall checks situation like +// SuiteNoExtraAssertCall detects situation like // // func (s *MySuite) TestSomething() { // s.Assert().Equal(42, value) // } // -// and requires e.g. +// and requires // // func (s *MySuite) TestSomething() { // s.Equal(42, value) diff --git a/internal/checkers/suite_thelper.go b/internal/checkers/suite_thelper.go index f61f3ce..f57c2b5 100644 --- a/internal/checkers/suite_thelper.go +++ b/internal/checkers/suite_thelper.go @@ -12,7 +12,7 @@ import ( "github.com/Antonboom/testifylint/internal/analysisutil" ) -// SuiteTHelper wants t.Helper() call in suite helpers: +// SuiteTHelper requires t.Helper() call in suite helpers: // // func (s *RoomSuite) assertRoomRound(roundID RoundID) { // s.T().Helper()