testify icon indicating copy to clipboard operation
testify copied to clipboard

Published 20 hours ago • verified publisher icon stretchr

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

Open bstpierre opened this issue 3 years ago • 2 comments

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/[email protected]/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/[email protected]/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/[email protected]/suite/suite.go:144 +0x2f4
  github.com/stretchr/testify/suite.Run.func1()
      /home/brian/go/pkg/mod/github.com/stretchr/[email protected]/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/[email protected]/suite/suite.go:203 +0x18f
  github.com/stretchr/testify/suite.Run()
      /home/brian/go/pkg/mod/github.com/stretchr/[email protected]/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/[email protected]/suite/suite.go:203 +0x18f
  github.com/stretchr/testify/suite.Run()
      /home/brian/go/pkg/mod/github.com/stretchr/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/assert/forward_assertions.go:12 +0x84
  github.com/stretchr/testify/suite.(*Suite).SetT()
      /home/brian/go/pkg/mod/github.com/stretchr/[email protected]/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/[email protected]/suite/suite.go:144 +0x2f4
  github.com/stretchr/testify/suite.Run.func1()
      /home/brian/go/pkg/mod/github.com/stretchr/[email protected]/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/[email protected]/suite/suite.go:203 +0x18f
  github.com/stretchr/testify/suite.Run()
      /home/brian/go/pkg/mod/github.com/stretchr/[email protected]/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/[email protected]/suite/suite.go:203 +0x18f
  github.com/stretchr/testify/suite.Run()
      /home/brian/go/pkg/mod/github.com/stretchr/[email protected]/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.

bstpierre avatar Dec 28 '21 02:12 bstpierre

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.

brackendawson avatar Dec 29 '21 17:12 brackendawson

@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))
}

bstpierre avatar Dec 30 '21 01:12 bstpierre

Any update on the ETA of this fix?

MaggieMa21 avatar Jul 18 '23 23:07 MaggieMa21

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.

brackendawson avatar Jul 20 '23 08:07 brackendawson

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

dolmen avatar Jul 25 '23 05:07 dolmen