chore: clean up tests (#37715)

1. use MockVariableValue as much as possible
2. use wg.Go as much as possible instead of Add/Done
3. simplify global lock's DefaultLocker logic to make it easier to test
4. introduce a general approach for getting external service config in
CI
5. remove unclear & unnecessary "t.Skip"
6. use modern generic syntax for remaining "DecodeJSON" calls
7. clarify test result for "list gitignore templates" and "list
licenses"

modules/globallock/globallock.go

@@ -6,31 +6,39 @@ package globallock
 import (
 	"context"
 	"sync"
+	"sync/atomic"
 
 	"code.gitea.io/gitea/modules/setting"
 )
 
 var (
-	defaultLocker Locker
-	initOnce      sync.Once
-	initFunc      = func() {
-		switch setting.GlobalLock.ServiceType {
-		case "redis":
-			defaultLocker = NewRedisLocker(setting.GlobalLock.ServiceConnStr)
-		case "memory":
-			fallthrough
-		default:
-			defaultLocker = NewMemoryLocker()
-		}
-	} // define initFunc as a variable to make it possible to change it in tests
+	defaultLocker atomic.Pointer[Locker]
+	defaultMutex  sync.Mutex
 )
 
+func initDefaultLocker() Locker {
+	switch setting.GlobalLock.ServiceType {
+	case "redis":
+		return NewRedisLocker(setting.GlobalLock.ServiceConnStr)
+	default: // "memory"
+		return NewMemoryLocker()
+	}
+}
+
 // DefaultLocker returns the default locker.
 func DefaultLocker() Locker {
-	initOnce.Do(func() {
-		initFunc()
-	})
-	return defaultLocker
+	ptr := defaultLocker.Load()
+	if ptr == nil {
+		defaultMutex.Lock()
+		ptr = defaultLocker.Load()
+		if ptr == nil {
+			ptr = new(initDefaultLocker())
+			defaultLocker.Store(ptr)
+		}
+		defaultMutex.Unlock()
+		ptr = defaultLocker.Load()
+	}
+	return *ptr
 }
 
 // Lock tries to acquire a lock for the given key, it uses the default locker.

modules/globallock/globallock_test.go

@@ -5,7 +5,6 @@ package globallock
 
 import (
 	"context"
-	"os"
 	"sync"
 	"testing"
 
@@ -15,50 +14,13 @@ import (
 
 func TestLockAndDo(t *testing.T) {
 	t.Run("redis", func(t *testing.T) {
-		url := "redis://127.0.0.1:6379/0"
-		if os.Getenv("CI") == "" {
-			// Make it possible to run tests against a local redis instance
-			url = os.Getenv("TEST_REDIS_URL")
-			if url == "" {
-				t.Skip("TEST_REDIS_URL not set and not running in CI")
-				return
-			}
-		}
-
-		oldDefaultLocker := defaultLocker
-		oldInitFunc := initFunc
-		defer func() {
-			defaultLocker = oldDefaultLocker
-			initFunc = oldInitFunc
-			if defaultLocker == nil {
-				initOnce = sync.Once{}
-			}
-		}()
-
-		initOnce = sync.Once{}
-		initFunc = func() {
-			defaultLocker = NewRedisLocker(url)
-		}
-
+		locker := newTestRedisLocker(t)
+		defaultLocker.Store(new(locker))
 		testLockAndDo(t)
-		require.NoError(t, defaultLocker.(*redisLocker).Close())
+		require.NoError(t, locker.(*redisLocker).Close())
 	})
 	t.Run("memory", func(t *testing.T) {
-		oldDefaultLocker := defaultLocker
-		oldInitFunc := initFunc
-		defer func() {
-			defaultLocker = oldDefaultLocker
-			initFunc = oldInitFunc
-			if defaultLocker == nil {
-				initOnce = sync.Once{}
-			}
-		}()
-
-		initOnce = sync.Once{}
-		initFunc = func() {
-			defaultLocker = NewMemoryLocker()
-		}
-
+		defaultLocker.Store(new(NewMemoryLocker()))
 		testLockAndDo(t)
 	})
 }
@@ -69,13 +31,10 @@ func testLockAndDo(t *testing.T) {
 	ctx := t.Context()
 	count := 0
 	wg := sync.WaitGroup{}
-	wg.Add(concurrency)
 	for range concurrency {
-		go func() {
-			defer wg.Done()
+		wg.Go(func() {
 			err := LockAndDo(ctx, "test", func(ctx context.Context) error {
 				count++
-
 				// It's impossible to acquire the lock inner the function
 				ok, err := TryLockAndDo(ctx, "test", func(ctx context.Context) error {
 					assert.Fail(t, "should not acquire the lock")
@@ -83,13 +42,11 @@ func testLockAndDo(t *testing.T) {
 				})
 				assert.False(t, ok)
 				assert.NoError(t, err)
-
 				return nil
 			})
-			require.NoError(t, err)
-		}()
+			assert.NoError(t, err)
+		})
 	}
-
 	wg.Wait()
 
 	assert.Equal(t, concurrency, count)

modules/globallock/locker_test.go

@@ -10,29 +10,30 @@ import (
 	"testing"
 	"time"
 
+	"code.gitea.io/gitea/modules/test"
+	"code.gitea.io/gitea/modules/util"
+
 	"github.com/go-redsync/redsync/v4"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
+func newTestRedisLocker(t *testing.T) Locker {
+	t.Helper()
+	redisURL := util.IfZero(os.Getenv("TEST_REDIS_URL"), "redis://127.0.0.1:6379/0")
+	rl := NewRedisLocker(redisURL).(*redisLocker)
+	err := rl.conn.Ping(t.Context()).Err()
+	if err != nil && test.AllowSkipExternalService() {
+		t.Skip("no redis server for testing, skipped")
+	}
+	require.NoError(t, err, "redis error for testing: %v", err)
+	return rl
+}
+
 func TestLocker(t *testing.T) {
 	t.Run("redis", func(t *testing.T) {
-		url := "redis://127.0.0.1:6379/0"
-		if os.Getenv("CI") == "" {
-			// Make it possible to run tests against a local redis instance
-			url = os.Getenv("TEST_REDIS_URL")
-			if url == "" {
-				t.Skip("TEST_REDIS_URL not set and not running in CI")
-				return
-			}
-		}
-		oldExpiry := redisLockExpiry
-		redisLockExpiry = 5 * time.Second // make it shorter for testing
-		defer func() {
-			redisLockExpiry = oldExpiry
-		}()
-
-		locker := NewRedisLocker(url)
+		defer test.MockVariableValue(&redisLockExpiry, 5*time.Second)() // make it shorter for testing
+		locker := newTestRedisLocker(t)
 		testLocker(t, locker)
 		testRedisLocker(t, locker.(*redisLocker))
 		require.NoError(t, locker.(*redisLocker).Close())

modules/globallock/redis_locker.go

@@ -14,6 +14,7 @@ import (
 
 	"github.com/go-redsync/redsync/v4"
 	"github.com/go-redsync/redsync/v4/redis/goredis/v9"
+	"github.com/redis/go-redis/v9"
 )
 
 const redisLockKeyPrefix = "gitea:globallock:"
@@ -23,7 +24,8 @@ const redisLockKeyPrefix = "gitea:globallock:"
 var redisLockExpiry = 30 * time.Second
 
 type redisLocker struct {
-	rs *redsync.Redsync
+	conn redis.UniversalClient
+	rs   *redsync.Redsync
 
 	mutexM   sync.Map
 	closed   atomic.Bool
@@ -33,17 +35,13 @@ type redisLocker struct {
 var _ Locker = &redisLocker{}
 
 func NewRedisLocker(connection string) Locker {
+	conn := nosql.GetManager().GetRedisClient(connection)
 	l := &redisLocker{
-		rs: redsync.New(
-			goredis.NewPool(
-				nosql.GetManager().GetRedisClient(connection),
-			),
-		),
+		conn: conn,
+		rs:   redsync.New(goredis.NewPool(conn)),
 	}
-
 	l.extendWg.Add(1)
 	l.startExtend()
-
 	return l
 }