Add support for subtests (go 1.7)

[itzamna314]

Adds a .Run(..) method onto test suites, mimicking the semantics of testing.Run. This will make writing table-driven tests in test suites cleaner and simpler, by handling the management of *testing.T instances transparently so that sub-tests can use the same suite.Assert() and suite.Require() methods, and have all output and test logic properly fall under the correct subtest.

The major benefits of managing the *testing.T instance are:

• Subtest output will be properly printed under each subtest, rather than all grouped under the test method
• Subtests can be correctly executed using go test -run MyTestSuite/MyTestMethod/MySubTest

[itzamna314]

@kwk Is there anything else we need to get this approved and merged?

[kwk]

@kwk Is there anything else we need to get this approved and merged?

@itzamna314 I'm not a maintainer and not a contributor, so I cannot tell.

[itzamna314]

Still looking for a review from a contributor/maintainer.

Looks like I'll keep copy-pasting this code into every project that uses testify/suite till then 😿

[itzamna314]

@kwk Could you try adding an approving review, to see if that will allow this to be merged? I can't find any specific documentation of the contribution policies, so maybe that will work?

[kwk]

LGTM

[letientai299]

LGTM

[letientai299]

This PR is there for 3 months already. Cc @viswajithiii and @georgelesica-wf since you are the most recent contributors on master branch. Anything we can do to have this get reviewed and merged?

[georgelesica-wf]

@letientai299 looking at it right now. I just came on to help a couple months back, and I haven't had time to look much at the backlog of PRs, I've just been dealing with them as they are brought up.

[georgelesica-wf]

These would seem to pass trivially since you capture and increment on lines 158 and 159. What am I missing?

[itzamna314]

Its comparing TestOneRunCount, and the thing that got incremented was TestSubtestRunCount.

But as far as I can tell, that's an arbitrary test that was passing coincidentally. Its been a few months since I wrote that, and maybe I had a good reason then, but I can't figure it out now at all. So I pulled those lines of the test out, to avoid future confusion. Good catch!

[georgelesica-wf]

@itzamna314 hey would you mind resolving merge conflicts?

[itzamna314]

I got the merge conflicts resolved, but ci is failing. It built on all of the targets except for tip, and for some reason tip took 4 minutes while the other targets took about 1 each. I couldn't figure out how to re-run.

[georgelesica-wf]

The failure on tip is a known issue. I'm hoping to fix it soon, certainly before the next release. I'm going to have one other person take a look at this tomorrow and then I'll merge it. Sorry again for the delay.

[itzamna314]

Thanks!

[brandonbodnar-wk]

Overall, I support this change to allow for subtests within a test suite. I have a few suggested documentation changes.

But, my primary concern is with the interaction between these subtests and the use of T.Parallel(), both from a documentation standpoint and regarding the defer to reset the suite's testing context. Granted, someone calling T.Parallel would probably understand the risks associated with running parallel tests on a stateful thing like a Suite instance, but I would like to support them the best we can, knowing the difficulty of handling each edge case. I could also have my understanding completely wrong with regards to the interaction with T.Parallel, so please feel free to correct me if that is the case.

[brandonbodnar-wk]

to expose setup and teardown functionality for each subtest.

This comment is confusing to me. Based on its wording, I would expect some type of before and after method is ran by the suite runner before and after each subtest (similar to the SetupSuite and TearDownSuite ran before and after the suite itself, or the SetupTest, BeforeTest, AfterTest, and TearDownTest ran before and after each test). I don't believe that is the intent though based on the code, and that the intent is to allow people to write subtests in which they may themselves execute setup and teardown code within their passed in function.

To address, I suggest altering this statement to something along the lines of

It may be called in place of t.Run(name, func(t *testing.T)) within tests inside a test suite.

[brandonbodnar-wk]

will be executed as a subtest with a fresh instance of t.

I think this can create some confusion that a *testing.T instance will be passed to the passed-in function as a parameter. While obviously not the case based on the function signature of subtest for Suite.Run, we could probably clean up this documentation to make clear that it is the suite's testing context (as returned by Suite.T()) that is switched prior to execution of the given function.

[brandonbodnar-wk]

