From 0ed160ffea91ac1903bd33866fa93c24e3ace65c Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sun, 29 Dec 2024 09:05:56 +0800 Subject: [PATCH] Refactor tests (#33021) 1. fix incorrect tests, for example: BeanExists doesn't do assert and shouldn't be used 2. remove unnecessary test functions 3. introduce DumpQueryResult to help to see the database rows during test (at least I need it) ``` ====== DumpQueryResult: SELECT * FROM action_runner_token ====== - # row[0] id: 1 token: xeiWBL5kuTYxGPynHCqQdoeYmJAeG3IzGXCYTrDX owner_id: 0 ... ``` --- models/activities/user_heatmap_test.go | 8 ++- models/auth/webauthn_test.go | 6 +-- models/issues/issue_user_test.go | 6 +-- models/issues/label_test.go | 2 +- models/organization/org_user_test.go | 2 +- models/unittest/fixtures.go | 20 +++---- models/unittest/reflection.go | 4 +- models/unittest/testdb.go | 5 -- models/unittest/unit_tests.go | 73 ++++++++++++++++++-------- routers/web/repo/issue_label_test.go | 9 ++-- services/org/user_test.go | 2 +- tests/integration/auth_ldap_test.go | 2 +- tests/integration/pull_review_test.go | 8 +-- tests/integration/signin_test.go | 4 +- 14 files changed, 83 insertions(+), 68 deletions(-) diff --git a/models/activities/user_heatmap_test.go b/models/activities/user_heatmap_test.go index a039fd3613..380045d3c5 100644 --- a/models/activities/user_heatmap_test.go +++ b/models/activities/user_heatmap_test.go @@ -64,11 +64,9 @@ func TestGetUserHeatmapDataByUser(t *testing.T) { for _, tc := range testCases { user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: tc.userID}) - doer := &user_model.User{ID: tc.doerID} - _, err := unittest.LoadBeanIfExists(doer) - assert.NoError(t, err) - if tc.doerID == 0 { - doer = nil + var doer *user_model.User + if tc.doerID != 0 { + doer = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: tc.doerID}) } // get the action for comparison diff --git a/models/auth/webauthn_test.go b/models/auth/webauthn_test.go index f1cf398adf..654427e974 100644 --- a/models/auth/webauthn_test.go +++ b/models/auth/webauthn_test.go @@ -44,7 +44,7 @@ func TestWebAuthnCredential_UpdateSignCount(t *testing.T) { cred := unittest.AssertExistsAndLoadBean(t, &auth_model.WebAuthnCredential{ID: 1}) cred.SignCount = 1 assert.NoError(t, cred.UpdateSignCount(db.DefaultContext)) - unittest.AssertExistsIf(t, true, &auth_model.WebAuthnCredential{ID: 1, SignCount: 1}) + unittest.AssertExistsAndLoadBean(t, &auth_model.WebAuthnCredential{ID: 1, SignCount: 1}) } func TestWebAuthnCredential_UpdateLargeCounter(t *testing.T) { @@ -52,7 +52,7 @@ func TestWebAuthnCredential_UpdateLargeCounter(t *testing.T) { cred := unittest.AssertExistsAndLoadBean(t, &auth_model.WebAuthnCredential{ID: 1}) cred.SignCount = 0xffffffff assert.NoError(t, cred.UpdateSignCount(db.DefaultContext)) - unittest.AssertExistsIf(t, true, &auth_model.WebAuthnCredential{ID: 1, SignCount: 0xffffffff}) + unittest.AssertExistsAndLoadBean(t, &auth_model.WebAuthnCredential{ID: 1, SignCount: 0xffffffff}) } func TestCreateCredential(t *testing.T) { @@ -63,5 +63,5 @@ func TestCreateCredential(t *testing.T) { assert.Equal(t, "WebAuthn Created Credential", res.Name) assert.Equal(t, []byte("Test"), res.CredentialID) - unittest.AssertExistsIf(t, true, &auth_model.WebAuthnCredential{Name: "WebAuthn Created Credential", UserID: 1}) + unittest.AssertExistsAndLoadBean(t, &auth_model.WebAuthnCredential{Name: "WebAuthn Created Credential", UserID: 1}) } diff --git a/models/issues/issue_user_test.go b/models/issues/issue_user_test.go index ce47adb53a..7c21aa15ee 100644 --- a/models/issues/issue_user_test.go +++ b/models/issues/issue_user_test.go @@ -12,6 +12,7 @@ import ( "code.gitea.io/gitea/models/unittest" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func Test_NewIssueUsers(t *testing.T) { @@ -27,9 +28,8 @@ func Test_NewIssueUsers(t *testing.T) { } // artificially insert new issue - unittest.AssertSuccessfulInsert(t, newIssue) - - assert.NoError(t, issues_model.NewIssueUsers(db.DefaultContext, repo, newIssue)) + require.NoError(t, db.Insert(db.DefaultContext, newIssue)) + require.NoError(t, issues_model.NewIssueUsers(db.DefaultContext, repo, newIssue)) // issue_user table should now have entries for new issue unittest.AssertExistsAndLoadBean(t, &issues_model.IssueUser{IssueID: newIssue.ID, UID: newIssue.PosterID}) diff --git a/models/issues/label_test.go b/models/issues/label_test.go index 1d4b6f4684..185fa11bbc 100644 --- a/models/issues/label_test.go +++ b/models/issues/label_test.go @@ -387,7 +387,7 @@ func TestDeleteIssueLabel(t *testing.T) { expectedNumIssues := label.NumIssues expectedNumClosedIssues := label.NumClosedIssues - if unittest.BeanExists(t, &issues_model.IssueLabel{IssueID: issueID, LabelID: labelID}) { + if unittest.GetBean(t, &issues_model.IssueLabel{IssueID: issueID, LabelID: labelID}) != nil { expectedNumIssues-- if issue.IsClosed { expectedNumClosedIssues-- diff --git a/models/organization/org_user_test.go b/models/organization/org_user_test.go index 55abb63203..c5110b2a34 100644 --- a/models/organization/org_user_test.go +++ b/models/organization/org_user_test.go @@ -131,7 +131,7 @@ func TestAddOrgUser(t *testing.T) { testSuccess := func(orgID, userID int64, isPublic bool) { org := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: orgID}) expectedNumMembers := org.NumMembers - if !unittest.BeanExists(t, &organization.OrgUser{OrgID: orgID, UID: userID}) { + if unittest.GetBean(t, &organization.OrgUser{OrgID: orgID, UID: userID}) == nil { expectedNumMembers++ } assert.NoError(t, organization.AddOrgUser(db.DefaultContext, orgID, userID)) diff --git a/models/unittest/fixtures.go b/models/unittest/fixtures.go index 4dde5410d6..517584fdc2 100644 --- a/models/unittest/fixtures.go +++ b/models/unittest/fixtures.go @@ -1,12 +1,10 @@ // Copyright 2021 The Gitea Authors. All rights reserved. // SPDX-License-Identifier: MIT -//nolint:forbidigo package unittest import ( "fmt" - "os" "time" "code.gitea.io/gitea/models/db" @@ -37,7 +35,7 @@ func InitFixtures(opts FixturesOptions, engine ...*xorm.Engine) (err error) { } else { fixtureOptionFiles = testfixtures.Files(opts.Files...) } - dialect := "unknown" + var dialect string switch e.Dialect().URI().DBType { case schemas.POSTGRES: dialect = "postgres" @@ -48,8 +46,7 @@ func InitFixtures(opts FixturesOptions, engine ...*xorm.Engine) (err error) { case schemas.SQLITE: dialect = "sqlite3" default: - fmt.Println("Unsupported RDBMS for integration tests") - os.Exit(1) + return fmt.Errorf("unsupported RDBMS for integration tests: %q", e.Dialect().URI().DBType) } loaderOptions := []func(loader *testfixtures.Loader) error{ testfixtures.Database(e.DB().DB), @@ -69,9 +66,7 @@ func InitFixtures(opts FixturesOptions, engine ...*xorm.Engine) (err error) { // register the dummy hash algorithm function used in the test fixtures _ = hash.Register("dummy", hash.NewDummyHasher) - setting.PasswordHashAlgo, _ = hash.SetDefaultPasswordHashAlgorithm("dummy") - return err } @@ -87,7 +82,7 @@ func LoadFixtures(engine ...*xorm.Engine) error { time.Sleep(200 * time.Millisecond) } if err != nil { - fmt.Printf("LoadFixtures failed after retries: %v\n", err) + return fmt.Errorf("LoadFixtures failed after retries: %w", err) } // Now if we're running postgres we need to tell it to update the sequences if e.Dialect().URI().DBType == schemas.POSTGRES { @@ -108,21 +103,18 @@ func LoadFixtures(engine ...*xorm.Engine) error { AND T.relname = PGT.tablename ORDER BY S.relname;`) if err != nil { - fmt.Printf("Failed to generate sequence update: %v\n", err) - return err + return fmt.Errorf("failed to generate sequence update: %w", err) } for _, r := range results { for _, value := range r { _, err = e.Exec(value) if err != nil { - fmt.Printf("Failed to update sequence: %s Error: %v\n", value, err) - return err + return fmt.Errorf("failed to update sequence: %s, error: %w", value, err) } } } } _ = hash.Register("dummy", hash.NewDummyHasher) setting.PasswordHashAlgo, _ = hash.SetDefaultPasswordHashAlgorithm("dummy") - - return err + return nil } diff --git a/models/unittest/reflection.go b/models/unittest/reflection.go index 141fc66b99..bc96a05973 100644 --- a/models/unittest/reflection.go +++ b/models/unittest/reflection.go @@ -4,7 +4,7 @@ package unittest import ( - "log" + "fmt" "reflect" ) @@ -14,7 +14,7 @@ func fieldByName(v reflect.Value, field string) reflect.Value { } f := v.FieldByName(field) if !f.IsValid() { - log.Panicf("can not read %s for %v", field, v) + panic(fmt.Errorf("can not read %s for %v", field, v)) } return f } diff --git a/models/unittest/testdb.go b/models/unittest/testdb.go index 12f3c25676..3fb53541df 100644 --- a/models/unittest/testdb.go +++ b/models/unittest/testdb.go @@ -34,11 +34,6 @@ var ( fixturesDir string ) -// FixturesDir returns the fixture directory -func FixturesDir() string { - return fixturesDir -} - func fatalTestError(fmtStr string, args ...any) { _, _ = fmt.Fprintf(os.Stderr, fmtStr, args...) os.Exit(1) diff --git a/models/unittest/unit_tests.go b/models/unittest/unit_tests.go index 4ac858e04e..1c5595aef8 100644 --- a/models/unittest/unit_tests.go +++ b/models/unittest/unit_tests.go @@ -4,13 +4,17 @@ package unittest import ( + "fmt" "math" + "os" + "strings" "code.gitea.io/gitea/models/db" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "xorm.io/builder" + "xorm.io/xorm" ) // Code in this file is mainly used by unittest.CheckConsistencyFor, which is not in the unit test for various reasons. @@ -51,22 +55,23 @@ func whereOrderConditions(e db.Engine, conditions []any) db.Engine { return e.OrderBy(orderBy) } -// LoadBeanIfExists loads beans from fixture database if exist -func LoadBeanIfExists(bean any, conditions ...any) (bool, error) { +func getBeanIfExists(bean any, conditions ...any) (bool, error) { e := db.GetEngine(db.DefaultContext) return whereOrderConditions(e, conditions).Get(bean) } -// BeanExists for testing, check if a bean exists -func BeanExists(t assert.TestingT, bean any, conditions ...any) bool { - exists, err := LoadBeanIfExists(bean, conditions...) - assert.NoError(t, err) - return exists +func GetBean[T any](t require.TestingT, bean T, conditions ...any) (ret T) { + exists, err := getBeanIfExists(bean, conditions...) + require.NoError(t, err) + if exists { + return bean + } + return ret } // AssertExistsAndLoadBean assert that a bean exists and load it from the test database func AssertExistsAndLoadBean[T any](t require.TestingT, bean T, conditions ...any) T { - exists, err := LoadBeanIfExists(bean, conditions...) + exists, err := getBeanIfExists(bean, conditions...) require.NoError(t, err) require.True(t, exists, "Expected to find %+v (of type %T, with conditions %+v), but did not", @@ -112,25 +117,11 @@ func GetCount(t assert.TestingT, bean any, conditions ...any) int { // AssertNotExistsBean assert that a bean does not exist in the test database func AssertNotExistsBean(t assert.TestingT, bean any, conditions ...any) { - exists, err := LoadBeanIfExists(bean, conditions...) + exists, err := getBeanIfExists(bean, conditions...) assert.NoError(t, err) assert.False(t, exists) } -// AssertExistsIf asserts that a bean exists or does not exist, depending on -// what is expected. -func AssertExistsIf(t assert.TestingT, expected bool, bean any, conditions ...any) { - exists, err := LoadBeanIfExists(bean, conditions...) - assert.NoError(t, err) - assert.Equal(t, expected, exists) -} - -// AssertSuccessfulInsert assert that beans is successfully inserted -func AssertSuccessfulInsert(t assert.TestingT, beans ...any) { - err := db.Insert(db.DefaultContext, beans...) - assert.NoError(t, err) -} - // AssertCount assert the count of a bean func AssertCount(t assert.TestingT, bean, expected any) bool { return assert.EqualValues(t, expected, GetCount(t, bean)) @@ -155,3 +146,39 @@ func AssertCountByCond(t assert.TestingT, tableName string, cond builder.Cond, e return assert.EqualValues(t, expected, GetCountByCond(t, tableName, cond), "Failed consistency test, the counted bean (of table %s) was %+v", tableName, cond) } + +// DumpQueryResult dumps the result of a query for debugging purpose +func DumpQueryResult(t require.TestingT, sqlOrBean any, sqlArgs ...any) { + x := db.GetEngine(db.DefaultContext).(*xorm.Engine) + goDB := x.DB().DB + sql, ok := sqlOrBean.(string) + if !ok { + sql = fmt.Sprintf("SELECT * FROM %s", db.TableName(sqlOrBean)) + } else if !strings.Contains(sql, " ") { + sql = fmt.Sprintf("SELECT * FROM %s", sql) + } + rows, err := goDB.Query(sql, sqlArgs...) + require.NoError(t, err) + defer rows.Close() + columns, err := rows.Columns() + require.NoError(t, err) + + _, _ = fmt.Fprintf(os.Stdout, "====== DumpQueryResult: %s ======\n", sql) + idx := 0 + for rows.Next() { + row := make([]any, len(columns)) + rowPointers := make([]any, len(columns)) + for i := range row { + rowPointers[i] = &row[i] + } + require.NoError(t, rows.Scan(rowPointers...)) + _, _ = fmt.Fprintf(os.Stdout, "- # row[%d]\n", idx) + for i, col := range columns { + _, _ = fmt.Fprintf(os.Stdout, " %s: %v\n", col, row[i]) + } + idx++ + } + if idx == 0 { + _, _ = fmt.Fprintf(os.Stdout, "(no result, columns: %s)\n", strings.Join(columns, ", ")) + } +} diff --git a/routers/web/repo/issue_label_test.go b/routers/web/repo/issue_label_test.go index c86a03da51..8a613e2c7e 100644 --- a/routers/web/repo/issue_label_test.go +++ b/routers/web/repo/issue_label_test.go @@ -162,10 +162,11 @@ func TestUpdateIssueLabel_Toggle(t *testing.T) { UpdateIssueLabel(ctx) assert.EqualValues(t, http.StatusOK, ctx.Resp.Status()) for _, issueID := range testCase.IssueIDs { - unittest.AssertExistsIf(t, testCase.ExpectedAdd, &issues_model.IssueLabel{ - IssueID: issueID, - LabelID: testCase.LabelID, - }) + if testCase.ExpectedAdd { + unittest.AssertExistsAndLoadBean(t, &issues_model.IssueLabel{IssueID: issueID, LabelID: testCase.LabelID}) + } else { + unittest.AssertNotExistsBean(t, &issues_model.IssueLabel{IssueID: issueID, LabelID: testCase.LabelID}) + } } unittest.CheckConsistencyFor(t, &issues_model.Label{}) } diff --git a/services/org/user_test.go b/services/org/user_test.go index 56d01a3b63..96d1a1c8ca 100644 --- a/services/org/user_test.go +++ b/services/org/user_test.go @@ -47,7 +47,7 @@ func TestRemoveOrgUser(t *testing.T) { testSuccess := func(org *organization.Organization, user *user_model.User) { expectedNumMembers := org.NumMembers - if unittest.BeanExists(t, &organization.OrgUser{OrgID: org.ID, UID: user.ID}) { + if unittest.GetBean(t, &organization.OrgUser{OrgID: org.ID, UID: user.ID}) != nil { expectedNumMembers-- } assert.NoError(t, RemoveOrgUser(db.DefaultContext, org, user)) diff --git a/tests/integration/auth_ldap_test.go b/tests/integration/auth_ldap_test.go index 5d37244331..5c50fd0288 100644 --- a/tests/integration/auth_ldap_test.go +++ b/tests/integration/auth_ldap_test.go @@ -332,7 +332,7 @@ func TestLDAPUserSyncWithGroupFilter(t *testing.T) { // Assert members of LDAP group "cn=git" are added for _, gitLDAPUser := range te.gitLDAPUsers { - unittest.BeanExists(t, &user_model.User{ + unittest.AssertExistsAndLoadBean(t, &user_model.User{ Name: gitLDAPUser.UserName, }) } diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 5ecf3ef469..68de421413 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -92,7 +92,7 @@ func TestPullView_CodeOwner(t *testing.T) { testPullCreate(t, session, "user2", "test_codeowner", false, repo.DefaultBranch, "codeowner-basebranch", "Test Pull Request") pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadRepoID: repo.ID, HeadBranch: "codeowner-basebranch"}) - unittest.AssertExistsIf(t, true, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 5}) + unittest.AssertExistsAndLoadBean(t, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 5}) assert.NoError(t, pr.LoadIssue(db.DefaultContext)) err := issue_service.ChangeTitle(db.DefaultContext, pr.Issue, user2, "[WIP] Test Pull Request") @@ -139,7 +139,7 @@ func TestPullView_CodeOwner(t *testing.T) { testPullCreate(t, session, "user2", "test_codeowner", false, repo.DefaultBranch, "codeowner-basebranch2", "Test Pull Request2") pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadBranch: "codeowner-basebranch2"}) - unittest.AssertExistsIf(t, true, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 8}) + unittest.AssertExistsAndLoadBean(t, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 8}) }) t.Run("Forked Repo Pull Request", func(t *testing.T) { @@ -169,13 +169,13 @@ func TestPullView_CodeOwner(t *testing.T) { testPullCreateDirectly(t, session, "user5", "test_codeowner", forkedRepo.DefaultBranch, "", "", "codeowner-basebranch-forked", "Test Pull Request on Forked Repository") pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: forkedRepo.ID, HeadBranch: "codeowner-basebranch-forked"}) - unittest.AssertExistsIf(t, false, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 8}) + unittest.AssertNotExistsBean(t, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 8}) // create a pull request to base repository, code reviewers should be mentioned testPullCreateDirectly(t, session, repo.OwnerName, repo.Name, repo.DefaultBranch, forkedRepo.OwnerName, forkedRepo.Name, "codeowner-basebranch-forked", "Test Pull Request3") pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadRepoID: forkedRepo.ID, HeadBranch: "codeowner-basebranch-forked"}) - unittest.AssertExistsIf(t, true, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 8}) + unittest.AssertExistsAndLoadBean(t, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 8}) }) }) } diff --git a/tests/integration/signin_test.go b/tests/integration/signin_test.go index abad9eb5e5..d7c0b1bcd3 100644 --- a/tests/integration/signin_test.go +++ b/tests/integration/signin_test.go @@ -9,6 +9,7 @@ import ( "strings" "testing" + "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/setting" @@ -17,6 +18,7 @@ import ( "code.gitea.io/gitea/tests" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func testLoginFailed(t *testing.T, username, password, message string) { @@ -42,7 +44,7 @@ func TestSignin(t *testing.T) { user.Name = "testuser" user.LowerName = strings.ToLower(user.Name) user.ID = 0 - unittest.AssertSuccessfulInsert(t, user) + require.NoError(t, db.Insert(db.DefaultContext, user)) samples := []struct { username string