Add test race detection to pipeline. Fix race conditions in tests (#1956

)
NethermindEth · Jul 18, 2024 · 407673a · 407673a
1 parent c2edc41
commit 407673a
Show file tree

Hide file tree

Showing 4 changed files with 22 additions and 15 deletions.
diff --git a/.github/workflows/juno-test.yml b/.github/workflows/juno-test.yml
@@ -50,6 +50,10 @@ jobs:
       - name: Tests (No Coverage)
         if: matrix.os != 'ubuntu-latest'
         run: make test
+
+      - name: Tests (Race Detection)
+        if: matrix.os == 'ubuntu-latest'
+        run: make test-race
 
       - name: Benchmark
         run: make benchmarks

diff --git a/Makefile b/Makefile
@@ -12,8 +12,12 @@ endif
 
 ifeq ($(shell uname -s),Darwin)
 	export CGO_LDFLAGS=-framework Foundation -framework SystemConfiguration
+	# for test-race we need to pass -ldflags to fix linker warnings on macOS
+	# see https://github.com/golang/go/issues/61229#issuecomment-1988965927
+	TEST_RACE_LDFLAGS=-ldflags=-extldflags=-Wl,-ld_classic
 else
 	export CGO_LDFLAGS=-ldl -lm
+	TEST_RACE_LDFLAGS=
 endif
 
 rustdeps: vm core-rust compiler
@@ -49,7 +53,7 @@ test-cached: rustdeps ## tests with existing cache
 	go test $(GO_TAGS) ./...
 
 test-race: clean-testcache rustdeps
-	go test $(GO_TAGS) ./... -race
+	go test $(GO_TAGS) ./... -race $(TEST_RACE_LDFLAGS)
 
 benchmarks: rustdeps ## benchmarking
 	go test $(GO_TAGS) ./... -run=^# -bench=. -benchmem

diff --git a/sync/sync_test.go b/sync/sync_test.go
@@ -24,8 +24,6 @@ import (
 const timeout = time.Second
 
 func TestSyncBlocks(t *testing.T) {
-	t.Parallel()
-
 	mockCtrl := gomock.NewController(t)
 	t.Cleanup(mockCtrl.Finish)
 
@@ -56,7 +54,6 @@ func TestSyncBlocks(t *testing.T) {
 	}
 	log := utils.NewNopZapLogger()
 	t.Run("sync multiple blocks in an empty db", func(t *testing.T) {
-		t.Parallel()
 		testDB := pebble.NewMemTest(t)
 		bc := blockchain.New(testDB, &utils.Mainnet)
 		synchronizer := sync.New(bc, gw, log, time.Duration(0), false)
@@ -69,7 +66,6 @@ func TestSyncBlocks(t *testing.T) {
 	})
 
 	t.Run("sync multiple blocks in a non-empty db", func(t *testing.T) {
-		t.Parallel()
 		testDB := pebble.NewMemTest(t)
 		bc := blockchain.New(testDB, &utils.Mainnet)
 		b0, err := gw.BlockByNumber(context.Background(), 0)
@@ -88,7 +84,6 @@ func TestSyncBlocks(t *testing.T) {
 	})
 
 	t.Run("sync multiple blocks, with an unreliable gw", func(t *testing.T) {
-		t.Parallel()
 		testDB := pebble.NewMemTest(t)
 		bc := blockchain.New(testDB, &utils.Mainnet)
 
@@ -142,7 +137,6 @@ func TestSyncBlocks(t *testing.T) {
 }
 
 func TestReorg(t *testing.T) {
-	t.Parallel()
 	mainClient := feeder.NewTestClient(t, &utils.Mainnet)
 	mainGw := adaptfeeder.New(mainClient)
 
@@ -153,22 +147,21 @@ func TestReorg(t *testing.T) {
 
 	// sync to integration for 2 blocks
 	bc := blockchain.New(testDB, &utils.Integration)
-	synchronizer := sync.New(bc, integGw, utils.NewNopZapLogger(), time.Duration(0), false)
+	synchronizer := sync.New(bc, integGw, utils.NewNopZapLogger(), 0, false)
 
 	ctx, cancel := context.WithTimeout(context.Background(), timeout)
 	require.NoError(t, synchronizer.Run(ctx))
 	cancel()
 
 	t.Run("resync to mainnet with the same db", func(t *testing.T) {
-		t.Parallel()
-		bc = blockchain.New(testDB, &utils.Mainnet)
+		bc := blockchain.New(testDB, &utils.Mainnet)
 
 		// Ensure current head is Integration head
 		head, err := bc.HeadsHeader()
 		require.NoError(t, err)
 		require.Equal(t, utils.HexToFelt(t, "0x34e815552e42c5eb5233b99de2d3d7fd396e575df2719bf98e7ed2794494f86"), head.Hash)
 
-		synchronizer = sync.New(bc, mainGw, utils.NewNopZapLogger(), time.Duration(0), false)
+		synchronizer = sync.New(bc, mainGw, utils.NewNopZapLogger(), 0, false)
 		ctx, cancel = context.WithTimeout(context.Background(), timeout)
 		require.NoError(t, synchronizer.Run(ctx))
 		cancel()

diff --git a/utils/throttler_test.go b/utils/throttler_test.go
@@ -2,6 +2,8 @@ package utils_test
 
 import (
 	"errors"
+	"sync"
+	"sync/atomic"
 	"testing"
 	"time"
 
@@ -13,18 +15,22 @@ import (
 func TestThrottler(t *testing.T) {
 	throttledRes := utils.NewThrottler(2, new(int)).WithMaxQueueLen(2)
 	waitOn := make(chan struct{})
-	ranCount := 0
 
+	var runCount int64
 	doer := func(ptr *int) error {
 		if ptr == nil {
 			return errors.New("nilptr")
 		}
 		<-waitOn
-		ranCount++
+		atomic.AddInt64(&runCount, 1)
 		return nil
 	}
+
+	var wg sync.WaitGroup
 	do := func() {
+		wg.Add(1)
 		go func() {
+			defer wg.Done()
 			require.NoError(t, throttledRes.Do(doer))
 		}()
 		time.Sleep(time.Millisecond)
@@ -52,6 +58,6 @@ func TestThrottler(t *testing.T) {
 	// release the jobs waiting
 	waitOn <- struct{}{}
 	waitOn <- struct{}{}
-	time.Sleep(time.Millisecond)
-	assert.Equal(t, 4, ranCount)
+	wg.Wait()
+	assert.Equal(t, int64(4), runCount)
 }