chore: do not require empty fixtures to clean tables (#8353)

- A mistake that is often made is to not put a empty fixture for tables that gets populated in a test and should be cleaned up between runs. The CI does not detect such issues as these never arise on the first run of the test suite and are only noticed when developers run a test for a second time, unless you are aware of this behavior (which very few do as this is not documented and pure folk knowledge) can end up in chasing a bug that does not exist for hours. forgejo/forgejo#7041, forgejo/forgejo#7419, meissa/forgejo#21, forgejo/forgejo#5771
- Because we now own the fixture loader (forgejo/forgejo#7715), its rather simple to ensure that all tables that did not receive fixtures should be cleaned between runs and removes the need to place empty fixture files.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8353
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>
Reviewed-by: Otto <otto@codeberg.org>
Co-authored-by: Gusted <postmaster@gusted.xyz>
Co-committed-by: Gusted <postmaster@gusted.xyz>

.deadcode-out

@@ -27,6 +27,7 @@ forgejo.org/models/db
 	TruncateBeans
 	InTransaction
 	DumpTables
+	GetTableNames
 
 forgejo.org/models/dbfs
 	file.renameTo

models/asymkey/main_test.go

@@ -15,8 +15,6 @@ func TestMain(m *testing.M) {
 			"gpg_key.yml",
 			"public_key.yml",
 			"TestParseCommitWithSSHSignature/public_key.yml",
-			"deploy_key.yml",
-			"gpg_key_import.yml",
 			"user.yml",
 			"email_address.yml",
 		},

models/db/engine.go

@@ -15,6 +15,7 @@ import (
 	"strings"
 	"time"
 
+	"forgejo.org/modules/container"
 	"forgejo.org/modules/log"
 	"forgejo.org/modules/setting"
 
@@ -438,3 +439,12 @@ func GetMasterEngine(x Engine) (*xorm.Engine, error) {
 
 	return engine, nil
 }
+
+// GetTableNames returns the table name of all registered models.
+func GetTableNames() container.Set[string] {
+	names := make(container.Set[string])
+	for _, table := range tables {
+		names.Add(x.TableName(table))
+	}
+	return names
+}

models/db/table_names_test.go

@@ -0,0 +1,40 @@
+// Copyright 2025 The Forgejo Authors. All rights reserved.
+// SPDX-License-Identifier: GPL-3.0-or-later
+
+package db
+
+import (
+	"slices"
+	"testing"
+
+	"forgejo.org/modules/test"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestGetTableNames(t *testing.T) {
+	t.Run("Simple", func(t *testing.T) {
+		defer test.MockVariableValue(&tables, []any{new(GPGKey)})()
+
+		assert.Equal(t, []string{"gpg_key"}, GetTableNames().Values())
+	})
+
+	t.Run("Multiple tables", func(t *testing.T) {
+		defer test.MockVariableValue(&tables, []any{new(GPGKey), new(User), new(BlockedUser)})()
+
+		tableNames := GetTableNames().Values()
+		slices.Sort(tableNames)
+
+		assert.Equal(t, []string{"forgejo_blocked_user", "gpg_key", "user"}, tableNames)
+	})
+}
+
+type GPGKey struct{}
+
+type User struct{}
+
+type BlockedUser struct{}
+
+func (*BlockedUser) TableName() string {
+	return "forgejo_blocked_user"
+}

models/fixtures/action_variable.yml

@@ -1 +0,0 @@
-[] # empty

models/fixtures/deploy_key.yml

@@ -1 +0,0 @@
-[] # empty

models/fixtures/external_login_user.yml

@@ -1 +0,0 @@
-[] # empty

models/fixtures/federated_user.yml

@@ -1 +0,0 @@
-[] # empty

models/fixtures/federation_host.yml

@@ -1 +0,0 @@
-[] # empty

models/fixtures/gpg_key_import.yml

@@ -1 +0,0 @@
-[] # empty

models/fixtures/login_source.yml

@@ -1 +0,0 @@
-[] # empty

models/fixtures/protected_branch.yml

@@ -1 +0,0 @@
-[] # empty

models/fixtures/pull_auto_merge.yml

@@ -1 +0,0 @@
-[] # empty

models/fixtures/push_mirror.yml

@@ -1 +0,0 @@
-[] # empty

models/fixtures/repo_archiver.yml

@@ -1 +0,0 @@
-[] # empty

models/fixtures/repo_indexer_status.yml

@@ -1 +0,0 @@
-[] # empty

models/fixtures/secret.yml

@@ -1 +0,0 @@
-[] # empty

models/migrations/test/tests.go

@@ -95,7 +95,8 @@ func PrepareTestEnv(t *testing.T, skip int, syncModels ...any) (*xorm.Engine, fu
 		t.Logf("initializing fixtures from: %s", fixturesDir)
 		if err := unittest.InitFixtures(
 			unittest.FixturesOptions{
-				Dir: fixturesDir,
+				Dir:                     fixturesDir,
+				SkipCleanRegistedModels: true,
 			}, x); err != nil {
 			t.Errorf("error whilst initializing fixtures from %s: %v", fixturesDir, err)
 			return x, deferFn

models/unittest/fixture_loader.go

@@ -12,6 +12,8 @@ import (
 	"path/filepath"
 	"strings"
 
+	"forgejo.org/modules/container"
+
 	"gopkg.in/yaml.v3"
 )
 
@@ -32,13 +34,15 @@ type loader struct {
 	fixtureFiles []*fixtureFile
 }
 
-func newFixtureLoader(db *sql.DB, dialect string, fixturePaths []string) (*loader, error) {
+func newFixtureLoader(db *sql.DB, dialect string, fixturePaths []string, allTableNames container.Set[string]) (*loader, error) {
 	l := &loader{
 		db:           db,
 		dialect:      dialect,
 		fixtureFiles: []*fixtureFile{},
 	}
 
+	tablesWithoutFixture := allTableNames
+
 	// Load fixtures
 	for _, fixturePath := range fixturePaths {
 		stat, err := os.Stat(fixturePath)
@@ -60,6 +64,7 @@ func newFixtureLoader(db *sql.DB, dialect string, fixturePaths []string) (*loade
 						return nil, err
 					}
 					l.fixtureFiles = append(l.fixtureFiles, fixtureFile)
+					tablesWithoutFixture.Remove(fixtureFile.name)
 				}
 			}
 		} else {
@@ -71,6 +76,14 @@ func newFixtureLoader(db *sql.DB, dialect string, fixturePaths []string) (*loade
 		}
 	}
 
+	// Even though these tables have no fixtures, they can still be used and ensure
+	// they are cleaned.
+	for table := range tablesWithoutFixture.Seq() {
+		l.fixtureFiles = append(l.fixtureFiles, &fixtureFile{
+			name: table,
+		})
+	}
+
 	return l, nil
 }
 
@@ -178,13 +191,13 @@ func (l *loader) Load() error {
 	}()
 
 	// Clean the table and re-insert the fixtures.
-	tableDeleted := map[string]struct{}{}
+	tableDeleted := make(container.Set[string])
 	for _, fixture := range l.fixtureFiles {
-		if _, ok := tableDeleted[fixture.name]; !ok {
+		if !tableDeleted.Contains(fixture.name) {
 			if _, err := tx.Exec(fmt.Sprintf("DELETE FROM %s", l.quoteKeyword(fixture.name))); err != nil {
 				return fmt.Errorf("cannot delete table %s: %w", fixture.name, err)
 			}
-			tableDeleted[fixture.name] = struct{}{}
+			tableDeleted.Add(fixture.name)
 		}
 
 		for _, insertSQL := range fixture.insertSQLs {

models/unittest/fixtures.go

@@ -7,10 +7,12 @@ package unittest
 import (
 	"fmt"
 	"path/filepath"
+	"sync"
 	"time"
 
 	"forgejo.org/models/db"
 	"forgejo.org/modules/auth/password/hash"
+	"forgejo.org/modules/container"
 	"forgejo.org/modules/setting"
 
 	"xorm.io/xorm"
@@ -44,6 +46,8 @@ func OverrideFixtures(dir string) func() {
 	}
 }
 
+var allTableNames = sync.OnceValue(db.GetTableNames)
+
 // InitFixtures initialize test fixtures for a test database
 func InitFixtures(opts FixturesOptions, engine ...*xorm.Engine) (err error) {
 	e, err := GetXORMEngine(engine...)
@@ -75,7 +79,12 @@ func InitFixtures(opts FixturesOptions, engine ...*xorm.Engine) (err error) {
 		panic("Unsupported RDBMS for test")
 	}
 
-	fixturesLoader, err = newFixtureLoader(e.DB().DB, dialect, fixturePaths)
+	var allTables container.Set[string]
+	if !opts.SkipCleanRegistedModels {
+		allTables = allTableNames().Clone()
+	}
+
+	fixturesLoader, err = newFixtureLoader(e.DB().DB, dialect, fixturePaths, allTables)
 	if err != nil {
 		return err
 	}

models/unittest/testdb.go

@@ -217,6 +217,10 @@ type FixturesOptions struct {
 	Files []string
 	Dirs  []string
 	Base  string
+	// By default all registered models are cleaned, even if they do not have
+	// fixture. Enabling this will skip that and only models with fixtures are
+	// considered.
+	SkipCleanRegistedModels bool
 }
 
 // CreateTestEngine creates a memory database and loads the fixture data from fixturesDir

modules/container/set.go

@@ -74,3 +74,8 @@ func (s Set[T]) Values() []T {
 func (s Set[T]) Seq() iter.Seq[T] {
 	return maps.Keys(s)
 }
+
+// Clone returns a identical shallow copy of this set.
+func (s Set[T]) Clone() Set[T] {
+	return maps.Clone(s)
+}

modules/container/set_test.go

@@ -47,4 +47,11 @@ func TestSet(t *testing.T) {
 	assert.False(t, s.IsSubset([]string{"key1"}))
 
 	assert.True(t, s.IsSubset([]string{}))
+
+	t.Run("Clone", func(t *testing.T) {
+		clonedSet := s.Clone()
+		clonedSet.Remove("key6")
+		assert.False(t, clonedSet.Contains("key6"))
+		assert.True(t, s.Contains("key6"))
+	})
 }