Add opt-in parallel test methods.

[FGasper]

Summary

Add “ParallelTest” methods that run in parallel.

Changes

• Add SetupParallelTestSuite, TearDownParalleltestSuite, BeforeParallelTest, & AfterParallelTest interfaces that allow the relevant suite methods to accept a *testing.T.
• Teach Suite to recognize ParallelTest* methods and, if present, execute them after the other methods in a parallel subtest.

Motivation

Issue #187 has vexed testify users for almost a decade. The library is too widely used for breaking changes such as PR #1109. This changeset proposes an alternative solution that preserves backward compatibility while allowing users to opt-in to the new functionality.

For example usage, see the new suite_parallel_test.go file (derived from PR #1109’s tests).

Related issues

Closes #187.

[brackendawson]

Which of the many parallel test bugs this this aim to fix? #187 is about the parallel tests continuing to run after the suite has returned, which can cause failing tests to be marked as passing. #934 covers teardown being run while the parallel tests are still running.

I've tried to convert such a situation to use this PR and it now panics:

package kata_test

import (
	"runtime"
	"testing"

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

type My struct {
	suite.Suite
}

func (m *My) TestSequntial() {
	m.Assert().True(true)
	// passes during suite
}

func (m *My) ParallelTestPass() {
	m.T().Parallel()
	runtime.Gosched()
	m.Assert().True(true)
	// passes after suite
}

func (m *My) ParallelTestFail() {
	m.T().Parallel()
	runtime.Gosched()
	m.Assert().True(false) // fails after suite
}

func TestIfy(t *testing.T) {
	suite.Run(t, &My{})
}

--- FAIL: TestIfy (0.01s)
    --- FAIL: TestIfy/parallel (0.00s)
        --- FAIL: TestIfy/parallel/ParallelTestFail (0.00s)
            /Users/bracken/src/githib.com/brackendawson/kata/value.go:430: test panicked: reflect: Call with too many input arguments
                goroutine 25 [running]:
                runtime/debug.Stack()
                	/usr/local/go/src/runtime/debug/stack.go:26 +0x6c
                github.com/stretchr/testify/suite.failOnPanic(0xc000083a40, {0x1010186a0, 0x1010a6550})
                	/Users/bracken/src/githib.com/stretchr/testify/suite/suite.go:97 +0x44
                github.com/stretchr/testify/suite.Run.func1.1()
                	/Users/bracken/src/githib.com/stretchr/testify/suite/suite.go:222 +0x438
                panic({0x1010186a0?, 0x1010a6550?})
                	/usr/local/go/src/runtime/panic.go:792 +0x124
                reflect.Value.call({0xc0000abe40?, 0xc00009ad60?, 0x13?}, {0x100f3685d, 0x4}, {0xc0002e2d20, 0x2, 0x2?})
                	/usr/local/go/src/reflect/value.go:430 +0x1a10
                reflect.Value.Call({0xc0000abe40?, 0xc00009ad60?, 0x1?}, {0xc0002e2d20, 0x2, 0x2})
                	/usr/local/go/src/reflect/value.go:368 +0x94
                github.com/stretchr/testify/suite.Run.func1(0xc000083a40)
                	/Users/bracken/src/githib.com/stretchr/testify/suite/suite.go:259 +0x794
                testing.tRunner(0xc000083a40, 0xc0002c4aa0)
                	/usr/local/go/src/testing/testing.go:1792 +0x184
                created by testing.(*T).Run in goroutine 24
                	/usr/local/go/src/testing/testing.go:1851 +0x688
        --- FAIL: TestIfy/parallel/ParallelTestPass (0.00s)
            /Users/bracken/src/githib.com/brackendawson/kata/value.go:430: test panicked: reflect: Call with too many input arguments
                goroutine 26 [running]:
                runtime/debug.Stack()
                	/usr/local/go/src/runtime/debug/stack.go:26 +0x6c
                github.com/stretchr/testify/suite.failOnPanic(0xc000083dc0, {0x1010186a0, 0x1010a6550})
                	/Users/bracken/src/githib.com/stretchr/testify/suite/suite.go:97 +0x44
                github.com/stretchr/testify/suite.Run.func1.1()
                	/Users/bracken/src/githib.com/stretchr/testify/suite/suite.go:222 +0x438
                panic({0x1010186a0?, 0x1010a6550?})
                	/usr/local/go/src/runtime/panic.go:792 +0x124
                reflect.Value.call({0xc0000abe40?, 0xc00009ad78?, 0x13?}, {0x100f3685d, 0x4}, {0xc000112030, 0x2, 0x2?})
                	/usr/local/go/src/reflect/value.go:430 +0x1a10
                reflect.Value.Call({0xc0000abe40?, 0xc00009ad78?, 0x1?}, {0xc000112030, 0x2, 0x2})
                	/usr/local/go/src/reflect/value.go:368 +0x94
                github.com/stretchr/testify/suite.Run.func1(0xc000083dc0)
                	/Users/bracken/src/githib.com/stretchr/testify/suite/suite.go:259 +0x794
                testing.tRunner(0xc000083dc0, 0xc0002c4dc0)
                	/usr/local/go/src/testing/testing.go:1792 +0x184
                created by testing.(*T).Run in goroutine 24
                	/usr/local/go/src/testing/testing.go:1851 +0x688
FAIL
coverage: [no statements]
FAIL	github.com/brackendawson/kata	0.227s
FAIL

My example is pretty racy, it can also fail before go exits and generate a warning, and it can also attribute the failure to the wrong test.

[FGasper]

@brackendawson The underlying problem is that the suite’s T() method can’t work in parallel tests. This changeset doesn’t fix that; instead, it just provides an alternative path to achieve parallel methods.

Your Parallel* suite methods need to accept a *testing.T.

Also, anything that references the suite’s internal *testing.T will behave undesirably. Thus, you need to use assert.True(t, …) rather than m.Assert().True(…).

The fixed suite behaves as I think you expect:

package suite_test

import (
	"runtime"
	"testing"

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

type My struct {
	suite.Suite
}

func (m *My) TestSequntial() {
	m.Assert().True(true)
	// passes during suite
}

func (m *My) ParallelTestPass(t *testing.T) {
	runtime.Gosched()
	assert.True(t, true)
}

func (m *My) ParallelTestFail(t *testing.T) {
	runtime.Gosched()
	assert.True(t, false)
}

func TestKataSuite(t *testing.T) {
	suite.Run(t, &My{})
}

I’ve added guard rails to ensure that Parallel methods take the required t argument (and sequential methods don’t).

A guard-rail might also be added that introspects the call stack (like this) to prevent folks from calling T() or Assert() from within parallel methods, but it’s potentially messy.

I also realized that the suite will need an alternate solution for Run() in parallel tests. Before I do that, though, please share your thoughts on the above when you can.

Thanks!

[brackendawson]

I see, so the feature comes with some caveats:

• You must accept a *testing.T.
• You must not use suite.Suite.T().
• You should not call t.Parallel() yourself.

Given this removes a lot of the functionality of suite from these methods, basically the only thing you should touch on receiver is any instance variables or methods you defined yourself, this is a very tenuous version of suite. It's also quite hard to document and it's likely people will use it incorrectly. I'm sorry to say don't think I'm sold.

[FGasper]

removes a lot of the functionality of suite from these methods

What functionality are you talking about? t.Parallel() can still be called; it’s just redundant because the suite will already do it. Static assert and require calls furnish all the functionality that the suite methods provide, right?

It seems to me a fairly trivial syntactic difference … and one that breaks no existing code.

For me, anyhow, the real purposes of Suite are the before/after hooks and recursive grouping of test functions. Those remain intact and still quite natural.

it's likely people will use it incorrectly

Currently there are guard rails in place to prevent the following misuses up-front:

• Parallel methods that don’t accept t.
• Parallel methods alongside non-t hook methods (e.g., BeforeTest()).
• Parallel methods that access T(), Assert(), or Require().

The only other avenue to “easy” misuse is t.Assertions … but linters should complain about that, at least.

quite hard to document

Fair enough, but the misuse prevention will also guide users to proper usage, which seems to me to mitigate the impact here.

[FGasper]

@brackendawson FYI, I’ve left the branch failing to illustrate the guard rail.

[brackendawson]

suite.go:94: test panicked: Avoid Assert() in parallel tests. Use assert(t, ...) instead.

I see, though it's quite ambiguous. Avoid? Assert vs assert? Also the assert package's functions are not equivalent. Better wording might be:

suite.go:94: test panicked: Suite.Assert() called from parallel test. Use assert.New(t) instead.

Though the difficulty of documenting this, and the effort the documentation and validation has to go to to steer people away from APIs that they must no longer use, but which are still available, tells me that this is not an optimal solution. I really don't relish the idea of having to support this.