From 84f5a0bc62568b6d0a67969e460f4d41339a07d6 Mon Sep 17 00:00:00 2001
From: zeripath <art27@cantab.net>
Date: Sat, 10 Apr 2021 09:27:29 +0100
Subject: [PATCH] Always set the merge base used to merge the commit (#15352)
The issue is that the TestPatch will reset the PR MergeBase - and it is possible for TestPatch to update the MergeBase whilst a merge is ongoing. The ensuing merge will then complete but it doesn't re-set the MergeBase it used to merge the PR.
Fixes the intermittent error in git test.
Signed-off-by: Andrew Thornton art27@cantab.net
---
.../api_helper_for_declarative_test.go | 20 +++++++++++++++++++
integrations/git_test.go | 20 ++++++++++++++-----
models/pull.go | 3 ++-
modules/queue/manager.go | 8 +++++++-
services/pull/merge.go | 2 +-
5 files changed, 45 insertions(+), 8 deletions(-)
diff --git a/integrations/api_helper_for_declarative_test.go b/integrations/api_helper_for_declarative_test.go
index 9399924b5e..d56ae5d27c 100644
--- a/integrations/api_helper_for_declarative_test.go
+++ b/integrations/api_helper_for_declarative_test.go
@@ -239,6 +239,26 @@ func doAPICreatePullRequest(ctx APITestContext, owner, repo, baseBranch, headBra
}
}
+func doAPIGetPullRequest(ctx APITestContext, owner, repo string, index int64) func(*testing.T) (api.PullRequest, error) {
+ return func(t *testing.T) (api.PullRequest, error) {
+ urlStr := fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d?token=%s",
+ owner, repo, index, ctx.Token)
+ req := NewRequest(t, http.MethodGet, urlStr)
+
+ expected := 200
+ if ctx.ExpectedCode != 0 {
+ expected = ctx.ExpectedCode
+ }
+ resp := ctx.Session.MakeRequest(t, req, expected)
+
+ json := jsoniter.ConfigCompatibleWithStandardLibrary
+ decoder := json.NewDecoder(resp.Body)
+ pr := api.PullRequest{}
+ err := decoder.Decode(&pr)
+ return pr, err
+ }
+}
+
func doAPIMergePullRequest(ctx APITestContext, owner, repo string, index int64) func(*testing.T) {
return func(t *testing.T) {
urlStr := fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge?token=%s",
diff --git a/integrations/git_test.go b/integrations/git_test.go
index aa1b3ee2ac..13a60076a7 100644
--- a/integrations/git_test.go
+++ b/integrations/git_test.go
@@ -5,6 +5,7 @@
package integrations
import (
+ "encoding/hex"
"fmt"
"io/ioutil"
"math/rand"
@@ -452,26 +453,34 @@ func doMergeFork(ctx, baseCtx APITestContext, baseBranch, headBranch string) fun
// Then get the diff string
var diffHash string
+ var diffLength int
t.Run("GetDiff", func(t *testing.T) {
req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d.diff", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index))
resp := ctx.Session.MakeRequestNilResponseHashSumRecorder(t, req, http.StatusOK)
diffHash = string(resp.Hash.Sum(nil))
+ diffLength = resp.Length
})
// Now: Merge the PR & make sure that doesn't break the PR page or change its diff
t.Run("MergePR", doAPIMergePullRequest(baseCtx, baseCtx.Username, baseCtx.Reponame, pr.Index))
t.Run("EnsureCanSeePull", doEnsureCanSeePull(baseCtx, pr))
- t.Run("EnsureDiffNoChange", doEnsureDiffNoChange(baseCtx, pr, diffHash))
+ t.Run("CheckPR", func(t *testing.T) {
+ oldMergeBase := pr.MergeBase
+ pr2, err := doAPIGetPullRequest(baseCtx, baseCtx.Username, baseCtx.Reponame, pr.Index)(t)
+ assert.NoError(t, err)
+ assert.Equal(t, oldMergeBase, pr2.MergeBase)
+ })
+ t.Run("EnsurDiffNoChange", doEnsureDiffNoChange(baseCtx, pr, diffHash, diffLength))
// Then: Delete the head branch & make sure that doesn't break the PR page or change its diff
t.Run("DeleteHeadBranch", doBranchDelete(baseCtx, baseCtx.Username, baseCtx.Reponame, headBranch))
t.Run("EnsureCanSeePull", doEnsureCanSeePull(baseCtx, pr))
- t.Run("EnsureDiffNoChange", doEnsureDiffNoChange(baseCtx, pr, diffHash))
+ t.Run("EnsureDiffNoChange", doEnsureDiffNoChange(baseCtx, pr, diffHash, diffLength))
// Delete the head repository & make sure that doesn't break the PR page or change its diff
t.Run("DeleteHeadRepository", doAPIDeleteRepository(ctx))
t.Run("EnsureCanSeePull", doEnsureCanSeePull(baseCtx, pr))
- t.Run("EnsureDiffNoChange", doEnsureDiffNoChange(baseCtx, pr, diffHash))
+ t.Run("EnsureDiffNoChange", doEnsureDiffNoChange(baseCtx, pr, diffHash, diffLength))
}
}
@@ -515,14 +524,15 @@ func doEnsureCanSeePull(ctx APITestContext, pr api.PullRequest) func(t *testing.
}
}
-func doEnsureDiffNoChange(ctx APITestContext, pr api.PullRequest, diffHash string) func(t *testing.T) {
+func doEnsureDiffNoChange(ctx APITestContext, pr api.PullRequest, diffHash string, diffLength int) func(t *testing.T) {
return func(t *testing.T) {
req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d.diff", url.PathEscape(ctx.Username), url.PathEscape(ctx.Reponame), pr.Index))
resp := ctx.Session.MakeRequestNilResponseHashSumRecorder(t, req, http.StatusOK)
actual := string(resp.Hash.Sum(nil))
+ actualLength := resp.Length
equal := diffHash == actual
- assert.True(t, equal, "Unexpected change in the diff string: expected hash: %s but was actually: %s", diffHash, actual)
+ assert.True(t, equal, "Unexpected change in the diff string: expected hash: %s size: %d but was actually: %s size: %d", hex.EncodeToString([]byte(diffHash)), diffLength, hex.EncodeToString([]byte(actual)), actualLength)
}
}
diff --git a/models/pull.go b/models/pull.go
index 47e699e192..133f196aae 100644
--- a/models/pull.go
+++ b/models/pull.go
@@ -406,7 +406,8 @@ func (pr *PullRequest) SetMerged() (bool, error) {
return false, fmt.Errorf("Issue.changeStatus: %v", err)
}
- if _, err := sess.Where("id = ?", pr.ID).Cols("has_merged, status, merged_commit_id, merger_id, merged_unix").Update(pr); err != nil {
+ // We need to save all of the data used to compute this merge as it may have already been changed by TestPatch. FIXME: need to set some state to prevent TestPatch from running whilst we are merging.
+ if _, err := sess.Where("id = ?", pr.ID).Cols("has_merged, status, merge_base, merged_commit_id, merger_id, merged_unix").Update(pr); err != nil {
return false, fmt.Errorf("Failed to update pr[%d]: %v", pr.ID, err)
}
diff --git a/modules/queue/manager.go b/modules/queue/manager.go
index d44007a0f0..da0fc606e6 100644
--- a/modules/queue/manager.go
+++ b/modules/queue/manager.go
@@ -174,6 +174,7 @@ func (m *Manager) FlushAll(baseCtx context.Context, timeout time.Duration) error
default:
}
mqs := m.ManagedQueues()
+ log.Debug("Found %d Managed Queues", len(mqs))
wg := sync.WaitGroup{}
wg.Add(len(mqs))
allEmpty := true
@@ -184,6 +185,7 @@ func (m *Manager) FlushAll(baseCtx context.Context, timeout time.Duration) error
}
allEmpty = false
if flushable, ok := mq.Managed.(Flushable); ok {
+ log.Debug("Flushing (flushable) queue: %s", mq.Name)
go func(q *ManagedQueue) {
localCtx, localCancel := context.WithCancel(ctx)
pid := q.RegisterWorkers(1, start, hasTimeout, end, localCancel, true)
@@ -196,7 +198,11 @@ func (m *Manager) FlushAll(baseCtx context.Context, timeout time.Duration) error
wg.Done()
}(mq)
} else {
- wg.Done()
+ log.Debug("Queue: %s is non-empty but is not flushable - adding 100 millisecond wait", mq.Name)
+ go func() {
+ <-time.After(100 * time.Millisecond)
+ wg.Done()
+ }()
}
}
diff --git a/services/pull/merge.go b/services/pull/merge.go
index 518ffa4849..7e6a214b87 100644
--- a/services/pull/merge.go
+++ b/services/pull/merge.go
@@ -64,7 +64,7 @@ func Merge(pr *models.PullRequest, doer *models.User, baseGitRepo *git.Repositor
pr.Merger = doer
pr.MergerID = doer.ID
- if _, err = pr.SetMerged(); err != nil {
+ if _, err := pr.SetMerged(); err != nil {
log.Error("setMerged [%d]: %v", pr.ID, err)
}