new checker 'suite-method-signature' (#228)

* new checker 'suite-method-signature'

* suite-method-signature: lost files

* suite-method-signature: review fixes

* docs: add FAQ in contribution.md

* review fixes

Author: Antonboom

.golangci.yml

@@ -25,7 +25,7 @@ issues:
     - path: "_test\\.go"
       linters: [ "lll" ]
 
-    - source: ' // want "'
+    - source: ' // want '
       linters: [ "lll" ]
 
 linters-settings:

CONTRIBUTING.md

@@ -354,3 +354,18 @@ func (s *HandlersSuite) Test_UsecaseSuccess()
 
 Any other figments of your imagination are welcome 🙏<br>
 List of `testify` functions [here](https://pkg.go.dev/github.com/stretchr/testify@master/assert#pkg-functions).
+
+# FAQ
+
+### Why do we use `internal/testify` instead of `github.com/stretchr/testify`?
+
+1) [internal/testify](./internal/testify) is not a local copy of `stretchr/testify`. The package contains domain (for 
+linter context) entities, which absent in `testify` itself.
+
+2) We cannot depend on `stretch/testify`, because it causes dependency bomb for linter's module. We should keep as min
+   dependencies as possible.
+
+### Why do we import `github.com/stretchr/testify` in tests?
+
+Such imports in `testdata` do not affect linter module's dependencies. Moreover, of course, tests should use the real
+`testify`.

README.md

