proposal: cmd/go: run benchmarks for one iteration when testing

[zeebo]

Currently, benchmarks are only run when passing the -bench flag to go test. Usually, CI checks don't run benchmarks because of the extra time required and noise reducing the usefulness of the results. This leads to situations where benchmark code can rot, causing it to become a liability instead of an asset. Instead, go test without the -bench flag could also run all of the benchmarks for a single iteration (b.N = 1) and just report PASS or FAIL like a normal test. Then benchmark code would be exercised at least as often as test code, preventing rot.

Some details to work out:

• What about -run? I would suggest it additionally filters the benchmarks to run when -bench is not passed.

[davecheney]

go test -run=. -bench=. -benchtime=1x

[zeebo]

That's a good way of saying the default behavior this proposes.

Are you trying to make an argument one way or the other with that statement? If so, what argument?

[bcmills]

To give a recent example: cmd/api.TestBenchmark was broken from CL 164623 to CL 224619, and nobody had noticed because the benchmark wasn't covered in the Go project's own CI checks until CL 224038.

[mvdan]

I generally agree with this; I've been bitten by benchmarks going stale as well. I think benchmarks should be run as part of go test for the same reason that most of vet is now also run when testing; because otherwise, users will forget to check regularly. go test -run=. -bench=. -benchtime=1x exists, but it's not easy to type nor remember.

The only trouble I can imagine with this change is that it would cause pain if there are any low-quality benchmarks which would now suddenly run as part of go test. For example:

1. Benchmarks which do a lot of work upfront, before looping over N. Imagine one that takes a full second before the loop starts, so even with N=1, the added cost would be high.
2. Benchmarks where each iteration is expensive. Imagine a benchmark where each iteration takes a full second, so N=1 is still heavy.

You could say "you should never do either of those things", but sometimes what you need to benchmark is just very costly, especially when benchmarking a feature end-to-end. And I've seen a number of such heavy benchmarks in the past, where even with -benchtime=5s you'd get results with N=10.

I still think it's good to incentivize developers to write cheaper benchmarks, so I'm still in favor of this change. But for those cases where heavy benchmarks are useful, we should have a recommendation for keeping them around without slowing down go test. Perhaps skip them with testing.Short()? Put them in separate packages, so that go test ./... runs them in parallel? Use build tags like // +build slowbench?

The concern above reminds me of #36586 too, because runnable examples and benchmarks are similar in how they can't run in parallel with each other, or with tests.

[mvdan]

What about -run? I would suggest it additionally filters the benchmarks to run when -bench is not passed.

I would further suggest that go test -short should ignore benchmarks entirely, too.

[bcmills]

But for those cases where heavy benchmarks are useful, we should have a recommendation for keeping them around without slowing down go test.

Perhaps skip them with testing.Short()?

Yes.

Put them in separate packages, so that go test ./... runs them in parallel?

That seems worse than skipping with testing.Short(): go test ./... would run them in parallel, but go test -bench=. foo wouldn't give any hint that some benchmarks of foo were skipped. (And go test -bench=. all wouldn't find those benchmarks when the package is imported from some other module.)

Use build tags like // +build slowbench?

That seems excessive, given the option of using testing.Short()..?

[seebs]

I endorse a default of -bench=. -benchtime=1x for go test, it would make my life better.

[rsc]

There are a bunch of benchmarks that do a lot of setup work anticipating that they will be run for the full 2s or more.
I don't believe this change is well-advised at all. If your benchmark is testing something, make a test.

[rsc]

Usually, CI checks don't run benchmarks because of the extra time required and noise reducing the usefulness of the results. This leads to situations where benchmark code can rot, causing it to become a liability instead of an asset. Instead, go test without the -bench flag could also run all of the benchmarks for a single iteration (b.N = 1) and just report PASS or FAIL like a normal test.

CI systems today can use -bench=1x. Do any CI systems do that?

[josharian]

One way to make this opt-in would be a method on testing.T. Maybe something like:

// RunBenchmark runs the benchmark b once with b.N == 1.
// Any TB method called on b, such as Errorf or Skip, are treated as though they were called on b.
func (t *T) RunBenchmark(bench func(b *B))

Basically, sub-tests but crossing the test/benchmark line. This would also provide an opportunity for the calling testing.T to do other work, like calling Parallel.

(You could also imagine doing this for examples, although then the examples would be executed twice during a normal run.)

[cespare]

@mvdan I disagree with your implication that any benchmarks which do expensive setup work are "low-quality".

One system I work on, for instance, is a database which includes benchmarks that initialize themselves with a large synthetic table; the benchmark loops run queries against the table. It takes a lot of CPU and RAM to do the initial setup and I wouldn't want to run these every time I do the usual go test ./.... In CI we use -benchtime 1x to ensure that the code is exercised.

From my perspective, things work well as-is and I would prefer we not change the existing behavior.

[zeebo]

I don't believe this change is well-advised at all. If your benchmark is testing something, make a test.

Of course tests should be tests, but this proposal is to make it so that benchmarks don't go stale and become broken over time because no one runs them. Phrased differently, it's about testing the benchmark, not having the benchmark test the code.

CI systems today can use -bench=1x. Do any CI systems do that?

Picking a sample of systems I'm aware of, plus whatever was at the top of the github marketplace for CI

• Travis does not by default: https://docs.travis-ci.com/user/languages/go/#default-build-script
• Codefresh's example does not: https://codefresh.io/docs/docs/learn-by-example/golang/golang-hello-world/#run-unit-tests-as-part-of-the-pipeline
• CircleCI's example for Go uses gotestsum which does not: https://circleci.com/docs/2.0/language-go/#sample-configuration and https://github.com/gotestyourself/gotestsum#custom-go-test-command
• SemaphoreCI's example does not: https://docs.semaphoreci.com/examples/golang-continuous-integration/#sample-configuration

I would be surprised if running the benchmarks as part of go test was included in any documentation for any CI system. Furthermore, many examples do include more advanced features like generating code coverage reports, so it can't be explained by keeping examples simple.

As far as users overriding the default configuration to run benchmarks, I could not find a single .travis.yaml that executes benchmarks as part of go test using github search:

• None that explicitly run "go test"
• All of these use the default action (no benchmarks)
• Zero results contain the word "bench"

In my personal experience, 3 out of 3 different workplaces did not run benchmarks as part of the tests. Indeed, as @bcmills said, the Go project itself had a broken benchmark for a period of over a year.

I believe this is strong evidence that running benchmarks as part of testing is rare.

[kortschak]

I think this should be opt in, as it currently is with -benchtime=1x. I just ran the Gonum test suite with -benchtime=1x and it goes from completing in 2 minutes to taking more than half an hour. It may well be that we are an outlier (we have a lot of benchmarks and due to the nature of the things we do, we need to test a variety of problem sizes with algorithms that have — intrinsically — fairly unpleasant time complexity).