Remove unnecessary AddTestPullRequestTask invoking and adjust adding push comments before checking

Author: lunny

services/issue/comments.go

@@ -164,6 +164,13 @@ func LoadCommentPushCommits(ctx context.Context, c *issues_model.Comment) (err e
 
 	c.IsForcePush = data.IsForcePush
 
+	if err := c.LoadIssue(ctx); err != nil {
+		return err
+	}
+	if err := c.Issue.LoadRepo(ctx); err != nil {
+		return err
+	}
+
 	if c.IsForcePush {
 		if len(data.CommitIDs) != 2 {
 			return nil

services/pull/merge.go

@@ -247,18 +247,8 @@ func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.U
 		return fmt.Errorf("lock.Lock: %w", err)
 	}
 	defer releaser()
-	defer func() {
-		go AddTestPullRequestTask(TestPullRequestOptions{
-			RepoID:      pr.BaseRepo.ID,
-			Doer:        doer,
-			Branch:      pr.BaseBranch,
-			IsSync:      false,
-			IsForcePush: false,
-			OldCommitID: "",
-			NewCommitID: "",
-		})
-	}()
 
+	// since test pull request task will be called after push, so we don't need to check again here
 	_, err = doMergeAndPush(ctx, pr, doer, mergeStyle, expectedHeadCommitID, message, repo_module.PushTriggerPRMergeToBase)
 	releaser()
 	if err != nil {

services/pull/pull.go

@@ -376,7 +376,6 @@ func AddTestPullRequestTask(opts TestPullRequestOptions) {
 	graceful.GetManager().RunWithShutdownContext(func(ctx context.Context) {
 		// There is no sensible way to shut this down ":-("
 		// If you don't let it run all the way then you will lose data
-		// TODO: graceful: AddTestPullRequestTask needs to become a queue!
 
 		repo, err := repo_model.GetRepositoryByID(ctx, opts.RepoID)
 		if err != nil {
@@ -402,11 +401,15 @@ func AddTestPullRequestTask(opts TestPullRequestOptions) {
 				continue
 			}
 
-			StartPullRequestCheckImmediately(ctx, pr)
+			// create push comment first then check pull request status so the test
+			// will get a stable result
 			comment, err := CreatePushPullComment(ctx, opts.Doer, pr, opts.OldCommitID, opts.NewCommitID, opts.IsForcePush)
 			if err == nil && comment != nil {
 				notify_service.PullRequestPushCommits(ctx, opts.Doer, pr, comment)
 			}
+			// FIXME: since AddTestPullRequestTask was called in a goroutine or a queue worker, is it still necessary to
+			// call another queue to handle the PR check? Maybe we can use checkPullRequestMergeable directly here?
+			StartPullRequestCheckImmediately(ctx, pr)
 		}
 
 		if opts.IsSync {

services/pull/update.go

@@ -62,18 +62,7 @@ func Update(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.
 		return fmt.Errorf("unable to load HeadRepo for PR[%d] during update-by-merge: %w", pr.ID, err)
 	}
 
-	defer func() {
-		go AddTestPullRequestTask(TestPullRequestOptions{
-			RepoID:      pr.BaseRepo.ID,
-			Doer:        doer,
-			Branch:      pr.BaseBranch,
-			IsSync:      false,
-			IsForcePush: false,
-			OldCommitID: "",
-			NewCommitID: "",
-		})
-	}()
-
+	// we don't need to run pull request check here because it will be triggered by push
 	if rebase {
 		return updateHeadByRebaseOnToBase(ctx, pr, doer)
 	}

services/repository/push.go

@@ -310,7 +310,8 @@ func pushUpdateBranch(_ context.Context, repo *repo_model.Repository, pusher *us
 	}
 
 	// only update branch can trigger pull request task because the pull request hasn't been created yet when creating a branch
-	go pull_service.AddTestPullRequestTask(pull_service.TestPullRequestOptions{
+	// since pushUpdateBrach is called in a queue worker, we don't need to call it in a go routine again
+	pull_service.AddTestPullRequestTask(pull_service.TestPullRequestOptions{
 		RepoID:      repo.ID,
 		Doer:        pusher,
 		Branch:      branch,

tests/integration/pull_comment_test.go

@@ -62,7 +62,8 @@ func testPullCommentRebase(t *testing.T, u *url.URL, session *TestSession) {
 	doGitPushTestRepositoryFail(dstPath, "base-repo", "local-branch/rebase:test-branch/rebase")(t)
 	doGitPushTestRepository(dstPath, "--force", "base-repo", "local-branch/rebase:test-branch/rebase")(t)
 
-	// reload the pr
+	// FIXME: Both the status of the pull request before pushing and the waiting are mergeable. So that
+	// if it's fast enough here, the pull request checking might be not process yet.
 	prIssue := testWaitForPullRequestStatus(t, &issues_model.Issue{Title: testPRTitle}, issues_model.PullRequestStatusMergeable)
 	comments, err := issues_model.FindComments(t.Context(), &issues_model.FindCommentsOptions{
 		IssueID: prIssue.ID,