Skip to content
This repository was archived by the owner on Sep 9, 2020. It is now read-only.

always enable race detector when testing #155

Closed
wants to merge 1 commit into from

Conversation

rakyll
Copy link
Contributor

@rakyll rakyll commented Jan 25, 2017

No description provided.

@mvdan
Copy link
Member

mvdan commented Jan 25, 2017

I usually leave both versions, go test and go test -race. Not particularly useful I guess, but if CI fails in the first, I know the race detector instrumentation had nothing to do with it. Thoughts?

@rakyll
Copy link
Contributor Author

rakyll commented Jan 25, 2017

I don't truly understand. If someone is contributing code with a race condition, we should never merge that. There is practically no difference between introducing bugs by a change and introducing a race with a change.

@sdboyer
Copy link
Member

sdboyer commented Jan 26, 2017

There is practically no difference between introducing bugs by a change and introducing a race with a change.

I don't think @mvdan was arguing there is a difference - just that it's useful to quickly know what the cause of the test failure is. My colloquialism for this is "test failure locality" - how easy your tests/testing harness make it to understand exactly where the problem is. So, something like this is valuable for the same class of reason that golang/go#4899 is valuable.

I think I probably would prefer to have a run both with and without -race, at least for now. Test running times aren't too terrible, so doubling them isn't the end of the world. It's also helpful because our tests are really spammy right now, so it's already hard enough to locate where the problems are.

@jessfraz thoughts?

@rakyll
Copy link
Contributor Author

rakyll commented Jan 26, 2017

It will obviously slow down the tests (and ugh, tests are already sloooooww). But, hey, that's why we run tests without the race detector locally when developing and want to depend on the CI to report them. I totally can add two lines (with and without race detector) but it will make waiting for CI even harder.

@sdboyer
Copy link
Member

sdboyer commented Jan 26, 2017

Eh, yeah, looking through travis and appveyor again...with spinup time, they already are frustratingly slow.

OK, let's just do the one, with -race on. (So, PR as-is, with conflicts resolved). We can always change it later :)

@mattn
Copy link
Member

mattn commented Jan 26, 2017

please rebase master.

@freeformz
Copy link

Thanks! I rebased and merged this.

@freeformz freeformz closed this Jan 26, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants