Skip to content

Latest commit

 

History

History
485 lines (352 loc) · 13.4 KB

CONTRIBUTING.md

File metadata and controls

485 lines (352 loc) · 13.4 KB

Contribution guideline

⚠️ The module version is 1.20, but for development you need Go >= 1.21

1) Install developer tools

# https://taskfile.dev/installation/
$ go install github.com/go-task/task/v3/cmd/task@latest
$ task tools:install
Install dev tools...

2) Create a checker skeleton in internal/checkers/{checker_name}.go

For example, we want to create a new checker Zero in internal/checkers/zero.go:

package checkers

// Zero detects situations like
//
//	assert.Equal(t, 0, count)
//	assert.Equal(t, nil, userObj)
//
// and requires
//
//	assert.Zero(t, count)
//	assert.Zero(t, userObj)
type Zero struct{}

// NewZero constructs Zero checker.
func NewZero() Zero      { return Zero{} }
func (Zero) Name() string { return "zero" }

The above code is enough to satisfy the checkers.Checker interface.

3) Add the new checker to the registry in order of priority

The earlier the checker is in the registry, the more priority it is.

For example, the zero checker takes precedence over the expected-actual or empty, because its check is more "narrow" and when you fix the warning from zero, the rest of the checkers will become irrelevant.

var registry = checkersRegistry{
    // ...
    {factory: asCheckerFactory(NewZero), enabledByDefault: false},
    // ...
    {factory: asCheckerFactory(NewEmpty), enabledByDefault: true},
    // ...
    {factory: asCheckerFactory(NewExpectedActual), enabledByDefault: true},
    // ...
}

By default, we disable the checker if we doubt its 100% usefulness.

4) Create new tests generator in internal/testgen/gen_{checker_name}.go

Create new ZeroTestsGenerator in internal/testgen/gen_zero.go.

See examples in adjacent files.

In the first iteration, these can be a very simple tests for debugging checker's proof of concept. And after the implementation of the checker, you can add various cycles, variables, etc. to the template.

GoldenTemplate is usually an ErroredTemplate with some strings replaced.

5) Add generator into checkerTestsGenerators

Look at internal/testgen/main.go.

6) Generate and run tests

Tests should fail.

$ task test
Generate analyzer tests...
Test...
...
--- FAIL: TestTestifyLint_CheckersDefault
FAIL

7) Implement the checker

Zero is an example of checkers.RegularChecker because it works with "general" assertion call. For more complex checkers, use the checkers.AdvancedChecker interface.

If the checker turns out to be too “fat”, then you can omit some obviously rare combinations, especially if they are covered by other checkers. Usually these are expressions in assert.True/False.

Remember that assert.TestingT and require.TestingT are different interfaces, which may be important in some contexts.

Also, pay attention to internal/checkers/helpers_*.go files. Try to reuse existing code as much as possible.

8) Improve tests from p.4 if necessary

Pay attention to Assertion and NewAssertionExpander, but keep your tests as small as possible. Usually 100-200 lines are enough for checker testing. You need to find balance between coverage, common sense, and processing of boundary conditions. See existing tests as example.

For testing checker replacements use testdata/src/debug.

9) Run the task command from the project's root directory

$ task
Tidy...
Fmt...
Lint...
Generate analyzer tests...
Test...
Install...

Fix linter issues and broken tests (probably related to the checkers registry).

10) Update README.md, commit the changes and submit a pull request 🔥

Describe a new checker in checkers section.

Open for contribution


elements-match

