code review 5492060: testing: allow benchmarks to print and fail

testing: allow benchmarks to print and fail
Refactors the benchmarks and test code.
Now benchmarks can call Errorf, Fail, etc.,
and the runner will act accordingly.

Because functionality has been folded into an
embedded type, a number of methods' docs
no longer appear in godoc output. A fix is
underway; if it doesn't happen fast enough,
I'll add wrapper methods to restore the
documentation.

[r, 2011-12-19 23:25:37]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[bradfitz, 2011-12-19 23:39:54]

yay!

On Mon, Dec 19, 2011 at 3:25 PM, <r@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> testing: allow benchmarks to print and fail
> Refactors the benchmarks and test code.
> Now benchmarks can call Errorf, Fail, etc.,
> and the runner will act accordingly.
>
> Can't be checked in until godoc issue is
> resolved with embedded methods.
>
> Please review this at
http://codereview.appspot.com/**5492060/<http://codereview.appspot.com/5492060/>
>
> Affected files:
>  M src/pkg/testing/benchmark.go
>  M src/pkg/testing/testing.go
>
>
>

[adg1, 2011-12-20 02:26:15]

http://codereview.appspot.com/5492060/diff/5006/src/pkg/testing/benchmark.go
File src/pkg/testing/benchmark.go (right):

http://codereview.appspot.com/5492060/diff/5006/src/pkg/testing/benchmark.go#...
src/pkg/testing/benchmark.go:165: b.result = BenchmarkResult{b.N, b.duration,
b.bytes}
there's a race here. set this before sending the signal

http://codereview.appspot.com/5492060/diff/5006/src/pkg/testing/benchmark.go#...
src/pkg/testing/benchmark.go:243: } else {
continue in the previous if block instead of this else block?

http://codereview.appspot.com/5492060/diff/5006/src/pkg/testing/benchmark.go#...
src/pkg/testing/benchmark.go:248: // The output is likely to appear multiple
times because the benchmark
put this in a function (trimOutput?) and then you can use it to address the TODO
on line 241.

http://codereview.appspot.com/5492060/diff/5006/src/pkg/testing/testing.go
File src/pkg/testing/testing.go (right):

http://codereview.appspot.com/5492060/diff/5006/src/pkg/testing/testing.go#ne...
src/pkg/testing/testing.go:73: // common holds the elements common between T and
B.
say something about common methods, too?

[r, 2011-12-20 16:39:01]

Hello golang-dev@googlegroups.com, bradfitz@golang.org, adg@google.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-12-20 17:36:27]

LGTM

http://codereview.appspot.com/5492060/diff/11001/src/pkg/testing/benchmark.go
File src/pkg/testing/benchmark.go (right):

http://codereview.appspot.com/5492060/diff/11001/src/pkg/testing/benchmark.go...
src/pkg/testing/benchmark.go:225: signal := make(chan interface{})
Please move the chan into the construction of b, so that every chan
gets used only once.  This will ameliorate the problem where
a lingering goroutine sends on a channel unexpectedly
during a later benchmark/test.

http://codereview.appspot.com/5492060/diff/11001/src/pkg/testing/benchmark.go...
src/pkg/testing/benchmark.go:278: // Benchmark benchmarks a single function.
Useful for creating
It would be nice to have a function like this for tests (func Test).

http://codereview.appspot.com/5492060/diff/11001/src/pkg/testing/testing.go
File src/pkg/testing/testing.go (right):

http://codereview.appspot.com/5492060/diff/11001/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:239: signal := make(chan interface{})
Same comment: use each channel just once.
This will require keeping a list of the running tests
for the parallel loop below (instead of numParallel)
but it will be worth it.

[r, 2011-12-20 17:51:44]

*** Submitted as 67b329eb1ecb ***

testing: allow benchmarks to print and fail
Refactors the benchmarks and test code.
Now benchmarks can call Errorf, Fail, etc.,
and the runner will act accordingly.

Because functionality has been folded into an
embedded type, a number of methods' docs
no longer appear in godoc output. A fix is
underway; if it doesn't happen fast enough,
I'll add wrapper methods to restore the
documentation.

R=bradfitz, adg, rsc
CC=golang-dev
http://codereview.appspot.com/5492060