Add a Run helper method

[JelteF]

trafficstars

I love table tests, especially when using Run. And this adds that functionality to the suite library.

[JelteF]

The name might be a bit confusing, but I think it's quite useful for clarity to use the same name as the normal testing library uses.

Regarding race conditions I was convinced the suite based tests didn't support parallel tests anyway, because of their setup and teardown mechanics. If so I don't think it's a problem at the moment to not support T().Parallel() here either. (I'm not at my computer right now, so I can't look up the issue describing the lack of support, but I think it's quite easy to find on Google)

On Sat, Dec 30, 2017, 14:36 Ernesto Jiménez [email protected] wrote:

@ernesto-jimenez requested changes on this pull request.

I think this might have some race conditions.

It would be good to have some accompanying tests too.

In suite/suite.go https://github.com/stretchr/testify/pull/510#discussion_r159122640:

@@ -55,6 +55,21 @@ func (suite *Suite) Assert() *assert.Assertions { return suite.Assertions }

+// Run is a helper method to do a simple subtest. It sets T() to the +// *testing.T of the subtest. +func (suite *Suite) Run(name string, f func()) {

@JelteF https://github.com/jeltef could the name be confusing considering we already have suite.Run?

In suite/suite.go https://github.com/stretchr/testify/pull/510#discussion_r159122685:

@@ -55,6 +55,21 @@ func (suite *Suite) Assert() *assert.Assertions { return suite.Assertions }

+// Run is a helper method to do a simple subtest. It sets T() to the +// *testing.T of the subtest. +func (suite *Suite) Run(name string, f func()) {

• oldT := suite.T()
• suite.T().Run(name, func(t *testing.T) {

• suite.SetT(t)
  

• defer func() {
  

• 	suite.SetT(oldT)
  

• }()
  

Could this result in race conditions if somebody called suite.T().Parallel() within the subtest?

In suite/suite.go https://github.com/stretchr/testify/pull/510#discussion_r159122714:

@@ -55,6 +55,21 @@ func (suite *Suite) Assert() *assert.Assertions { return suite.Assertions }

+// Run is a helper method to do a simple subtest. It sets T() to the +// *testing.T of the subtest. +func (suite *Suite) Run(name string, f func()) {

• oldT := suite.T()
• suite.T().Run(name, func(t *testing.T) {

• suite.SetT(t)
  

• defer func() {
  

• 	suite.SetT(oldT)
  

• }()
  

• f()
  

Rather than receiving an empty func(), maybe we should receive func(*Suite) and send a test suite with the new *testingT?

Would that prevent race conditions?

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/stretchr/testify/pull/510#pullrequestreview-86030543, or mute the thread https://github.com/notifications/unsubscribe-auth/ABG8JltFMeZM6xKwv4LTC1iQfrg3lPMcks5tFjxBgaJpZM4P2X5m .

[ernesto-jimenez]

@JelteF parallel suite tests are currently unsupported because, but I'm unsure what the situation would be with subtests and whether we would be able to support parallel suite tests in the future.

The following might help with that:

func (s *Suite) RunSubtest(name string, fn fun(*Suite))

Where the suite sent is a different one with the new *testing.T.

What do you think?

[JelteF]

First off, did you mean to close the PR?

Regarding the proposed change, I don't think I really like it. Mainly because I wouldn't have access to the helper methods I have on my custom Suite type.

Renaming it to RunSubtest is fine by me though if you think that causes less confusion.

[ernesto-jimenez]

Hey, sorry, clicked the wrong button.

You could implement this:

// RunSubtest receives the name of the subtest and a function taking a single argument of a struct that embeds `Suite`. The subtest will be called with a new instance of the type provided in the argument with the subtest’s `*testing.T`
func (s *Suite) RunSubtest(name string, fn interface{})

Then, using reflection you would set things up. We do something similar in the mock.MatchedBy.

Even if running tests in parallel is currently broken, it would be good to fix that bug rather than relying on the bug.

[JelteF]

Would it be okay if I add a RunSubtest and RunSubtestParallel, where the RunSubtestParallel also calls T().Parallel(). Because I especially like the implementation I made for non parallel test, because it's very straightforward to use.

[Antonboom]

PR is not actual more? https://pkg.go.dev/github.com/stretchr/testify/suite#Suite.Run

[brackendawson]

Apparently not.