From 4b9fb513eb0dd2990568c69be8b434c4d8cc949a Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Tue, 16 Jan 2024 15:03:45 +0100 Subject: [PATCH] [IMP] *: make to_pr more resilient to webhook delays Github delivery delays keep getting worse. Depending on what comes before `to_pr`, this leads it to fail more often as it runs before the PR it's looking for was signaled to the mergebot. In order to mitigate this issue, add a wait loop in `to_pr`, waiting up to 4 seconds for the PR it's looking for before aborting. Also replace manual lookups by `to_pr` in every method of `TestPRUpdate` while at it since it hit a few of the issues. And remove the xfail test case since it seems unlikely github will change tack (maybe? could be worth testing to be sure). --- mergebot_test_utils/utils.py | 18 ++++--- runbot_merge/tests/test_basic.py | 84 +++++++------------------------- 2 files changed, 30 insertions(+), 72 deletions(-) diff --git a/mergebot_test_utils/utils.py b/mergebot_test_utils/utils.py index 5f61bbb1..22f69ec5 100644 --- a/mergebot_test_utils/utils.py +++ b/mergebot_test_utils/utils.py @@ -1,6 +1,7 @@ # -*- coding: utf-8 -*- import itertools import re +import time from lxml import html @@ -128,12 +129,17 @@ def pr_page(page, pr): return html.fromstring(page(f'/{pr.repo.name}/pull/{pr.number}')) def to_pr(env, pr): - pr = env['runbot_merge.pull_requests'].search([ - ('repository.name', '=', pr.repo.name), - ('number', '=', pr.number), - ]) - assert len(pr) == 1, f"Expected to find {pr.repo.name}#{pr.number}, got {pr}." - return pr + for _ in range(5): + pr_id = env['runbot_merge.pull_requests'].search([ + ('repository.name', '=', pr.repo.name), + ('number', '=', pr.number), + ]) + if pr_id: + assert len(pr_id) == 1, f"Expected to find {pr.repo.name}#{pr.number}, got {pr_id}." + return pr_id + time.sleep(1) + + raise TimeoutError(f"Unable to find {pr.repo.name}#{pr.number}") def part_of(label, pr_id, *, separator='\n\n'): """ Adds the "part-of" pseudo-header in the footer. diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index 1194283e..bbde05e4 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -2121,10 +2121,8 @@ class TestPRUpdate(object): c = repo.make_commit(m, 'fist', None, tree={'m': 'c1'}) prx = repo.make_pr(title='title', body='body', target='master', head=c) - pr = env['runbot_merge.pull_requests'].search([ - ('repository.name', '=', repo.name), - ('number', '=', prx.number), - ]) + + pr = to_pr(env, prx) assert pr.head == c # alter & push force PR entirely with repo: @@ -2139,10 +2137,8 @@ class TestPRUpdate(object): c = repo.make_commit(m, 'fist', None, tree={'m': 'c1'}) prx = repo.make_pr(title='title', body='body', target='master', head=c) - pr = env['runbot_merge.pull_requests'].search([ - ('repository.name', '=', repo.name), - ('number', '=', prx.number), - ]) + + pr = to_pr(env, prx) with repo: prx.close() assert pr.state == 'closed' @@ -2169,10 +2165,7 @@ class TestPRUpdate(object): repo.post_status(prx.head, 'success', 'legal/cla') repo.post_status(prx.head, 'success', 'ci/runbot') env.run_crons() - pr = env['runbot_merge.pull_requests'].search([ - ('repository.name', '=', repo.name), - ('number', '=', prx.number), - ]) + pr = to_pr(env, prx) assert pr.head == c assert pr.state == 'validated' @@ -2190,10 +2183,8 @@ class TestPRUpdate(object): c = repo.make_commit(m, 'fist', None, tree={'m': 'c1'}) prx = repo.make_pr(title='title', body='body', target='master', head=c) prx.post_comment('hansen r+', config['role_reviewer']['token']) - pr = env['runbot_merge.pull_requests'].search([ - ('repository.name', '=', repo.name), - ('number', '=', prx.number), - ]) + + pr = to_pr(env, prx) assert pr.head == c assert pr.state == 'approved' @@ -2216,10 +2207,7 @@ class TestPRUpdate(object): repo.post_status(prx.head, 'success', 'ci/runbot') prx.post_comment('hansen r+', config['role_reviewer']['token']) env.run_crons() - pr = env['runbot_merge.pull_requests'].search([ - ('repository.name', '=', repo.name), - ('number', '=', prx.number), - ]) + pr = to_pr(env, prx) assert pr.head == c assert pr.state == 'ready' @@ -2241,11 +2229,9 @@ class TestPRUpdate(object): repo.post_status(prx.head, 'success', 'legal/cla') repo.post_status(prx.head, 'success', 'ci/runbot') prx.post_comment('hansen r+', config['role_reviewer']['token']) - pr = env['runbot_merge.pull_requests'].search([ - ('repository.name', '=', repo.name), - ('number', '=', prx.number), - ]) + env.run_crons() + pr = to_pr(env, prx) assert pr.state == 'ready' assert pr.staging_id @@ -2314,11 +2300,8 @@ class TestPRUpdate(object): repo.post_status(prx.head, 'success', 'legal/cla') repo.post_status(prx.head, 'success', 'ci/runbot') prx.post_comment('hansen r+', config['role_reviewer']['token']) - pr = env['runbot_merge.pull_requests'].search([ - ('repository.name', '=', repo.name), - ('number', '=', prx.number), - ]) env.run_crons() + pr = to_pr(env, prx) assert pr.state == 'ready' assert pr.staging_id @@ -2371,10 +2354,7 @@ class TestPRUpdate(object): with repo: prx = repo.make_pr(title='title', body='body', target='master', head=c) - pr = env['runbot_merge.pull_requests'].search([ - ('repository.name', '=', repo.name), - ('number', '=', prx.number), - ]) + pr = to_pr(env, prx) assert pr.head == c assert pr.state == 'opened' @@ -2407,10 +2387,9 @@ class TestPRUpdate(object): repo.post_status(pr.head, 'success', 'legal/cla') repo.post_status(pr.head, 'success', 'ci/runbot') pr.post_comment('hansen r+', config['role_reviewer']['token']) - pr_id = env['runbot_merge.pull_requests'].search([ - ('repository.name', '=', repo.name), - ('number', '=', pr.number), - ]) + + env.run_crons() + pr_id = to_pr(env, pr) env.run_crons('runbot_merge.process_updated_commits') assert pr_id.message == 'title\n\nbody' assert pr_id.state == 'ready' @@ -2536,10 +2515,7 @@ Please check and re-approve. [c] = repo.make_commits(m, repo.Commit('first', tree={'m': 'm3'}), ref='heads/abranch') prx = repo.make_pr(title='title', body='body', target='master', head=c) env.run_crons() - pr = env['runbot_merge.pull_requests'].search([ - ('repository.name', '=', repo.name), - ('number', '=', prx.number) - ]) + pr = to_pr(env, prx) assert pr.state == 'opened' assert pr.head == c assert pr.squash @@ -2575,10 +2551,8 @@ Please check and re-approve. repo.post_status(c, 'success', 'ci/runbot') prx = repo.make_pr(title='title', body='body', target='master', head=c) - pr = env['runbot_merge.pull_requests'].search([ - ('repository.name', '=', repo.name), - ('number', '=', prx.number), - ]) + env.run_crons() + pr = to_pr(env, prx) assert pr.state == 'validated', \ "if a PR is created on a CI'd commit, it should be validated immediately" @@ -2589,28 +2563,6 @@ Please check and re-approve. assert pr.state == 'validated', \ "if a PR is reopened and had a CI'd head, it should be validated immediately" - @pytest.mark.xfail(reason="github doesn't allow reopening force-pushed PRs", strict=True) - def test_force_update_closed(self, env, repo): - with repo: - [m] = repo.make_commits(None, repo.Commit('initial', tree={'m': 'm'}), ref='heads/master') - - [c] = repo.make_commits(m, repo.Commit('first', tree={'m': 'm3'}), ref='heads/abranch') - prx = repo.make_pr(title='title', body='body', target='master', head=c) - env.run_crons() - pr = env['runbot_merge.pull_requests'].search([ - ('repository.name', '=', repo.name), - ('number', '=', prx.number) - ]) - with repo: - prx.close() - - with repo: - c2 = repo.make_commit(m, 'xxx', None, tree={'m': 'm4'}) - repo.update_ref(prx.ref, c2, force=True) - - with repo: - prx.open() - assert pr.head == c2 class TestBatching(object): def _pr(self, repo, prefix, trees, *, target='master', user, reviewer,