fix: misc fixes in pub/gpg/tests (#38809)

- The pub registry reported the oldest version as `latest`, because the
descriptor slice is sorted ascending but the first element was used.
- Verifying a GPG or SSH key flashed success and redirected after
already writing an error response, so a failure was reported as a
success with an empty key id.
- Test packages sharing redis could tear down each other's server.
`PrepareTestRedis` started its own on the well-known port, so a package
running in parallel borrowed it and lost it when the owner's cleanup
fired. It now listens on a socket of its own.

modules/globallock/globallock_test.go

@@ -7,6 +7,9 @@ import (
 	"context"
 	"sync"
 	"testing"
+	"time"
+
+	"gitea.dev/modules/test"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -14,6 +17,7 @@ import (
 
 func TestLockAndDo(t *testing.T) {
 	t.Run("redis", func(t *testing.T) {
+		defer test.MockVariableValue(&redisLockExpiry, 5*time.Second)() // Close waits for the extend goroutine's next tick
 		locker := newTestRedisLocker(t)
 		defaultLocker.Store(new(locker))
 		testLockAndDo(t)

modules/globallock/locker_test.go

@@ -5,13 +5,11 @@ package globallock
 
 import (
 	"context"
-	"os"
 	"sync"
 	"testing"
 	"time"
 
 	"gitea.dev/modules/test"
-	"gitea.dev/modules/util"
 
 	"github.com/go-redsync/redsync/v4"
 	"github.com/stretchr/testify/assert"
@@ -20,14 +18,7 @@ import (
 
 func newTestRedisLocker(t *testing.T) Locker {
 	t.Helper()
-	redisURL := util.IfZero(os.Getenv("TEST_REDIS_URL"), "redis://127.0.0.1:6379/0")
+	return NewRedisLocker(test.PrepareTestRedis(t))
-	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) {

modules/test/redis.go

@@ -7,23 +7,19 @@ import (
 	"net"
 	"os"
 	"os/exec"
+	"path/filepath"
 	"time"
 )
 
-const (
+const testRedisAddr = "127.0.0.1:6379"
-	testRedisHost    = "127.0.0.1"
-	testRedisPort    = "6379"
-	testRedisAddr    = testRedisHost + ":" + testRedisPort
-	testRedisConnStr = "redis://" + testRedisAddr + "/0"
-)
 
-// waitRedisReady reports whether redis accepts connections within dur. Redis
+// waitRedisReady reports whether redis accepts connections on addr within dur.
-// binds its listener last during startup, so a successful dial means it can
+// Redis binds its listener last during startup, so a successful dial means it
-// serve. A plain dial, not a redis PING: the client retries its pool on a
+// can serve. A plain dial, not a redis PING: the client retries its pool on a
 // refused connect, which makes the "is one already running" probe take ~1s.
-func waitRedisReady(dur time.Duration) bool {
+func waitRedisReady(network, addr string, dur time.Duration) bool {
-	for start := time.Now(); ; time.Sleep(50 * time.Millisecond) {
+	for start := time.Now(); ; time.Sleep(5 * time.Millisecond) {
-		conn, err := net.DialTimeout("tcp", testRedisAddr, time.Second)
+		conn, err := net.DialTimeout(network, addr, time.Second)
 		if err == nil {
 			_ = conn.Close()
 			return true
@@ -34,35 +30,33 @@ func waitRedisReady(dur time.Duration) bool {
 	}
 }
 
-func redisServerCmd(t TestingT) *exec.Cmd {
+// PrepareTestRedis returns a connection string to a running redis, reusing one
+// already listening on the well-known port, otherwise starting one for the
+// duration of the test.
+func PrepareTestRedis(t TestingT) string {
+	if waitRedisReady("tcp", testRedisAddr, 0) {
+		return "redis://" + testRedisAddr + "/0"
+	}
 	redisServerProg, err := exec.LookPath("redis-server")
 	if err != nil {
-		return nil
-	}
-	return &exec.Cmd{
-		Path:   redisServerProg,
-		Args:   []string{redisServerProg, "--bind", testRedisHost, "--port", testRedisPort},
-		Dir:    t.TempDir(),
-		Stdin:  os.Stdin,
-		Stdout: os.Stdout,
-		Stderr: os.Stderr,
-	}
-}
-
-// PrepareTestRedis returns a connection string to a running redis, starting one
-// for the duration of the test if the port is free.
-func PrepareTestRedis(t TestingT) string {
-	if waitRedisReady(0) {
-		return testRedisConnStr
-	}
-	redisServer := redisServerCmd(t)
-	if redisServer == nil {
 		if AllowSkipExternalService() {
 			t.Skipf("redis-server command not found, skipped")
 		} else {
 			t.Fatalf("no redis server or command, but skipping is not allowed")
 		}
-		return testRedisConnStr
+		return ""
+	}
+	// listen on a socket of our own rather than a port, so that packages running
+	// in parallel can neither reach nor tear down each other's server
+	dir := t.TempDir()
+	socket := filepath.Join(dir, "redis.sock")
+	redisServer := &exec.Cmd{
+		Path:   redisServerProg,
+		Args:   []string{redisServerProg, "--port", "0", "--unixsocket", socket},
+		Dir:    dir,
+		Stdin:  os.Stdin,
+		Stdout: os.Stdout,
+		Stderr: os.Stderr,
 	}
 	if err := redisServer.Start(); err != nil {
 		t.Fatalf("failed to start redis-server: %v", err)
@@ -71,8 +65,8 @@ func PrepareTestRedis(t TestingT) string {
 		_ = redisServer.Process.Signal(os.Interrupt)
 		_ = redisServer.Wait()
 	})
-	if !waitRedisReady(5 * time.Second) {
+	if !waitRedisReady("unix", socket, 5*time.Second) {
 		t.Fatalf("failed to start redis-server")
 	}
-	return testRedisConnStr
+	return "redis+socket://" + socket
 }

routers/api/packages/pub/pub.go

@@ -108,7 +108,7 @@ func EnumeratePackageVersions(ctx *context.Context) {
 
 	jsonResponse(ctx, http.StatusOK, &packageVersions{
 		Name:     pds[0].Package.Name,
-		Latest:   packageDescriptorToMetadata(baseURL, pds[0]),
+		Latest:   versions[len(versions)-1], // versions mirrors pds, sorted ascending
 		Versions: versions,
 	})
 }

routers/web/user/setting/keys.go

@@ -161,6 +161,7 @@ func KeysPost(ctx *context.Context) {
 			default:
 				ctx.ServerError("VerifyGPG", err)
 			}
+			return
 		}
 		ctx.Flash.Success(ctx.Tr("settings.verify_gpg_key_success", keyID))
 		ctx.Redirect(setting.AppSubURL + "/user/settings/keys")
@@ -232,6 +233,7 @@ func KeysPost(ctx *context.Context) {
 			default:
 				ctx.ServerError("VerifySSH", err)
 			}
+			return
 		}
 		ctx.Flash.Success(ctx.Tr("settings.verify_ssh_key_success", fingerprint))
 		ctx.Redirect(setting.AppSubURL + "/user/settings/keys")

tests/integration/api_packages_pub_test.go

@@ -4,7 +4,6 @@
 package integration
 
 import (
-	"archive/tar"
 	"bytes"
 	"compress/gzip"
 	"fmt"
@@ -20,6 +19,7 @@ import (
 	"gitea.dev/models/unittest"
 	user_model "gitea.dev/models/user"
 	pub_module "gitea.dev/modules/packages/pub"
+	"gitea.dev/modules/test"
 	"gitea.dev/tests"
 
 	"github.com/stretchr/testify/assert"
@@ -34,26 +34,19 @@ func TestPackagePub(t *testing.T) {
 
 	packageName := "test_package"
 	packageVersion := "1.0.1"
+	packageVersionLatest := "1.0.2"
 	packageDescription := "Test Description"
 
 	filename := packageVersion + ".tar.gz"
 
-	pubspecContent := `name: ` + packageName + `
+	buildPackage := func(version string) []byte {
-version: ` + packageVersion + `
+		return test.WriteTarCompression(gzip.NewWriter, map[string]string{
-description: ` + packageDescription
+			"pubspec.yaml": `name: ` + packageName + `
-
+version: ` + version + `
-	var buf bytes.Buffer
+description: ` + packageDescription,
-	zw := gzip.NewWriter(&buf)
+		}).Bytes()
-	archive := tar.NewWriter(zw)
+	}
-	archive.WriteHeader(&tar.Header{
+	content := buildPackage(packageVersion)
-		Name: "pubspec.yaml",
-		Mode: 0o600,
-		Size: int64(len(pubspecContent)),
-	})
-	archive.Write([]byte(pubspecContent))
-	archive.Close()
-	zw.Close()
-	content := buf.Bytes()
 
 	root := fmt.Sprintf("/api/packages/%s/pub", user.Name)
 
@@ -120,6 +113,8 @@ description: ` + packageDescription
 		assert.Equal(t, int64(len(content)), pb.Size)
 
 		_ = uploadFile(t, result.URL, content, http.StatusConflict)
+
+		uploadFile(t, result.URL, buildPackage(packageVersionLatest), http.StatusNoContent)
 	})
 
 	t.Run("Download", func(t *testing.T) {
@@ -169,9 +164,10 @@ description: ` + packageDescription
 
 		assert.Equal(t, packageName, result.Name)
 		assert.NotNil(t, result.Latest)
-		assert.Len(t, result.Versions, 1)
+		assert.Len(t, result.Versions, 2)
-		assert.Equal(t, result.Latest.Version, result.Versions[0].Version)
+		assert.Equal(t, packageVersion, result.Versions[0].Version)
-		assert.Equal(t, packageVersion, result.Latest.Version)
+		assert.Equal(t, packageVersionLatest, result.Versions[1].Version)
+		assert.Equal(t, packageVersionLatest, result.Latest.Version)
 		assert.NotNil(t, result.Latest.Pubspec)
 	})
 }