From 9a98e5543d1519b3871a397a36b6872ef5bc5a7c Mon Sep 17 00:00:00 2001 From: Anton Telyshev Date: Thu, 6 Jun 2024 10:56:49 +0300 Subject: [PATCH 1/3] suite-broken-parallel: debug cases --- .../src/debug/suite_broken_parallel_test.go | 190 ++++++++++++++++++ internal/checkers/suite_broken_parallel.go | 1 + 2 files changed, 191 insertions(+) create mode 100644 analyzer/testdata/src/debug/suite_broken_parallel_test.go create mode 100644 internal/checkers/suite_broken_parallel.go diff --git a/analyzer/testdata/src/debug/suite_broken_parallel_test.go b/analyzer/testdata/src/debug/suite_broken_parallel_test.go new file mode 100644 index 0000000..24e7b30 --- /dev/null +++ b/analyzer/testdata/src/debug/suite_broken_parallel_test.go @@ -0,0 +1,190 @@ +package debug + +import ( + "fmt" + "testing" + "time" + + "github.com/stretchr/testify/suite" +) + +// go test -race -run=TestSuiteWithParallelTests suite_broken_parallel_test.go +// +// DATARACE! +func TestSuiteWithParallelTests(t *testing.T) { + suite.Run(t, new(SuiteWithParallelTests)) +} + +type SuiteWithParallelTests struct { + suite.Suite +} + +func (s *SuiteWithParallelTests) TestOne() { + s.T().Parallel() + + for i := 0; i < 100; i++ { + s.GreaterOrEqual(i, 0) + } +} + +func (s *SuiteWithParallelTests) TestTwo() { + s.T().Parallel() + + for i := 0; i < 100; i++ { + s.GreaterOrEqual(i, 0) + } +} + +// go test -race -run=TestSuiteWithParallelSubTests suite_broken_parallel_test.go +// +// NO DATARACE, but s.T().Parallel() not working (18.407s): +// +// TestOne: 0: I am started at 2024-06-06 09:18:56.72165 +// TestOne: 1: I am started at 2024-06-06 09:18:59.72348 +// TestOne: 2: I am started at 2024-06-06 09:19:02.72491 +// TestTwo: 1: I am started at 2024-06-06 09:19:08.72770 +// TestTwo: 2: I am started at 2024-06-06 09:19:11.72916 +// TestTwo: 0: I am started at 2024-06-06 09:19:05.72648 +func TestSuiteWithParallelSubTests(t *testing.T) { + suite.Run(t, new(SuiteWithParallelSubTests)) +} + +type SuiteWithParallelSubTests struct { + suite.Suite +} + +func (s *SuiteWithParallelSubTests) TestOne() { + for i := 0; i <= 2; i++ { + i := i + s.Run(fmt.Sprintf("%d", i), func() { + s.T().Parallel() + + s.T().Logf("%s: %d: I am started at %s", "TestOne", i, time.Now()) + s.GreaterOrEqual(i, 0) + time.Sleep(3 * time.Second) + }) + } +} + +func (s *SuiteWithParallelSubTests) TestTwo() { + for i := 0; i <= 2; i++ { + i := i + s.Run(fmt.Sprintf("%d", i), func() { + s.T().Parallel() + + s.T().Logf("%s: %d: I am started at %s", "TestTwo", i, time.Now()) + s.GreaterOrEqual(i, 0) + time.Sleep(3 * time.Second) + }) + } +} + +// go test -race -run=TestSuiteWithParallelRawTForRunSubTest suite_broken_parallel_test.go +// +// NO DATARACE, but: +// +// panic: testing: t.Parallel called multiple times +func TestSuiteWithParallelRawTForRunSubTest(t *testing.T) { + suite.Run(t, new(SuiteWithParallelRawTForRunSubTest)) +} + +type SuiteWithParallelRawTForRunSubTest struct { + suite.Suite +} + +func (s *SuiteWithParallelRawTForRunSubTest) TestOne() { + for i := 0; i <= 2; i++ { + i := i + s.T().Run(fmt.Sprintf("%d", i), func(t *testing.T) { + s.T().Parallel() + + for i := 0; i < 100; i++ { + s.GreaterOrEqual(i, 0) + } + }) + } +} + +func (s *SuiteWithParallelRawTForRunSubTest) TestTwo() { + for i := 0; i <= 2; i++ { + i := i + s.T().Run(fmt.Sprintf("%d", i), func(t *testing.T) { + s.T().Parallel() + + for i := 0; i < 100; i++ { + s.GreaterOrEqual(i, 0) + } + }) + } +} + +// go test -race -run=TestSuiteWithParallelRawTForRunSubTestAndTParallel suite_broken_parallel_test.go +// +// NO DATARACE, and t.Parallel() working fine (6.279s): +// +// TestOne: 0: I am started at 2024-06-06 09:32:57.42371 +// TestOne: 2: I am started at 2024-06-06 09:32:57.42374 +// TestOne: 1: I am started at 2024-06-06 09:32:57.42375 +// TestTwo: 0: I am started at 2024-06-06 09:33:00.42553 +// TestTwo: 2: I am started at 2024-06-06 09:33:00.42554 +// TestTwo: 1: I am started at 2024-06-06 09:33:00.42563 +func TestSuiteWithParallelRawTForRunSubTestAndTParallel(t *testing.T) { + suite.Run(t, new(SuiteWithParallelRawTForRunSubTestAndTParallel)) +} + +type SuiteWithParallelRawTForRunSubTestAndTParallel struct { + suite.Suite +} + +func (s *SuiteWithParallelRawTForRunSubTestAndTParallel) TestOne() { + for i := 0; i <= 2; i++ { + i := i + s.T().Run(fmt.Sprintf("%d", i), func(t *testing.T) { + t.Parallel() + + s.T().Logf("%s: %d: I am started at %s", "TestOne", i, time.Now()) + s.GreaterOrEqual(i, 0) + time.Sleep(3 * time.Second) + }) + } +} + +func (s *SuiteWithParallelRawTForRunSubTestAndTParallel) TestTwo() { + for i := 0; i <= 2; i++ { + i := i + s.T().Run(fmt.Sprintf("%d", i), func(t *testing.T) { + t.Parallel() + + s.T().Logf("%s: %d: I am started at %s", "TestTwo", i, time.Now()) + s.GreaterOrEqual(i, 0) + time.Sleep(3 * time.Second) + }) + } +} + +// go test -race -run=TestSuiteWithParallelInDifferentRunSubtest suite_broken_parallel_test.go +// +// DATARACE! But fun fact – if s.Run before s.T().Run then everything is ok. +func TestSuiteWithParallelInDifferentRunSubtest(t *testing.T) { + suite.Run(t, new(SuiteWithParallelInDifferentRunSubtest)) +} + +type SuiteWithParallelInDifferentRunSubtest struct { + suite.Suite +} + +func (s *SuiteWithParallelInDifferentRunSubtest) TestOne() { + s.T().Run("2", func(t *testing.T) { + t.Parallel() + + s.GreaterOrEqual(1, 0) + time.Sleep(time.Second) + }) + + s.Run("1", func() { + s.T().Parallel() + + s.GreaterOrEqual(1, 0) + time.Sleep(time.Second) + }) +} diff --git a/internal/checkers/suite_broken_parallel.go b/internal/checkers/suite_broken_parallel.go new file mode 100644 index 0000000..d0ae62b --- /dev/null +++ b/internal/checkers/suite_broken_parallel.go @@ -0,0 +1 @@ +package checkers From f4d3de665e774d4d920739739cb6e4352d34895f Mon Sep 17 00:00:00 2001 From: Anton Telyshev Date: Fri, 7 Jun 2024 22:26:02 +0300 Subject: [PATCH 2/3] new checker 'suite-broken-parallel' --- README.md | 57 +++++++- analyzer/checkers_factory_test.go | 2 + .../suite_broken_parallel_test.go | 92 ++++++++++++ .../suite_broken_parallel_test.go.golden | 83 +++++++++++ .../src/debug/suite_broken_parallel_test.go | 85 ++++++++--- internal/checkers/checkers_registry.go | 1 + internal/checkers/checkers_registry_test.go | 2 + internal/checkers/suite_broken_parallel.go | 88 ++++++++++++ internal/testgen/gen_suite_broken_parallel.go | 136 ++++++++++++++++++ internal/testgen/gen_suite_dont_use_pkg.go | 14 +- internal/testgen/main.go | 3 +- 11 files changed, 533 insertions(+), 30 deletions(-) create mode 100644 analyzer/testdata/src/checkers-default/suite-broken-parallel/suite_broken_parallel_test.go create mode 100644 analyzer/testdata/src/checkers-default/suite-broken-parallel/suite_broken_parallel_test.go.golden create mode 100644 internal/testgen/gen_suite_broken_parallel.go diff --git a/README.md b/README.md index 8e33f2e..b57ae1d 100644 --- a/README.md +++ b/README.md @@ -103,6 +103,7 @@ https://golangci-lint.run/usage/linters/#testifylint | [negative-positive](#negative-positive) | ✅ | ✅ | | [nil-compare](#nil-compare) | ✅ | ✅ | | [require-error](#require-error) | ✅ | ❌ | +| [suite-broken-parallel](#suite-broken-parallel) | ✅ | ✅ | | [suite-dont-use-pkg](#suite-dont-use-pkg) | ✅ | ✅ | | [suite-extra-assert-call](#suite-extra-assert-call) | ✅ | ✅ | | [suite-subtest-run](#suite-subtest-run) | ✅ | ❌ | @@ -667,11 +668,63 @@ Also, to minimize false positives, `require-error` ignores: --- -### suite-dont-use-pkg +### suite-broken-parallel ```go -import "github.com/stretchr/testify/assert" +func (s *MySuite) SetupTest() { + s.T().Parallel() ❌ +} + +func (s *MySuite) BeforeTest(suiteName, testName string) { + s.T().Parallel() ❌ +} + +func (s *MySuite) TestSomething() { + s.T().Parallel() ❌ + + for _, tt := range cases { + s.Run(tt.name, func() { + s.T().Parallel() ❌ + }) + + s.T().Run(tt.name, func(t *testing.T) { + t.Parallel() ❌ + }) + } +} +``` + +**Autofix**: true.
+**Enabled by default**: true.
+**Reason**: Protection from undefined behaviour. + +`v1` of `testify` doesn't support suite's parallel tests and subtests. +Depending on [case](./analyzer/testdata/src/debug/suite_broken_parallel_test.go) using of `t.Parallel()` leads to + +- data race +- panic +- non-working suite hooks +- silent ignoring of this directive + +So, `testify`'s maintainers recommend discourage parallel tests inside suite. + +
+Related issues... + +- https://github.com/stretchr/testify/issues/187 +- https://github.com/stretchr/testify/issues/466 +- https://github.com/stretchr/testify/issues/934 +- https://github.com/stretchr/testify/issues/1139 +- https://github.com/stretchr/testify/issues/1253 + +
+ +--- + +### suite-dont-use-pkg + +```go func (s *MySuite) TestSomething() { ❌ assert.Equal(s.T(), 42, value) ✅ s.Equal(42, value) diff --git a/analyzer/checkers_factory_test.go b/analyzer/checkers_factory_test.go index 2f603a0..a061305 100644 --- a/analyzer/checkers_factory_test.go +++ b/analyzer/checkers_factory_test.go @@ -51,12 +51,14 @@ func Test_newCheckers(t *testing.T) { checkers.NewBlankImport(), checkers.NewGoRequire(), checkers.NewRequireError(), + checkers.NewSuiteBrokenParallel(), checkers.NewSuiteSubtestRun(), } allAdvancedCheckers := []checkers.AdvancedChecker{ checkers.NewBlankImport(), checkers.NewGoRequire(), checkers.NewRequireError(), + checkers.NewSuiteBrokenParallel(), checkers.NewSuiteSubtestRun(), checkers.NewSuiteTHelper(), } diff --git a/analyzer/testdata/src/checkers-default/suite-broken-parallel/suite_broken_parallel_test.go b/analyzer/testdata/src/checkers-default/suite-broken-parallel/suite_broken_parallel_test.go new file mode 100644 index 0000000..e2adb43 --- /dev/null +++ b/analyzer/testdata/src/checkers-default/suite-broken-parallel/suite_broken_parallel_test.go @@ -0,0 +1,92 @@ +// Code generated by testifylint/internal/testgen. DO NOT EDIT. + +package suitebrokenparallel + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/suite" +) + +type SuiteBrokenParallelCheckerSuite struct { + suite.Suite +} + +func TestSuiteBrokenParallelCheckerSuite(t *testing.T) { + t.Parallel() + suite.Run(t, new(SuiteBrokenParallelCheckerSuite)) +} + +func (s *SuiteBrokenParallelCheckerSuite) BeforeTest(_, _ string) { + s.T().Parallel() // want "suite-broken-parallel: testify v1 does not support suite's parallel tests and subtests" +} + +func (s *SuiteBrokenParallelCheckerSuite) SetupTest() { + s.T().Parallel() // want "suite-broken-parallel: testify v1 does not support suite's parallel tests and subtests" +} + +func (s *SuiteBrokenParallelCheckerSuite) TestAll() { + s.T().Parallel() // want "suite-broken-parallel: testify v1 does not support suite's parallel tests and subtests" + + s.Run("", func() { + s.T().Parallel() // want "suite-broken-parallel: testify v1 does not support suite's parallel tests and subtests" + s.Equal(1, 2) + }) + + s.T().Run("", func(t *testing.T) { + t.Parallel() // want "suite-broken-parallel: testify v1 does not support suite's parallel tests and subtests" + assert.Equal(t, 1, 2) + }) + + s.Run("", func() { + s.Equal(1, 2) + + s.Run("", func() { + s.Equal(1, 2) + + s.Run("", func() { + s.T().Parallel() // want "suite-broken-parallel: testify v1 does not support suite's parallel tests and subtests" + s.Equal(1, 2) + }) + + s.T().Run("", func(t *testing.T) { + t.Parallel() // want "suite-broken-parallel: testify v1 does not support suite's parallel tests and subtests" + assert.Equal(t, 1, 2) + }) + }) + }) +} + +func (s *SuiteBrokenParallelCheckerSuite) TestTable() { + cases := []struct{ Name string }{} + + for _, tt := range cases { + tt := tt + s.T().Run(tt.Name, func(t *testing.T) { + t.Parallel() // want "suite-broken-parallel: testify v1 does not support suite's parallel tests and subtests" + s.Equal(1, 2) + }) + } + + for _, tt := range cases { + tt := tt + s.Run(tt.Name, func() { + s.T().Parallel() // want "suite-broken-parallel: testify v1 does not support suite's parallel tests and subtests" + s.Equal(1, 2) + }) + } +} + +func TestSimpleTable(t *testing.T) { + t.Parallel() + + cases := []struct{ Name string }{} + for _, tt := range cases { + tt := tt + t.Run(tt.Name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, 1, 2) + }) + } +} diff --git a/analyzer/testdata/src/checkers-default/suite-broken-parallel/suite_broken_parallel_test.go.golden b/analyzer/testdata/src/checkers-default/suite-broken-parallel/suite_broken_parallel_test.go.golden new file mode 100644 index 0000000..ed68e64 --- /dev/null +++ b/analyzer/testdata/src/checkers-default/suite-broken-parallel/suite_broken_parallel_test.go.golden @@ -0,0 +1,83 @@ +// Code generated by testifylint/internal/testgen. DO NOT EDIT. + +package suitebrokenparallel + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/suite" +) + +type SuiteBrokenParallelCheckerSuite struct { + suite.Suite +} + +func TestSuiteBrokenParallelCheckerSuite(t *testing.T) { + t.Parallel() + suite.Run(t, new(SuiteBrokenParallelCheckerSuite)) +} + +func (s *SuiteBrokenParallelCheckerSuite) BeforeTest(_, _ string) { +} + +func (s *SuiteBrokenParallelCheckerSuite) SetupTest() { +} + +func (s *SuiteBrokenParallelCheckerSuite) TestAll() { + + s.Run("", func() { + s.Equal(1, 2) + }) + + s.T().Run("", func(t *testing.T) { + assert.Equal(t, 1, 2) + }) + + s.Run("", func() { + s.Equal(1, 2) + + s.Run("", func() { + s.Equal(1, 2) + + s.Run("", func() { + s.Equal(1, 2) + }) + + s.T().Run("", func(t *testing.T) { + assert.Equal(t, 1, 2) + }) + }) + }) +} + +func (s *SuiteBrokenParallelCheckerSuite) TestTable() { + cases := []struct{ Name string }{} + + for _, tt := range cases { + tt := tt + s.T().Run(tt.Name, func(t *testing.T) { + s.Equal(1, 2) + }) + } + + for _, tt := range cases { + tt := tt + s.Run(tt.Name, func() { + s.Equal(1, 2) + }) + } +} + +func TestSimpleTable(t *testing.T) { + t.Parallel() + + cases := []struct{ Name string }{} + for _, tt := range cases { + tt := tt + t.Run(tt.Name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, 1, 2) + }) + } +} diff --git a/analyzer/testdata/src/debug/suite_broken_parallel_test.go b/analyzer/testdata/src/debug/suite_broken_parallel_test.go index 24e7b30..37dd404 100644 --- a/analyzer/testdata/src/debug/suite_broken_parallel_test.go +++ b/analyzer/testdata/src/debug/suite_broken_parallel_test.go @@ -37,7 +37,7 @@ func (s *SuiteWithParallelTests) TestTwo() { // go test -race -run=TestSuiteWithParallelSubTests suite_broken_parallel_test.go // -// NO DATARACE, but s.T().Parallel() not working (18.407s): +// NO DATARACE, but s.T().Parallel() for subtests not working (18.407s): // // TestOne: 0: I am started at 2024-06-06 09:18:56.72165 // TestOne: 1: I am started at 2024-06-06 09:18:59.72348 @@ -79,6 +79,41 @@ func (s *SuiteWithParallelSubTests) TestTwo() { } } +// go test -race -run=TestSuiteWithParallelThroughSetupTest suite_broken_parallel_test.go +// +// DATARACE! +func TestSuiteWithParallelThroughSetupTest(t *testing.T) { + suite.Run(t, new(SuiteWithParallelThroughSetupTest)) +} + +type SuiteWithParallelThroughSetupTest struct { + suite.Suite +} + +func (s *SuiteWithParallelThroughSetupTest) SetupTest() { + s.T().Parallel() +} + +func (s *SuiteWithParallelThroughSetupTest) TestOne() { + for i := 0; i <= 2; i++ { + s.Run(fmt.Sprintf("%d", i), func() { + s.T().Logf("%s: %d: I am started at %s", "TestOne", i, time.Now()) + s.GreaterOrEqual(i, 0) + time.Sleep(3 * time.Second) + }) + } +} + +func (s *SuiteWithParallelThroughSetupTest) TestTwo() { + for i := 0; i <= 2; i++ { + s.Run(fmt.Sprintf("%d", i), func() { + s.T().Logf("%s: %d: I am started at %s", "TestTwo", i, time.Now()) + s.GreaterOrEqual(i, 0) + time.Sleep(3 * time.Second) + }) + } +} + // go test -race -run=TestSuiteWithParallelRawTForRunSubTest suite_broken_parallel_test.go // // NO DATARACE, but: @@ -98,9 +133,9 @@ func (s *SuiteWithParallelRawTForRunSubTest) TestOne() { s.T().Run(fmt.Sprintf("%d", i), func(t *testing.T) { s.T().Parallel() - for i := 0; i < 100; i++ { - s.GreaterOrEqual(i, 0) - } + s.T().Logf("%s: %d: I am started at %s", "TestOne", i, time.Now()) + s.GreaterOrEqual(i, 0) + time.Sleep(3 * time.Second) }) } } @@ -111,9 +146,9 @@ func (s *SuiteWithParallelRawTForRunSubTest) TestTwo() { s.T().Run(fmt.Sprintf("%d", i), func(t *testing.T) { s.T().Parallel() - for i := 0; i < 100; i++ { - s.GreaterOrEqual(i, 0) - } + s.T().Logf("%s: %d: I am started at %s", "TestOne", i, time.Now()) + s.GreaterOrEqual(i, 0) + time.Sleep(3 * time.Second) }) } } @@ -122,12 +157,16 @@ func (s *SuiteWithParallelRawTForRunSubTest) TestTwo() { // // NO DATARACE, and t.Parallel() working fine (6.279s): // -// TestOne: 0: I am started at 2024-06-06 09:32:57.42371 -// TestOne: 2: I am started at 2024-06-06 09:32:57.42374 -// TestOne: 1: I am started at 2024-06-06 09:32:57.42375 -// TestTwo: 0: I am started at 2024-06-06 09:33:00.42553 -// TestTwo: 2: I am started at 2024-06-06 09:33:00.42554 -// TestTwo: 1: I am started at 2024-06-06 09:33:00.42563 +// AfterTest of TestOne: I am started at 2024-06-07 08:29:54.81907 +// TestOne: 0: I am started at 2024-06-07 08:29:54.81969 +// TestOne: 2: I am started at 2024-06-07 08:29:54.81982 +// TestOne: 1: I am started at 2024-06-07 08:29:54.81981 +// AfterTest of TestTwo: I am started at 2024-06-07 08:29:57.82127 +// TestTwo: 0: I am started at 2024-06-07 08:29:57.82130 +// TestTwo: 2: I am started at 2024-06-07 08:29:57.82132 +// TestTwo: 1: I am started at 2024-06-07 08:29:57.82132 +// +// But AfterTest (and other hooks) don't work correctly. func TestSuiteWithParallelRawTForRunSubTestAndTParallel(t *testing.T) { suite.Run(t, new(SuiteWithParallelRawTForRunSubTestAndTParallel)) } @@ -136,6 +175,10 @@ type SuiteWithParallelRawTForRunSubTestAndTParallel struct { suite.Suite } +func (s *SuiteWithParallelRawTForRunSubTestAndTParallel) AfterTest(_, testName string) { + s.T().Logf("AfterTest of %s: I am started at %s", testName, time.Now()) +} + func (s *SuiteWithParallelRawTForRunSubTestAndTParallel) TestOne() { for i := 0; i <= 2; i++ { i := i @@ -164,7 +207,7 @@ func (s *SuiteWithParallelRawTForRunSubTestAndTParallel) TestTwo() { // go test -race -run=TestSuiteWithParallelInDifferentRunSubtest suite_broken_parallel_test.go // -// DATARACE! But fun fact – if s.Run before s.T().Run then everything is ok. +// DATARACE! But difficult to catch (try several times to see it). func TestSuiteWithParallelInDifferentRunSubtest(t *testing.T) { suite.Run(t, new(SuiteWithParallelInDifferentRunSubtest)) } @@ -174,17 +217,19 @@ type SuiteWithParallelInDifferentRunSubtest struct { } func (s *SuiteWithParallelInDifferentRunSubtest) TestOne() { - s.T().Run("2", func(t *testing.T) { + s.T().Run("1", func(t *testing.T) { t.Parallel() - s.GreaterOrEqual(1, 0) - time.Sleep(time.Second) + for i := 0; i < 100; i++ { + s.GreaterOrEqual(i, 0) + } }) - s.Run("1", func() { + s.Run("2", func() { s.T().Parallel() - s.GreaterOrEqual(1, 0) - time.Sleep(time.Second) + for i := 0; i < 100; i++ { + s.GreaterOrEqual(i, 0) + } }) } diff --git a/internal/checkers/checkers_registry.go b/internal/checkers/checkers_registry.go index d2b3710..17c7d14 100644 --- a/internal/checkers/checkers_registry.go +++ b/internal/checkers/checkers_registry.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(NewSuiteBrokenParallel), enabledByDefault: true}, {factory: asCheckerFactory(NewSuiteSubtestRun), enabledByDefault: true}, {factory: asCheckerFactory(NewSuiteTHelper), enabledByDefault: false}, } diff --git a/internal/checkers/checkers_registry_test.go b/internal/checkers/checkers_registry_test.go index 3574ff8..40eac8a 100644 --- a/internal/checkers/checkers_registry_test.go +++ b/internal/checkers/checkers_registry_test.go @@ -52,6 +52,7 @@ func TestAll(t *testing.T) { "blank-import", "go-require", "require-error", + "suite-broken-parallel", "suite-subtest-run", "suite-thelper", } @@ -85,6 +86,7 @@ func TestEnabledByDefault(t *testing.T) { "blank-import", "go-require", "require-error", + "suite-broken-parallel", "suite-subtest-run", } if !slices.Equal(expected, checkerList) { diff --git a/internal/checkers/suite_broken_parallel.go b/internal/checkers/suite_broken_parallel.go index d0ae62b..5cf99f9 100644 --- a/internal/checkers/suite_broken_parallel.go +++ b/internal/checkers/suite_broken_parallel.go @@ -1 +1,89 @@ 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" +) + +// SuiteBrokenParallel detects unsupported t.Parallel() call in suite tests +// +// func (s *MySuite) SetupTest() { +// s.T().Parallel() +// } +// +// func (s *MySuite) BeforeTest(suiteName, testName string) { +// s.T().Parallel() +// } +// +// func (s *MySuite) TestSomething() { +// s.T().Parallel() +// +// for _, tt := range cases { +// s.Run(tt.name, func() { +// s.T().Parallel() +// }) +// +// s.T().Run(tt.name, func(t *testing.T) { +// t.Parallel() +// }) +// } +// } +type SuiteBrokenParallel struct{} + +// NewSuiteBrokenParallel constructs SuiteBrokenParallel checker. +func NewSuiteBrokenParallel() SuiteBrokenParallel { return SuiteBrokenParallel{} } +func (SuiteBrokenParallel) Name() string { return "suite-broken-parallel" } + +func (checker SuiteBrokenParallel) Check(pass *analysis.Pass, insp *inspector.Inspector) (diagnostics []analysis.Diagnostic) { + const report = "testify v1 does not support suite's parallel tests and subtests" + + insp.WithStack([]ast.Node{(*ast.CallExpr)(nil)}, func(node ast.Node, push bool, stack []ast.Node) bool { + if !push { + return false + } + ce := node.(*ast.CallExpr) + + se, ok := ce.Fun.(*ast.SelectorExpr) + if !ok { + return true + } + if !isIdentWithName("Parallel", se.Sel) { + return true + } + if !implementsTestingT(pass, se.X) { + return true + } + + for i := len(stack) - 2; i >= 0; i-- { + fd, ok := stack[i].(*ast.FuncDecl) + if !ok { + continue + } + + if isSuiteMethod(pass, fd) { + nextLine := pass.Fset.Position(ce.Pos()).Line + 1 + d := newDiagnostic(checker.Name(), ce, report, &analysis.SuggestedFix{ + Message: fmt.Sprintf("Remove `%s` call", analysisutil.NodeString(pass.Fset, ce)), + TextEdits: []analysis.TextEdit{ + { + Pos: ce.Pos(), + End: pass.Fset.File(ce.Pos()).LineStart(nextLine), + NewText: []byte(""), + }, + }, + }) + + diagnostics = append(diagnostics, *d) + } + return true + } + + return true + }) + return diagnostics +} diff --git a/internal/testgen/gen_suite_broken_parallel.go b/internal/testgen/gen_suite_broken_parallel.go new file mode 100644 index 0000000..71abf91 --- /dev/null +++ b/internal/testgen/gen_suite_broken_parallel.go @@ -0,0 +1,136 @@ +package main + +import ( + "regexp" + "text/template" + + "github.com/Antonboom/testifylint/internal/checkers" +) + +type SuiteBrokenParallelTestsGenerator struct{} + +func (SuiteBrokenParallelTestsGenerator) Checker() checkers.Checker { + return checkers.NewSuiteBrokenParallel() +} + +func (g SuiteBrokenParallelTestsGenerator) TemplateData() any { + var ( + checker = g.Checker().Name() + report = QuoteReport(checker + ": testify v1 does not support suite's parallel tests and subtests") + ) + + return struct { + CheckerName CheckerName + Report string + }{ + CheckerName: CheckerName(checker), + Report: report, + } +} + +func (SuiteBrokenParallelTestsGenerator) ErroredTemplate() Executor { + return template.Must(template.New("SuiteBrokenParallelTestsGenerator.ErroredTemplate"). + Parse(suiteBrokenParallelTestTmpl)) +} + +func (SuiteBrokenParallelTestsGenerator) GoldenTemplate() Executor { + parallelRe := regexp.MustCompile(`\n.*(s\.T\(\)|t)\.Parallel\(\) // want \{\{ \$\.Report }}`) + return template.Must(template.New("SuiteBrokenParallelTestsGenerator.GoldenTemplate"). + Parse(parallelRe.ReplaceAllString(suiteBrokenParallelTestTmpl, ""))) +} + +const suiteBrokenParallelTestTmpl = header + ` + +package {{ .CheckerName.AsPkgName }} + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/suite" +) + +{{ $suiteName := .CheckerName.AsSuiteName }} + +type {{ $suiteName }} struct { + suite.Suite +} + +func Test{{ $suiteName }}(t *testing.T) { + t.Parallel() + suite.Run(t, new({{ $suiteName }})) +} + +func (s *SuiteBrokenParallelCheckerSuite) BeforeTest(_, _ string) { + s.T().Parallel() // want {{ $.Report }} +} + +func (s *{{ $suiteName }}) SetupTest() { + s.T().Parallel() // want {{ $.Report }} +} + +func (s *{{ $suiteName }}) TestAll() { + s.T().Parallel() // want {{ $.Report }} + + s.Run("", func() { + s.T().Parallel() // want {{ $.Report }} + s.Equal(1, 2) + }) + + s.T().Run("", func(t *testing.T) { + t.Parallel() // want {{ $.Report }} + assert.Equal(t, 1, 2) + }) + + s.Run("", func() { + s.Equal(1, 2) + + s.Run("", func() { + s.Equal(1, 2) + + s.Run("", func() { + s.T().Parallel() // want {{ $.Report }} + s.Equal(1, 2) + }) + + s.T().Run("", func(t *testing.T) { + t.Parallel() // want {{ $.Report }} + assert.Equal(t, 1, 2) + }) + }) + }) +} + +func (s *{{ $suiteName }}) TestTable() { + cases := []struct{ Name string }{} + + for _, tt := range cases { + tt := tt + s.T().Run(tt.Name, func(t *testing.T) { + t.Parallel() // want {{ $.Report }} + s.Equal(1, 2) + }) + } + + for _, tt := range cases { + tt := tt + s.Run(tt.Name, func() { + s.T().Parallel() // want {{ $.Report }} + s.Equal(1, 2) + }) + } +} + +func TestSimpleTable(t *testing.T) { + t.Parallel() + + cases := []struct{ Name string }{} + for _, tt := range cases { + tt := tt + t.Run(tt.Name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, 1, 2) + }) + } +} +` diff --git a/internal/testgen/gen_suite_dont_use_pkg.go b/internal/testgen/gen_suite_dont_use_pkg.go index b4b4a55..0c3e676 100644 --- a/internal/testgen/gen_suite_dont_use_pkg.go +++ b/internal/testgen/gen_suite_dont_use_pkg.go @@ -7,13 +7,13 @@ import ( "github.com/Antonboom/testifylint/internal/checkers" ) -type SuiteDontUsePkg struct{} +type SuiteDontUsePkgTestsGenerator struct{} -func (SuiteDontUsePkg) Checker() checkers.Checker { +func (SuiteDontUsePkgTestsGenerator) Checker() checkers.Checker { return checkers.NewSuiteDontUsePkg() } -func (g SuiteDontUsePkg) TemplateData() any { +func (g SuiteDontUsePkgTestsGenerator) TemplateData() any { var ( checker = g.Checker().Name() report = checker + ": use %s.%s" @@ -30,16 +30,16 @@ func (g SuiteDontUsePkg) TemplateData() any { } } -func (SuiteDontUsePkg) ErroredTemplate() Executor { - return template.Must(template.New("SuiteDontUsePkg.ErroredTemplate"). +func (SuiteDontUsePkgTestsGenerator) ErroredTemplate() Executor { + return template.Must(template.New("SuiteDontUsePkgTestsGenerator.ErroredTemplate"). Funcs(fm). Parse(suiteDontUsePkgTestTmpl)) } -func (SuiteDontUsePkg) GoldenTemplate() Executor { +func (SuiteDontUsePkgTestsGenerator) GoldenTemplate() Executor { golden := strings.ReplaceAll(suiteDontUsePkgTestTmpl, "NewAssertionExpander", "NewAssertionExpander.AsGolden") golden = strings.Replace(golden, "s.T()", "", 4) - return template.Must(template.New("SuiteDontUsePkg.GoldenTemplate").Funcs(fm).Parse(golden)) + return template.Must(template.New("SuiteDontUsePkgTestsGenerator.GoldenTemplate").Funcs(fm).Parse(golden)) } const suiteDontUsePkgTestTmpl = header + ` diff --git a/internal/testgen/main.go b/internal/testgen/main.go index 1d92c67..d41dbba 100644 --- a/internal/testgen/main.go +++ b/internal/testgen/main.go @@ -38,7 +38,8 @@ var checkerTestsGenerators = []CheckerTestsGenerator{ NegativePositiveTestsGenerator{}, NilCompareTestsGenerator{}, RequireErrorTestsGenerator{}, - SuiteDontUsePkg{}, + SuiteBrokenParallelTestsGenerator{}, + SuiteDontUsePkgTestsGenerator{}, SuiteExtraAssertCallTestsGenerator{}, SuiteSubtestRunTestsGenerator{}, SuiteTHelperTestsGenerator{}, From 4966cc4463545a0862afe49e0bb48fb586f524e2 Mon Sep 17 00:00:00 2001 From: Anton Telyshev Date: Sat, 8 Jun 2024 21:46:26 +0300 Subject: [PATCH 3/3] suite-broken-parallel: self-review --- README.md | 4 +-- internal/checkers/suite_broken_parallel.go | 34 +++++++++++----------- 2 files changed, 18 insertions(+), 20 deletions(-) diff --git a/README.md b/README.md index b57ae1d..9555334 100644 --- a/README.md +++ b/README.md @@ -675,9 +675,7 @@ func (s *MySuite) SetupTest() { s.T().Parallel() ❌ } -func (s *MySuite) BeforeTest(suiteName, testName string) { - s.T().Parallel() ❌ -} +// And other hooks... func (s *MySuite) TestSomething() { s.T().Parallel() ❌ diff --git a/internal/checkers/suite_broken_parallel.go b/internal/checkers/suite_broken_parallel.go index 5cf99f9..f830fd2 100644 --- a/internal/checkers/suite_broken_parallel.go +++ b/internal/checkers/suite_broken_parallel.go @@ -16,9 +16,7 @@ import ( // s.T().Parallel() // } // -// func (s *MySuite) BeforeTest(suiteName, testName string) { -// s.T().Parallel() -// } +// // And other hooks... // // func (s *MySuite) TestSomething() { // s.T().Parallel() @@ -65,22 +63,24 @@ func (checker SuiteBrokenParallel) Check(pass *analysis.Pass, insp *inspector.In continue } - if isSuiteMethod(pass, fd) { - nextLine := pass.Fset.Position(ce.Pos()).Line + 1 - d := newDiagnostic(checker.Name(), ce, report, &analysis.SuggestedFix{ - Message: fmt.Sprintf("Remove `%s` call", analysisutil.NodeString(pass.Fset, ce)), - TextEdits: []analysis.TextEdit{ - { - Pos: ce.Pos(), - End: pass.Fset.File(ce.Pos()).LineStart(nextLine), - NewText: []byte(""), - }, + if !isSuiteMethod(pass, fd) { + continue + } + + nextLine := pass.Fset.Position(ce.Pos()).Line + 1 + d := newDiagnostic(checker.Name(), ce, report, &analysis.SuggestedFix{ + Message: fmt.Sprintf("Remove `%s` call", analysisutil.NodeString(pass.Fset, ce)), + TextEdits: []analysis.TextEdit{ + { + Pos: ce.Pos(), + End: pass.Fset.File(ce.Pos()).LineStart(nextLine), + NewText: []byte(""), }, - }) + }, + }) - diagnostics = append(diagnostics, *d) - } - return true + diagnostics = append(diagnostics, *d) + return false } return true