chore: fix broken tests; remove || true in CI

.github/workflows/test.yml

@@ -16,13 +16,15 @@ jobs:
     strategy:
       matrix:
         runs-on: [ macos-15, macos-14, macos-13, ubuntu-22.04, ubuntu-20.04, windows-latest, arm-4core-linux ]
-        go-version: [ "1.23", "1.22" ]
+        go-version: [ "1.24", "1.23" ]
         cgo_enabled: [ "0", "1" ] # test it compiles with and without cgo
       fail-fast: false
     runs-on: ${{ matrix.runs-on }}
     steps:
-      - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
-      - uses: actions/setup-go@f111f3307d8850f501ac008e886eec1fd1932a34 # v5.3.0
+      - name: Checkout
+        uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
+      - name: Setup Go
+        uses: actions/setup-go@f111f3307d8850f501ac008e886eec1fd1932a34 # v5.3.0
         with:
           go-version: ${{ matrix.go-version }}
           cache: true
@@ -34,13 +36,22 @@ jobs:
           key: go-pkg-mod-${{ hashFiles('**/go.sum') }}
           restore-keys: go-pkg-mod-
 
+      # Install build-essential on ARM linux runners if CGO is to be enabled
+      - name: Install build-essential (ARM linux)
+        if: matrix.runs-on == 'arm-4core-linux' && matrix.cgo_enabled == '1'
+        run: |-
+          sudo apt update
+          sudo apt install -y build-essential
+
       - name: go test
         shell: bash
         run: |
           # Install gotestsum
           env GOBIN=$PWD go install gotest.tools/gotestsum@latest
           # Run the tests with gotestsum
-          env CGO_ENABLED=${{ matrix.cgo_enabled }} ./gotestsum -- -v ./... || true
+          ./gotestsum -- -v ${{ runner.os == 'Linux' && matrix.cgo_enabled == '1' && '-race' || '' }} ./...
+        env:
+          CGO_ENABLED: ${{ matrix.cgo_enabled }}
 
   # Same tests but on the official golang container for linux
   golang-linux-container:
@@ -50,14 +61,16 @@ jobs:
       image: golang:${{ matrix.go-version }}-${{ matrix.distribution }}
     strategy:
       matrix:
-        go-version: [ "1.23", "1.22" ]
+        go-version: [ "1.24", "1.23" ]
         distribution: [ bookworm, bullseye, alpine ]
         cgo_enabled: [ "0", "1" ] # test it compiles with and without cgo
       fail-fast: false
     steps:
-      - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
+      - name: Checkout
+        uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
       # Install gcc and the libc headers on alpine images
-      - if: ${{ matrix.distribution == 'alpine' }}
+      - if: matrix.distribution == 'alpine'
+        name: Install necessary packages (Alpine)
         run: apk add gcc musl-dev libc6-compat git
 
       - name: Go modules cache
@@ -72,7 +85,9 @@ jobs:
           # Install gotestsum
           env GOBIN=$PWD go install gotest.tools/gotestsum@latest
           # Run the tests with gotestsum
-          env CGO_ENABLED=${{ matrix.cgo_enabled }} ./gotestsum -- -v ./... || true
+          ./gotestsum -- -v ${{ matrix.cgo_enabled == '1' && '-race' || '' }} ./...
+        env:
+          CGO_ENABLED: ${{ matrix.cgo_enabled }}
 
   everything:
     name: All Tests

apisec/internal/timed/set_test.go → apisec/internal/timed/lru_test.go

@@ -6,17 +6,18 @@
 package timed
 
 import (
+	"context"
 	"runtime"
 	"sync"
-	"sync/atomic"
 	"testing"
 	"time"
 
 	"github.com/DataDog/appsec-internal-go/apisec/internal/config"
+	"github.com/DataDog/appsec-internal-go/apisec/internal/timed/testutil"
 	"github.com/stretchr/testify/require"
 )
 
