Start using -race flag in tests

[b00ris]

Problem

We have dataraces in status-go. And don't use -race flag in tests.

Implementation

blocking issues:
External data races:

1. go-ethereum
   1.1) les: wg data race fixed ethereum/go-ethereum#15365
   1.2) les: fix channels assignment data race ethereum/go-ethereum#15441
2. otto
   2.1) add mutex guard to object properties robertkrimen/otto#284

Internal data races:

1. Refactor access with proper mutex guard #258
2. Refactor TxQueueManager and TxQueue #338
3. Fix datarace in geth/signal  #452
4. Fix dataraces in transaction tests  #451
5. Fix dataraces in jail tests #457

Acceptance Criteria

• unit tests and e2e test in CI should run with -race flag and should pass

[divan]

+1 for start using race detector heavily.

It's important to keep in mind that race detector can only catch the races on the path covered by tests, so tests should be implemented with it in mind. We already have some tests that target race conditions, but that's rather an exception now. Also, race detector has performance penalty up to 10x-20x in memory and execution time.

Thus I suggest having a separate set of tests, probably under its own tag (go test -tags race?) and add a separate Makefile target for running tests with race detector. That will probably introduce some duplication of tests running during full CI cycle, but that's a good tradeoff and can be resolved if needed by inverse tags (// +build !race) at the top of the test file.

And the main challenge here still will be designing tests properly so they reflect actual program usage and cover actual code.

[tiabc]

@b00ris the scope of this issue is too huge, let's move step by step. Can you start from a small innocent package first and keep it really small? I'm afraid in the current setup this issue is going to take infinite time.

[b00ris]

@tiabc No, scope of this issue is not huge. This is issue only adding -race flag to tests and updating vendor libs to version with fixed races. I add to this issues all blocking race issues, which could be done independenly. After all blocked issues this issue could be done fastly.

[adambabik]

@divan I am not sure what problem do you want to resolve with -tags race. I guess we don't need special tests that run some code in a loop to detect races. It's enough that particular code paths are covered by regular unit tests, right? I am not a fan of introducing this kind of things because it makes it difficult to decide whether I should write separate tests or not. If so, what conditions need to be met etc.

As you suggested, creating a separate target in Makefile for unit tests with -race and running it always in CI should be enough. I don't think performance penalty is that huge. Let's measure it :)

[adambabik]

@b00ris Very nice, I like this type of issues that collect dependencies 👍

Btw. do you use ZenHub? You could set up dependencies with it and it would be easier to navigate.

I would also update AC:

update go-ethereum with fixed data races

This is an implementation detail actually.

unit tests and e2e tests runs with -race flag

I am not sure we need to run e2e tests with -race. We definitely need to run unit tests with it. Please make it specific like: "unit tests in CI should run with -race flag and should pass" and add something like: "there is a new target in Makefile that allows running unit tests with -race flag".

[JekaMas]

@adambabik I've just finished #456 and fixed races in code of e2e tests TestNonExistentQueuedTransactions, TestNonExistentQueuedTransactions, TestDiscardMultipleQueuedTransactions, TestEvictionOfQueuedTransactions, TestCompleteTransaction.
I think we need to run all tests with -race flag in other case we cant rely on our code and tests.

[divan]

@adambabik

It's enough that particular code paths are covered by regular unit tests, right?

Actually, not. Imagine you have package with data race:

package foo
var v int64
func foo() { v = 42 }
func bar() { _ = v }

and test:

package foo
import "testing"
func TestFoo(t *testing.T) { foo() }
func TestBar(t *testing.T) { bar() }

Running go test -race will not report any problem here, even that all code is covered by test.

That's what I meant by "should be implemented with it in mind". We need to look at the real world usage of functions being tested and ask questions like - "are foo() and bar() supposed to be called concurrently?". If so, we need either write new test that simulates this and allows race detector to catch possible races:

func TestFooBar(t *testing.T) { go foo(); bar() }

or instrument individual tests with t.Parallel():

func TestFoo(t *testing.T) { t.Parallel(); foo() }
func TestBar(t *testing.T) { t.Parallel(); bar() }

But I agree that I may be extending the purpose of this issue too much — it's rather about "fixing existing data races" than about "improve chances to catch race detection". Anyway, that being said, another comment on:

I am not sure we need to run e2e tests with -race

Actually, that makes sense for many cases, because our e2e tests try to simulate real-world usage and increase chances to reveal race condition if any. But as they're mostly long-running, -race mode will make them run even longer.

I am not sure what problem do you want to resolve with -tags race
I am not a fan of introducing this kind of things

Yeah, I was trying to find a sweet spot between "run tests quickly to catch implementation errors" vs "run long race-enabled testing" — first one is something that we use often (I run tests from IDE just to make sure new code didn't break anything, for example), while the second one is one-time run before submitting PR. Just having Makefile target might be enough — but our e2e tests are long-running anyway, so running it two times in CI (without and with race detector) will more than double testing time. But I agree that it'll bring some confusion, so we can go without it for now and decide later if there is benefit to adding build tags.

[adambabik]

@JekaMas I agree that our e2e tests itself should be free from race conditions. But if we have proper unit tests coverage and run unit tests with -race flag I believe that running e2e tests with -race flag will be a duplication of the same checks. Have you checked maybe how long our e2e tests with -race flag execute vs without it? It's my only concern that instead of 6 mins it will be much much longer.

[adambabik]

@divan Right, so we need to have proper integration tests as well, not only unit tests (that's why I was arguing about using mocks in other PRs). If any function call calls any other functions concurrently, this should be tested. That's why I think that we don't need to bring special category of "race" tests. We just need to keep that in mind writing regular unit/integration tests. How does it sound? We should put that into a document about writing tests btw.

With e2e tests, I have the same concern that it will take much longer but it won't bring a lot of value because race issues should be discovered by proper unit and integration.

[divan]

@adambabik, to be honest, I never did so (implementing special category for "race" tests), but as a result, most of the tests I've written and seen are poor from the simulating real usage. At best, I just add t.Parallel() and hope that it's enough. But still I think the important step is to understand how each package is used and design test to test this behavior. Note, how this differs from "test individual functions". I would say that e2e tests (assuming they're covering real-world scenarios) are best candidates for being run under race detector. The performance penalty is a thing we cannot avoid, unfortunately.

That's why I proposed to make a tradeoff and run some tests without race detector, but again, I agree that it can bring more confusion. Let's ditch this idea for now.

[mandrigin]

looks like a duplicate of #710, hence, closing
feel free to reopen if you disagree