search

stretchr / testify

A toolkit with common assertions and mocks that plays nicely with the standard library
MIT License
22.5k stars 1.56k forks source link

suite.Run() triggers data race errors with parallel tests #1139

Open bstpierre opened 2 years ago

bstpierre commented 2 years ago

Running suite.Run() when test cases have called suite.T().Parallel() triggers a data race warning when using go test -race.

==> go.mod <==

module testify-race

go 1.17

require (
        github.com/davecgh/go-spew v1.1.0 // indirect
        github.com/pmezard/go-difflib v1.0.0 // indirect
        github.com/stretchr/objx v0.1.0 // indirect
        github.com/stretchr/testify v1.7.0 // indirect
        gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c // indirect
)

==> main.go <==

package main

import "fmt"

func add(x, y int) int {
        return x + y
}

func main() {
        fmt.Println("hello")
}

==> main_test.go <==

package main

import (
        "testing"

        "github.com/stretchr/testify/suite"
)

type MainSuite struct {
        suite.Suite
}

func (s *MainSuite) TestA() {
        s.T().Parallel()
        for x := 0; x < 100; x++ {
                s.Equal(x, add(x, 0))
        }
}

func (s *MainSuite) TestB() {
        s.T().Parallel()
        for x := 0; x < 100; x++ {
                s.Equal(x+1, add(x, 1))
        }
}

func Test_Main(t *testing.T) {
        suite.Run(t, new(MainSuite))
}

Running the test with the race detector:

$ go test -race .
==================                                                                                              
WARNING: DATA RACE        
Read at 0x00c0000b6138 by goroutine 9:            
  testify-race.(*MainSuite).TestB()
      /tmp/testify-bugreport/main_test.go:23 +0x73                                                              
  runtime.call16()                                                                                              
      /home/brian/sdk/go1.17.2/src/runtime/asm_amd64.s:625 +0x48                                     
  reflect.Value.Call()             
      /home/brian/sdk/go1.17.2/src/reflect/value.go:339 +0xd7
  github.com/stretchr/testify/suite.Run.func1()
      /home/brian/go/pkg/mod/github.com/stretchr/testify@v1.7.0/suite/suite.go:158 +0x71c
  testing.tRunner()   
      /home/brian/sdk/go1.17.2/src/testing/testing.go:1259 +0x22f
  testing.(*T).Run·dwrap·21()                  
      /home/brian/sdk/go1.17.2/src/testing/testing.go:1306 +0x47                                                                                                                                                                 

Previous write at 0x00c0000b6138 by goroutine 8:
  github.com/stretchr/testify/suite.(*Suite).SetT()
      /home/brian/go/pkg/mod/github.com/stretchr/testify@v1.7.0/suite/suite.go:36 +0xc4
  testify-race.(*MainSuite).SetT()
      <autogenerated>:1 +0x44
  github.com/stretchr/testify/suite.Run.func1.1()
      /home/brian/go/pkg/mod/github.com/stretchr/testify@v1.7.0/suite/suite.go:144 +0x2f4
  github.com/stretchr/testify/suite.Run.func1()
      /home/brian/go/pkg/mod/github.com/stretchr/testify@v1.7.0/suite/suite.go:159 +0x71f
  testing.tRunner()
      /home/brian/sdk/go1.17.2/src/testing/testing.go:1259 +0x22f
  testing.(*T).Run·dwrap·21()
      /home/brian/sdk/go1.17.2/src/testing/testing.go:1306 +0x47

Goroutine 9 (running) created at:
  testing.(*T).Run()
      /home/brian/sdk/go1.17.2/src/testing/testing.go:1306 +0x726
  github.com/stretchr/testify/suite.runTests()
      /home/brian/go/pkg/mod/github.com/stretchr/testify@v1.7.0/suite/suite.go:203 +0x18f
  github.com/stretchr/testify/suite.Run()
      /home/brian/go/pkg/mod/github.com/stretchr/testify@v1.7.0/suite/suite.go:176 +0x994
  testify-race.Test_Main()
      /tmp/testify-bugreport/main_test.go:28 +0x44
  testing.tRunner()
      /home/brian/sdk/go1.17.2/src/testing/testing.go:1259 +0x22f
  testing.(*T).Run·dwrap·21()
      /home/brian/sdk/go1.17.2/src/testing/testing.go:1306 +0x47

Goroutine 8 (finished) created at:
  testing.(*T).Run()
      /home/brian/sdk/go1.17.2/src/testing/testing.go:1306 +0x726
  github.com/stretchr/testify/suite.runTests()
      /home/brian/go/pkg/mod/github.com/stretchr/testify@v1.7.0/suite/suite.go:203 +0x18f
  github.com/stretchr/testify/suite.Run()
      /home/brian/go/pkg/mod/github.com/stretchr/testify@v1.7.0/suite/suite.go:176 +0x994
  testify-race.Test_Main()
      /tmp/testify-bugreport/main_test.go:28 +0x44
  testing.tRunner()
      /home/brian/sdk/go1.17.2/src/testing/testing.go:1259 +0x22f
  testing.(*T).Run·dwrap·21()
      /home/brian/sdk/go1.17.2/src/testing/testing.go:1306 +0x47
