proposal: testing: middleware for testing.TB (?)

[seebs]

What version of Go are you using (go version)?

N/A, Any, or 1.14/1.15?

Does this issue reproduce with the latest release?

Sure.

What operating system and processor architecture are you using (go env)?

N/A.

What did you do?

Tried to write smarter tests.

What did you expect to see?

A thing sort of parallel to TestMain which provided a way to do things before/after individual tests.

What did you see instead?

Nothing like that.

This grew out of some discussions in the #tools channel in Gopher Slack, and some vague thoughts I've had about this a few times before. The original use case that got me thinking about this was trying to track down stray goroutines that were appearing during testing, because some tests weren't closing some resources. I implemented a tracker, which can check when these resources are opened or closed, and use TestMain to check, and report on, unclosed resources after all tests complete.

But what would be sort of neat would be a way to have each individual test check for unclosed resources after it's done, and then potentially, a corresponding way to make them use separate trackers so that they could coexist with parallel testing.

dominikh made a passing remark about this being a little structurally similar to something like middleware implementations in net/http, which take a handler and make a new handler that fixes a thing up, so that individual handlers people are writing in their problem domain don't need to think about all the other things that might be getting done for system-wide reasons.

I am not sure whether I like this idea or not, but I think it merits exploration. A significant challenge: Fatal/Fatalf exit the goroutine, and set test failure status, but some test wrappers might want a way to override that, and at least to run their own cleanup code.

Approximate sketch of an idea:

// this feels like the intro to an infomercial advertising Generics
type TestWrapper func(func (*testing.T)) func(*testing.T)
type BenchmarkWrapper func(func (*testing.B)) func(*testing.B)
type TBWrapper func(func (testing.TB)) func(testing.TB)
func (m *M) WrapTests(TestWrapper) { ... }

But it's not clear how the actual wrapper would be written. For an example, say you have a resource tracker which can confirm/deny that some class of resources has been correctly closed up. The obvious corresponding code for the above signatures is clunky:

func TrackTest(fn func (*testing.T)) (func (*testing.T)) {
    return func(t *testing.T) {
        tracker := newTracker()
        fn()
        if err := tracker.Check(); err != nil {
            t.Fatalf("resource tracker: %v", err)
        }
    }
}

That feels very duplicative. It might be nicer to have a simpler interface where the wrapper function is just expected to be invoked in place:

func TrackTest(fn func (*testing.T), t *testing.T) {
    tracker := newTracker()
    fn(t)
    if err := tracker.Check(); err != nil {
        t.Fatalf("resource tracker: %v", err)
    }
}

But what if we want this code to run even if the test failed? I suppose one answer would be that the function passed to this is not the original test function, but rather, a fancy wrapper synthesized by pkg/testing which arranges that the goroutine this is called in doesn't abort if the inner test fails.

