From 0ecb25fdcbffbad9882131e5633c39feb6b171ca Mon Sep 17 00:00:00 2001 From: Gusted Date: Mon, 30 Jun 2025 23:04:16 +0200 Subject: [PATCH] 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 Reviewed-by: Otto Co-authored-by: Gusted Co-committed-by: Gusted --- .deadcode-out | 1 + models/asymkey/main_test.go | 2 -- models/db/engine.go | 10 +++++++ models/db/table_names_test.go | 40 +++++++++++++++++++++++++ models/fixtures/action_variable.yml | 1 - models/fixtures/deploy_key.yml | 1 - models/fixtures/external_login_user.yml | 1 - models/fixtures/federated_user.yml | 1 - models/fixtures/federation_host.yml | 1 - models/fixtures/gpg_key_import.yml | 1 - models/fixtures/login_source.yml | 1 - models/fixtures/protected_branch.yml | 1 - models/fixtures/pull_auto_merge.yml | 1 - models/fixtures/push_mirror.yml | 1 - models/fixtures/repo_archiver.yml | 1 - models/fixtures/repo_indexer_status.yml | 1 - models/fixtures/secret.yml | 1 - models/migrations/test/tests.go | 3 +- models/unittest/fixture_loader.go | 21 ++++++++++--- models/unittest/fixtures.go | 11 ++++++- models/unittest/testdb.go | 4 +++ modules/container/set.go | 5 ++++ modules/container/set_test.go | 7 +++++ 23 files changed, 96 insertions(+), 21 deletions(-) create mode 100644 models/db/table_names_test.go delete mode 100644 models/fixtures/action_variable.yml delete mode 100644 models/fixtures/deploy_key.yml delete mode 100644 models/fixtures/external_login_user.yml delete mode 100644 models/fixtures/federated_user.yml delete mode 100644 models/fixtures/federation_host.yml delete mode 100644 models/fixtures/gpg_key_import.yml delete mode 100644 models/fixtures/login_source.yml delete mode 100644 models/fixtures/protected_branch.yml delete mode 100644 models/fixtures/pull_auto_merge.yml delete mode 100644 models/fixtures/push_mirror.yml delete mode 100644 models/fixtures/repo_archiver.yml delete mode 100644 models/fixtures/repo_indexer_status.yml delete mode 100644 models/fixtures/secret.yml diff --git a/.deadcode-out b/.deadcode-out index 61c5bcb055..0e3a97ffa8 100644 --- a/.deadcode-out +++ b/.deadcode-out @@ -27,6 +27,7 @@ forgejo.org/models/db TruncateBeans InTransaction DumpTables + GetTableNames forgejo.org/models/dbfs file.renameTo diff --git a/models/asymkey/main_test.go b/models/asymkey/main_test.go index 316e8f1d54..aa13a93f0a 100644 --- a/models/asymkey/main_test.go +++ b/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", }, diff --git a/models/db/engine.go b/models/db/engine.go index 05a119b08d..76025f7d67 100755 --- a/models/db/engine.go +++ b/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 +} diff --git a/models/db/table_names_test.go b/models/db/table_names_test.go new file mode 100644 index 0000000000..176ce9905d --- /dev/null +++ b/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" +} diff --git a/models/fixtures/action_variable.yml b/models/fixtures/action_variable.yml deleted file mode 100644 index ca780a73aa..0000000000 --- a/models/fixtures/action_variable.yml +++ /dev/null @@ -1 +0,0 @@ -[] # empty diff --git a/models/fixtures/deploy_key.yml b/models/fixtures/deploy_key.yml deleted file mode 100644 index ca780a73aa..0000000000 --- a/models/fixtures/deploy_key.yml +++ /dev/null @@ -1 +0,0 @@ -[] # empty diff --git a/models/fixtures/external_login_user.yml b/models/fixtures/external_login_user.yml deleted file mode 100644 index ca780a73aa..0000000000 --- a/models/fixtures/external_login_user.yml +++ /dev/null @@ -1 +0,0 @@ -[] # empty diff --git a/models/fixtures/federated_user.yml b/models/fixtures/federated_user.yml deleted file mode 100644 index ca780a73aa..0000000000 --- a/models/fixtures/federated_user.yml +++ /dev/null @@ -1 +0,0 @@ -[] # empty diff --git a/models/fixtures/federation_host.yml b/models/fixtures/federation_host.yml deleted file mode 100644 index ca780a73aa..0000000000 --- a/models/fixtures/federation_host.yml +++ /dev/null @@ -1 +0,0 @@ -[] # empty diff --git a/models/fixtures/gpg_key_import.yml b/models/fixtures/gpg_key_import.yml deleted file mode 100644 index ca780a73aa..0000000000 --- a/models/fixtures/gpg_key_import.yml +++ /dev/null @@ -1 +0,0 @@ -[] # empty diff --git a/models/fixtures/login_source.yml b/models/fixtures/login_source.yml deleted file mode 100644 index ca780a73aa..0000000000 --- a/models/fixtures/login_source.yml +++ /dev/null @@ -1 +0,0 @@ -[] # empty diff --git a/models/fixtures/protected_branch.yml b/models/fixtures/protected_branch.yml deleted file mode 100644 index ca780a73aa..0000000000 --- a/models/fixtures/protected_branch.yml +++ /dev/null @@ -1 +0,0 @@ -[] # empty diff --git a/models/fixtures/pull_auto_merge.yml b/models/fixtures/pull_auto_merge.yml deleted file mode 100644 index ca780a73aa..0000000000 --- a/models/fixtures/pull_auto_merge.yml +++ /dev/null @@ -1 +0,0 @@ -[] # empty diff --git a/models/fixtures/push_mirror.yml b/models/fixtures/push_mirror.yml deleted file mode 100644 index ca780a73aa..0000000000 --- a/models/fixtures/push_mirror.yml +++ /dev/null @@ -1 +0,0 @@ -[] # empty diff --git a/models/fixtures/repo_archiver.yml b/models/fixtures/repo_archiver.yml deleted file mode 100644 index ca780a73aa..0000000000 --- a/models/fixtures/repo_archiver.yml +++ /dev/null @@ -1 +0,0 @@ -[] # empty diff --git a/models/fixtures/repo_indexer_status.yml b/models/fixtures/repo_indexer_status.yml deleted file mode 100644 index ca780a73aa..0000000000 --- a/models/fixtures/repo_indexer_status.yml +++ /dev/null @@ -1 +0,0 @@ -[] # empty diff --git a/models/fixtures/secret.yml b/models/fixtures/secret.yml deleted file mode 100644 index ca780a73aa..0000000000 --- a/models/fixtures/secret.yml +++ /dev/null @@ -1 +0,0 @@ -[] # empty diff --git a/models/migrations/test/tests.go b/models/migrations/test/tests.go index c1f0caf19b..6be3b3c2fc 100644 --- a/models/migrations/test/tests.go +++ b/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 diff --git a/models/unittest/fixture_loader.go b/models/unittest/fixture_loader.go index 67ef1b28df..dda5c48d35 100644 --- a/models/unittest/fixture_loader.go +++ b/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 { diff --git a/models/unittest/fixtures.go b/models/unittest/fixtures.go index 6dc5c8412d..829cc16466 100644 --- a/models/unittest/fixtures.go +++ b/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 } diff --git a/models/unittest/testdb.go b/models/unittest/testdb.go index d34c9e9a0a..29ec82c55f 100644 --- a/models/unittest/testdb.go +++ b/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 diff --git a/modules/container/set.go b/modules/container/set.go index 70f837bc66..d3719dc552 100644 --- a/modules/container/set.go +++ b/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) +} diff --git a/modules/container/set_test.go b/modules/container/set_test.go index af5e9126ab..44e4847f6b 100644 --- a/modules/container/set_test.go +++ b/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")) + }) }