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: middleware for testing.TB (?) #40984

Closed
seebs opened this issue Aug 22, 2020 · 16 comments
Closed

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

seebs opened this issue Aug 22, 2020 · 16 comments
Labels
Projects
Milestone

Comments

@seebs
Copy link
Contributor

@seebs seebs commented Aug 22, 2020

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.

@gopherbot gopherbot added this to the Proposal milestone Aug 22, 2020
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Aug 23, 2020
@ianlancetaylor ianlancetaylor changed the title proposal: middleware for testing.TB (?) proposal: testing: middleware for testing.TB (?) Aug 23, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Aug 23, 2020

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
Copy link
Contributor Author

@seebs seebs commented Aug 23, 2020

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
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Aug 23, 2020

There is some relevant discussion at #27927.

@cornfeedhobo
Copy link

@cornfeedhobo cornfeedhobo commented Aug 23, 2020

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
Copy link
Contributor

@ConradIrwin ConradIrwin commented Aug 26, 2020

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: #27927 (comment), or another way of iterating over all tests. (I think this is more powerful than needed and harder to describe).
@cornfeedhobo
Copy link

@cornfeedhobo cornfeedhobo commented Aug 26, 2020

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
Copy link
Contributor Author

@seebs seebs commented Aug 26, 2020

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
Copy link
Contributor

@ConradIrwin ConradIrwin commented Aug 26, 2020

@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
Copy link
Contributor Author

@seebs seebs commented Aug 26, 2020

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 rsc moved this from Incoming to Active in Proposals Sep 23, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Sep 30, 2020

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
Copy link
Contributor Author

@seebs seebs commented Sep 30, 2020

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.

@rsc
Copy link
Contributor

@rsc rsc commented Sep 30, 2020

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.

For what it's worth, here is a Unix command that adds setup(t) to the top of every test function in a collection of files:

sed -i '' '/^func Test/a\
	setup(t)
' *_test.go

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.

There are many possible errors. This doesn't seem like one worth adding new API to package testing for. Especially since many setup functions will do something like return a database handle (far better than setting a global variable), and so it's clearly not going to be possible to forget.

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.

Again, this is a job for tools. At the least you need to make a change to every package in your large existing test suite. So if you're already doing per-directory work, it's not that hard to run the sed command above in that directory, or write an equivalent command in Go.

This is not something testing needs to do. Unlike, say, t.Setenv or t.Cleanup, there's no special benefit to doing it in testing.

@rsc
Copy link
Contributor

@rsc rsc commented Oct 7, 2020

Based on the discussion above, this seems like a likely decline.

@rsc rsc moved this from Active to Likely Decline in Proposals Oct 7, 2020
@dprotaso
Copy link

@dprotaso dprotaso commented Oct 13, 2020

I've written a small lib recently that seems relevant to this issue. It allows test authors to create their own test context (which embeds testing.T) with additional properties and allows for things to be scoped to the lifetime of a test (ie. http client). It accepts signatures utilizing this custom context (ie. TestFoo(t *myTestContext)) and subtest invocation require a func with the same types.

One of the main challenges this was addressing was the lack of discipline when accessing global test vars. This came about because we needed to access custom test flags. We're running e2e tests that have external services (k8s clusters). Eventually random test helpers were accessing these globals directly. To clarify one point this library is meant as a base to build more specialized tests for different domains (ie. serving & eventing) so these custom tests types & flags need to be composable across go modules.

The other angle: I'm trying to improve the utility of people writing new tests to an existing suite and reducing the complexity when reading them.

The recommendation of calling setup for each top-level test doesn't address the use case where each subtest might require it's own setup & context. Thus requires more effort in the suggested tooling.

I'm not sure what the right path forward but to summarize my thoughts:

  1. There's a need to hook into the lifecycle of the test without reducing it's readability/complexity etc.
  2. These hooks are not just behavioural but may require some properties to be associated with a test - ideally in a way that's easy to access (ie. no globals)
@rsc
Copy link
Contributor

@rsc rsc commented Oct 14, 2020

@dprotaso thanks for the feedback. It may well be that there is a need here, but we have yet to see a specific proposal that fits well into the testing package.

@rsc
Copy link
Contributor

@rsc rsc commented Oct 14, 2020

No change in consensus, so declined.

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

Successfully merging a pull request may close this issue.

None yet
7 participants