Reviewing some other existing areas of code with relation to using T.Parallel within a suite, it looks like we have a number of other problematic areas within Suite that prevent T.Parallel from working as expected (for example as pointed out in #466, the suite teardown method runs before executing the parallel tests). While I still feel that the defer suite.SetT(oldT) issue I noted above should be addressed in this PR as related to its code addition, it looks like we have a similar issue within the actual test runner for suite that already exists. @georgelesica-wf I assume we should create a new issue for this.

I bring this up because testing for the T.Parallel issue I noted in my prior comments may be problematic if we have existing issues related to using T.Parallel within a suite.

[georgelesica-wf]

@itzamna314 Hey can you look at addressing Brandon's comments? Ideally I think we want to try not to break parallel runs any more than we already do.

[brandonbodnar-wk]

LGTM @georgelesica-wf

[georgelesica-wf]

@itzamna314 thank you for your contribution and patience!

[xcile]

Shouldn't SetupTest() and other interface methods be called for each golang subtest? It's a bit confusing right now that the Suite has subtests, but the Suite instance is common for each test.

[itzamna314]

I don't think so. The desired behavior on each subtest may be different than the desired behavior on each top-level test.

It's pretty simple to run some setup/teardown for each subtest. Just add a .Run() method with your suite as the receiver, and call the embedded TestSuite.Run() from your method. This shadowing approach is similar to overriding parent methods in languages with inheritance.

You could make that behavior more explicit, by adding an interface that does subtest setup and teardown and then using type assertions to run that interface's setup/teardown methods. It would work similar to test-level and suite-level setup/teardown. That behavior would be fully compatible with this implementation.

[xcile]

I still think it's confusing that the golang subtests does not work seamless with testify/suite. If they would, you could use the flexibility of table testing together with the power of testify/suite. However, I also agree with you that its quite easy to accomplish what I'm asking for. Thanks for the overriding tip, that might come useful sometime.

I've solved it now by using a separate suite instance for each subtest which becomes quite powerful:

func (s *MySuite) TestSomeCases() {

   subTests := map[string]MySuite {
      "test1": func(suite MySuite) MySuite {
         suite.SUT = mySutFunc
         suite.inputData = "some data"
         suite.expectedOutput = "ok"
         return suite
      }(*s),
   }

   for name, test : subTests {
      test.Run(name, func(){
         result := test.SUT(test.inputData)
         ...
      })
   }
}

[BenSayers]

I arrived here after looking for an approach to writing table driven tests using testify/suite. I agree with @xcile it's unexpected that suite.Run does not invoke the setup and teardown methods. Having something that automatically calls these methods is the primary driver for using the suite feature in the first place.

[mattolenik]

I also came here looking to write table-driven tests with Testify suites. As I understand it, this PR may be solving a slightly different problem than what some of us are looking to solve when we think "table-driven tests with Testify". I used the following pattern to run a single suite many times with different parameters. It will give output separated by TestName in the way you'd expect. Posting it here as it may be useful to other folks who also stumble upon this thread 😃

type TestParams struct {
    // Name to be given for each run of the suite
    TestName String
    // Various test parameters etc
    Param1 string
    Param2 int
}

// Your standard Testify suite struct
type MySuite struct {
    suite.Suite
    params *TestParams
}

// This is your test table
var params = []TestParams{
    {"FirstTest", "abc", 123},
    {"SecondTest", "def", 456},
    {"ThirdTest", "xyz", 789},
    // etc...
}

// Top-level function invoked by `go test`
func TestMySuite(t *testing.T) {
    for _, param := range params {
        t.Run(param.TestName, func(tt *testing.T) {
            s := new(MySuite)
            s.params = &param
            suite.Run(tt, s)
        })
    }
}

// Parameters can be utilized in setup/teardown or individual test cases
func (s *MySuite) SetupTest() {
    foo(s.params.Param1)
    bar(s.params.Param2)
}

func (s *MySuite) TestMyFeature() {
    // test code here...
    DoSomethingTesty(s.params.Param1)
}

Thanks to everyone in this thread for the contribution and discussion, I think we all appreciate the work that goes into this fantastic library! 👍

[0cv]

@mattolenik thanks that was helpful. It also works well in combination with BeforeTest and AfterTest. I'd wish this library to have documentation and such examples