-func TestSet(t *testing.T) {
+func TestLRU(t *testing.T) {
 	t.Run("New", func(t *testing.T) {
 		require.PanicsWithError(t, "NewSet: interval must be at least 1s, got 0s", func() { NewSet(0, UnixTime) })
 		require.PanicsWithError(t, "NewSet: interval must be at least 1s, got 10ms", func() { NewSet(10*time.Millisecond, UnixTime) })
@@ -54,30 +55,31 @@ func TestSet(t *testing.T) {
 	})
 
 	t.Run("rebuild", func(t *testing.T) {
-		var fakeTime atomic.Int64
-		fakeTime.Store(time.Now().Unix())
-		fakeClock := func() int64 { return fakeTime.Load() }
+		goCount := runtime.GOMAXPROCS(0) * 10
+		ctx, cancel := context.WithCancel(context.Background())
+		clock := testutil.NewFakeClock(ctx, t, goCount)
+		defer func() {
+			cancel()
+			clock.WaitUntilDone()
+		}()
 
-		subject := NewSet(config.Interval, fakeClock)
+		subject := NewSet(config.Interval, clock.Unix)
 
 		var (
-			goCount       = runtime.GOMAXPROCS(0) * 10
 			startBarrier  sync.WaitGroup
 			finishBarrier sync.WaitGroup
 		)
 		startBarrier.Add(goCount + 1)
 		finishBarrier.Add(goCount)
-		for g := range goCount {
+		for range goCount {
 			go func() {
 				defer finishBarrier.Done()
 				startBarrier.Done()
 				startBarrier.Wait()
 
 				for key := range uint64(config.MaxItemCount * 4) {
 					_ = subject.Hit(key)
-					if g == 0 {
-						fakeTime.Add(1)
-					}
+					clock.WaitForTick()
 				}
 			}()
 		}

apisec/internal/timed/table.go

@@ -168,11 +168,11 @@ func (d entryData) WithAccessTime(atime uint32) entryData {
 func (e copiableEntry) Compare(other copiableEntry) int {
 	tst := e.Data.SampleTime()
 	ost := other.Data.SampleTime()
-	if tst > ost {
+	if tst < ost {
 		// Receiver was sampled more recently (sorts higher)
 		return 1
 	}
-	if tst < ost {
+	if tst > ost {
 		// Receiver was sampled less recently (sorts lower)
 		return -1
 	}

apisec/internal/timed/testutil/fakeclock.go

@@ -0,0 +1,80 @@
+package testutil
+
+import (
+	"context"
+	"sync"
+	"sync/atomic"
+	"testing"
+	"time"
+)
+
+type FakeClock struct {
+	t        testing.TB
+	closed   chan struct{}
+	goCount  int
+	now      int64
+	needTick atomic.Bool
+	wg       sync.WaitGroup
+	cnd      sync.Cond
+}
+
+func NewFakeClock(ctx context.Context, t testing.TB, goroutineCount int) *FakeClock {
+	res := &FakeClock{
+		t:       t,
+		closed:  make(chan struct{}),
+		goCount: goroutineCount,
+		now:     time.Now().Unix(),
+		cnd:     *sync.NewCond(&sync.Mutex{}),
+	}
+	res.wg.Add(goroutineCount)
+	go res.tick(ctx)
+	return res
+}
+
+func (c *FakeClock) Unix() int64 {
+	return c.now
+}
+
+func (c *FakeClock) WaitForTick() {
+	c.cnd.L.Lock()
+
+	c.needTick.CompareAndSwap(false, true)
+	curTime := c.now
+
+	c.wg.Done()
+	for curTime == c.now && c.now > 0 {
+		c.cnd.Wait()
+	}
+	c.cnd.L.Unlock()
+}
+
+func (c *FakeClock) WaitUntilDone() {
+	<-c.closed
+}
+
+func (c *FakeClock) tick(ctx context.Context) {
+	c.t.Log("Start ticker!")
+	for {
+		select {
+		case <-ctx.Done():
+			// Context is cancelled, stop the ticker...
+			c.cnd.L.Lock()
+			c.now = 0
+			c.cnd.L.Unlock()
+			c.t.Log("Stop ticker!")
+			close(c.closed)
+			return
+		default:
+			if !c.needTick.Load() {
+				continue
+			}
+			c.wg.Wait()
+			c.cnd.L.Lock()
+			c.now++
+			c.needTick.Store(false)
+			c.cnd.Broadcast()
+			c.wg.Add(c.goCount)
+			c.cnd.L.Unlock()
+		}
+	}
+}

apisec/sampler_test.go

@@ -17,6 +17,7 @@ import (
 	"time"
 
 	"github.com/DataDog/appsec-internal-go/apisec/internal/config"
+	"github.com/DataDog/appsec-internal-go/apisec/internal/timed/testutil"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -77,21 +78,24 @@ func TestSampler(t *testing.T) {
 
 	for name, tc := range testCases {
 		t.Run(name, func(t *testing.T) {
-			ctx, cancel := context.WithCancel(ctx)
-			defer cancel()
-
 			assert.Positive(t, tc.SimulatedTPS)
 			assert.GreaterOrEqual(t, tc.ExpectedKeepRate, 0.0)
 			assert.GreaterOrEqual(t, 1.0, tc.ExpectedKeepRate)
 
+			const goroutineCount = 10 // Fixed to ensure consistent keep rate...
+			ctx, cancel := context.WithCancel(ctx)
+			clock := testutil.NewFakeClock(ctx, t, goroutineCount)
+			defer func() {
+				cancel()
+				clock.WaitForTick()
+			}()
+
 			var (
-				goroutineCount = max(2, runtime.GOMAXPROCS(0))
-				clock          = newFakeClock(ctx, t, goroutineCount)
-				subject        = newSampler(30*time.Second, clock.Unix)
-				sb             sync.WaitGroup // Start barrier
-				wg             sync.WaitGroup // Completion barrier
-				kept           atomic.Uint64
-				dropped        atomic.Uint64
+				subject = newSampler(30*time.Second, clock.Unix)
+				sb      sync.WaitGroup // Start barrier
+				wg      sync.WaitGroup // Completion barrier
+				kept    atomic.Uint64
+				dropped atomic.Uint64
 			)
 			sb.Add(1 + goroutineCount) // All child goroutines + this one...
 			wg.Add(goroutineCount)
@@ -207,70 +211,6 @@ func BenchmarkSampler(b *testing.B) {
 	}
 }
 
-type fakeClock struct {
-	t        testing.TB
-	goCount  int
-	now      int64
-	needTick atomic.Bool
-	wg       sync.WaitGroup
-	cnd      sync.Cond
-}
-
-func newFakeClock(ctx context.Context, t testing.TB, goroutineCount int) *fakeClock {
-	res := &fakeClock{
-		t:       t,
-		goCount: goroutineCount,
-		now:     time.Now().Unix(),
-		cnd:     *sync.NewCond(&sync.Mutex{}),
-	}
-	res.wg.Add(goroutineCount)
-	go res.tick(ctx)
-	return res
-}
-
-func (c *fakeClock) Unix() int64 {
-	return c.now
-}
-
-func (c *fakeClock) WaitForTick() {
-	c.cnd.L.Lock()
-
-	c.needTick.CompareAndSwap(false, true)
-	curTime := c.now
-
-	c.wg.Done()
-	for curTime == c.now && c.now > 0 {
-		c.cnd.Wait()
-	}
-	c.cnd.L.Unlock()
-}
-
-func (c *fakeClock) tick(ctx context.Context) {
-	c.t.Log("Start ticker!")
-	for {
-		select {
-		case <-ctx.Done():
-			// Context is cancelled, stop the ticker...
-			c.cnd.L.Lock()
-			c.now = 0
-			c.cnd.L.Unlock()
-			c.t.Log("Stop ticker!")
-			return
-		default:
-			if !c.needTick.Load() {
-				continue
-			}
-			c.wg.Wait()
-			c.cnd.L.Lock()
-			c.now++
-			c.needTick.Store(false)
-			c.cnd.Broadcast()
-			c.wg.Add(c.goCount)
-			c.cnd.L.Unlock()
-		}
-	}
-}
-
 func randomOne[T any](list []T) T {
 	return list[rand.Intn(len(list))]
 }