test: speed up tests, fix transaction bug (#39030)

Speed up tests: `make test-backend` 103s to 37s, `make test-integration`
908s to 852s.

Most of it is a detached system notice insert blocking on the SQLite
write lock until the busy timeout expired, and `ExternalServiceHTTP`
re-probing on every call with an untimed `http.Get`.

- fixed one correctness bug with nested transactions: files were deleted
while the outer transaction was open, so a later failure could roll the
database back with the files gone
- git push branch counts were far above the hook batch size
- Fix makefile dependencies so running tests and lint work in fresh
worktrees.

---------

Co-authored-by: Giteabot <teabot@gitea.io>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

.github/workflows/pull-compliance.yml

@@ -24,8 +24,7 @@ jobs:
         with:
           lint-cache: "true"
       - run: make deps-backend deps-tools
-      - run: TAGS="bindata" make generate-go # lint-go also lints with "bindata" tags which requires "_bindata.go"
+      - run: TAGS="bindata" make generate-go lint-backend # lint-go can lint with "bindata" tags
-      - run: make lint-backend
 
   lint-on-demand:
     needs: files-changed

Makefile

@@ -451,7 +451,7 @@ $(GO_LICENSE_FILE): go.mod go.sum
 	GO=$(GO) $(GO) run build/generate-go-licenses.go $(GO_LICENSE_FILE)
 
 .PHONY: test-integration
-test-integration:
+test-integration: $(EXECUTABLE)
 	@# Use a compiled binary: testlogger forwards gitea logs to t.Log, so `go test -v`
 	@# would flood output per passing test. testcache can't help these tests anyway —
 	@# they mutate the work directory, so cache inputs change between runs.
@@ -463,7 +463,7 @@ test-integration-compile:
 	$(GO) test $(GOTEST_FLAGS) -tags '$(TAGS)' -c -o /dev/null gitea.dev/tests/integration
 
 .PHONY: test-integration\#%
-test-integration\#%:
+test-integration\#%: $(EXECUTABLE)
 	$(GO) test $(GOTEST_FLAGS) -tags '$(TAGS)' -run $(subst .,/,$*) gitea.dev/tests/integration
 
 .PHONY: test-migration

modules/test/utils.go

@@ -18,6 +18,7 @@ import (
 	"strconv"
 	"strings"
 	"sync"
+	"time"
 
 	"gitea.dev/modules/json"
 	"gitea.dev/modules/util"
@@ -160,29 +161,47 @@ type TestingT interface {
 	TempDir() string
 }
 
+var externalServiceCheckResult sync.Map
+
 func ExternalServiceHTTP(t TestingT, envVarName, def string) string {
 	t.Helper()
-	val := util.IfZero(os.Getenv(envVarName), def)
+	extSvc := util.IfZero(os.Getenv(envVarName), def)
-	if val == "" {
+	if extSvc == "" {
 		if AllowSkipExternalService() {
 			t.Skipf("skipping test because %s is not set", envVarName)
 		} else {
 			t.Fatalf("%s is not set, but skipping is not allowed in CI", envVarName)
 		}
 	}
-	// minio's endpoint is "host:port" pattern
+
-	testURL := util.Iif(strings.Contains(val, "://"), val, "http://"+val)
+	// only need to check once, if there are saved check result, just use it
-	resp, err := http.Get(testURL)
+	lastCheckErrAny, lastCheckExists := externalServiceCheckResult.Load(extSvc)
-	if err != nil {
+	var lastCheckErr error
-		if AllowSkipExternalService() {
+	if !lastCheckExists {
-			t.Skipf("skipping test because %s is not ready", val)
+		{
-		} else {
+			// minio's endpoint is "host:port" pattern
-			t.Fatalf("%s is not ready, but skipping is not allowed in CI", val)
+			testURL := util.Iif(strings.Contains(extSvc, "://"), extSvc, "http://"+extSvc)
+			// do a quick check with short timeout
+			client := &http.Client{Timeout: 2 * time.Second}
+			resp, err := client.Get(testURL)
+			if err == nil {
+				_ = resp.Body.Close()
+			}
+			lastCheckErr = err
 		}
+		externalServiceCheckResult.Store(extSvc, lastCheckErr)
 	} else {
-		_ = resp.Body.Close()
+		lastCheckErr, _ = lastCheckErrAny.(error)
 	}
-	return val
+
+	if lastCheckErr != nil {
+		if AllowSkipExternalService() {
+			t.Skipf("skipping test because %s is not ready", extSvc)
+		} else {
+			t.Fatalf("%s is not ready, but skipping is not allowed in CI", extSvc)
+		}
+	}
+	return extSvc
 }
 
 var normalizeHTMLSpacesRegexp = sync.OnceValue(func() (ret struct {

services/org/org.go

@@ -54,14 +54,14 @@ func deleteOrganization(ctx context.Context, org *org_model.Organization) error
 
 // DeleteOrganization completely and permanently deletes everything of organization.
 func DeleteOrganization(ctx context.Context, org *org_model.Organization, purge bool) error {
-	if err := db.WithTx(ctx, func(ctx context.Context) error {
+	// outside the transaction below, because each repository deletion owns one and deletes storage after committing
-		if purge {
+	if purge {
-			err := repo_service.DeleteOwnerRepositoriesDirectly(ctx, org.AsUser())
+		if err := repo_service.DeleteOwnerRepositoriesDirectly(ctx, org.AsUser()); err != nil {
-			if err != nil {
+			return err
-				return err
-			}
 		}
+	}
 
+	if err := db.WithTx(ctx, func(ctx context.Context) error {
 		// Check ownership of repository.
 		count, err := repo_model.CountRepositories(ctx, repo_model.CountRepositoryOptions{OwnerID: org.ID})
 		if err != nil {

services/repository/delete.go

@@ -5,6 +5,7 @@ package repository
 
 import (
 	"context"
+	"errors"
 	"fmt"
 
 	actions_model "gitea.dev/models/actions"
@@ -48,9 +49,11 @@ func deleteDBRepository(ctx context.Context, repoID int64) error {
 	return nil
 }
 
-// DeleteRepository deletes a repository for a user or organization.
+// DeleteRepositoryDirectly deletes a repository for a user or organization.
-// make sure if you call this func to close open sessions (sqlite will otherwise get a deadlock)
 func DeleteRepositoryDirectly(ctx context.Context, repoID int64, ignoreOrgTeams ...bool) error {
+	if db.InTransaction(ctx) {
+		return errors.New("DeleteRepositoryDirectly must not be called within a transaction, it deletes storage once its own transaction commits")
+	}
 	ctx, committer, err := db.TxContext(ctx)
 	if err != nil {
 		return err

services/repository/delete_test.go

@@ -4,6 +4,7 @@
 package repository_test
 
 import (
+	"context"
 	"testing"
 
 	actions_model "gitea.dev/models/actions"
@@ -54,6 +55,16 @@ func TestDeleteOwnerRepositoriesDirectly(t *testing.T) {
 	user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
 
 	assert.NoError(t, repo_service.DeleteOwnerRepositoriesDirectly(t.Context(), user))
+
+	t.Run("RejectedInTransaction", func(t *testing.T) {
+		unittest.PrepareTestEnv(t)
+
+		err := db.WithTx(t.Context(), func(ctx context.Context) error {
+			return repo_service.DeleteRepositoryDirectly(ctx, 1)
+		})
+		assert.ErrorContains(t, err, "must not be called within a transaction")
+		unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
+	})
 }
 
 func TestDeleteRepositoryDirectlyPurgesRepoScopedRows(t *testing.T) {

tests/integration/git_push_test.go

@@ -29,7 +29,7 @@ func TestGitPush(t *testing.T) {
 func testGitPush(t *testing.T, u *url.URL) {
 	t.Run("Push branches at once", func(t *testing.T) {
 		runTestGitPush(t, u, func(t *testing.T, gitPath string) (pushed, deleted []string) {
-			for i := range 100 {
+			for i := range 10 {
 				branchName := fmt.Sprintf("branch-%d", i)
 				pushed = append(pushed, branchName)
 				doGitCreateBranch(gitPath, branchName)(t)
@@ -81,7 +81,7 @@ func testGitPush(t *testing.T, u *url.URL) {
 
 	t.Run("Push branches one by one", func(t *testing.T) {
 		runTestGitPush(t, u, func(t *testing.T, gitPath string) (pushed, deleted []string) {
-			for i := range 100 {
+			for i := range 10 {
 				branchName := fmt.Sprintf("branch-%d", i)
 				doGitCreateBranch(gitPath, branchName)(t)
 				doGitPushTestRepository(gitPath, "origin", branchName)(t)
@@ -107,14 +107,14 @@ func testGitPush(t *testing.T, u *url.URL) {
 			doGitPushTestRepository(gitPath, "origin", "master")(t) // make sure master is the default branch instead of a branch we are going to delete
 			pushed = append(pushed, "master")
 
-			for i := range 100 {
+			for i := range 10 {
 				branchName := fmt.Sprintf("branch-%d", i)
 				pushed = append(pushed, branchName)
 				doGitCreateBranch(gitPath, branchName)(t)
 			}
 			doGitPushTestRepository(gitPath, "origin", "--all")(t)
 
-			for i := range 10 {
+			for i := range 5 {
 				branchName := fmt.Sprintf("branch-%d", i)
 				doGitPushTestRepository(gitPath, "origin", "--delete", branchName)(t)
 				deleted = append(deleted, branchName)

tools/lint-go-all.go

@@ -91,13 +91,21 @@ func main() {
 
 	_, _ = fmt.Fprintln(os.Stdout, "lint go header ...")
 	succeed := lintGoHeader()
+
 	_, _ = fmt.Fprintln(os.Stdout, "lint for linux ...")
-	succeed = runCmd([]string{"GOOS=linux", "TAGS=bindata"}, "golangci-lint", append([]string{"run", "--build-tags=linux,bindata"}, os.Args[1:]...)) && succeed
+	lintTagsLinux := ""
+	if os.Getenv("CI") != "" || strings.Contains(os.Getenv("TAGS"), "bindata") {
+		// also lint with bindata tag if we are in CI or the "bindata" is explicitly set in the env TAGS
+		lintTagsLinux = "bindata"
+	}
+	succeed = runCmd([]string{"GOOS=linux", "TAGS=" + lintTagsLinux}, "golangci-lint", append([]string{"run", "--build-tags=linux," + lintTagsLinux}, os.Args[1:]...)) && succeed
+
 	if os.Getenv("CI") != "" {
 		// only lint for other platforms when in CI, to keep local lint fast
 		_, _ = fmt.Fprintln(os.Stdout, "lint for windows ...")
 		succeed = runCmd([]string{"GOOS=windows", "TAGS=gogit"}, "golangci-lint", append([]string{"run", "--build-tags=windows,gogit"}, os.Args[1:]...)) && succeed
 	}
+
 	if !succeed {
 		os.Exit(1)
 	}