@@ -112,6 +112,7 @@ https://golangci-lint.run/usage/linters/#testifylint
 | [suite-broken-parallel](#suite-broken-parallel)     | ✅                  | ✅       |
 | [suite-dont-use-pkg](#suite-dont-use-pkg)           | ✅                  | ✅       |
 | [suite-extra-assert-call](#suite-extra-assert-call) | ✅                  | ✅       |
+| [suite-method-signature](#suite-method-signature)   | ✅                  | ❌       |
 | [suite-subtest-run](#suite-subtest-run)             | ✅                  | ❌       |
 | [suite-thelper](#suite-thelper)                     | ❌                  | ✅       |
 | [useless-assert](#useless-assert)                   | ✅                  | ❌       |
@@ -963,6 +964,29 @@ You can enable such behavior through `--suite-extra-assert-call.mode=require`.
 
 ---
 
+### suite-method-signature
+
+```go
+❌
+func (s *MySuite) SetupTest(i int) { /* ... */ }
+func (s *MySuite) TestAlice(t *testing.T) { s.SetupTest(rand()) /* ... */ }
+func (s *MySuite) TestBob() { s.SetupTest(rand()) /* ... */ }
+
+✅
+func (s *MySuite) SetupTest() { s.setupTest(rand()) } // Use inversion-of-control's methods from suite interfaces.  
+func (s *MySuite) TestAlice() { /* ... */  }          // Keep test methods clean from args and returning values.
+func (s *MySuite) TestBob() { /* ... */  }
+func (s *MySuite) setupTest(i int) { /* ... */ }
+```
+
+**Autofix**: false. <br>
+**Enabled by default**: true. <br>
+**Reason**: Protection from panics and bugs.
+
+All suite functionality methods are presented [here](https://github.com/stretchr/testify/blob/master/suite/interfaces.go).
+
+---
+
 ### suite-subtest-run
 
 ```go

analyzer/checkers_factory_test.go

@@ -60,13 +60,15 @@ func Test_newCheckers(t *testing.T) {
 		checkers.NewGoRequire(),
 		checkers.NewRequireError(),
 		checkers.NewSuiteBrokenParallel(),
+		checkers.NewSuiteMethodSignature(),
 		checkers.NewSuiteSubtestRun(),
 	}
 	allAdvancedCheckers := []checkers.AdvancedChecker{
 		checkers.NewBlankImport(),
 		checkers.NewGoRequire(),
 		checkers.NewRequireError(),
 		checkers.NewSuiteBrokenParallel(),
+		checkers.NewSuiteMethodSignature(),
 		checkers.NewSuiteSubtestRun(),
 		checkers.NewSuiteTHelper(),
 	}

analyzer/testdata/src/checkers-default/suite-method-signature/suite_method_signature_test.go

@@ -0,0 +1,34 @@
+package debug
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/suite"
+)
+
+func TestBrokenSuite(t *testing.T) {
+	suite.Run(t, new(BrokenSuite))
+}
+
+type BrokenSuite struct {
+	suite.Suite
+}
+
+func (s *BrokenSuite) SetupTest() {
+	var _ suite.TearDownSubTest
+}
+
+func (s *BrokenSuite) SetT() { // *BrokenSuite does not implement suite.TestingSuite (wrong type for method SetT)
+}
+
+func (s *BrokenSuite) TestTypo(_ *testing.T) { // value.go:424: test panicked: reflect: Call with too few input arguments
+	s.True(true)
+}
+
+/*
+1) Flagged by govet (copylocks).
+2) Related to https://github.com/go-critic/go-critic/issues/331.
+*/
+func (s BrokenSuite) TestValueReceiver() {
+	s.True(true)
+}

analyzer/testdata/src/debug/suite_method_signature_test.go

@@ -30,6 +30,7 @@ var registry = checkersRegistry{
 	{factory: asCheckerFactory(NewGoRequire), enabledByDefault: true},
 	{factory: asCheckerFactory(NewRequireError), enabledByDefault: true},
 	{factory: asCheckerFactory(NewSuiteBrokenParallel), enabledByDefault: true},
+	{factory: asCheckerFactory(NewSuiteMethodSignature), enabledByDefault: true},
 	{factory: asCheckerFactory(NewSuiteSubtestRun), enabledByDefault: true},
 	{factory: asCheckerFactory(NewSuiteTHelper), enabledByDefault: false},
 }

internal/checkers/checkers_registry.go

@@ -57,6 +57,7 @@ func TestAll(t *testing.T) {
 		"go-require",
 		"require-error",
 		"suite-broken-parallel",
+		"suite-method-signature",
 		"suite-subtest-run",
 		"suite-thelper",
 	}
@@ -95,6 +96,7 @@ func TestEnabledByDefault(t *testing.T) {
 		"go-require",
 		"require-error",
 		"suite-broken-parallel",
+		"suite-method-signature",
 		"suite-subtest-run",
 	}
 	if !slices.Equal(expected, checkerList) {

internal/checkers/checkers_registry_test.go

@@ -0,0 +1,71 @@
+package checkers
+
+import (
+	"fmt"
+	"go/ast"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/ast/inspector"
+
+	"github.com/Antonboom/testifylint/internal/analysisutil"
+	"github.com/Antonboom/testifylint/internal/testify"
+)
+
+// SuiteMethodSignature warns, when
+//   - suite's test methods have arguments or returning values;
+//   - suite's custom (user) methods conflicts with (shadows/overrides) suite functionality methods.
+type SuiteMethodSignature struct{}
+
+// NewSuiteMethodSignature constructs SuiteMethodSignature checker.
+func NewSuiteMethodSignature() SuiteMethodSignature { return SuiteMethodSignature{} }
+func (SuiteMethodSignature) Name() string           { return "suite-method-signature" }
+
+func (checker SuiteMethodSignature) Check(pass *analysis.Pass, inspector *inspector.Inspector) (diags []analysis.Diagnostic) {
+	inspector.Preorder([]ast.Node{(*ast.FuncDecl)(nil)}, func(node ast.Node) {
+		fd := node.(*ast.FuncDecl)
+		if !isSuiteMethod(pass, fd) {
+			return
+		}
+
+		methodName := fd.Name.Name
+		rcv := fd.Recv.List[0]
+
+		if isSuiteTestMethod(methodName) {
+			if fd.Type.Params.NumFields() > 0 || fd.Type.Results.NumFields() > 0 {
+				const msg = "test method should not have any arguments or returning values"
+				diags = append(diags, *newDiagnostic(checker.Name(), fd, msg))
+				return
+			}
+		}
+
+		iface, ok := suiteMethodToInterface[methodName]
+		if !ok {
+			return
+		}
+		ifaceObj := analysisutil.ObjectOf(pass.Pkg, testify.SuitePkgPath, iface)
+		if ifaceObj == nil {
+			return
+		}
+		if !implements(pass, rcv.Type, ifaceObj) {
+			msg := fmt.Sprintf("method conflicts with %s.%s interface", testify.SuitePkgName, iface)
+			diags = append(diags, *newDiagnostic(checker.Name(), fd, msg))
+		}
+	})
+	return diags
+}
+
+// https://github.com/stretchr/testify/blob/master/suite/interfaces.go
+var suiteMethodToInterface = map[string]string{
+	// NOTE(a.telyshev): We ignore suite.TestingSuite interface, because
+	//  - suite.Run will not work for suite-types that do not implement it;
+	//  - this interface has several methods, which may cause a false positive for one of the methods in the suite-type.
+	"SetupSuite":      "SetupAllSuite",
+	"SetupTest":       "SetupTestSuite",
+	"TearDownSuite":   "TearDownAllSuite",
+	"TearDownTest":    "TearDownTestSuite",
+	"BeforeTest":      "BeforeTest",
+	"AfterTest":       "AfterTest",
+	"HandleStats":     "WithStats",
+	"SetupSubTest":    "SetupSubTest",
+	"TearDownSubTest": "TearDownSubTest",
+}