Skip to content

Attempt to prevent intermittent failure TestGit/xxx/BranchProtectMerge/MergePR #18451

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

zeripath
Copy link
Contributor

One of the repeated intermittent failures we see in testing is a failure due to
branches not being ready to merge.

Prior to the immediate queue implementation we would attempt to flush all the queues
and this would prevent the issue. However, the immediate queue is not flushable so
the flushall is not successful at preventing this.

This PR proposes an alternative solution - wait some time and try again up to 5 times.

If this fails then there is a genuine issue and we should fail.

Related #17719

Signed-off-by: Andrew Thornton [email protected]

…e/MergePR

One of the repeated intermittent failures we see in testing is a failure due to
branches not being ready to merge.

Prior to the immediate queue implementation we would attempt to flush all the queues
and this would prevent the issue. However, the immediate queue is not flushable so
the flushall is not successful at preventing this.

This PR proposes an alternative solution - wait some time and try again up to 5 times.

If this fails then there is a genuine issue and we should fail.

Related go-gitea#17719

Signed-off-by: Andrew Thornton <[email protected]>
@zeripath zeripath added type/testing skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Jan 29, 2022
@zeripath zeripath added this to the 1.17.0 milestone Jan 29, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jan 29, 2022

Codecov Report

❗ No coverage uploaded for pull request base (main@b34923d). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main   #18451   +/-   ##
=======================================
  Coverage        ?   45.99%           
=======================================
  Files           ?      842           
  Lines           ?    93192           
  Branches        ?        0           
=======================================
  Hits            ?    42863           
  Misses          ?    43524           
  Partials        ?     6805           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b34923d...38970c5. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 29, 2022
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jan 29, 2022
@wxiaoguang
Copy link
Contributor

How about remove the duplicated code block.

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", owner, repo, index, ctx.Token)

		var req *http.Request
		var resp *httptest.ResponseRecorder
		for i := 0; i < 5; i++ {
			req = NewRequestWithJSON(t, http.MethodPost, urlStr, &forms.MergePullRequestForm{
				MergeMessageField: "doAPIMergePullRequest Merge",
				Do:                string(repo_model.MergeStyleMerge),
			})
			resp = ctx.Session.MakeRequest(t, req, NoExpectedStatus)
			if resp.Code != http.StatusMethodNotAllowed {
				break
			}
			err := api.APIError{}
			DecodeJSON(t, resp, &err)
			if !assert.EqualValues(t, "Please try again later", err.Message) {
				break
			}
			_ = queue.GetManager().FlushAll(context.Background(), 5*time.Second)
			<-time.After(1 * time.Second)
		}

		expected := ctx.ExpectedCode
		if expected == 0 {
			expected = 200
		}

		if !assert.EqualValues(t, expected, resp.Code,
			"Request: %s %s", req.Method, req.URL.String()) {
			logUnexpectedResponse(t, resp)
		}
	}
}

Signed-off-by: Andrew Thornton <[email protected]>
@zeripath
Copy link
Contributor Author

@wxiaoguang done.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jan 29, 2022
@zeripath zeripath merged commit 2ad74a5 into go-gitea:main Jan 29, 2022
@zeripath zeripath deleted the repeatedly-try-to-merge-in-doAPIMergePullRequest branch January 29, 2022 15:35
zeripath added a commit to zeripath/gitea that referenced this pull request Feb 5, 2022
…e/MergePR (go-gitea#18451)

Backport go-gitea#18451

One of the repeated intermittent failures we see in testing is a failure due to
branches not being ready to merge.

Prior to the immediate queue implementation we would attempt to flush all the queues
and this would prevent the issue. However, the immediate queue is not flushable so
the flushall is not successful at preventing this.

This PR proposes an alternative solution - wait some time and try again up to 5 times.

If this fails then there is a genuine issue and we should fail.

Related go-gitea#17719

Signed-off-by: Andrew Thornton <[email protected]>
@zeripath zeripath added backport/done All backports for this PR have been created backport/v1.1 labels Feb 5, 2022
zeripath added a commit that referenced this pull request Feb 5, 2022
…e/MergePR (#18451) (#18619)

Backport #18451

One of the repeated intermittent failures we see in testing is a failure due to
branches not being ready to merge.

Prior to the immediate queue implementation we would attempt to flush all the queues
and this would prevent the issue. However, the immediate queue is not flushable so
the flushall is not successful at preventing this.

This PR proposes an alternative solution - wait some time and try again up to 5 times.

If this fails then there is a genuine issue and we should fail.

Related #17719

Signed-off-by: Andrew Thornton <[email protected]>
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
…e/MergePR (go-gitea#18451)

One of the repeated intermittent failures we see in testing is a failure due to
branches not being ready to merge.

Prior to the immediate queue implementation we would attempt to flush all the queues
and this would prevent the issue. However, the immediate queue is not flushable so
the flushall is not successful at preventing this.

This PR proposes an alternative solution - wait some time and try again up to 5 times.

If this fails then there is a genuine issue and we should fail.

Related go-gitea#17719

Signed-off-by: Andrew Thornton <[email protected]>
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants