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.nettags/v1.15.0-rc1
@@ -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", |
@@ -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) | |||
} | |||
} | |||
@@ -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) | |||
} | |||
@@ -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() | |||
}() | |||
} | |||
} |
@@ -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) | |||
} | |||