From 3c4153b19584b50af10ede28577f717462484cdf Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 1 Nov 2024 23:18:29 +0800 Subject: [PATCH] [PORT] Refactor tests to prevent from unnecessary preparations (gitea#32398) Some preparations are only used by a few tests, so to make the tests fast, they should only be prepared when they are used. By the way, this PR splits PrepareTestEnv into small functions to make it simple. --- Conflict resolution: Mostly magical and just re-pasting the code into the right places. Done differently: use `require.NoError` instead of `assert.NoError`. (cherry picked from commit ec2d1593c269e06655525deb96f74b8094221b6f) --- .deadcode-out | 3 + modules/testlogger/testlogger.go | 6 +- modules/util/util.go | 19 +++++ modules/util/util_test.go | 13 ++++ .../integration/api_actions_artifact_test.go | 25 ++++--- .../api_actions_artifact_v4_test.go | 14 ++-- tests/test_utils.go | 75 +++++++++---------- 7 files changed, 96 insertions(+), 59 deletions(-) diff --git a/.deadcode-out b/.deadcode-out index 8c3e3090d2..c82a229317 100644 --- a/.deadcode-out +++ b/.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 diff --git a/modules/testlogger/testlogger.go b/modules/testlogger/testlogger.go index 7897cc6b07..b83cb323c9 100644 --- a/modules/testlogger/testlogger.go +++ b/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 - if len(skip) > 0 { - actualSkip = skip[0] + 1 - } + actualSkip := util.OptionalArg(skip) + 1 _, filename, line, _ := runtime.Caller(actualSkip) if log.CanColorStdout { diff --git a/modules/util/util.go b/modules/util/util.go index dcd7cf4f29..88ac07567b 100644 --- a/modules/util/util.go +++ b/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. diff --git a/modules/util/util_test.go b/modules/util/util_test.go index 549b53f5a7..7344c8fbb7 100644 --- a/modules/util/util_test.go +++ b/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)) +} diff --git a/tests/integration/api_actions_artifact_test.go b/tests/integration/api_actions_artifact_test.go index 2e35d4390a..5b300b96b1 100644 --- a/tests/integration/api_actions_artifact_test.go +++ b/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 diff --git a/tests/integration/api_actions_artifact_v4_test.go b/tests/integration/api_actions_artifact_v4_test.go index bf28f23536..43df1aeee0 100644 --- a/tests/integration/api_actions_artifact_v4_test.go +++ b/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) diff --git a/tests/test_utils.go b/tests/test_utils.go index 89016f9f64..d6516dd99a 100644 --- a/tests/test_utils.go +++ b/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) { - // 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 +func PrepareGitRepoDirectory(t testing.TB) { 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 - PrepareArtifactsStorage(t) +func PrepareArtifactsStorage(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 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 - if len(skip) > 0 { - actualSkip = skip[0] + 1 - } - return testlogger.PrintCurrentTest(t, actualSkip) + return testlogger.PrintCurrentTest(t, util.OptionalArg(skip)+1) } // Printf takes a format and args and prints the string to os.Stdout