code review 107280043: testing: add testing.Race to allow authors to reduce th...

testing: add testing.Race to allow authors to reduce the scope of their tests under the race runtime

Fixes issue 7706.

Discussion:

This CL adds a new function, testing.Race to the testing package. The author of issue 7706 original requested that something similar be added to the runtime/race package however I feel this approach is more convenient for the following reasons:

* The use case in issue 7706 spoke of reducing or skipping tests when run under the race enabled runtime. Adding the helper method to the testing package rather than the runtime/race package means authors don't need to import an additional package.

* The use of this function is similar to the existing testing.Short function, an example usage might be:

    if testing.Short() || testing.Race() {
           t.Skip("skipping expensive test")
    }

[dave_cheney.net, 2014-06-21 05:37:25]

Hello rsc@golang.org, dvyukov@google.com (cc:
golang-codereviews@googlegroups.com),

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

[dvyukov, 2014-06-21 16:17:01]

in the first line of description: Russ says to not add () after function names;
and also remove trailing dot

https://codereview.appspot.com/107280043/diff/40001/src/pkg/testing/testing.go
File src/pkg/testing/testing.go (right):

https://codereview.appspot.com/107280043/diff/40001/src/pkg/testing/testing.g...
src/pkg/testing/testing.go:192: // Race reports whether this test is run under
the race enabled runtime.
"race enabled runtime" is a bit cryptic for ordinary people
s/is run under the race enabled runtime/is run under the race detector/

[dvyukov, 2014-06-21 16:17:55]

LGTM to me, but obviously wait for rsc/r

[dave_cheney.net, 2014-06-22 07:47:30]

Hello rsc@golang.org, dvyukov@google.com (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[r2, 2014-06-26 14:32:11]

NOT LGTM

testing.Short is used to make a test suite run fast enough on small machines.
Proper use expects that correctness is well covered even if long tests are
skipped.

Code that skips a test when the operating system is Windows, for example, is
dealing with unpleasant realities. Ideally such tests would be protected by a
build tag, but it is inconvenient to break them out just for the sake of one
peculiar property of a host operating system. Also, it's important to remember
to revisit such tests from time to time and make sure they really do need to be
skipped.

This CL suggests a third category: correctness under the race detector. (If it's
just a matter of slowness, there's testing.Short for that.) All tests should
pass under the race detector, unless the race detector has a bug, in which case
that should be fixed. Here the idea of skipping a test for correctness issues
feels very wrong, since tests will pass but the code is incorrect - it has a
race! Therefore it seems very unwise to encourage people to skip tests that
should succeed, and the addition of a special function to do this is
counterproductive.

For those rare cases where, for whatever reason, it is necessary, custom
solutions are easy enough to implement. But making it feel normal to deny the
race detector a chance to verify a test is working goes against the very idea of
the facility.

[rsc, 2014-06-26 14:47:49]

I am very uncomfortable with this. I think it sends the wrong message.
Tests shouldn't be changing their behavior based on whether the race
detector is watching.

Russ

[r, 2014-06-26 15:12:47]

I sent a long review to the CL, but it hasn't shown up in this thread.
NOT LGTM is the short version. Click on the CL link for the full one.



On Thu, Jun 26, 2014 at 7:47 AM, Russ Cox <rsc@golang.org> wrote:
> I am very uncomfortable with this. I think it sends the wrong message. Tests
> shouldn't be changing their behavior based on whether the race detector is
> watching.
>
> Russ
>

[rsc, 2014-06-26 15:32:28]

testing

[rsc, 2014-06-26 15:41:02]

testing 2

[rsc, 2014-06-26 15:47:20]

testing 3

[dave_cheney.net, 2014-07-01 05:31:52]

*** Abandoned ***