Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Running Parallel Tests in Suite #187

Open
eyphka opened this issue Jun 23, 2015 · 23 comments
Open

Running Parallel Tests in Suite #187

eyphka opened this issue Jun 23, 2015 · 23 comments
Labels
concurrency enhancement pkg-suite Change related to package testify/suite

Comments

@eyphka
Copy link

eyphka commented Jun 23, 2015

When running multiple tests in a suite which have been set to suite.t().Parallel(), tests will pass even if they should fail.

@dwlnetnl
Copy link

That's true, but as it's currently structured it isn't possible to run suite tests in parallel. This is because before and after each suite test SetupTest and TearDownTest is run.

@ernesto-jimenez
Copy link
Member

@eyphka do you want to make a PR to address it? :)

@ernesto-jimenez
Copy link
Member

Related: #52

@eyphka
Copy link
Author

eyphka commented Nov 3, 2015

Sure

On Sunday, November 1, 2015, Ernesto Jiménez notifications@github.com
wrote:

Related: #52 #52


Reply to this email directly or view it on GitHub
#187 (comment).

@oryband
Copy link

oryband commented May 8, 2016

Reviving this issue. Presumably it should be possible to run suite tests in parallel.

The problem with parallelism here is that the suite members used in tests have a shared single instance for all. Tests running in parallel could potentially access these members at the same time, causing them to "collide" with each other via these members.

However, my suggestion is that testify will add a special Parallel() flag to the suite. This new flag will instantiate new instance members for every new Parallel() test, calling Setup/TearDownTest() as well.

