From c25a16e04bf8bbf7d885ee38c41de5dfe712fafb Mon Sep 17 00:00:00 2001 From: Jason Song Date: Sun, 29 Jan 2023 15:30:58 +0800 Subject: [PATCH 01/12] fix: PushToBaseRepo first --- services/pull/pull.go | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index c983c4f3e78ec..f112516c0c7c7 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -263,6 +263,24 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string, return } + for _, pr := range prs { + log.Trace("Updating PR[%d]: composing new test task", pr.ID) + if pr.Flow == issues_model.PullRequestFlowGithub { + if err := PushToBaseRepo(ctx, pr); err != nil { + log.Error("PushToBaseRepo: %v", err) + continue + } + } else { + continue + } + + AddToTaskQueue(pr) + comment, err := CreatePushPullComment(ctx, doer, pr, oldCommitID, newCommitID) + if err == nil && comment != nil { + notification.NotifyPullRequestPushCommits(ctx, doer, pr, comment) + } + } + if isSync { requests := issues_model.PullRequestList(prs) if err = requests.LoadAttributes(); err != nil { @@ -304,24 +322,6 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string, } } - for _, pr := range prs { - log.Trace("Updating PR[%d]: composing new test task", pr.ID) - if pr.Flow == issues_model.PullRequestFlowGithub { - if err := PushToBaseRepo(ctx, pr); err != nil { - log.Error("PushToBaseRepo: %v", err) - continue - } - } else { - continue - } - - AddToTaskQueue(pr) - comment, err := CreatePushPullComment(ctx, doer, pr, oldCommitID, newCommitID) - if err == nil && comment != nil { - notification.NotifyPullRequestPushCommits(ctx, doer, pr, comment) - } - } - log.Trace("AddTestPullRequestTask [base_repo_id: %d, base_branch: %s]: finding pull requests", repoID, branch) prs, err = issues_model.GetUnmergedPullRequestsByBaseInfo(repoID, branch) if err != nil { From fc94e9f3b153a6b11306f4133cdd2c0afe42e137 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Mon, 30 Jan 2023 14:29:53 +0800 Subject: [PATCH 02/12] chore: debug CI --- tests/integration/pull_update_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/integration/pull_update_test.go b/tests/integration/pull_update_test.go index bd416e5bcfac3..a2e22059d47a7 100644 --- a/tests/integration/pull_update_test.go +++ b/tests/integration/pull_update_test.go @@ -58,6 +58,12 @@ func TestAPIPullUpdateByRebase(t *testing.T) { org26 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 26}) pr := createOutdatedPR(t, user, org26) + // debug code, I'll delete it before PR get merged. + if user.MaxRepoCreation != -1 { + t.Fatalf("cannot create repo: %+v", user) + return + } + // Test GetDiverging diffCount, err := pull_service.GetDiverging(git.DefaultContext, pr) assert.NoError(t, err) From 9f2f3df6fcc353963c2f18a86a030be4d1aab80e Mon Sep 17 00:00:00 2001 From: Jason Song Date: Mon, 30 Jan 2023 15:03:54 +0800 Subject: [PATCH 03/12] chore: debug CI --- tests/integration/pull_update_test.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/integration/pull_update_test.go b/tests/integration/pull_update_test.go index a2e22059d47a7..26f731a4a557c 100644 --- a/tests/integration/pull_update_test.go +++ b/tests/integration/pull_update_test.go @@ -56,13 +56,14 @@ func TestAPIPullUpdateByRebase(t *testing.T) { // Create PR to test user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) org26 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 26}) - pr := createOutdatedPR(t, user, org26) - // debug code, I'll delete it before PR get merged. - if user.MaxRepoCreation != -1 { - t.Fatalf("cannot create repo: %+v", user) + if user.MaxRepoCreation != -1 || org26.MaxRepoCreation != -1 { + t.Errorf("cannot create repo: %+v", user) + t.Errorf("cannot create repo: %+v", org26) + t.FailNow() return } + pr := createOutdatedPR(t, user, org26) // Test GetDiverging diffCount, err := pull_service.GetDiverging(git.DefaultContext, pr) From a64347ef5324d0639ac7739896bebe38f0c36a81 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Mon, 30 Jan 2023 15:40:34 +0800 Subject: [PATCH 04/12] Revert "fix: PushToBaseRepo first" This reverts commit c25a16e04bf8bbf7d885ee38c41de5dfe712fafb. --- services/pull/pull.go | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index f112516c0c7c7..c983c4f3e78ec 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -263,24 +263,6 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string, return } - for _, pr := range prs { - log.Trace("Updating PR[%d]: composing new test task", pr.ID) - if pr.Flow == issues_model.PullRequestFlowGithub { - if err := PushToBaseRepo(ctx, pr); err != nil { - log.Error("PushToBaseRepo: %v", err) - continue - } - } else { - continue - } - - AddToTaskQueue(pr) - comment, err := CreatePushPullComment(ctx, doer, pr, oldCommitID, newCommitID) - if err == nil && comment != nil { - notification.NotifyPullRequestPushCommits(ctx, doer, pr, comment) - } - } - if isSync { requests := issues_model.PullRequestList(prs) if err = requests.LoadAttributes(); err != nil { @@ -322,6 +304,24 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string, } } + for _, pr := range prs { + log.Trace("Updating PR[%d]: composing new test task", pr.ID) + if pr.Flow == issues_model.PullRequestFlowGithub { + if err := PushToBaseRepo(ctx, pr); err != nil { + log.Error("PushToBaseRepo: %v", err) + continue + } + } else { + continue + } + + AddToTaskQueue(pr) + comment, err := CreatePushPullComment(ctx, doer, pr, oldCommitID, newCommitID) + if err == nil && comment != nil { + notification.NotifyPullRequestPushCommits(ctx, doer, pr, comment) + } + } + log.Trace("AddTestPullRequestTask [base_repo_id: %d, base_branch: %s]: finding pull requests", repoID, branch) prs, err = issues_model.GetUnmergedPullRequestsByBaseInfo(repoID, branch) if err != nil { From 127ec2ab1d527bf36e708122ff324e72e9dc5a7f Mon Sep 17 00:00:00 2001 From: Jason Song Date: Mon, 30 Jan 2023 17:04:52 +0800 Subject: [PATCH 05/12] chore: add debug log --- tests/integration/pull_update_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/integration/pull_update_test.go b/tests/integration/pull_update_test.go index 26f731a4a557c..3c72b66f96b01 100644 --- a/tests/integration/pull_update_test.go +++ b/tests/integration/pull_update_test.go @@ -57,6 +57,8 @@ func TestAPIPullUpdateByRebase(t *testing.T) { user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) org26 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 26}) // debug code, I'll delete it before PR get merged. + t.Logf("user: %+v", user) + t.Logf("org26: %+v", org26) if user.MaxRepoCreation != -1 || org26.MaxRepoCreation != -1 { t.Errorf("cannot create repo: %+v", user) t.Errorf("cannot create repo: %+v", org26) From faae1b29f18bf4ca8150a8cf1401c3f26dcdc761 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Sun, 29 Jan 2023 15:30:58 +0800 Subject: [PATCH 06/12] fix: PushToBaseRepo first --- services/pull/pull.go | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index c983c4f3e78ec..f112516c0c7c7 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -263,6 +263,24 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string, return } + for _, pr := range prs { + log.Trace("Updating PR[%d]: composing new test task", pr.ID) + if pr.Flow == issues_model.PullRequestFlowGithub { + if err := PushToBaseRepo(ctx, pr); err != nil { + log.Error("PushToBaseRepo: %v", err) + continue + } + } else { + continue + } + + AddToTaskQueue(pr) + comment, err := CreatePushPullComment(ctx, doer, pr, oldCommitID, newCommitID) + if err == nil && comment != nil { + notification.NotifyPullRequestPushCommits(ctx, doer, pr, comment) + } + } + if isSync { requests := issues_model.PullRequestList(prs) if err = requests.LoadAttributes(); err != nil { @@ -304,24 +322,6 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string, } } - for _, pr := range prs { - log.Trace("Updating PR[%d]: composing new test task", pr.ID) - if pr.Flow == issues_model.PullRequestFlowGithub { - if err := PushToBaseRepo(ctx, pr); err != nil { - log.Error("PushToBaseRepo: %v", err) - continue - } - } else { - continue - } - - AddToTaskQueue(pr) - comment, err := CreatePushPullComment(ctx, doer, pr, oldCommitID, newCommitID) - if err == nil && comment != nil { - notification.NotifyPullRequestPushCommits(ctx, doer, pr, comment) - } - } - log.Trace("AddTestPullRequestTask [base_repo_id: %d, base_branch: %s]: finding pull requests", repoID, branch) prs, err = issues_model.GetUnmergedPullRequestsByBaseInfo(repoID, branch) if err != nil { From 919d953fc073b7d8ca4bd52ee03798380546a1e3 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Mon, 30 Jan 2023 18:37:25 +0800 Subject: [PATCH 07/12] chore: debug CI --- tests/integration/pull_update_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/integration/pull_update_test.go b/tests/integration/pull_update_test.go index 3c72b66f96b01..e431161289ec6 100644 --- a/tests/integration/pull_update_test.go +++ b/tests/integration/pull_update_test.go @@ -53,6 +53,9 @@ func TestAPIPullUpdate(t *testing.T) { func TestAPIPullUpdateByRebase(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { + // debug code, I'll delete it before PR get merged. + time.Sleep(5 * time.Second) + // Create PR to test user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) org26 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 26}) @@ -85,6 +88,9 @@ func TestAPIPullUpdateByRebase(t *testing.T) { assert.NoError(t, err) assert.EqualValues(t, 0, diffCount.Behind) assert.EqualValues(t, 1, diffCount.Ahead) + + // debug code, I'll delete it before PR get merged. + time.Sleep(5 * time.Second) }) } From 9d09267ed62c61f610b5bad17ca1cb683bcbbb8e Mon Sep 17 00:00:00 2001 From: Jason Song Date: Tue, 31 Jan 2023 14:16:13 +0800 Subject: [PATCH 08/12] chore: debug CI --- tests/integration/pull_update_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/integration/pull_update_test.go b/tests/integration/pull_update_test.go index e431161289ec6..37f60e8eb72c3 100644 --- a/tests/integration/pull_update_test.go +++ b/tests/integration/pull_update_test.go @@ -54,6 +54,7 @@ func TestAPIPullUpdate(t *testing.T) { func TestAPIPullUpdateByRebase(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { // debug code, I'll delete it before PR get merged. + t.Logf("db.DefaultContext.Err: %#v", db.DefaultContext.Err()) time.Sleep(5 * time.Second) // Create PR to test From 36a60cb7576c4819c2696882abcea4cc9545c1e9 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Tue, 31 Jan 2023 15:28:09 +0800 Subject: [PATCH 09/12] chore: remove debug code --- tests/integration/pull_update_test.go | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/tests/integration/pull_update_test.go b/tests/integration/pull_update_test.go index 37f60e8eb72c3..bd416e5bcfac3 100644 --- a/tests/integration/pull_update_test.go +++ b/tests/integration/pull_update_test.go @@ -53,22 +53,9 @@ func TestAPIPullUpdate(t *testing.T) { func TestAPIPullUpdateByRebase(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { - // debug code, I'll delete it before PR get merged. - t.Logf("db.DefaultContext.Err: %#v", db.DefaultContext.Err()) - time.Sleep(5 * time.Second) - // Create PR to test user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) org26 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 26}) - // debug code, I'll delete it before PR get merged. - t.Logf("user: %+v", user) - t.Logf("org26: %+v", org26) - if user.MaxRepoCreation != -1 || org26.MaxRepoCreation != -1 { - t.Errorf("cannot create repo: %+v", user) - t.Errorf("cannot create repo: %+v", org26) - t.FailNow() - return - } pr := createOutdatedPR(t, user, org26) // Test GetDiverging @@ -89,9 +76,6 @@ func TestAPIPullUpdateByRebase(t *testing.T) { assert.NoError(t, err) assert.EqualValues(t, 0, diffCount.Behind) assert.EqualValues(t, 1, diffCount.Ahead) - - // debug code, I'll delete it before PR get merged. - time.Sleep(5 * time.Second) }) } From e2ac98c0a02a2921fbe055b71c5e8a859937770e Mon Sep 17 00:00:00 2001 From: Jason Song Date: Tue, 31 Jan 2023 15:29:00 +0800 Subject: [PATCH 10/12] test: wait for async operations --- tests/integration/pull_update_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/integration/pull_update_test.go b/tests/integration/pull_update_test.go index bd416e5bcfac3..8b306e1a3f022 100644 --- a/tests/integration/pull_update_test.go +++ b/tests/integration/pull_update_test.go @@ -48,6 +48,9 @@ func TestAPIPullUpdate(t *testing.T) { assert.NoError(t, err) assert.EqualValues(t, 0, diffCount.Behind) assert.EqualValues(t, 2, diffCount.Ahead) + + // wait for async operations triggered by AddTestPullRequestTask + time.Sleep(time.Second) }) } @@ -76,6 +79,9 @@ func TestAPIPullUpdateByRebase(t *testing.T) { assert.NoError(t, err) assert.EqualValues(t, 0, diffCount.Behind) assert.EqualValues(t, 1, diffCount.Ahead) + + // wait for async operations triggered by AddTestPullRequestTask + time.Sleep(time.Second) }) } From 6e7238dc1a5cf576214db4e49a99990c487bd620 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Tue, 31 Jan 2023 16:12:45 +0800 Subject: [PATCH 11/12] test: wait longer --- tests/integration/pull_update_test.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/integration/pull_update_test.go b/tests/integration/pull_update_test.go index 8b306e1a3f022..1df64eede32d2 100644 --- a/tests/integration/pull_update_test.go +++ b/tests/integration/pull_update_test.go @@ -49,8 +49,10 @@ func TestAPIPullUpdate(t *testing.T) { assert.EqualValues(t, 0, diffCount.Behind) assert.EqualValues(t, 2, diffCount.Ahead) - // wait for async operations triggered by AddTestPullRequestTask - time.Sleep(time.Second) + // Wait for async operations triggered by AddTestPullRequestTask. + // A few seconds is safe enough, but maybe a bit long. + // Considering there are many tests that take tens of seconds, I think that's OK. + time.Sleep(5 * time.Second) }) } @@ -80,8 +82,10 @@ func TestAPIPullUpdateByRebase(t *testing.T) { assert.EqualValues(t, 0, diffCount.Behind) assert.EqualValues(t, 1, diffCount.Ahead) - // wait for async operations triggered by AddTestPullRequestTask - time.Sleep(time.Second) + // Wait for async operations triggered by AddTestPullRequestTask. + // A few seconds is safe enough, but maybe a bit long. + // Considering there are many tests that take tens of seconds, I think that's OK. + time.Sleep(5 * time.Second) }) } From 259e55e83b90c68d79cc274a79287faf5dcd0d5e Mon Sep 17 00:00:00 2001 From: Jason Song Date: Sun, 5 Feb 2023 17:57:00 +0800 Subject: [PATCH 12/12] fix: return error if issues and prs may be not in sync --- models/issues/pull_list.go | 13 ++++++++++++- tests/integration/pull_update_test.go | 10 ---------- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/models/issues/pull_list.go b/models/issues/pull_list.go index 007c2fd90333a..68f62ffc4338a 100644 --- a/models/issues/pull_list.go +++ b/models/issues/pull_list.go @@ -13,6 +13,7 @@ import ( user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/base" "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/util" "xorm.io/xorm" ) @@ -175,7 +176,17 @@ func (prs PullRequestList) loadAttributes(ctx context.Context) error { } for _, pr := range prs { pr.Issue = set[pr.IssueID] - pr.Issue.PullRequest = pr // panic here means issueIDs and prs are not in sync + /* + Old code: + pr.Issue.PullRequest = pr // panic here means issueIDs and prs are not in sync + + It's worth panic because it's almost impossible to happen under normal use. + But in integration testing, an asynchronous task could read a database that has been reset. + So returning an error would make more sense, let the caller has a choice to ignore it. + */ + if pr.Issue == nil { + return fmt.Errorf("issues and prs may be not in sync: cannot find issue %v for pr %v: %w", pr.IssueID, pr.ID, util.ErrNotExist) + } } return nil } diff --git a/tests/integration/pull_update_test.go b/tests/integration/pull_update_test.go index 1df64eede32d2..bd416e5bcfac3 100644 --- a/tests/integration/pull_update_test.go +++ b/tests/integration/pull_update_test.go @@ -48,11 +48,6 @@ func TestAPIPullUpdate(t *testing.T) { assert.NoError(t, err) assert.EqualValues(t, 0, diffCount.Behind) assert.EqualValues(t, 2, diffCount.Ahead) - - // Wait for async operations triggered by AddTestPullRequestTask. - // A few seconds is safe enough, but maybe a bit long. - // Considering there are many tests that take tens of seconds, I think that's OK. - time.Sleep(5 * time.Second) }) } @@ -81,11 +76,6 @@ func TestAPIPullUpdateByRebase(t *testing.T) { assert.NoError(t, err) assert.EqualValues(t, 0, diffCount.Behind) assert.EqualValues(t, 1, diffCount.Ahead) - - // Wait for async operations triggered by AddTestPullRequestTask. - // A few seconds is safe enough, but maybe a bit long. - // Considering there are many tests that take tens of seconds, I think that's OK. - time.Sleep(5 * time.Second) }) }