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

runtime/race: consider adding IsEnabled const #7706

Closed
dvyukov opened this issue Apr 4, 2014 · 2 comments
Closed

runtime/race: consider adding IsEnabled const #7706

dvyukov opened this issue Apr 4, 2014 · 2 comments

Comments

@dvyukov
Copy link
Member

dvyukov commented Apr 4, 2014

Sometimes it's required to temporary disable tests, reduce number of iterations or scale
timeouts under race detector.

We use this extensively for C++ race detector and memory checker in large projects that
develop quickly (e.g. Chromium).

Currently it's quite difficult to do, you need to add 2 new files (+build race/!race).
When the test is fixed, you need to delete the files consequently.

See e.g.:
https://golang.org/cl/44180043/diff/60001/src/pkg/runtime/pprof/pprof_test.go
where we have to completely disable the test, while we want to do it only under race
detector.

The proposal is to add something like
package race
const IsEnabled = true/false
@gopherbot
Copy link

Comment 1:

CL https://golang.org/cl/107280043 mentions this issue.

@robpike
Copy link
Contributor

robpike commented Jun 26, 2014

Comment 2:

Will not fix.
Text added in my rejection of this CL:
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.

Status changed to WontFix.

@rsc rsc added this to the Go1.4 milestone Apr 14, 2015
@rsc rsc removed the release-go1.4 label Apr 14, 2015
@golang golang locked and limited conversation to collaborators Jun 25, 2016
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants