Always set the merge base used to merge the commit (#15352) (#15385)

Backport #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
This commit is contained in:
zeripath 2021-04-10 13:08:30 +01:00 committed by GitHub
parent 67a12b8fac
commit 1fe5fe419e
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 45 additions and 8 deletions

View File

@ -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) { func doAPIMergePullRequest(ctx APITestContext, owner, repo string, index int64) func(*testing.T) {
return func(t *testing.T) { return func(t *testing.T) {
urlStr := fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge?token=%s", urlStr := fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge?token=%s",

View File

@ -5,6 +5,7 @@
package integrations package integrations
import ( import (
"encoding/hex"
"fmt" "fmt"
"io/ioutil" "io/ioutil"
"math/rand" "math/rand"
@ -451,26 +452,34 @@ func doMergeFork(ctx, baseCtx APITestContext, baseBranch, headBranch string) fun
// Then get the diff string // Then get the diff string
var diffHash string var diffHash string
var diffLength int
t.Run("GetDiff", func(t *testing.T) { 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)) 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) resp := ctx.Session.MakeRequestNilResponseHashSumRecorder(t, req, http.StatusOK)
diffHash = string(resp.Hash.Sum(nil)) 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 // 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("MergePR", doAPIMergePullRequest(baseCtx, baseCtx.Username, baseCtx.Reponame, pr.Index))
t.Run("EnsureCanSeePull", doEnsureCanSeePull(baseCtx, pr)) 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 // 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("DeleteHeadBranch", doBranchDelete(baseCtx, baseCtx.Username, baseCtx.Reponame, headBranch))
t.Run("EnsureCanSeePull", doEnsureCanSeePull(baseCtx, pr)) 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 // Delete the head repository & make sure that doesn't break the PR page or change its diff
t.Run("DeleteHeadRepository", doAPIDeleteRepository(ctx)) t.Run("DeleteHeadRepository", doAPIDeleteRepository(ctx))
t.Run("EnsureCanSeePull", doEnsureCanSeePull(baseCtx, pr)) t.Run("EnsureCanSeePull", doEnsureCanSeePull(baseCtx, pr))
t.Run("EnsureDiffNoChange", doEnsureDiffNoChange(baseCtx, pr, diffHash)) t.Run("EnsureDiffNoChange", doEnsureDiffNoChange(baseCtx, pr, diffHash, diffLength))
} }
} }
@ -514,14 +523,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) { 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)) 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) resp := ctx.Session.MakeRequestNilResponseHashSumRecorder(t, req, http.StatusOK)
actual := string(resp.Hash.Sum(nil)) actual := string(resp.Hash.Sum(nil))
actualLength := resp.Length
equal := diffHash == actual 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)
} }
} }

View File

@ -406,7 +406,8 @@ func (pr *PullRequest) SetMerged() (bool, error) {
return false, fmt.Errorf("Issue.changeStatus: %v", err) 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) return false, fmt.Errorf("Failed to update pr[%d]: %v", pr.ID, err)
} }

View File

@ -174,6 +174,7 @@ func (m *Manager) FlushAll(baseCtx context.Context, timeout time.Duration) error
default: default:
} }
mqs := m.ManagedQueues() mqs := m.ManagedQueues()
log.Debug("Found %d Managed Queues", len(mqs))
wg := sync.WaitGroup{} wg := sync.WaitGroup{}
wg.Add(len(mqs)) wg.Add(len(mqs))
allEmpty := true allEmpty := true
@ -184,6 +185,7 @@ func (m *Manager) FlushAll(baseCtx context.Context, timeout time.Duration) error
} }
allEmpty = false allEmpty = false
if flushable, ok := mq.Managed.(Flushable); ok { if flushable, ok := mq.Managed.(Flushable); ok {
log.Debug("Flushing (flushable) queue: %s", mq.Name)
go func(q *ManagedQueue) { go func(q *ManagedQueue) {
localCtx, localCancel := context.WithCancel(ctx) localCtx, localCancel := context.WithCancel(ctx)
pid := q.RegisterWorkers(1, start, hasTimeout, end, localCancel, true) 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() wg.Done()
}(mq) }(mq)
} else { } 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()
}()
} }
} }

View File

@ -64,7 +64,7 @@ func Merge(pr *models.PullRequest, doer *models.User, baseGitRepo *git.Repositor
pr.Merger = doer pr.Merger = doer
pr.MergerID = doer.ID pr.MergerID = doer.ID
if _, err = pr.SetMerged(); err != nil { if _, err := pr.SetMerged(); err != nil {
log.Error("setMerged [%d]: %v", pr.ID, err) log.Error("setMerged [%d]: %v", pr.ID, err)
} }