new checker 'suite-subtest-run' (#104)

* new checker 'suite-subtest-run'

* suite-subtest-run: self-review #1

* suite-subtest-run: self-review #2

Author: Antonboom

CONTRIBUTING.md

@@ -135,7 +135,6 @@ Describe a new checker in [checkers section](./README.md#checkers).
 - [http-const](#http-const)
 - [http-sugar](#http-sugar)
 - [require-len](#require-len)
-- [suite-run](#suite-run)
 - [suite-test-name](#suite-test-name)
 - [useless-assert](#useless-assert)
 
@@ -320,32 +319,6 @@ then before that there must be a length constraint through `require`.
 
 Or maybe do something similar for maps? And come up with better name for the checker.
 
-### suite-run
-
-```go
-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. <br>
-**Enabled by default**: probably yes. <br>
-**Reason**: Code simplification and consistency. <br>
-**Related issues**: [#35](https://github.com/Antonboom/testifylint/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](README.md#suite-dont-use-pkg)?
-
 ---
 
 ### suite-test-name

README.md

@@ -105,6 +105,7 @@ https://golangci-lint.run/usage/linters/#testifylint
 | [require-error](#require-error)                     | ✅                  | ❌       |
 | [suite-dont-use-pkg](#suite-dont-use-pkg)           | ✅                  | ✅       |
 | [suite-extra-assert-call](#suite-extra-assert-call) | ✅                  | ✅       |
+| [suite-subtest-run](#suite-subtest-run)             | ✅                  | ❌       |
 | [suite-thelper](#suite-thelper)                     | ❌                  | ✅       |
 | [useless-assert](#useless-assert)                   | ✅                  | ❌       |
 
@@ -443,8 +444,8 @@ but did not take into account its package, thereby reacting to everything that i
 ```go
 // isPrint records the unformatted-print functions.
 var isPrint = map[string]bool{
-    "error":    true,
-    "fatal":    true,
+    "error": true,
+    "fatal": true,
     // ...
 }
 ```
@@ -456,8 +457,8 @@ in Go 1.10 after a related [issue](https://github.com/golang/go/issues/22936):
 ```go
 // isPrint records the print functions.
 var isPrint = map[string]bool{
-    "fmt.Errorf":         true,
-    "fmt.Fprint":         true,
+    "fmt.Errorf": true,
+    "fmt.Fprint": true,
     // ...
 }
 ```
@@ -672,8 +673,8 @@ Also, to minimize false positives, `require-error` ignores:
 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)
 }
 ```
 
@@ -689,24 +690,24 @@ By default, the checker wants you to remove unnecessary `Assert()` calls:
 
 ```go
 func (s *MySuite) TestSomething() {
-     ❌ s.Assert().Equal(42, value)
-     ✅ s.Equal(42, value)
+    ❌ s.Assert().Equal(42, value)
+    ✅ s.Equal(42, value)
 }
 ```
 
 But sometimes, on the contrary, people want consistency with `s.Assert()` and `s.Require()`:
 
 ```go
 func (s *MySuite) TestSomething() {
-     // ...
+    // ...
 
-     ❌
-     s.Require().NoError(err)
-     s.Equal(42, value)
+    ❌
+    s.Require().NoError(err)
+    s.Equal(42, value)
 
-     ✅
-     s.Require().NoError(err)
-     s.Assert().Equal(42, value)
+    ✅
+    s.Require().NoError(err)
+    s.Assert().Equal(42, value)
 }
 ```
 
@@ -718,18 +719,49 @@ You can enable such behavior through `--suite-extra-assert-call.mode=require`.
 
 ---
 
+### suite-subtest-run
+
+```go
+func (s *MySuite) TestSomething() {
+    ❌
+    s.T().Run("subtest", func(t *testing.T) {
+        assert.Equal(t, 42, result)
+    })
+     
+    ✅
+    s.Run("subtest", func() {
+        s.Equal(42, result)
+    }) 
+}
+```
+
+**Autofix**: false. <br>
+**Enabled by default**: true. <br>
+**Reason**: Protection from undefined behavior.
+
+According to `testify` [documentation](https://pkg.go.dev/github.com/stretchr/testify/suite#Suite.Run), `s.Run` should
+be used for running subtests. This call (among other things) initializes the suite with a fresh instance of `t` and
+protects tests from undefined behavior (such as data races).
+
+Autofix is disabled because in the most cases it requires rewriting the assertions in the subtest and can leads to dead
+code.
+
+The checker is especially useful in combination with [suite-dont-use-pkg](#suite-dont-use-pkg).
+
+---
+
 ### 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)
 }
 ```
 

analyzer/checkers_factory_test.go

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

analyzer/testdata/src/checkers-default/suite-dont-use-pkg/suite_dont_use_pkg_test.go

@@ -46,7 +46,7 @@ func (s *SuiteDontUsePkgCheckerSuite) TestAll() {
 	s.Require().Equal(42, result)                                   // want "suite-dont-use-pkg: use s\\.Require\\(\\)\\.Equal"
 	s.Require().Equalf(42, result, "msg with args %d %s", 42, "42") // want "suite-dont-use-pkg: use s\\.Require\\(\\)\\.Equalf"
 
-	s.T().Run("not detected", func(t *testing.T) {
+	s.T().Run("not detected in order to avoid conflict with suite-subtest-run", func(t *testing.T) {
 		var result any
 		assObj, reqObj := assert.New(t), require.New(t)
 

analyzer/testdata/src/checkers-default/suite-dont-use-pkg/suite_dont_use_pkg_test.go.golden

@@ -0,0 +1,59 @@
+package debug
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/suite"
+)
+
+type SomeSuite struct {
+	suite.Suite
+}
+
+func TestSomeSuite(t *testing.T) {
+	suite.Run(t, new(SomeSuite))
+}
+
+func (s *SomeSuite) BeforeTest(suiteName, testName string) {
+	s.Equal(1, 2)
+
+	s.T().Run("init 1", func(t *testing.T) {
+		s.Require().Equal(1, 2)
+	})
+
+	s.Run("init 2", func() {
+		s.Require().Equal(2, 1)
+	})
+}
+
+func (s *SomeSuite) TestSomething() {
+	s.T().Parallel()
+
+	s.Run("sum", func() {
+		dummy := 3 + 1
+		s.Equal(4, dummy)
+	})
+
+	s.Run("mul", func() {
+		dummy := 3 * 1
+		s.Equal(3, dummy)
+	})
+}
+
+func (s *SomeSuite) TestSomething_ThroughT() {
+	s.T().Parallel()
+
+	s.T().Run("sum", func(t *testing.T) {
+		t.Parallel()
+
+		dummy := 3 + 1
+		s.Equal(4, dummy)
+	})
+
+	s.T().Run("mul", func(t *testing.T) {
+		t.Parallel()
+
+		dummy := 3 * 1
+		s.Equal(3, dummy)
+	})
+}

analyzer/testdata/src/checkers-default/suite-subtest-run/suite_subtest_run_test.go

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

analyzer/testdata/src/debug/suite_run_test.go

@@ -52,6 +52,7 @@ func TestAll(t *testing.T) {
 		"blank-import",
 		"go-require",
 		"require-error",
+		"suite-subtest-run",
 		"suite-thelper",
 	}
 	if !slices.Equal(expected, checkerList) {
@@ -84,6 +85,7 @@ func TestEnabledByDefault(t *testing.T) {
 		"blank-import",
 		"go-require",
 		"require-error",
+		"suite-subtest-run",
 	}
 	if !slices.Equal(expected, checkerList) {
 		t.Fatalf("unexpected list: %#v", checkerList)

internal/checkers/checkers_registry.go

@@ -14,7 +14,7 @@ import (
 var DefaultExpectedVarPattern = regexp.MustCompile(
 	`(^(exp(ected)?|want(ed)?)([A-Z]\w*)?$)|(^(\w*[a-z])?(Exp(ected)?|Want(ed)?)$)`)
 
-// ExpectedActual detects situation like
+// ExpectedActual detects situations like
 //
 //	assert.Equal(t, result, expected)
 //	assert.EqualExportedValues(t, resultObj, User{Name: "Anton"})
@@ -130,9 +130,9 @@ func (checker ExpectedActual) isExpectedValueCandidate(pass *analysis.Pass, expr
 	return isBasicLit(expr) ||
 		isUntypedConst(pass, expr) ||
 		isTypedConst(pass, expr) ||
-		isIdentNamedAsExpected(checker.expVarPattern, expr) ||
-		isStructVarNamedAsExpected(checker.expVarPattern, expr) ||
-		isStructFieldNamedAsExpected(checker.expVarPattern, expr)
+		isIdentNamedAfterPattern(checker.expVarPattern, expr) ||
+		isStructVarNamedAfterPattern(checker.expVarPattern, expr) ||
+		isStructFieldNamedAfterPattern(checker.expVarPattern, expr)
 }
 
 func isParenExpr(ce *ast.CallExpr) bool {
@@ -158,7 +158,7 @@ func isCastedBasicLitOrExpectedValue(ce *ast.CallExpr, pattern *regexp.Regexp) b
 		"int", "int8", "int16", "int32", "int64",
 		"float32", "float64",
 		"rune", "string":
-		return isBasicLit(ce.Args[0]) || isIdentNamedAsExpected(pattern, ce.Args[0])
+		return isBasicLit(ce.Args[0]) || isIdentNamedAfterPattern(pattern, ce.Args[0])
 	}
 	return false
 }