And you might want the abiity to intercept failures, or check that you got "expected" failures (see the discussion in #39903). Which might imply a rather more elaborate setup for a test wrapper, but I'm not able to think of one that seems reasonable and sensible. It seems non-ideal to require the user to implement the full testing.T interface (and raises its own new questions in the case where T picks up new methods, like Cleanup).

I think the underlying proposal is roughly "it would be nice to be able to run things before/after individual tests, which could cause those tests to fail". The middleware-flavored solution might be the wrong one.

[ianlancetaylor]

Sorry, I'm not really clear on what is being proposed here. You want to run some code before and after each test? Could you just use subtests?

[seebs]

Well, in theory I could, but say I have 600+ tests, already written, in functions with names like TestThisThing. I could, in principle, just write gigantic test wrappers and indent them all a tab into a t.Run(), but then the thing I actually want to run is sort of a helper function which does whatever pre-test things, runs the test function, then does whatever post-test things, so it ends up looking like

func TestThingWithWrapper(t *testing.T) {
    t.Run("foo", func(t *testing.T) {
        preTestHook(t)
        deferPostTestHook(t)
        actualCodeForFoo()
    })
}

only with one of those for every test function. Also, that rewrite actually has to happen -- I can perhaps automate it, but the actual code Go gets has to be rewritten like that, which seems like a lot of boilerplate to be repeating for every single test function.

So I'm thinking of something with basically the same role as TestMain(), except one-call-per-test, rather than one-call-program-wide. And in particular, the ability to non-intrusively apply this to potentially-hundreds of existing tests, without changing their code.

So, as an example, say I think some tests are doing something they shouldn't be doing -- maybe leaking temporary files or something. So I write a TestMain() which sets up the directory that TempDir will use to point it at an empty directory, and then I want to, after each test, check that the directory is empty, because if it isn't, that test has done something wrong. And if I can just quickly toss in "m.Wrap(func(t *testing.T) { checkTmpDir(t) })` or something like that, that seems like a way better workflow that requires a lot less manual effort or overseeing automated changes to all those functions.

The exact signature of such a thing is tricky to get right, though.

[ianlancetaylor]

There is some relevant discussion at #27927.

[cornfeedhobo]

I think your code snippets make the intent unclear, but I agree in general; I'd really like a way of wrapping each individual test, maybe even including each sub-test.

Sure one could use sub-tests, but that doesn't help an existing large project.

[ConradIrwin]

Now that we have t.Cleanup, I think we don't need middleware per-se. If we could run some code just before each test and pass it a t *testing.T, then it could register code to run after the test using t.Cleanup().

In my experience, there are two different kinds of test setup. One is setting up the preconditions for the test (creating the right data, and configuring things just so), and I think this kind of setup should live in each test. The second is ensuring that each test runs in a correct, safe, environment.

Environment setup tends to appear most often when writing tests that exercise large systems. It comes up for me in trying to ensure that network requests are intercepted in each test, or (as @seebs mentions) for ensuring that tests doesn't leak resources, or from #27927, ensuring that goroutines do not linger after the test exits. This environmental setup is best handled at a package level, not per-test, because it removes the burden of remembering to set and check incidental properties of the system from each test author.

Problem statement

Packages often need to ensure that every test is run in a safe, isolated environment (for example, by disallowing network calls, or ensuring resources are cleaned up).

There are two ways to do this in Go today:

1. Call a function in each test.
2. Re-arrange the code test to call sub-tests and a setup function in a loop (example)

They both suffer from different downsides. Method 1. requires each test to opt-in to the setup (which is easy to forget when writing new tests, and time-consuming to change when your requirements change). Method 2. doesn't allow your setup code to access t *testing.T (which is useful for allowing the test environment to fail the test cleanly) and requires structuring your code differently (either with loops you write manually, or using a third-party library).

Proposal:

1. Add a method Setup(func (t testing.TB)) to testing.M. Each time this method is called, it appends the function to an internal slice. Immediately before a top-level Test or Benchmark is run, each function in the slice is executed (in order) with the value of *testing.T or *testing.B that will be passed to the Test/Benchmark. If the function calls SkipNow or FailNow or any of the related functions, then the failure is reported as though the Test/Benchmark itself failed, and the Test/Benchmark is not run.
2. Add a method Setup(func (t testing.TB)) to testing.T and testing.B. It works in the same way for each sub-test or sub-benchmark that is subsequently passed to t.Run or b.Run.

For example, the following code snippets would have identical behaviour:

func TestBoops(t *testing.T) {
  t.Run("boop1", func (t *testing.T) {
    setup(t)
    if boop1 != "1" {
      t.Fatal("boop1 is not 1")
    }
  }
  t.Run("boop2", func (t *testing.T) {
    setup(t)
    if boop2 != "2" {
      t.Fatal("boop2 is not 2")
    }
  }
}

func TestBoops(t *testing.T) {
  t.Setup(setup)
  t.Run("boop1", func (t *testing.T) {
    if boop1 != "1" {
      t.Fatal("boop1 is not 1")
    }
  }
  t.Run("boop2", func (t *testing.T) {
    if boop2 != "2" {
      t.Fatal("boop2 is not 2")
    }
  }
}

Or these two:

func TestMain(m *testing.M) {
  os.Exit(m.Run())
}

func TestBoop1(t *testing.T) {
  setup(t)
  if boop1 != "1" {
     t.Fatal("boop1 is not 1")
   }
}

func TestMain(m *testing.M) {
  m.Setup(setup)
  os.Exit(m.Run())
}

func TestBoop1(t *testing.T) {
  if boop1 != "1" {
     t.Fatal("boop1 is not 1")
   }
}

I'm not sure about the name Setup, other frameworks use BeforeEach; but Setup seemed to pair with Cleanup. Other ideas are welcome.

Alternative ideas:

• (a slight modification to this proposal) t.Setup(func (t *testing.T) and b.Setup(func (b *testing.B), this would maybe feel a bit more natural in terms of making it clear the type passed to the function; but it means you can't share setup functions between benchmarks and tests, and it might encourage people to start calling t.Parallel or b.Run in the Setup code, which is probably not a good idea, although it will work.
• Add special behaviour to go test so that if a function matching SetupTest(t *testing.T) exists in the package, that will be called immediately prior to executing the body of every top-level test. (This has the disadvantage of not being backward compatible for anyone who already has a method like that which they call manually, is also more magic; but the advantage of reducing the number of uses for TestMain).
• Add a RunEach method to testing.M as described here: proposal: testing: per-test setup and teardown support #27927 (comment), or another way of iterating over all tests. (I think this is more powerful than needed and harder to describe).

[cornfeedhobo]

One is setting up the preconditions for the test (creating the right data, and configuring things just so), and I think this kind of setup should live in each test.

This seems really opinionated, especially when @seebs has explicitly pointed out this is to avoid refactoring a large and existing code base, and what you have purposed is just another flavor of refactoring. Or maybe I've misunderstood?

Python's unittest is an iconic example with support for setUp/tearDown as well as setUpClass/tearDownClass. It seems rational enough that golang should support similar functionality that does not require refactoring.

[seebs]

The specific case that I was thinking about when I drafted this was "each test opens-or-starts some resources/services/etc, they should get closed", and while it's perfectly reasonable to say "well, that seems like it should be the responsibility of the user", that raises the question of "okay, and how do I test that the user did it?" And if you have to manually call a cleanup/verify/whatever function in every test, it is possible to forget it. And I would like to make it impossible to make the mistake of "forget to verify that things get shut down", and that's where it would be nice to be able to externally impose this on every test without having to touch the test.

I basically like the idea of a RunEach or something similar, but it does have the same sort of problems.

There's a secondary difficulty -- there's no way to pass customized information in to the individual test functions that get called. So, for instance, if you wanted to have your wrapper set up some environment, and then point each test at the environment it was supposed to use, you couldn't, because all the test function gets is the testing.T. I don't have a solution to that. (t.WithValue()...?)

[ConradIrwin]

@seebs I strongly agree with supporting the use-case "each test opens-or-starts some resources/services/etc, they should get closed", but I was clearly unclear about how to do it :) In the given proposal can use the existing t.Cleanup inside a setup function as it runs at the start of each test, so, to rewrite your original example:

func setupTracker(t testing.TB) {
      tracker := newTracker()
      t.Cleanup(func () {
           if err := tracker.Check(); err != nil {
                t.Fatalf("resource tracker: %v", err)
            }
       }
}

func TestMain(m *testing.M) {
  m.Setup(setupTracker)
  os.Exit(m.Run())
}

I hadn't thought about the setup code passing values to the test. Currently our environment setup code initializes our "dependencies" package which contains a bunch of global variables (like the http client to use, the database connection, and configuration tokens), and all our app and test code reads out of that. You could implement the same thing for tests without any further changes to the test object by using package-level variables:

var db *sql.Conn

func setup(t testing.TB) {
  db = sql.Open(...)
  t.Cleanup(func () {
    if err := db.Close(); err != nil {
      t.Fatalf("db.Close failed: %v", err)
    }
  })
}

It does make it a little harder to use parallel tests (as the test has to take a local copy of each variable before calling t.Parallel()), but this hasn't been a problem for us in practice (we get our parallelism at the package level with go test -p).

[seebs]

So, consider that to avoid being intrusive, the tracker needs to somehow be either global or part of a context the test runs in. And if it's global, you can't do after-each-test checking and also have tests that run in parallel. And unfortunately, in our structure, it's actually really hard for things to pass the context they want into things, but then, that's a problem for us with or without the test infrastructure.

I think m.Setup would actually work reasonably well for my use case. As you note, m.Setup is sufficient to provide post-test cleanup. I keep thinking of the idea of making it so that the cleanup function can Unfail the test, but then I think of all the many reasons for which that's an incredibly bad idea.

[rsc]

I'm at quite a loss to understand what is being proposed exactly.
It seems like if you have setup and teardown that is global, you can add it to TestMain.
If you have per-test setup and teardown, the addition of Cleanup means you can add one line to each test:

func TestFoo(t *testing.T) {
    setup(t)
    ...
}

where setup calls t.Cleanup to register the cleanup.

It does not seem like adding one line to each test is a significant burden requiring new API.
Pretty much any test helper requires a single call at minimum.
We do this kind of thing all the time in our own tests and it works well.

What is the specific functionality that you suggest we add, and how does it simplify the TestFoo above?

[seebs]

Adding one line to each test doesn't seem like a huge burden, if you aren't looking at an existing code base with hundreds of tests in it. It seems slightly larger then. It's also an easy point of failure; if you write enough tests, sooner or later the helper function gets forgotten, and if it's possible to run a test without it, that may never get noticed.

I think I like ConradIrwin's suggestion about m.Setup() or something like that. So, when writing TestMain, you can specify a function to be invoked on every testing.TB immediately before the actual TestFoo or BenchmarkFoo function is called. I'm not sure it's the best possible interface, but it seems like a very small change from an API perspective: test.Main gets one new function, and that function should be (I think) easy to implement.

The utility here is mostly that, if you have a large existing test suite, and think of a helper you would like to apply globally to it, retroactively, it's easy to do that, and it gives you a much more solid guarantee that you don't forget even-once.

So, basically: Yes, any test helper requires a single call minimum. But if every test in an entire package should be making exactly the same single call, it seems like it would be nice to have an idiomatic way to express that once instead of in all those places.