Fix Flaky Tests

[rawfalafel]

This fixes #377, as well as flaky tests in sync and rpc that were reproduced on buildkite but haven't been reported on GitHub.

I didn't find any race conditions for blockchain, casper, types, so I re-enabled race there as well.

[rawfalafel]

The problem here is that this goroutine is still running during execution of the next test. This isn't actually testing anything, so I just deleted it.

[rawfalafel]

The issue here is that the test isn't waiting for the goroutine to finish.

[codecov]

Codecov Report

Merging #636 into master will decrease coverage by 0.17%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #636      +/-   ##
==========================================
- Coverage   73.14%   72.96%   -0.18%     
==========================================
  Files          52       52              
  Lines        3459     3459              
==========================================
- Hits         2530     2524       -6     
- Misses        706      713       +7     
+ Partials      223      222       -1

Impacted Files	Coverage Δ
beacon-chain/node/node.go	48.46% <0%> (-3.07%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd2073e...3cabce9. Read the comment docs.

[rawfalafel]

Same here. The assertion needs to come after exitRoutine <- true.

Another issue is that this test originally combined two tests that should have been separated.

func TestBadExample(t *testing.T) {
  go func() {
    testSomething(testChan)
    <-exitRoutine
  }
  testChan <- struct{}{}
  assertSomething()

  go func() {
    testSomething(testChan)
    <-exitRoutine
  }
  testChan <- struct{}{}
  exitRoutine <- true
  assertSomethingElse() 
}

The above is a race condition on assertSomething(). The fix is to separate into two tests:

func TestSomething(t *testing.T) {
  go func() {
    testSomething(testChan)
    <-exitRoutine
  }
  testChan <- struct{}{}
  exitRountine <- true
  assertSomething()
}

fun TestSomethingElse(t *testing.T){
  go func() {
    testSomething(testChan)
    <-exitRoutine
  }
  testChan <- struct{}{}
  exitRoutine <- true
  assertSomethingElse() 
}

In general, each test should assert one case anyways. Long tests that sequentially assert multiple cases are hard to read/debug.

[rawfalafel]

This test had the same issue. They need to be separated, and the exitRoutine check was missing.

[rauljordan]

Amazing - thank you!

[terencechain]

👍