This will of course require extra work from the test developer, to make sure Setup/TearDownTest() are suitable for being called with/without parallel (As some tests will be run in parallel, and some don't).

Thoughts?

@dansimau
Copy link

I just whipped up a slightly different approach to running tests in parallel:
https://gist.github.com/dansimau/128826e692d7834eb594bb7fd41d2926

Similar to #369, it introduces a RunParallel, except that it uses reflection to create a new instance of the suite for each test run.

Because a new suite is initialised before every test, it means tests can't share data on the suite struct. This is a change in behaviour, though I imagine you could change the patch to make a copy of the suite struct instead of initialising a new one (that way tests could still share data and setup/teardown code using SetupSuite).

I actually prefer the share-nothing approach though; it feels like a bad practice to modify data on the suite struct while tests are running, especially now in parallel. I can't see why tests should ever share data (outside what is set up in SetupTest); but maybe I'm missing a use case.

@varunrau
Copy link

is it still the case that suite tests can't be run in parallel?

@alessio
Copy link

alessio commented Sep 30, 2020

Hi! Any updates on this? Are you planning to do work on this feature request?

@ysmood
Copy link

ysmood commented Oct 1, 2020

One solution:

package main_test

import (
	"testing"
	"time"

	"github.com/stretchr/testify/assert"
	"github.com/ysmood/got"
)

func Test(t *testing.T) {
	got.Each(t, beforeEach)
}

func beforeEach(t *testing.T) Suite {
	t.Parallel()
	return Suite{assert.New(t)}
}

type Suite struct { // struct that holds subtests
	*assert.Assertions
}

func (s Suite) A() { // test case A
	time.Sleep(time.Second)
	s.Equal(1, 1)
}

func (s Suite) B() { // test case B
	time.Sleep(time.Second)
	s.Equal(2, 1)
}

As you can see, test A is not affected:

$ go test       
--- FAIL: Test (0.00s)
    --- FAIL: Test/B (1.00s)
        main_test.go:31: 
            	Error Trace:	main_test.go:31
            	            				value.go:475
            	            				value.go:336
            	            				each.go:42
            	            				value.go:564
            	            				asm_amd64.s:20
            	Error:      	Not equal: 
            	            	expected: 2
            	            	actual  : 1
            	Test:       	Test/B
FAIL
exit status 1
FAIL	goplay/default	1.254s

@tisonkun
Copy link

Reviving this issue. Presumably it should be possible to run suite tests in parallel.

The problem with parallelism here is that the suite members used in tests have a shared single instance for all. Tests running in parallel could potentially access these members at the same time, causing them to "collide" with each other via these members.

However, my suggestion is that testify will add a special Parallel() flag to the suite. This new flag will instantiate new instance members for every new Parallel() test, calling Setup/TearDownTest() as well.

This will of course require extra work from the test developer, to make sure Setup/TearDownTest() are suitable for being called with/without parallel (As some tests will be run in parallel, and some don't).

Thoughts?

@oryband can we implement it now?

@tisonkun
Copy link

func (s *mySuiteA) TestA2() { // test case A
	t := s.T()
	t.Parallel()
	assert.Equal(t, 3, 1)
}

cache the *testing.T pointer can solve the problem.

This issue is caused by the following code snippet

test := testing.InternalTest{
  Name: method.Name,
  F: func(t *testing.T) {
    // ...
    defer func() {
      // ...
      suite.SetT(parentT)
      // ...
    }()
    // ...
    method.Func.Call([]reflect.Value{reflect.ValueOf(suite)})
  }
}

... and when run test in parallel, defer function executed from TestA even if the body of TestB not be executed, T set to the parent T so the child test failed but reported as passed. Because the fail happened on the parent T instead of the child T.

@tisonkun
Copy link

func (s *mySuiteA) TestA2() { // test case A
	t := s.T()
	t.Parallel()
	assert.Equal(t, 3, 1)
}

cache the *testing.T pointer can solve the problem.

This issue is caused by the following code snippet

test := testing.InternalTest{
  Name: method.Name,
  F: func(t *testing.T) {
    // ...
    defer func() {
      // ...
      suite.SetT(parentT)
      // ...
    }()
    // ...
    method.Func.Call([]reflect.Value{reflect.ValueOf(suite)})
  }
}

... and when run test in parallel, defer function executed from TestA even if the body of TestB not be executed, T set to the parent T so the child test failed but reported as passed. Because the fail happened on the parent T instead of the child T.

Well, it is not a solution because as tests grow the first place cache may cache a wrong pointer also.

@tisonkun
Copy link

Then I'd suggest we pass the inner T if the test method meet the signature, i.e.,

func (s *mySuite) TestT(t *testing.T)

@aw185176
Copy link

being able to force parallelism for suites would be helpful for us as well, we are looking to ditch ginkgo but want to make sure we can force random execution order to find flaky tests and keep our overall suite runtime low

@siasalar
Copy link

siasalar commented Jan 7, 2022

any update about this long open issue?

@jitendra-1217
Copy link

jitendra-1217 commented Apr 28, 2022

Implementing SetupTestSuite following way should work as expected:

No. The same problem is described in the above thread. Missed.

// SetupTest will run before each test in the suite.
func (s YourSuite) SetupTest() {
    s.T().Parallel()
}

@xobotyi
Copy link

xobotyi commented Aug 12, 2022

Any moves on that?

@Jerska
Copy link

Jerska commented Jan 12, 2023

Here is a solution we've used.
Our use case is for e2e tests that we want to run in parallel. All of them are defined in the same suite.

type E2ETestSuite struct {
	suite.Suite
	ts map[string]*testing.T // Map of test names > *testing.T
}

func (suite *E2ETestSuite) BeforeTest(_, testName string) {
	t := suite.T()
	if suite.ts == nil {
		suite.ts = make(map[string]*testing.T, 1)
	}
	suite.ts[testName] = t
	t.Parallel()
}

// T() overrides suite.Suite.T() with a way to find the proper *testing.T
// for the current test.
// This relies on `BeforeTest` storing the *testing.T pointers in a map
// before marking them parallel.
// This is a huge hack to make parallel testing work until
// https://github.com/stretchr/testify/issues/187 is fixed.
// There is still a small race:
// 1. test 1 calls SetT()
// 2. test 1 calls BeforeTest() with its own T
// 3. test 1 is marked as parallel and starts executing
// 4. test 2 calls SetT()
// 5. test 1 completes and calls SetT() to reset to the parent T
// 6. test 2 calls BeforeTest() with its parent T instead of its own
// The time between 4. & 6. is extremely low, enough that this should be really rare on our e2e tests.
func (suite *E2ETestSuite) T() *testing.T {
	// Try to find in the call stack a method name that is stored in `ts` (the test method).
	for i := 1; ; i++ {
		pc, _, _, ok := runtime.Caller(i)
		if !ok {
			break
		}
		// Example rawFuncName:
		// github.com/foo/bar/tests/e2e.(*E2ETestSuite).MyTest
		rawFuncName := runtime.FuncForPC(pc).Name()
		splittedFuncName := strings.Split(rawFuncName, ".")
		funcName := splittedFuncName[len(splittedFuncName)-1]
		t := suite.ts[funcName]
		if t != nil {
			return t
		}
	}
	// Fallback to the globally stored Suite.T()
	return suite.Suite.T()
}

@Jerska
Copy link

Jerska commented Jan 12, 2023

I've opened a PR to try to allow Test... methods to receive their own t as a parameter: #1322 that partially allows to run suite tests in parallel.

This is the approach suggested by @tisonkun in #187 (comment)

@MaggieMa21
Copy link

any update about this long open issue?

@brackendawson
Copy link
Collaborator

#1109 is the only good fix for this, but is a breaking change. It might be that a testify/v2/suite is made one day to address this, but for all of testify v1; do not use parallel tests or subtests with suite.

@Nikola-Milovic
Copy link

Any workaround in 2024 or is this still an ongoing issue?

@dolmen
Copy link
Collaborator

dolmen commented May 16, 2024

@Nikola-Milovic This is still broken and any fix would require a major API breaking change, so this will not be fixed in v1. I recommend anyone interested in building a suite package that allows parallel tests to build it elsewhere. See #1518.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
concurrency enhancement pkg-suite Change related to package testify/suite
Projects
None yet
Development

Successfully merging a pull request may close this issue.