Merge pull request '[PORT] Refactor tests to prevent from unnecessary preparations (gitea#32398)' (#5790) from gusted/forgejo-port-32398 into forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/5790
Reviewed-by: Otto <otto@codeberg.org>

.deadcode-out

@@ -247,6 +247,9 @@ code.gitea.io/gitea/modules/translation
 	MockLocale.TrSize
 	MockLocale.PrettyNumber
 
+code.gitea.io/gitea/modules/util
+	OptionalArg
+
 code.gitea.io/gitea/modules/util/filebuffer
 	CreateFromReader
 

modules/testlogger/testlogger.go

@@ -18,6 +18,7 @@ import (
 
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/queue"
+	"code.gitea.io/gitea/modules/util"
 )
 
 var (
@@ -445,10 +446,7 @@ func (w *testLoggerWriterCloser) Reset() error {
 func PrintCurrentTest(t testing.TB, skip ...int) func() {
 	t.Helper()
 	start := time.Now()
-	actualSkip := 1
+	actualSkip := util.OptionalArg(skip) + 1
-	if len(skip) > 0 {
-		actualSkip = skip[0] + 1
-	}
 	_, filename, line, _ := runtime.Caller(actualSkip)
 
 	if log.CanColorStdout {

modules/util/util.go

@@ -234,6 +234,25 @@ func IfZero[T comparable](v, def T) T {
 	return v
 }
 
+// OptionalArg helps the "optional argument" in Golang:
+//
+//	func foo(optArg ...int) { return OptionalArg(optArg) }
+//		calling `foo()` gets zero value 0, calling `foo(100)` gets 100
+//	func bar(optArg ...int) { return OptionalArg(optArg, 42) }
+//		calling `bar()` gets default value 42, calling `bar(100)` gets 100
+//
+// Passing more than 1 item to `optArg` or `defaultValue` is undefined behavior.
+// At the moment only the first item is used.
+func OptionalArg[T any](optArg []T, defaultValue ...T) (ret T) {
+	if len(optArg) >= 1 {
+		return optArg[0]
+	}
+	if len(defaultValue) >= 1 {
+		return defaultValue[0]
+	}
+	return ret
+}
+
 func ReserveLineBreakForTextarea(input string) string {
 	// Since the content is from a form which is a textarea, the line endings are \r\n.
 	// It's a standard behavior of HTML.

modules/util/util_test.go

@@ -275,3 +275,16 @@ func TestGeneratingEd25519Keypair(t *testing.T) {
 	assert.EqualValues(t, testPublicKey, string(publicKey))
 	assert.EqualValues(t, testPrivateKey, string(privateKey))
 }
+
+func TestOptionalArg(t *testing.T) {
+	foo := func(other any, optArg ...int) int {
+		return util.OptionalArg(optArg)
+	}
+	bar := func(other any, optArg ...int) int {
+		return util.OptionalArg(optArg, 42)
+	}
+	assert.Equal(t, 0, foo(nil))
+	assert.Equal(t, 100, foo(nil, 100))
+	assert.Equal(t, 42, bar(nil))
+	assert.Equal(t, 100, bar(nil, 100))
+}

tests/integration/api_actions_artifact_test.go

@@ -23,8 +23,15 @@ type getUploadArtifactRequest struct {
 	RetentionDays int64
 }
 
+func prepareTestEnvActionsArtifacts(t *testing.T) func() {
+	t.Helper()
+	f := tests.PrepareTestEnv(t, 1)
+	tests.PrepareArtifactsStorage(t)
+	return f
+}
+
 func TestActionsArtifactUploadSingleFile(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
+	defer prepareTestEnvActionsArtifacts(t)()
 
 	// acquire artifact upload url
 	req := NewRequestWithJSON(t, "POST", "/api/actions_pipeline/_apis/pipelines/workflows/791/artifacts", getUploadArtifactRequest{
@@ -58,7 +65,7 @@ func TestActionsArtifactUploadSingleFile(t *testing.T) {
 }
 
 func TestActionsArtifactUploadInvalidHash(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
+	defer prepareTestEnvActionsArtifacts(t)()
 
 	// artifact id 54321 not exist
 	url := "/api/actions_pipeline/_apis/pipelines/workflows/791/artifacts/8e5b948a454515dbabfc7eb718ddddddd/upload?itemPath=artifact/abc.txt"
@@ -73,7 +80,7 @@ func TestActionsArtifactUploadInvalidHash(t *testing.T) {
 }
 
 func TestActionsArtifactConfirmUploadWithoutName(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
+	defer prepareTestEnvActionsArtifacts(t)()
 
 	req := NewRequest(t, "PATCH", "/api/actions_pipeline/_apis/pipelines/workflows/791/artifacts").
 		AddTokenAuth("8061e833a55f6fc0157c98b883e91fcfeeb1a71a")
@@ -82,7 +89,7 @@ func TestActionsArtifactConfirmUploadWithoutName(t *testing.T) {
 }
 
 func TestActionsArtifactUploadWithoutToken(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
+	defer prepareTestEnvActionsArtifacts(t)()
 
 	req := NewRequestWithJSON(t, "POST", "/api/actions_pipeline/_apis/pipelines/workflows/1/artifacts", nil)
 	MakeRequest(t, req, http.StatusUnauthorized)
@@ -108,7 +115,7 @@ type (
 )
 
 func TestActionsArtifactDownload(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
+	defer prepareTestEnvActionsArtifacts(t)()
 
 	req := NewRequest(t, "GET", "/api/actions_pipeline/_apis/pipelines/workflows/791/artifacts").
 		AddTokenAuth("8061e833a55f6fc0157c98b883e91fcfeeb1a71a")
@@ -152,7 +159,7 @@ func TestActionsArtifactDownload(t *testing.T) {
 }
 
 func TestActionsArtifactUploadMultipleFile(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
+	defer prepareTestEnvActionsArtifacts(t)()
 
 	const testArtifactName = "multi-files"
 
@@ -208,7 +215,7 @@ func TestActionsArtifactUploadMultipleFile(t *testing.T) {
 }
 
 func TestActionsArtifactDownloadMultiFiles(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
+	defer prepareTestEnvActionsArtifacts(t)()
 
 	const testArtifactName = "multi-file-download"
 
@@ -263,7 +270,7 @@ func TestActionsArtifactDownloadMultiFiles(t *testing.T) {
 }
 
 func TestActionsArtifactUploadWithRetentionDays(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
+	defer prepareTestEnvActionsArtifacts(t)()
 
 	// acquire artifact upload url
 	req := NewRequestWithJSON(t, "POST", "/api/actions_pipeline/_apis/pipelines/workflows/791/artifacts", getUploadArtifactRequest{
@@ -299,7 +306,7 @@ func TestActionsArtifactUploadWithRetentionDays(t *testing.T) {
 }
 
 func TestActionsArtifactOverwrite(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
+	defer prepareTestEnvActionsArtifacts(t)()
 
 	{
 		// download old artifact uploaded by tests above, it should 1024 A

tests/integration/api_actions_artifact_v4_test.go

@@ -80,13 +80,13 @@ func uploadArtifact(t *testing.T, body string) string {
 }
 
 func TestActionsArtifactV4UploadSingleFile(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
+	defer prepareTestEnvActionsArtifacts(t)()
 	body := strings.Repeat("A", 1024)
 	uploadArtifact(t, body)
 }
 
 func TestActionsArtifactV4UploadSingleFileWrongChecksum(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
+	defer prepareTestEnvActionsArtifacts(t)()
 
 	token, err := actions_service.CreateAuthorizationToken(48, 792, 193)
 	require.NoError(t, err)
@@ -130,7 +130,7 @@ func TestActionsArtifactV4UploadSingleFileWrongChecksum(t *testing.T) {
 }
 
 func TestActionsArtifactV4UploadSingleFileWithRetentionDays(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
+	defer prepareTestEnvActionsArtifacts(t)()
 
 	token, err := actions_service.CreateAuthorizationToken(48, 792, 193)
 	require.NoError(t, err)
@@ -178,7 +178,7 @@ func TestActionsArtifactV4UploadSingleFileWithRetentionDays(t *testing.T) {
 }
 
 func TestActionsArtifactV4UploadSingleFileWithPotentialHarmfulBlockID(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
+	defer prepareTestEnvActionsArtifacts(t)()
 
 	token, err := actions_service.CreateAuthorizationToken(48, 792, 193)
 	require.NoError(t, err)
@@ -241,7 +241,7 @@ func TestActionsArtifactV4UploadSingleFileWithPotentialHarmfulBlockID(t *testing
 }
 
 func TestActionsArtifactV4UploadSingleFileWithChunksOutOfOrder(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
+	defer prepareTestEnvActionsArtifacts(t)()
 
 	token, err := actions_service.CreateAuthorizationToken(48, 792, 193)
 	require.NoError(t, err)
@@ -306,7 +306,7 @@ func TestActionsArtifactV4UploadSingleFileWithChunksOutOfOrder(t *testing.T) {
 }
 
 func TestActionsArtifactV4DownloadSingle(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
+	defer prepareTestEnvActionsArtifacts(t)()
 
 	token, err := actions_service.CreateAuthorizationToken(48, 792, 193)
 	require.NoError(t, err)
@@ -386,7 +386,7 @@ func TestActionsArtifactV4DownloadRange(t *testing.T) {
 }
 
 func TestActionsArtifactV4Delete(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
+	defer prepareTestEnvActionsArtifacts(t)()
 
 	token, err := actions_service.CreateAuthorizationToken(48, 792, 193)
 	require.NoError(t, err)

tests/test_utils.go

@@ -224,37 +224,7 @@ func cancelProcesses(t testing.TB, delay time.Duration) {
 	t.Logf("PrepareTestEnv: all processes cancelled within %s", time.Since(start))
 }
 
-func PrepareArtifactsStorage(t testing.TB) {
+func PrepareGitRepoDirectory(t testing.TB) {
-	// prepare actions artifacts directory and files
-	require.NoError(t, storage.Clean(storage.ActionsArtifacts))
-
-	s, err := storage.NewStorage(setting.LocalStorageType, &setting.Storage{
-		Path: filepath.Join(filepath.Dir(setting.AppPath), "tests", "testdata", "data", "artifacts"),
-	})
-	require.NoError(t, err)
-	require.NoError(t, s.IterateObjects("", func(p string, obj storage.Object) error {
-		_, err = storage.Copy(storage.ActionsArtifacts, p, s, p)
-		return err
-	}))
-}
-
-func PrepareTestEnv(t testing.TB, skip ...int) func() {
-	t.Helper()
-	ourSkip := 1
-	if len(skip) > 0 {
-		ourSkip += skip[0]
-	}
-	deferFn := PrintCurrentTest(t, ourSkip)
-
-	// kill all background processes to prevent them from interfering with the fixture loading
-	// see https://codeberg.org/forgejo/forgejo/issues/2962
-	cancelProcesses(t, 30*time.Second)
-	t.Cleanup(func() { cancelProcesses(t, 0) }) // cancel remaining processes in a non-blocking way
-
-	// load database fixtures
-	require.NoError(t, unittest.LoadFixtures())
-
-	// load git repo fixtures
 	require.NoError(t, util.RemoveAll(setting.RepoRootPath))
 	require.NoError(t, unittest.CopyDir(path.Join(filepath.Dir(setting.AppPath), "tests/gitea-repositories-meta"), setting.RepoRootPath))
 	ownerDirs, err := os.ReadDir(setting.RepoRootPath)
@@ -274,14 +244,28 @@ func PrepareTestEnv(t testing.TB, skip ...int) func() {
 			_ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "objects", "info"), 0o755)
 			_ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "refs", "heads"), 0o755)
 			_ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "refs", "tag"), 0o755)
+			_ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "refs", "pull"), 0o755)
 		}
 	}
+}
 
-	// Initialize actions artifact data
+func PrepareArtifactsStorage(t testing.TB) {
-	PrepareArtifactsStorage(t)
+	// prepare actions artifacts directory and files
+	require.NoError(t, storage.Clean(storage.ActionsArtifacts))
 
+	s, err := storage.NewStorage(setting.LocalStorageType, &setting.Storage{
+		Path: filepath.Join(filepath.Dir(setting.AppPath), "tests", "testdata", "data", "artifacts"),
+	})
+	require.NoError(t, err)
+	require.NoError(t, s.IterateObjects("", func(p string, obj storage.Object) error {
+		_, err = storage.Copy(storage.ActionsArtifacts, p, s, p)
+		return err
+	}))
+}
+
+func PrepareLFSStorage(t testing.TB) {
 	// load LFS object fixtures
-	// (LFS storage can be on any of several backends, including remote servers, so we init it with the storage API)
+	// (LFS storage can be on any of several backends, including remote servers, so init it with the storage API)
 	lfsFixtures, err := storage.NewStorage(setting.LocalStorageType, &setting.Storage{
 		Path: filepath.Join(filepath.Dir(setting.AppPath), "tests/gitea-lfs-meta"),
 	})
@@ -291,7 +275,9 @@ func PrepareTestEnv(t testing.TB, skip ...int) func() {
 		_, err := storage.Copy(storage.LFS, path, lfsFixtures, path)
 		return err
 	}))
+}
 
+func PrepareCleanPackageData(t testing.TB) {
 	// clear all package data
 	require.NoError(t, db.TruncateBeans(db.DefaultContext,
 		&packages_model.Package{},
@@ -303,17 +289,28 @@ func PrepareTestEnv(t testing.TB, skip ...int) func() {
 		&packages_model.PackageCleanupRule{},
 	))
 	require.NoError(t, storage.Clean(storage.Packages))
+}
 
+func PrepareTestEnv(t testing.TB, skip ...int) func() {
+	t.Helper()
+	deferFn := PrintCurrentTest(t, util.OptionalArg(skip)+1)
+
+	cancelProcesses(t, 30*time.Second)
+	t.Cleanup(func() { cancelProcesses(t, 0) }) // cancel remaining processes in a non-blocking way
+
+	// load database fixtures
+	require.NoError(t, unittest.LoadFixtures())
+
+	// do not add more Prepare* functions here, only call necessary ones in the related test functions
+	PrepareGitRepoDirectory(t)
+	PrepareLFSStorage(t)
+	PrepareCleanPackageData(t)
 	return deferFn
 }
 
 func PrintCurrentTest(t testing.TB, skip ...int) func() {
 	t.Helper()
-	actualSkip := 1
+	return testlogger.PrintCurrentTest(t, util.OptionalArg(skip)+1)
-	if len(skip) > 0 {
-		actualSkip = skip[0] + 1
-	}
-	return testlogger.PrintCurrentTest(t, actualSkip)
 }
 
 // Printf takes a format and args and prints the string to os.Stdout