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

proposal: testing: per-test setup and teardown support #27927

Closed
dfawley opened this issue Sep 28, 2018 · 24 comments
Closed

proposal: testing: per-test setup and teardown support #27927

dfawley opened this issue Sep 28, 2018 · 24 comments
Labels
Milestone

Comments

@dfawley
Copy link

@dfawley dfawley commented Sep 28, 2018

There are some checks that are helpful to perform after every test runs, e.g. monitoring for leaked goroutines. However, remembering to add these checks to every single test in a package is difficult and cumbersome. The testing package should provide a way to allow setup and cleanup code to be run globally for every test in a package, which has access to the testing.T for each test case. Note that TestMain does not allow you to run code between test functions; only at the very beginning and end of the whole package.

@gopherbot gopherbot added this to the Proposal milestone Sep 28, 2018
@gopherbot gopherbot added the Proposal label Sep 28, 2018
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 28, 2018

As a proposal, it would be helpful to suggest exactly how this should work, so we can see what new API would be required.

@dfawley

This comment has been minimized.

Copy link
Author

@dfawley dfawley commented Sep 28, 2018

There are many different ways this could work, and I don't want to prescribe one for fear of flailing wildly and having it be rejected when other ways of implementing the same feature would work. A few ideas:

  1. An interceptor-style function with the signature func(t *testing.T, f func(*testing.T)) could be registered in an init(). The interceptor is given the t and the test function f, and can perform setup, call f, and then perform cleanup.

  2. A specially-named function (e.g. TestInterceptor(*testing.I) where I is a new interceptor type that contains t and f, as above.

  3. New functionality in testing.M used via TestMain.

The last one sounds the most approachable to me, so I will flesh it out a bit.

How I would imagine the usage would look:

package my_test

func TestMain(m *testing.M) {
	for _, t := range m.Tests {
		setup()
		t.Run()  // invokes the current test function
		if err := cleanup(); err != nil {
			t.Error("error cleaning up test:", err)
		}
	}
}

Implementation:

package testing

type M struct {
	...
	Tests []TestRunner
}

type TestRunner struct {
	*T  // embed the testing.T that will be used for this test.
	// other unexported fields as needed
}

func (TestRunner) Run()  // runs this test with this TestRunner's *T

I haven't looked into the implementation of the testing package much; it's possible an iterator instead of a slice would be easier to implement, since creating all the testing.Ts at initialization might be difficult/impossible -- or an initializer on the TestRunner to create the T could be required instead. Also, a TestRunner.Done method may be required for implementation detail reasons. (Again, an iterator might be better, since it could automatically perform any necessary cleanup after the previous test.)

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Oct 3, 2018

/cc @mpvl
This seems too intrusive to me, fwiw.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Dec 12, 2018

Various of us who have had to write code like this have found it helpful to write it explicitly instead of having an implicit thing that applies to every test in the package. Sometimes only 9 of 10 tests need a particular setup/teardown. Another common pattern is to put setup/teardown in one Test and then put the tests that need that setup/teardown into subtests. Or write a helper that does the setup/teardown and pass in the 'body' to run in between. There are lots of ways. We shouldn't hard-code one.

@rsc rsc closed this Dec 12, 2018
@dfawley

This comment has been minimized.

Copy link
Author

@dfawley dfawley commented Dec 12, 2018

Per-test setup and teardown is a common feature in most testing frameworks, and is supported in Java, C++, and Python, in OSS and at Google. I know there are other ways to implement this, but they are all cumbersome or error-prone or both. Something included in the language's primary testing framework would make it both easy to use and reliable.

@jadekler

This comment has been minimized.

Copy link
Contributor

@jadekler jadekler commented Dec 12, 2018

+1. This is something extremely common in test frameworks. @rsc, please reconsider. This might not be something y'all use, but it is something the community uses:

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Dec 12, 2018

Why not just use a single test function with subtests? Why is that any harder to use than the suggestions in this issue?

@dfawley

This comment has been minimized.

Copy link
Author

@dfawley dfawley commented Dec 12, 2018

@ianlancetaylor What would that look like for a package with 10~100 tests? Something like this is what I was imagining:

func testFoo(t *testing.T) { ... }
func testBar(t *testing.T) { ... }
func testBaz(t *testing.T) { ... }

func TestEverything(t *testing.T) {
	// Maintaining this map is error-prone and cumbersome (note the subtle bug):
	fs := map[string]func(*testing.T){"testFoo": testFoo, "testBar": testBar, "testBaz": testBar}
	// You may be able to use the `importer` package to enumerate tests instead,
	// but that starts getting complicated.
	for name, f := range fs {
		setup()
		t.Run(name, f)
		teardown()
	}
}

Is there a better way to do this?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Dec 13, 2018

var tests = []func(t *testing.T){
    func(t *testing.T) {
        ...
    },
    func(t *testing.T) {
        ...
    },
    ...
}

func TestEverything(t *testing.T) {
    for i, fn := range tests {
        setup()
        t.Run(strconv.Itoa(i), fn)
        teardown()
    }
}

Yes, it's more awkward, but it seems manageable, and there are cases where a bit of awkwardness is preferable to hidden magic.

@dfawley

This comment has been minimized.

Copy link
Author

@dfawley dfawley commented Dec 13, 2018

There are several undesirable aspects to your variant:

  • The tests are not named. That makes it difficult to individually run them, and when one fails, you don't really know what failed. With any more than 2-3 tests, it's hard to even find the test case you're looking for since the numbers are implicit. There's also the issue of re-numbering due to inserting/deleting tests.
  • The tests all must be in the same _test.go file. This approach doesn't work for larger packages with tests in multiple files.
  • The tests themselves are not top-level functions. That adds an extra level of indent everywhere. This is bad for complex tests.
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Dec 13, 2018

There are obvious mitigations for those comments, but I said up front that the approach is awkward.

On the other hand, implicit test setup and teardown adds actions that are not clearly visible when looking at a large testsuite. And a single package will often have different kinds of tests, some of which need a particular kind of setup and teardown and some of which need a different kind, so over time the complexity of the setup and teardown tend to increase.

I don't think there is an obvious win here so we are choosing to be explicit and simple, as is the common pattern for the Go language.

@dfawley

This comment has been minimized.

Copy link
Author

@dfawley dfawley commented Dec 13, 2018

The 3rd approach proposed in #27927 (comment) is explicit, simple, and no more magical than any test that uses the existing TestMain functionality. Essentially, it proposes the ability to easily enumerate the test functions in the package, and run them individually instead of as a batch.

Just because some test suites don't want per-test setup/cleanup for all tests in the package doesn't mean that all test suites don't. I'd like to ensure that all tests in grpc check for goroutine leaks (similar to net/http's checks). We have almost 800 end-to-end + grpc package tests. As of now, only ~200 have this leak check. With per-test setup/cleanup functionality, I could guarantee 100% compliance, and improve my test coverage and ability to isolate and find bugs. If we do it manually, we need to visit hundreds of test functions to add it, and also hope that everyone remembers to include the check going forward. The suggestion to use subtests is extremely unwieldy at this kind of scale as well, and would require us to introduce our own magic that makes our tests no longer explicit and simple.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Dec 13, 2018

I suggest that you try writing a new proposal that changes the testing.M structure to provide a list of tests with some way to run them. That is, change the focus from a functionality that has some clear difficulties to a smaller, simpler change that lets you do what you want.

@dfawley

This comment has been minimized.

Copy link
Author

@dfawley dfawley commented Jan 7, 2019

I suggest that you try writing a new proposal that changes the testing.M structure to provide a list of tests with some way to run them.

That is exactly what this issue proposed. If you would like to change the title or file a new proposal based on this one, please feel free to do so.

Instead, I have found a workaround to accomplish what we need using reflection, sub-tests, and a simple grep command to enforce its usage. If anyone else needs per-test setup/teardown and is interested in seeing our approach, take a look at grpc/grpc-go#2523, and feel free to copy and use grpctest.go in accordance with the license.

I still believe this is a significant shortcoming of Go's built-in testing framework, and our workaround is not ideal, but it is effective enough and unobtrusive enough that I can live with it and move on.

@FloatingSunfish

This comment has been minimized.

Copy link

@FloatingSunfish FloatingSunfish commented Dec 9, 2019

I've written a work-around for setUp and tearDown in Golang based on @ianlancetaylor's comment here.

I hope it helps someone out there!

@houqp

This comment has been minimized.

Copy link

@houqp houqp commented Dec 12, 2019

For those who need fixture injection and setup/teardown at different scopes, please feel free to give https://github.com/houqp/gtest a try.

@mpvl

This comment has been minimized.

Copy link
Member

@mpvl mpvl commented Dec 16, 2019

This blog: https://blog.golang.org/subtests describes how to use subtests for setup and tear-down.

@dfawley: to your points:

  • tests can be named by assigning a name to each test case that is passed to Run. The --run command-line flag can be used to select individual subtests just as much as they can select top-level tests (subtests are implemented as a generalization of top-level tests).
  • You can spread tests across different files. t.Run can call functions. It is a bit more writing, but this seems minimal and avoids magic.
  • Using functions, there will not be an additional level of indent.

As it is already possible to do setup/tear-down, it comes down to a matter of style and opinion. It seems to me that this kind of functionality falls into the same category as BDD testing support, which belongs in a separate package possibly implemented on top of the testing package along the lines @houqp referred to.

But I would first and strongly consider whether the suggested techniques are not sufficient. Adding setup/tear-down magic is yet another thing to learn for readers of code.

@dfawley

This comment has been minimized.

Copy link
Author

@dfawley dfawley commented Dec 16, 2019

@mpvl,

This blog: blog.golang.org/subtests describes how to use subtests for setup and tear-down.

The only setup & teardown mentioned in this blog post is a single setup/teardown surrounding a group of sub-tests. This issue is to have the setup/teardown before/after each sub-test indivually.

t.Run can call functions. It is a bit more writing, but this seems minimal and avoids magic.

It's not the "bit more writing" that bothers me, but the duplication. Needing to list all the test functions as sub-tests means the same test needs to be mentioned three times: first when writing the test, second to give t.Run() a name string, and third to invoke it. This adds too much potential for error in either omission or substitution. What I came up with avoids any of this duplication in exchange for a small amount of magic. Ideally the language would provide some help for this, but given the absence of it, I'll take some magic over error-prone techniques any day.

@mgwidmann

This comment has been minimized.

Copy link

@mgwidmann mgwidmann commented Dec 17, 2019

I dont get why this has to be so hacky. Simply extend the existing TestMain to provide an area for setup/teardown for each test like the following:

func TestMain(m *testing.M) {
	setupAll()
	code := m.RunEach(func(t *testing.T) {
	    setup(t)
        t.Run()
        teardown(t)
    })
	teardownAll()
	os.Exit(code)
}

Why not something like this?

@mpvl

This comment has been minimized.

Copy link
Member

@mpvl mpvl commented Jan 2, 2020

I like @mgwidmann's approach. It is quite straightforward and clear.

@sineemore

This comment has been minimized.

Copy link

@sineemore sineemore commented Feb 2, 2020

How about to use context.Context to manage resources associated with the test?

Consider the following:

func dbHelper(t *testing.T) *sql.DB {
      t.Helper()

      // create database connection...
      
      // teardown routine
      go func() {
            // routine waits on context channel and closes database handler
            <-t.Context().Done()
            db.Close()
      }()

      return db
}

func TestFoo(t *testing.T) {
        db := dbHelper(t)

        // test code continues...
}

Lots of resource providers already use context.Context to manage resource lifetime.

@dfawley

This comment has been minimized.

Copy link
Author

@dfawley dfawley commented Feb 3, 2020

@sineemore this misses the purpose of this proposal. Needing to call a function at the beginning/end of every test in a package introduces a large chance of error, i.e. it's easy to forget to make the call. In your case it's providing a resource -- it's more traditional in that case to have dbHelper return db.Close so that the test function can defer the cleanup.

@sineemore

This comment has been minimized.

Copy link

@sineemore sineemore commented Feb 3, 2020

it's more traditional in that case to have dbHelper return db.Close so that the test function can defer the cleanup.

@dfawley, yes, that's the way I'm doing it right now.

For me most of the time a test case requires some kind of resource, so it is possible to add per test functionality to resource provider function.

There were proposals to add context support already, btw.

@quantonganh

This comment has been minimized.

Copy link

@quantonganh quantonganh commented Feb 7, 2020

I dont get why this has to be so hacky. Simply extend the existing TestMain to provide an area for setup/teardown for each test like the following:

func TestMain(m *testing.M) {
	setupAll()
	code := m.RunEach(func(t *testing.T) {
	    setup(t)
        t.Run()
        teardown(t)
    })
	teardownAll()
	os.Exit(code)
}

Why not something like this?

@mgwidmann In the source code, I can only see the Run() method: https://github.com/golang/go/blob/master/src/testing/testing.go#L1172

Where is the RunEach()?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.