From 6a8c13b1efa5928a5cdf7cc473402eab64ece59a Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 23 Jul 2021 15:45:23 +0200 Subject: [PATCH] [IMP] runbot_merge: show linked PRs during staging and after merging Previously, a PR's status page would only show the linked / related PRs when `open`. Since the relations between PRs remains useful, also make this information available during staging and after merging. Fixes #463 --- mergebot_test_utils/utils.py | 11 ++++++ runbot_merge/models/pull_requests.py | 4 ++ runbot_merge/tests/test_basic.py | 13 ++----- runbot_merge/tests/test_multirepo.py | 56 ++++++++++++++++++++-------- runbot_merge/views/templates.xml | 20 ++++++++++ 5 files changed, 79 insertions(+), 25 deletions(-) diff --git a/mergebot_test_utils/utils.py b/mergebot_test_utils/utils.py index 87d53a4c..57610839 100644 --- a/mergebot_test_utils/utils.py +++ b/mergebot_test_utils/utils.py @@ -2,6 +2,8 @@ import itertools import re +from lxml import html + MESSAGE_TEMPLATE = """{message} closes {repo}#{number} @@ -123,3 +125,12 @@ def make_basic(env, config, make_repo, *, reponame='proj', project_name='myproje }) return prod, other + +def pr_page(page, pr): + return html.fromstring(page(f'/{pr.repo.name}/pull/{pr.number}')) + +def to_pr(env, pr): + return env['runbot_merge.pull_requests'].search([ + ('repository.name', '=', pr.repo.name), + ('number', '=', pr.number), + ]) diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index a6e62e02..885b605c 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -756,6 +756,10 @@ class PullRequests(models.Model): def _linked_prs(self): if re.search(r':patch-\d+', self.label): return self.browse(()) + if self.state == 'merged': + return self.with_context(active_test=False).batch_ids\ + .filtered(lambda b: b.staging_id.state == 'success')\ + .prs - self return self.search([ ('target', '=', self.target.id), ('label', '=', self.label), diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index accd91b6..3c3f8bec 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -9,11 +9,7 @@ import pytest from lxml import html import odoo -from utils import _simple_init, seen, re_matches, get_partner, Commit - - -def pr_page(page, pr): - return html.fromstring(page(f'/{pr.repo.name}/pull/{pr.number}')) +from utils import _simple_init, seen, re_matches, get_partner, Commit, pr_page, to_pr @pytest.fixture def repo(env, project, make_repo, users, setreviewers): @@ -37,10 +33,7 @@ def test_trivial_flow(env, repo, page, users, config): c1 = repo.make_commit(c0, 'add file', None, tree={'a': 'some other content', 'b': 'a second file'}) pr = repo.make_pr(title="gibberish", body="blahblah", target='master', head=c1,) - [pr_id] = env['runbot_merge.pull_requests'].search([ - ('repository.name', '=', repo.name), - ('number', '=', pr.number), - ]) + pr_id = to_pr(env, pr) assert pr_id.state == 'opened' env.run_crons() assert pr.comments == [seen(env, pr, users)] @@ -92,7 +85,7 @@ def test_trivial_flow(env, repo, page, users, config): st = pr_id.staging_id env.run_crons() - assert set(tuple(t) for t in st.statuses) == { + assert {tuple(t) for t in st.statuses} == { (repo.name, 'legal/cla', 'success', ''), (repo.name, 'ci/runbot', 'success', 'http://foo.com/pog'), (repo.name, 'ci/lint', 'failure', 'http://ignored.com/whocares'), diff --git a/runbot_merge/tests/test_multirepo.py b/runbot_merge/tests/test_multirepo.py index 4b62a6bb..182621e3 100644 --- a/runbot_merge/tests/test_multirepo.py +++ b/runbot_merge/tests/test_multirepo.py @@ -10,8 +10,9 @@ import time import pytest import requests +from lxml.etree import XPath, tostring -from utils import seen, re_matches, get_partner +from utils import seen, re_matches, get_partner, pr_page, to_pr @pytest.fixture @@ -20,7 +21,8 @@ def repo_a(project, make_repo, setreviewers): r = project.env['runbot_merge.repository'].create({ 'project_id': project.id, 'name': repo.name, - 'required_statuses': 'legal/cla,ci/runbot' + 'required_statuses': 'legal/cla,ci/runbot', + 'group_id': False, }) setreviewers(r) return repo @@ -31,7 +33,8 @@ def repo_b(project, make_repo, setreviewers): r = project.env['runbot_merge.repository'].create({ 'project_id': project.id, 'name': repo.name, - 'required_statuses': 'legal/cla,ci/runbot' + 'required_statuses': 'legal/cla,ci/runbot', + 'group_id': False, }) setreviewers(r) return repo @@ -42,7 +45,8 @@ def repo_c(project, make_repo, setreviewers): r = project.env['runbot_merge.repository'].create({ 'project_id': project.id, 'name': repo.name, - 'required_statuses': 'legal/cla,ci/runbot' + 'required_statuses': 'legal/cla,ci/runbot', + 'group_id': False, }) setreviewers(r) return repo @@ -56,7 +60,6 @@ def make_pr(repo, prefix, trees, *, target='master', user, :type trees: list[dict] :type target: str :type user: str - :type label: str | None :type statuses: list[(str, str)] :type reviewer: str | None :rtype: fake_github.PR @@ -76,11 +79,7 @@ def make_pr(repo, prefix, trees, *, target='master', user, if reviewer: pr.post_comment('hansen r+', reviewer) return pr -def to_pr(env, pr): - return env['runbot_merge.pull_requests'].search([ - ('repository.name', '=', pr.repo.name), - ('number', '=', pr.number), - ]) + def make_branch(repo, name, message, tree, protect=True): c = repo.make_commit(None, message, None, tree=tree) repo.make_ref('heads/%s' % name, c) @@ -114,28 +113,34 @@ def test_stage_one(env, project, repo_a, repo_b, config): assert to_pr(env, pr_b).state == 'ready' assert not to_pr(env, pr_b).staging_id -def test_stage_match(env, project, repo_a, repo_b, config): +get_related_pr_labels = XPath('.//*[normalize-space(text()) = "Linked pull requests"]//a/text()') +def test_stage_match(env, project, repo_a, repo_b, config, page): """ First PR is matched from A, => should select matched PR from B """ project.batch_limit = 1 with repo_a: make_branch(repo_a, 'master', 'initial', {'a': 'a_0'}) - pr_a = make_pr( + prx_a = make_pr( repo_a, 'do-a-thing', [{'a': 'a_1'}], user=config['role_user']['token'], reviewer=config['role_reviewer']['token'], ) with repo_b: make_branch(repo_b, 'master', 'initial', {'a': 'b_0'}) - pr_b = make_pr(repo_b, 'do-a-thing', [{'a': 'b_1'}], + prx_b = make_pr(repo_b, 'do-a-thing', [{'a': 'b_1'}], user=config['role_user']['token'], reviewer=config['role_reviewer']['token'], ) + pr_a = to_pr(env, prx_a) + pr_b = to_pr(env, prx_b) + + # check that related PRs link to one another + assert get_related_pr_labels(pr_page(page, prx_a)) == pr_b.mapped('display_name') + assert get_related_pr_labels(pr_page(page, prx_b)) == pr_a.mapped('display_name') + env.run_crons() - pr_a = to_pr(env, pr_a) - pr_b = to_pr(env, pr_b) assert pr_a.state == 'ready' assert pr_a.staging_id assert pr_b.state == 'ready' @@ -144,6 +149,22 @@ def test_stage_match(env, project, repo_a, repo_b, config): assert pr_a.staging_id == pr_b.staging_id, \ "branch-matched PRs should be part of the same staging" + # check that related PRs *still* link to one another during staging + assert get_related_pr_labels(pr_page(page, prx_a)) == [pr_b.display_name] + assert get_related_pr_labels(pr_page(page, prx_b)) == [pr_a.display_name] + with repo_a: + repo_a.post_status('staging.master', 'failure', 'legal/cla') + env.run_crons() + + assert pr_a.state == 'error' + assert pr_b.state == 'ready' + + with repo_a: + prx_a.post_comment('hansen retry', config['role_reviewer']['token']) + env.run_crons() + + assert pr_a.state == pr_b.state == 'ready' + assert pr_a.staging_id and pr_b.staging_id for repo in [repo_a, repo_b]: with repo: repo.post_status('staging.master', 'success', 'legal/cla') @@ -155,6 +176,11 @@ def test_stage_match(env, project, repo_a, repo_b, config): assert 'Related: {}'.format(pr_b.display_name) in repo_a.commit('master').message assert 'Related: {}'.format(pr_a.display_name) in repo_b.commit('master').message + print(pr_a.batch_ids.read(['staging_id', 'prs'])) + # check that related PRs *still* link to one another after merge + assert get_related_pr_labels(pr_page(page, prx_a)) == [pr_b.display_name] + assert get_related_pr_labels(pr_page(page, prx_b)) == [pr_a.display_name] + def test_different_targets(env, project, repo_a, repo_b, config): """ PRs with different targets should not be matched together """ diff --git a/runbot_merge/views/templates.xml b/runbot_merge/views/templates.xml index 25befb4a..2322f21f 100644 --- a/runbot_merge/views/templates.xml +++ b/runbot_merge/views/templates.xml @@ -307,6 +307,16 @@ at + + +
+ Linked pull requests + +