==================
==================
WARNING: DATA RACE
Read at 0x00c000233a40 by goroutine 9:
  github.com/stretchr/testify/assert.(*Assertions).Equal()
      /home/brian/go/pkg/mod/github.com/stretchr/testify@v1.7.0/assert/assertion_forward.go:128 +0x64
  testify-race.(*MainSuite).TestB()
      /tmp/testify-bugreport/main_test.go:23 +0x9d
  runtime.call16()
      /home/brian/sdk/go1.17.2/src/runtime/asm_amd64.s:625 +0x48
  reflect.Value.Call()
      /home/brian/sdk/go1.17.2/src/reflect/value.go:339 +0xd7
  github.com/stretchr/testify/suite.Run.func1()
      /home/brian/go/pkg/mod/github.com/stretchr/testify@v1.7.0/suite/suite.go:158 +0x71c
  testing.tRunner()
      /home/brian/sdk/go1.17.2/src/testing/testing.go:1259 +0x22f
  testing.(*T).Run·dwrap·21()
      /home/brian/sdk/go1.17.2/src/testing/testing.go:1306 +0x47

Previous write at 0x00c000233a40 by goroutine 8:
  github.com/stretchr/testify/assert.New()
      /home/brian/go/pkg/mod/github.com/stretchr/testify@v1.7.0/assert/forward_assertions.go:12 +0x84
  github.com/stretchr/testify/suite.(*Suite).SetT()
      /home/brian/go/pkg/mod/github.com/stretchr/testify@v1.7.0/suite/suite.go:36 +0xb6
  testify-race.(*MainSuite).SetT()
      <autogenerated>:1 +0x44
  github.com/stretchr/testify/suite.Run.func1.1()
      /home/brian/go/pkg/mod/github.com/stretchr/testify@v1.7.0/suite/suite.go:144 +0x2f4
  github.com/stretchr/testify/suite.Run.func1()
      /home/brian/go/pkg/mod/github.com/stretchr/testify@v1.7.0/suite/suite.go:159 +0x71f
  testing.tRunner()
      /home/brian/sdk/go1.17.2/src/testing/testing.go:1259 +0x22f
  testing.(*T).Run·dwrap·21()
      /home/brian/sdk/go1.17.2/src/testing/testing.go:1306 +0x47

Goroutine 9 (running) created at:
  testing.(*T).Run()
      /home/brian/sdk/go1.17.2/src/testing/testing.go:1306 +0x726
  github.com/stretchr/testify/suite.runTests()
      /home/brian/go/pkg/mod/github.com/stretchr/testify@v1.7.0/suite/suite.go:203 +0x18f
  github.com/stretchr/testify/suite.Run()
      /home/brian/go/pkg/mod/github.com/stretchr/testify@v1.7.0/suite/suite.go:176 +0x994
  testify-race.Test_Main()
      /tmp/testify-bugreport/main_test.go:28 +0x44
  testing.tRunner()
      /home/brian/sdk/go1.17.2/src/testing/testing.go:1259 +0x22f
  testing.(*T).Run·dwrap·21()
      /home/brian/sdk/go1.17.2/src/testing/testing.go:1306 +0x47

Goroutine 8 (finished) created at:
  testing.(*T).Run()
      /home/brian/sdk/go1.17.2/src/testing/testing.go:1306 +0x726
  github.com/stretchr/testify/suite.runTests()
      /home/brian/go/pkg/mod/github.com/stretchr/testify@v1.7.0/suite/suite.go:203 +0x18f
  github.com/stretchr/testify/suite.Run()
      /home/brian/go/pkg/mod/github.com/stretchr/testify@v1.7.0/suite/suite.go:176 +0x994
  testify-race.Test_Main()
      /tmp/testify-bugreport/main_test.go:28 +0x44
  testing.tRunner()
      /home/brian/sdk/go1.17.2/src/testing/testing.go:1259 +0x22f
  testing.(*T).Run·dwrap·21()
      /home/brian/sdk/go1.17.2/src/testing/testing.go:1306 +0x47
==================
--- FAIL: Test_Main (0.01s)
    --- FAIL: Test_Main/TestB (0.00s)
        testing.go:1152: race detected during execution of test
FAIL
FAIL    testify-race    0.022s
FAIL

Based on a quick look at the code, it seems like the usage of suite.SetT() in various places in suite.Run is the cause of the race.

brackendawson commented 2 years ago

Parallel tests and subtests are broken in a number of ways with the suite package in the current release. This race, wrong subtest names in failed assertions, cleanup being run before the end of the test...

Does this branch work well for you? It is a breaking API change in the suite package but I hope that we can adopt it for testify v2 as it should allow us to fix a lot of the problems with parallel tests.

bstpierre commented 2 years ago

@brackendawson Yes. When I modify the test program above to match the new api and run against that branch, it runs without errors.

package main

import (
        "testing"

        "github.com/stretchr/testify/suite"
)

type MainSuite struct{}

func (s *MainSuite) TestA(t *suite.T) {
        t.Parallel()
        for x := 0; x < 1000000; x++ {
                t.Equal(x, add(x, 0))
        }
}

func (s *MainSuite) TestB(t *suite.T) {
        t.Parallel()
        for x := 0; x < 1000000; x++ {
                t.Equal(x+1, add(x, 1))
        }
}

func Test_Main(t *testing.T) {
        suite.Run(t, new(MainSuite))
}
MaggieMa21 commented 11 months ago

Any update on the ETA of this fix?

brackendawson commented 11 months ago

The fix for this wold require testify/v2/suite, this is not expected soon. For now; parallel tests and subtests are not supportable with testify/suite.

dolmen commented 11 months ago

Suite v2 will have to happen elsewhere. There is already too much maintenance work here on assert and mock.