require.Equal(t, len(expected), len(result)
     sort.Slice(expected, /* ... */)
     sort.Slice(result, /* ... */)
     for i := range result {
         assert.Equal(t, expected[i], result[i])
     }
     // Or for Go >= 1.21
     slices.Sort(expected)
     slices.Sort(result)
     assert.True(t, slices.Equal(expected, result))

✅   assert.ElementsMatch(t, expected, result)

Autofix: maybe (depends on implementation difficulty).
Enabled by default: maybe (depends on checker's stability).
Reason: Code simplification.


error-compare

assert.ErrorContains(t, err, "not found")
     assert.EqualError(t, err, "user not found")
     assert.Equal(t, err.Error(), "user not found")
     assert.Equal(t, err, errSentinel) // Through `reflect.DeepEqual` causes error strings to be compared.
     assert.NotEqual(t, err, errSentinel)
     // etc.assert.ErrorIs(t, err, ErrUserNotFound)

Autofix: false.
Enabled by default: true.
Reason: The Error() method on the error interface exists for humans, not code.
Related issues: #47


graceful-teardown

Warns about usage of require in t.Cleanup functions and suite teardown methods:

func (s *ServiceIntegrationSuite) TearDownTest() {
    if p := s.verdictsProducer; p != nil {
        s.Require().NoError(p.Close()) ❌
    }
    if c := s.dlqVerdictsConsumer; c != nil {
        s.NoError(c.Close())
    }
    s.DBSuite.TearDownTest()
    s.ks.TearDownTest()
}

Autofix: false.
Enabled by default: false.
Reason: Possible resource leaks, because require finishes the current goroutine.


equal-values

assert.Equal(t, int64(100), price.Amount)
✅   assert.EqualValues(t, 100, price.Amount)

Autofix: true.
Enabled by default: maybe?
Reason: Code simplification.


float-compare

  1. Support other tricky cases
assert.NotEqual(t, 42.42, a)
     assert.Greater(t, a, 42.42)
     assert.GreaterOrEqual(t, a, 42.42)
     assert.Less(t, a, 42.42)
     assert.LessOrEqual(t, a, 42.42)

     assert.True(t, a != 42.42) // assert.False(t, a == 42.42)
     assert.True(t, a > 42.42)  // assert.False(t, a <= 42.42)
     assert.True(t, a >= 42.42) // assert.False(t, a < 42.42)
     assert.True(t, a < 42.42)  // assert.False(t, a >= 42.42)
     assert.True(t, a <= 42.42) // assert.False(t, a > 42.42)

But I have no idea for equivalent. Probably we need a new API from testify.

  1. Support compares of "float containers" (structs, slices, arrays, maps, something else?), e.g.
type Tx struct {
    ID string
    Score float64
}

❌   assert.Equal(t, Tx{ID: "xxx", Score: 0.9643}, tx)

✅   assert.Equal(t, "xxx", tx.ID)
     assert.InEpsilon(t, 0.9643, tx.Score, 0.0001)

And similar idea for assert.InEpsilonSlice / assert.InDeltaSlice.

Autofix: false.
Enabled by default: true.
Reason: Work with floating properly.


http-const

assert.HTTPStatusCode(t, handler, "GET", "/index", nil, 200)
     assert.HTTPBodyContains(t, handler, "GET", "/index", nil, "counter")
     // etc.assert.HTTPStatusCode(t, handler, http.MethodGet, "/index", nil, http.StatusOK)
     assert.HTTPBodyContains(t, handler, http.MethodGet, "/index", nil, "counter")

Autofix: true.
Enabled by default: true.
Reason: Is similar to the usestdlibvars linter.


http-sugar

assert.HTTPStatusCode(t,
         handler, http.MethodGet, "/index", nil, http.StatusOK)
     assert.HTTPStatusCode(t,
         handler, http.MethodGet, "/admin", nil, http.StatusNotFound)
     assert.HTTPStatusCode(t,
         handler, http.MethodGet, "/oauth", nil, http.StatusFound)
     // etc.assert.HTTPSuccess(t, handler, http.MethodGet, "/index", nil)
     assert.HTTPError(t, handler, http.MethodGet, "/admin", nil)
     assert.HTTPRedirect(t, handler, http.MethodGet, "/oauth", nil)

Autofix: true.
Enabled by default: maybe?
Reason: Code simplification.


no-fmt-mess

Autofix: true.
Enabled by default: maybe?
Related issues: #33

Those who are new to testify may be discouraged by the duplicative API:

func Equal(t TestingT, expected, actual interface{}, msgAndArgs ...interface{}) bool
func Equalf(t TestingT, expected interface{}, actual interface{}, msg string, args ...interface{}) bool

The f-functions were added a long time ago to eliminate govet complain.

This introduces some inconsistency into the test code, and the next strategies are seen for the checker:

  1. Forbid f-functions at all (also could be done through the forbidigo linter).

This will make it easier to migrate to v2, because

Format functions should not be accepted as they are equivalent to their "non-f" counterparts.

But it doesn't look like a "go way" and the govet won't be happy.

  1. IMHO, a more appropriate option is to disallow the use of msgAndArgs in non-f assertions. Look at the comment.

But there will be no non-stylistic benefits from the checker in this case (depends on the view of API in v2).

Also, in the first iteration no-fmt-mess checker could help to avoid code like this:

assert.Error(t, err, fmt.Sprintf("Profile %s should not be valid", test.profile))

require-len

The main idea: if code contains array/slice indexing, then before that there must be a length constraint through require.

assert.Len(t, arr, 3) // Or assert.NotEmpty(t, arr) and other variations.
     assert.Equal(t, "gopher", arr[1])

✅   require.Len(t, arr, 3) // Or require.NotEmpty(t, arr) and other variations.
     assert.Equal(t, "gopher", arr[1])

Autofix: false?
Enabled by default: maybe?
Reason: Similar to require-error. Save you from annoying panics.

Or maybe do something similar for maps? And come up with better name for the checker.

suite-run

func (s *Suite) TestSomething() {
    ❌
    s.T().Run("subtest1", func(t *testing.T) {
        // ...
        assert.Equal(t, "gopher", result)
    })

    ✅
    s.Run("subtest1", func() {
        // ...
        s.Equal("gopher", result)
    })
}

Autofix: true.
Enabled by default: probably yes.
Reason: Code simplification and consistency.
Related issues: #35

But need to investigate the technical difference and the reasons for the appearance of s.Run. Also, maybe this case is already covered by suite-dont-use-pkg?


suite-test-name

import (
    "testing"
    "github.com/stretchr/testify/suite"
)

type BalanceSubscriptionSuite struct {
    suite.Suite
}

❌ func TestBalanceSubs_Run(t *testing.T) {
    suite.Run(t, new(BalanceSubscriptionSuite))
}


✅ func TestBalanceSubscriptionSuite(t *testing.T) {
    suite.Run(t, new(BalanceSubscriptionSuite))
}

Autofix: true.
Enabled by default: false.
Reason: Just unification of approach.
Related issues: #48

Also, maybe to check the configurable format of subtest name? Mess example:

func (s *HandlersSuite) Test_Usecase_Success()
func (s *HandlersSuite) TestUsecaseSuccess()
func (s *HandlersSuite) Test_UsecaseSuccess()

useless-assert

Support more complex cases, e.g.

body, err := io.ReadAll(rr.Body)
require.NoError(t, err)
require.NoError(t, err) ❌

expectedJSON, err := json.Marshal(expected)
require.NoError(t, err)
require.JSONEq(t, string(expectedJSON), string(body))
require.NoError(t, err)
assert.ErrorContains(t, err, "user") ❌

zero

assert.Equal(t, 0, count)
     assert.Equal(t, nil, userObj)
     assert.Equal(t, "", name)
     // etc.assert.Zero(t, count)
     assert.Zero(t, userObj)
     assert.Zero(t, name)

Autofix: true.
Enabled by default: false.
Reason: Just for your reflection and suggestion.
Related issues: #75

I'm not sure if anyone uses assert.Zero – it looks strange and conflicts with assert.Empty:

assert.Equal(t, "", result)
     assert.Nil(t, errCh)

✅   assert.Empty(t, result)
     assert.Empty(t, errCh)

Maybe it's better to make configurable support for other types in the empty checker and vice versa to prohibit the Zero?


Any other figments of your imagination are welcome 🙏
List of testify functions here.