diff --git a/conftest.py b/conftest.py index c9c95044..0c7e5568 100644 --- a/conftest.py +++ b/conftest.py @@ -55,7 +55,6 @@ import time import uuid import xmlrpc.client from contextlib import closing -from datetime import datetime import psutil import pytest @@ -80,6 +79,12 @@ def pytest_addoption(parser): "blow through the former); localtunnel has no rate-limiting but " "the servers are way less reliable") + +# noinspection PyUnusedLocal +def pytest_configure(config): + sys.path.insert(0, os.path.join(os.path.dirname(__file__), 'mergebot_test_utils')) + print(sys.path) + @pytest.fixture(scope='session', autouse=True) def _set_socket_timeout(): """ Avoid unlimited wait on standard sockets during tests, this is mostly diff --git a/forwardport/tests/test_batches.py b/forwardport/tests/test_batches.py index ebce4eb8..a637e7a0 100644 --- a/forwardport/tests/test_batches.py +++ b/forwardport/tests/test_batches.py @@ -1,4 +1,4 @@ -from utils import * +from utils import Commit, make_basic def test_single_updated(env, config, make_repo): @@ -86,4 +86,4 @@ def test_single_updated(env, config, make_repo): assert pr12_id.parent_id == pr11_id assert pr22_id.source_id == pr2_id - assert pr22_id.parent_id == pr21_id \ No newline at end of file + assert pr22_id.parent_id == pr21_id diff --git a/forwardport/tests/test_limit.py b/forwardport/tests/test_limit.py index 32f3e73e..e508c224 100644 --- a/forwardport/tests/test_limit.py +++ b/forwardport/tests/test_limit.py @@ -3,7 +3,7 @@ import collections import pytest -from utils import * +from utils import seen, Commit, make_basic Description = collections.namedtuple('Restriction', 'source limit') def test_configure(env, config, make_repo): @@ -164,6 +164,7 @@ def test_disable(env, config, make_repo, users, enabled): (users['other'], "Branch 'b' is disabled, it can't be used as a forward port target."), (users['other'], "There is no branch 'foo', it can't be used as a forward port target."), (users['other'], "Forward-porting to 'c'."), + seen(env, pr, users), } @@ -196,8 +197,9 @@ def test_default_disabled(env, config, make_repo, users): pr2 = prod.get_pr(p2.number) cs = pr2.comments - assert len(cs) == 1 + assert len(cs) == 2 assert pr2.comments == [ + seen(env, pr2, users), (users['user'], """\ Ping @%s, @%s This PR targets b and is the last of the forward-port chain. @@ -243,10 +245,12 @@ def test_limit_after_merge(env, config, make_repo, users): "check that limit was not updated" assert pr1.comments == [ (users['reviewer'], "hansen r+"), + seen(env, pr1, users), (users['reviewer'], bot_name + ' up to b'), (bot_name, "Sorry, forward-port limit can only be set before the PR is merged."), ] assert pr2.comments == [ + seen(env, pr2, users), (users['user'], """\ This PR targets b and is part of the forward-port chain. Further PRs will be created up to c. diff --git a/forwardport/tests/test_simple.py b/forwardport/tests/test_simple.py index 860e0d16..2919938b 100644 --- a/forwardport/tests/test_simple.py +++ b/forwardport/tests/test_simple.py @@ -1,12 +1,13 @@ # -*- coding: utf-8 -*- import collections +import re import time from datetime import datetime, timedelta from operator import itemgetter import pytest -from utils import * +from utils import seen, re_matches, Commit, make_basic, REF_PATTERN, MESSAGE_TEMPLATE, validate_all FMT = '%Y-%m-%d %H:%M:%S' FAKE_PREV_WEEK = (datetime.now() + timedelta(days=1)).strftime(FMT) @@ -124,6 +125,7 @@ def test_straightforward_flow(env, config, make_repo, users): assert pr.comments == [ (users['reviewer'], 'hansen r+ rebase-ff'), + seen(env, pr, users), (users['user'], 'Merge method set to rebase and fast-forward'), (users['user'], 'This pull request has forward-port PRs awaiting action (not merged or closed): ' + ', '.join((pr1 | pr2).mapped('display_name'))), ] @@ -143,7 +145,9 @@ def test_straightforward_flow(env, config, make_repo, users): 'h': 'a', 'x': '1' } - assert prod.get_pr(pr2.number).comments == [ + pr2_remote = prod.get_pr(pr2.number) + assert pr2_remote.comments == [ + seen(env, pr2_remote, users), (users['user'], """\ Ping @%s, @%s This PR targets c and is the last of the forward-port chain containing: @@ -163,7 +167,7 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port prod.post_status(pr2.head, 'success', 'ci/runbot') prod.post_status(pr2.head, 'success', 'legal/cla') - prod.get_pr(pr2.number).post_comment('%s r+' % project.fp_github_name, config['role_reviewer']['token']) + pr2_remote.post_comment('%s r+' % project.fp_github_name, config['role_reviewer']['token']) env.run_crons() @@ -225,7 +229,7 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port with pytest.raises(AssertionError, match='Not Found'): other.get_ref(pr1_ref) - pr2_ref = prod.get_pr(pr2.number).ref + pr2_ref = pr2_remote.ref with pytest.raises(AssertionError, match="Not Found"): other.get_ref(pr2_ref) @@ -279,7 +283,7 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port assert env['runbot_merge.pull_requests'].search([], order='number') == pr0_id | pr1_id,\ "forward port should not continue on CI failure" pr1_remote = prod.get_pr(pr1_id.number) - assert pr1_remote.comments == [fp_intermediate, ci_warning] + assert pr1_remote.comments == [seen(env, pr1_remote, users), fp_intermediate, ci_warning] # it was a false positive, rebuild... it fails again! with prod: @@ -288,7 +292,7 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port # check that FP did not resume & we have a ping on the PR assert env['runbot_merge.pull_requests'].search([], order='number') == pr0_id | pr1_id,\ "ensure it still hasn't restarted" - assert pr1_remote.comments == [fp_intermediate, ci_warning, ci_warning] + assert pr1_remote.comments == [seen(env, pr1_remote, users), fp_intermediate, ci_warning, ci_warning] # nb: updating the head would detach the PR and not put it in the warning # path anymore @@ -320,6 +324,7 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port assert pr1_id.head == new_c != pr1_head, "the FP PR should be updated" assert not pr1_id.parent_id, "the FP PR should be detached from the original" assert pr1_remote.comments == [ + seen(env, pr1_remote, users), fp_intermediate, ci_warning, ci_warning, (users['user'], "This PR was modified / updated and has become a normal PR. It should be merged the normal way (via @%s)" % pr1_id.repository.project_id.github_prefix), ], "users should be warned that the PR has become non-FP" @@ -373,6 +378,7 @@ a env.run_crons() # send feedback assert pr2.comments == [ + seen(env, pr2, users), (users['user'], """Ping @{}, @{} This PR targets c and is the last of the forward-port chain containing: * {} @@ -432,45 +438,47 @@ def test_update_merged(env, make_repo, config, users): prod.post_status('staging.a', 'success', 'ci/runbot') env.run_crons() - pr0, pr1 = env['runbot_merge.pull_requests'].search([], order='number') + pr0_id, pr1_id = env['runbot_merge.pull_requests'].search([], order='number') with prod: - prod.post_status(pr1.head, 'success', 'legal/cla') - prod.post_status(pr1.head, 'success', 'ci/runbot') + prod.post_status(pr1_id.head, 'success', 'legal/cla') + prod.post_status(pr1_id.head, 'success', 'ci/runbot') env.run_crons() - pr0, pr1, pr2 = env['runbot_merge.pull_requests'].search([], order='number') + pr0_id, pr1_id, pr2_id = env['runbot_merge.pull_requests'].search([], order='number') + pr2 = prod.get_pr(pr2_id.number) with prod: - prod.get_pr(pr2.number).post_comment('hansen r+', config['role_reviewer']['token']) - prod.post_status(pr2.head, 'success', 'legal/cla') - prod.post_status(pr2.head, 'success', 'ci/runbot') + pr2.post_comment('hansen r+', config['role_reviewer']['token']) + prod.post_status(pr2_id.head, 'success', 'legal/cla') + prod.post_status(pr2_id.head, 'success', 'ci/runbot') env.run_crons() - assert pr2.staging_id + assert pr2_id.staging_id with prod: prod.post_status('staging.c', 'success', 'legal/cla') prod.post_status('staging.c', 'success', 'ci/runbot') env.run_crons() - assert pr2.state == 'merged' - assert prod.get_pr(pr2.number).state == 'closed' + assert pr2_id.state == 'merged' + assert pr2.state == 'closed' # now we can try updating pr1 and see what happens - repo, ref = prod.get_pr(pr1.number).branch + repo, ref = prod.get_pr(pr1_id.number).branch with repo: repo.make_commits( - pr1.target.name, + pr1_id.target.name, Commit('2', tree={'0': '0', '1': '1'}), ref='heads/%s' % ref, make=False ) updates = env['forwardport.updates'].search([]) assert updates - assert updates.original_root == pr0 - assert updates.new_root == pr1 + assert updates.original_root == pr0_id + assert updates.new_root == pr1_id env.run_crons() - assert not pr1.parent_id + assert not pr1_id.parent_id assert not env['forwardport.updates'].search([]) - assert prod.get_pr(pr2.number).comments == [ + assert pr2.comments == [ + seen(env, pr2, users), (users['user'], '''This PR targets c and is part of the forward-port chain. Further PRs will be created up to d. More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port @@ -478,7 +486,7 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port (users['reviewer'], 'hansen r+'), (users['user'], """Ancestor PR %s has been updated but this PR is merged and can't be updated to match. -You may want or need to manually update any followup PR.""" % pr1.display_name) +You may want or need to manually update any followup PR.""" % pr1_id.display_name) ] def test_conflict(env, config, make_repo, users): @@ -541,7 +549,9 @@ xxx >>>\x3e>>> [0-9a-f]{7,}.* '''), } - assert prod.get_pr(prb_id.number).comments == [ + prb = prod.get_pr(prb_id.number) + assert prb.comments == [ + seen(env, prb, users), (users['user'], '''\ This PR targets b and is part of the forward-port chain. Further PRs will be created up to d. @@ -777,6 +787,7 @@ def test_empty(env, config, make_repo, users): assert pr1.comments == [ (users['reviewer'], 'hansen r+'), + seen(env, pr1, users), (users['other'], 'This pull request has forward-port PRs awaiting action (not merged or closed): ' + fail_id.display_name), (users['other'], 'This pull request has forward-port PRs awaiting action (not merged or closed): ' + fail_id.display_name), ], "each cron run should trigger a new message on the ancestor" @@ -786,6 +797,7 @@ def test_empty(env, config, make_repo, users): env.run_crons('forwardport.reminder', 'runbot_merge.feedback_cron', context={'forwardport_updated_before': FAKE_PREV_WEEK}) assert pr1.comments == [ (users['reviewer'], 'hansen r+'), + seen(env, pr1, users), (users['other'], 'This pull request has forward-port PRs awaiting action (not merged or closed): ' + fail_id.display_name), (users['other'], 'This pull request has forward-port PRs awaiting action (not merged or closed): ' + fail_id.display_name), ] @@ -1015,17 +1027,15 @@ def test_batched(env, config, make_repo, users): ('repository.name', '=', main2.name), ]) # check that relevant users were pinged - ping = [ - (users['user'], """\ + ping = (users['user'], """\ This PR targets b and is part of the forward-port chain. Further PRs will be created up to c. More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port -"""), - ] +""") pr_remote_1b = main1.get_pr(pr1b.number) pr_remote_2b = main2.get_pr(pr2b.number) - assert pr_remote_1b.comments == ping - assert pr_remote_2b.comments == ping + assert pr_remote_1b.comments == [seen(env, pr_remote_1b, users), ping] + assert pr_remote_2b.comments == [seen(env, pr_remote_2b, users), ping] with main1, main2: validate_all([main1], [pr1b.head]) @@ -1097,21 +1107,23 @@ class TestClosing: # should merge the staging then create the FP PR env.run_crons() - pr0, pr1 = env['runbot_merge.pull_requests'].search([], order='number') + pr0_id, pr1_id = env['runbot_merge.pull_requests'].search([], order='number') # close the FP PR then have CI validate it + pr1 = prod.get_pr(pr1_id.number) with prod: - prod.get_pr(pr1.number).close() - assert pr1.state == 'closed' - assert not pr1.parent_id, "closed PR should should be detached from its parent" + pr1.close() + assert pr1_id.state == 'closed' + assert not pr1_id.parent_id, "closed PR should should be detached from its parent" with prod: - prod.post_status(pr1.head, 'success', 'legal/cla') - prod.post_status(pr1.head, 'success', 'ci/runbot') + prod.post_status(pr1_id.head, 'success', 'legal/cla') + prod.post_status(pr1_id.head, 'success', 'ci/runbot') env.run_crons() env.run_crons('forwardport.reminder', 'runbot_merge.feedback_cron') - assert env['runbot_merge.pull_requests'].search([], order='number') == pr0 | pr1,\ + assert env['runbot_merge.pull_requests'].search([], order='number') == pr0_id | pr1_id,\ "closing the PR should suppress the FP sequence" - assert prod.get_pr(pr1.number).comments == [ + assert pr1.comments == [ + seen(env, pr1, users), (users['user'], """\ This PR targets b and is part of the forward-port chain. Further PRs will be created up to c. diff --git a/forwardport/tests/test_weird.py b/forwardport/tests/test_weird.py index 99509d62..9866e1cc 100644 --- a/forwardport/tests/test_weird.py +++ b/forwardport/tests/test_weird.py @@ -1,10 +1,8 @@ # -*- coding: utf-8 -*- -import sys - import pytest -import re -from utils import * +from utils import seen, Commit + def make_basic(env, config, make_repo, *, fp_token, fp_remote): """ Creates a basic repo with 3 forking branches @@ -401,6 +399,7 @@ class TestNotAllBranches: # different next target assert pr_a.comments == [ (users['reviewer'], 'hansen r+'), + seen(env, pr_a, users), (users['user'], "This pull request can not be forward ported: next " "branch is 'b' but linked pull request %s#%d has a" " next branch 'c'." % (b.name, pr_b.number) @@ -408,6 +407,7 @@ class TestNotAllBranches: ] assert pr_b.comments == [ (users['reviewer'], 'hansen r+'), + seen(env, pr_b, users), (users['user'], "This pull request can not be forward ported: next " "branch is 'c' but linked pull request %s#%d has a" " next branch 'b'." % (a.name, pr_a.number) diff --git a/forwardport/tests/utils.py b/mergebot_test_utils/utils.py similarity index 80% rename from forwardport/tests/utils.py rename to mergebot_test_utils/utils.py index 240edecb..87d53a4c 100644 --- a/forwardport/tests/utils.py +++ b/mergebot_test_utils/utils.py @@ -25,6 +25,20 @@ def validate_all(repos, refs, contexts=('ci/runbot', 'legal/cla')): for repo, branch, context in itertools.product(repos, refs, contexts): repo.post_status(branch, 'success', context) +def get_partner(env, gh_login): + return env['res.partner'].search([('github_login', '=', gh_login)]) + +def _simple_init(repo): + """ Creates a very simple initialisation: a master branch with a commit, + and a PR by 'user' with two commits, targeted to the master branch + """ + m = repo.make_commit(None, 'initial', None, tree={'m': 'm'}) + repo.make_ref('heads/master', m) + c1 = repo.make_commit(m, 'first', None, tree={'m': 'c1'}) + c2 = repo.make_commit(c1, 'second', None, tree={'m': 'c2'}) + prx = repo.make_pr(title='title', body='body', target='master', head=c2) + return prx + class re_matches: def __init__(self, pattern, flags=0): self._r = re.compile(pattern, flags) @@ -35,6 +49,12 @@ class re_matches: def __repr__(self): return '~' + self._r.pattern + '~' +def seen(env, pr, users): + pr_id = env['runbot_merge.pull_requests'].search([ + ('repository.name', '=', pr.repo.name), + ('number', '=', pr.number) + ]) + return users['user'], f'[Pull request status dashboard]({pr_id.url}).' def make_basic(env, config, make_repo, *, reponame='proj', project_name='myproject'): """ Creates a basic repo with 3 forking branches diff --git a/runbot_merge/controllers/dashboard.py b/runbot_merge/controllers/dashboard.py index 65078021..6ba51b4e 100644 --- a/runbot_merge/controllers/dashboard.py +++ b/runbot_merge/controllers/dashboard.py @@ -38,8 +38,15 @@ class MergebotDashboard(Controller): if not pr_id.repository.group_id <= request.env.user.groups_id: raise werkzeug.exceptions.NotFound() + st = {} + if pr_id.statuses: + # normalise `statuses` to map to a dict + st = { + k: {'state': v} if isinstance(v, str) else v + for k, v in ast.literal_eval(pr_id.statuses).items() + } return request.render('runbot_merge.view_pull_request', { 'pr': pr_id, 'merged_head': json.loads(pr_id.commits_map).get(''), - 'statuses': ast.literal_eval(pr_id.statuses) if pr_id.statuses else {} + 'statuses': st }) diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 27c79f7b..5ea973b5 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -16,6 +16,7 @@ import time from itertools import takewhile import requests +import werkzeug from werkzeug.datastructures import Headers from odoo import api, fields, models, tools @@ -607,18 +608,6 @@ For-Commit-Id: %s ) raise TimeoutError("Staged head not updated after %d seconds" % sum(WAIT_FOR_VISIBILITY)) - - # creating the staging doesn't trigger a write on the prs - # and thus the ->staging taggings, so do that by hand - Tagging = self.env['runbot_merge.pull_requests.tagging'] - for pr in st.mapped('batch_ids.prs'): - Tagging.create({ - 'pull_request': pr.number, - 'repository': pr.repository.id, - 'state_from': 'ready', - 'state_to': 'staged', - }) - logger.info("Created staging %s (%s) to %s", st, ', '.join( '%s[%s]' % (batch, batch.prs) for batch in staged @@ -703,6 +692,18 @@ class PullRequests(models.Model): help="PR is not currently stageable for some reason (mostly an issue if status is ready)" ) + url = fields.Char(compute='_compute_url') + github_url = fields.Char(compute='_compute_url') + + @api.depends('repository.name', 'number') + def _compute_url(self): + base = werkzeug.urls.url_parse(self.env['ir.config_parameter'].sudo().get_param('web.base.url', 'http://localhost:8069')) + gh_base = werkzeug.urls.url_parse('https://github.com') + for pr in self: + path = f'/{werkzeug.urls.url_quote(pr.repository.name)}/pull/{pr.number}' + pr.url = str(base.join(path)) + pr.github_url = str(gh_base.join(path)) + @api.depends('repository.name', 'number') def _compute_display_name(self): return super(PullRequests, self)._compute_display_name() @@ -1100,12 +1101,6 @@ class PullRequests(models.Model): pr.state = 'validated' elif oldstate == 'approved': pr.state = 'ready' - self.env['runbot_merge.pull_requests.tagging'].create({ - 'repository': pr.repository.id, - 'pull_request': pr.number, - 'tags_remove': json.dumps(r), - 'tags_add': json.dumps(a), - }) return failed def _notify_ci_new_failure(self, ci, st): @@ -1158,11 +1153,10 @@ class PullRequests(models.Model): pr._validate(json.loads(c.statuses or '{}')) if pr.state not in ('closed', 'merged'): - self.env['runbot_merge.pull_requests.tagging'].create({ - 'pull_request': pr.number, + self.env['runbot_merge.pull_requests.feedback'].create({ 'repository': pr.repository.id, - 'state_from': False, - 'state_to': pr._tagstate, + 'pull_request': pr.number, + 'message': f"[Pull request status dashboard]({pr.url}).", }) return pr @@ -1208,28 +1202,8 @@ class PullRequests(models.Model): if newhead: c = self.env['runbot_merge.commit'].search([('sha', '=', newhead)]) self._validate(json.loads(c.statuses or '{}')) - - for pr in self: - before, after = oldstate[pr], pr._tagstate - if after != before: - self.env['runbot_merge.pull_requests.tagging'].create({ - 'pull_request': pr.number, - 'repository': pr.repository.id, - 'state_from': oldstate[pr], - 'state_to': pr._tagstate, - }) return w - def unlink(self): - for pr in self: - self.env['runbot_merge.pull_requests.tagging'].create({ - 'pull_request': pr.number, - 'repository': pr.repository.id, - 'state_from': pr._tagstate, - 'state_to': False, - }) - return super().unlink() - def _check_linked_prs_statuses(self, commit=False): """ Looks for linked PRs where at least one of the PRs is in a ready state and the others are not, notifies the other PRs. @@ -1446,12 +1420,6 @@ class PullRequests(models.Model): self.env.cr.commit() self.modified(['state']) if self.env.cr.rowcount: - self.env['runbot_merge.pull_requests.tagging'].create({ - 'pull_request': self.number, - 'repository': self.repository.id, - 'state_from': res[1] if not self.staging_id else 'staged', - 'state_to': 'closed', - }) self.unstage( "PR %s closed by %s", self.display_name, diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index f4a15c08..18497d8f 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -2,23 +2,24 @@ import datetime import itertools import json import re -import time from unittest import mock import pytest from lxml import html import odoo +from utils import _simple_init, seen, re_matches, get_partner, Commit -from test_utils import re_matches, get_partner, _simple_init -from utils import Commit +def pr_page(page, pr): + return html.fromstring(page(f'/{pr.repo.name}/pull/{pr.number}')) @pytest.fixture def repo(env, project, make_repo, users, setreviewers): r = make_repo('repo') project.write({'repo_ids': [(0, 0, { 'name': r.name, + 'group_id': False, 'required_statuses': 'legal/cla,ci/runbot' })]}) setreviewers(*project.repo_ids) @@ -33,16 +34,18 @@ def test_trivial_flow(env, repo, page, users, config): # create PR with 2 commits c0 = repo.make_commit(m, 'replace file contents', None, tree={'a': 'some other content'}) c1 = repo.make_commit(c0, 'add file', None, tree={'a': 'some other content', 'b': 'a second file'}) - pr1 = repo.make_pr(title="gibberish", body="blahblah", target='master', head=c1,) + pr = repo.make_pr(title="gibberish", body="blahblah", target='master', head=c1,) - [pr] = env['runbot_merge.pull_requests'].search([ + [pr_id] = env['runbot_merge.pull_requests'].search([ ('repository.name', '=', repo.name), - ('number', '=', pr1.number), + ('number', '=', pr.number), ]) - assert pr.state == 'opened' + assert pr_id.state == 'opened' env.run_crons() - assert pr1.labels == {'seen 🙂', '☐ legal/cla', '☐ ci/runbot'} - # nothing happened + assert pr.comments == [seen(env, pr, users)] + s = pr_page(page, pr).cssselect('.alert-info > ul > li') + assert [it.get('class') for it in s] == ['fail', 'fail', ''],\ + "merge method unset, review missing, no CI" with repo: repo.post_status(c1, 'success', 'legal/cla') @@ -54,19 +57,27 @@ def test_trivial_flow(env, repo, page, users, config): repo.post_status(c1, 'success', 'ci/runbot') env.run_crons() - assert pr.state == 'validated' + assert pr_id.state == 'validated' + + s = pr_page(page, pr).cssselect('.alert-info > ul > li') + assert [it.get('class') for it in s] == ['fail', 'fail', 'ok'],\ + "merge method unset, review missing, CI" + statuses = [ + (l.find('a').text.split(':')[0], l.get('class').strip()) + for l in s[2].cssselect('ul li') + ] + assert statuses == [('legal/cla', 'ok'), ('ci/runbot', 'ok')] - assert pr1.labels == {'seen 🙂', 'CI 🤖', '☑ ci/runbot', '☑ legal/cla'} with repo: - pr1.post_comment('hansen r+ rebase-merge', config['role_reviewer']['token']) - assert pr.state == 'ready' + pr.post_comment('hansen r+ rebase-merge', config['role_reviewer']['token']) + assert pr_id.state == 'ready' # can't check labels here as running the cron will stage it env.run_crons() - assert pr.staging_id - assert pr1.labels == {'seen 🙂', 'CI 🤖', 'r+ 👌', 'merging 👷', '☑ ci/runbot', '☑ legal/cla'} + assert pr_id.staging_id + assert pr_page(page, pr).cssselect('.alert-primary') with repo: # get head of staging branch @@ -77,7 +88,7 @@ def test_trivial_flow(env, repo, page, users, config): repo.post_status(staging_head.id, 'failure', 'ci/lint', target_url='http://ignored.com/whocares') # need to store this because after the crons have run the staging will # have succeeded and been disabled - st = pr.staging_id + st = pr_id.staging_id env.run_crons() assert set(tuple(t) for t in st.statuses) == { @@ -95,8 +106,8 @@ def test_trivial_flow(env, repo, page, users, config): assert re.match('^force rebuild', staging_head.message) assert st.state == 'success' - assert pr.state == 'merged' - assert pr1.labels == {'seen 🙂', 'CI 🤖', 'r+ 👌', 'merged 🎉', '☑ ci/runbot', '☑ legal/cla'} + assert pr_id.state == 'merged' + assert pr_page(page, pr).cssselect('.alert-success') master = repo.commit('heads/master') # with default-rebase, only one parent is "known" @@ -358,7 +369,6 @@ def test_staging_ongoing(env, repo, config): ('number', '=', pr2.number) ]) assert p_2.state == 'ready', "PR2 should not have been staged since there is a pending staging for master" - assert pr2.labels == {'seen 🙂', 'CI 🤖', 'r+ 👌', '☑ ci/runbot', '☑ legal/cla'} staging_head = repo.commit('heads/staging.master') with repo: @@ -413,7 +423,7 @@ def test_staging_concurrent(env, repo, config): ]) assert pr2.staging_id -def test_staging_confict_first(env, repo, users, config): +def test_staging_confict_first(env, repo, users, config, page): """ If the first batch of a staging triggers a conflict, the PR should be marked as in error """ @@ -435,9 +445,10 @@ def test_staging_confict_first(env, repo, users, config): ('number', '=', prx.number) ]) assert pr1.state == 'error' - assert prx.labels == {'seen 🙂', 'error 🙅', '☑ legal/cla', '☑ ci/runbot'} + assert pr_page(page, prx).cssselect('.alert-danger') assert prx.comments == [ (users['reviewer'], 'hansen r+ rebase-merge'), + seen(env, prx, users), (users['user'], 'Merge method set to rebase and merge, using the PR as merge commit message'), (users['user'], re_matches('^Unable to stage PR')), ] @@ -566,6 +577,7 @@ def test_staging_ci_failure_single(env, repo, users, config): assert prx.comments == [ (users['reviewer'], 'hansen r+ rebase-merge'), + seen(env, prx, users), (users['user'], "Merge method set to rebase and merge, using the PR as merge commit message"), (users['user'], 'Staging failed: ci/runbot') ] @@ -710,7 +722,6 @@ class TestPREdition: with repo: prx.base = '2.0' assert not pr.exists() env.run_crons() - assert prx.labels == {'☐ legal/cla', '☐ ci/runbot'} with repo: prx.base = '1.0' assert env['runbot_merge.pull_requests'].search([ @@ -794,7 +805,7 @@ def test_edit_staged(env, repo): """ What should happen when editing the PR/metadata (not pushing) of a staged PR """ -def test_close_staged(env, repo, config): +def test_close_staged(env, repo, config, page): """ When closing a staged PR, cancel the staging """ @@ -822,7 +833,7 @@ def test_close_staged(env, repo, config): assert not pr.staging_id assert not env['runbot_merge.stagings'].search([]) assert pr.state == 'closed' - assert prx.labels == {'seen 🙂', 'closed 💔', '☑ ci/runbot', '☑ legal/cla'} + assert pr_page(page, prx).cssselect('.alert-light') def test_forward_port(env, repo, config): with repo: @@ -910,10 +921,12 @@ def test_rebase_failure(env, repo, users, config): assert pr_a.comments == [ (users['reviewer'], 'hansen r+'), + seen(env, pr_a, users), (users['user'], re_matches(r'^Unable to stage PR')), ] assert pr_b.comments == [ (users['reviewer'], 'hansen r+'), + seen(env, pr_b, users), ] assert repo.read_tree(repo.commit('heads/staging.master')) == { 'm': 'm', @@ -939,6 +952,7 @@ def test_ci_failure_after_review(env, repo, users, config): assert prx.comments == [ (users['reviewer'], 'hansen r+'), + seen(env, prx, users), (users['user'], "'legal/cla' failed on this reviewed PR.".format_map(users)), (users['user'], "'ci/runbot' failed on this reviewed PR.".format_map(users)), ] @@ -983,6 +997,7 @@ def test_reopen_merged_pr(env, repo, config, users): assert pr.state == 'merged' assert prx.comments == [ (users['reviewer'], 'hansen r+'), + seen(env, prx, users), (users['user'], "@%s ya silly goose you can't reopen a PR that's been merged PR." % users['other']) ] @@ -1142,6 +1157,7 @@ class TestRetry: assert prx.comments == [ (users['reviewer'], 'hansen r+'), (users['reviewer'], 'hansen retry'), + seen(env, prx, users), (users['user'], "I'm sorry, @{}. Retry makes no sense when the PR is not in error.".format(users['reviewer'])), ] @@ -1315,6 +1331,7 @@ class TestMergeMethod: assert prx.comments == [ (users['reviewer'], 'hansen r+'), + seen(env, prx, users), (users['user'], """Because this PR has multiple commits, I need to know how to merge it: * `merge` to merge directly, using the PR as merge commit message @@ -1359,6 +1376,7 @@ class TestMergeMethod: assert prx.comments == [ (users['reviewer'], 'hansen rebase-merge'), + seen(env, prx, users), (users['user'], "Merge method set to rebase and merge, using the PR as merge commit message"), (users['reviewer'], 'hansen merge'), (users['user'], "Merge method set to merge directly, using the PR as merge commit message"), @@ -2551,6 +2569,7 @@ class TestReviewing(object): env.run_crons() assert prx.comments == [ (users['other'], 'hansen r+'), + seen(env, prx, users), (users['user'], "I'm sorry, @{}. I'm afraid I can't do that.".format(users['other'])), (users['reviewer'], 'hansen r+'), (users['reviewer'], 'hansen r+'), @@ -2582,6 +2601,7 @@ class TestReviewing(object): env.run_crons() assert prx.comments == [ (users['reviewer'], 'hansen r+'), + seen(env, prx, users), (users['user'], "I'm sorry, @{}. You can't review+.".format(users['reviewer'])), ] @@ -2768,8 +2788,10 @@ class TestUnknownPR: 'ci/runbot': {'state': 'success', 'target_url': 'http://example.org/wheee', 'description': None} } assert prx.comments == [ + seen(env, prx, users), (users['reviewer'], 'hansen r+'), (users['reviewer'], 'hansen r+'), + seen(env, prx, users), (users['user'], "Sorry, I didn't know about this PR and had to " "retrieve its information, you may have to " "re-approve it."), @@ -2892,6 +2914,7 @@ class TestRecognizeCommands: assert pr.comments == [ (users['reviewer'], "hansen do the thing"), (users['reviewer'], "hansen @bobby-b r+ :+1:"), + seen(env, pr, users), ] class TestRMinus: @@ -3102,6 +3125,7 @@ class TestRMinus: assert pr.priority == 1, "priority should have been downgraded" assert prx.comments == [ (users['reviewer'], 'hansen p=0'), + seen(env, prx, users), (users['reviewer'], 'hansen r-'), (users['user'], "PR priority reset to 1, as pull requests with priority 0 ignore review state."), ] @@ -3201,6 +3225,7 @@ class TestFeedback: assert prx.comments == [ (users['reviewer'], 'hansen r+'), + seen(env, prx, users), (users['user'], "'ci/runbot' failed on this reviewed PR.") ] @@ -3229,6 +3254,7 @@ class TestFeedback: env.run_crons() assert prx.comments == [ + seen(env, prx, users), (users['reviewer'], 'hansen r+'), (users['user'], "@%s, you may want to rebuild or fix this PR as it has failed CI." % users['reviewer']) ] @@ -3287,58 +3313,3 @@ class TestEmailFormatting: 'github_login': 'Osmose99', }) assert p1.formatted_email == 'Shultz ' - -class TestLabelling: - def test_desync(self, env, repo, config): - with repo: - m = repo.make_commit(None, 'initial', None, tree={'a': 'some content'}) - repo.make_ref('heads/master', m) - - c = repo.make_commit(m, 'replace file contents', None, tree={'a': 'some other content'}) - pr = repo.make_pr(title='gibberish', body='blahblah', target='master', head=c) - env.run_crons('runbot_merge.feedback_cron') - assert pr.labels == {'seen 🙂', '☐ legal/cla', '☐ ci/runbot'},\ - "required statuses should be labelled as pending" - - with repo: - repo.post_status(c, 'success', 'legal/cla') - repo.post_status(c, 'success', 'ci/runbot') - - env.run_crons() - - assert pr.labels == {'seen 🙂', 'CI 🤖', '☑ legal/cla', '☑ ci/runbot'} - with repo: - # desync state and labels - pr.labels.remove('CI 🤖') - - pr.post_comment('hansen r+', config['role_reviewer']['token']) - env.run_crons() - - assert pr.labels == {'seen 🙂', 'CI 🤖', 'r+ 👌', 'merging 👷', '☑ legal/cla', '☑ ci/runbot'},\ - "labels should be resynchronised" - - def test_other_tags(self, env, repo, config): - with repo: - m = repo.make_commit(None, 'initial', None, tree={'a': 'some content'}) - repo.make_ref('heads/master', m) - - c = repo.make_commit(m, 'replace file contents', None, tree={'a': 'some other content'}) - pr = repo.make_pr(title='gibberish', body='blahblah', target='master', head=c) - - with repo: - # "foreign" labels - pr.labels.update(('L1', 'L2')) - - with repo: - repo.post_status(c, 'success', 'legal/cla') - repo.post_status(c, 'success', 'ci/runbot') - env.run_crons() - - assert pr.labels == {'seen 🙂', 'CI 🤖', '☑ legal/cla', '☑ ci/runbot', 'L1', 'L2'}, "should not lose foreign labels" - - with repo: - pr.post_comment('hansen r+', config['role_reviewer']['token']) - env.run_crons() - - assert pr.labels == {'seen 🙂', 'CI 🤖', 'r+ 👌', 'merging 👷', '☑ legal/cla', '☑ ci/runbot', 'L1', 'L2'},\ - "should not lose foreign labels" diff --git a/runbot_merge/tests/test_disabled_branch.py b/runbot_merge/tests/test_disabled_branch.py index 132ddff4..eaaca7c4 100644 --- a/runbot_merge/tests/test_disabled_branch.py +++ b/runbot_merge/tests/test_disabled_branch.py @@ -1,6 +1,4 @@ -import pytest - -from utils import Commit +from utils import seen, Commit def test_existing_pr_disabled_branch(env, project, make_repo, setreviewers, config, users): """ PRs to disabled branches are ignored, but what if the PR exists *before* @@ -59,6 +57,7 @@ def test_existing_pr_disabled_branch(env, project, make_repo, setreviewers, conf env.run_crons() assert pr.comments == [ (users['reviewer'], "hansen r+"), + seen(env, pr, users), (users['user'], "This PR targets the disabled branch %s:other, it can not be merged." % repo.name), ], "reopening a PR to an inactive branch should send feedback, but not closing it" @@ -133,6 +132,7 @@ def test_new_pr_disabled_branch(env, project, make_repo, setreviewers, users): assert pr_id.state == 'opened' assert pr.comments == [ (users['user'], "This PR targets the disabled branch %s:other, it can not be merged." % repo.name), + seen(env, pr, users), ] def test_retarget_from_disabled(env, make_repo, project, setreviewers): diff --git a/runbot_merge/tests/test_multirepo.py b/runbot_merge/tests/test_multirepo.py index a4f70614..4b62a6bb 100644 --- a/runbot_merge/tests/test_multirepo.py +++ b/runbot_merge/tests/test_multirepo.py @@ -11,7 +11,8 @@ import time import pytest import requests -from test_utils import re_matches, get_partner +from utils import seen, re_matches, get_partner + @pytest.fixture def repo_a(project, make_repo, setreviewers): @@ -395,6 +396,7 @@ def test_merge_fail(env, project, repo_a, repo_b, users, config): assert failed.state == 'error' assert pr1b.comments == [ (users['reviewer'], 'hansen r+'), + seen(env, pr1b, users), (users['user'], re_matches('^Unable to stage PR')), ] other = to_pr(env, pr1a) @@ -493,15 +495,17 @@ class TestCompanionsNotReady: assert p_a.comments == [ (users['reviewer'], 'hansen r+'), + seen(env, p_a, users), (users['user'], "Linked pull request(s) %s#%d not ready. Linked PRs are not staged until all of them are ready." % (repo_b.name, p_b.number)), ] # ensure the message is only sent once per PR env.run_crons('runbot_merge.check_linked_prs_status') assert p_a.comments == [ (users['reviewer'], 'hansen r+'), + seen(env, p_a, users), (users['user'], "Linked pull request(s) %s#%d not ready. Linked PRs are not staged until all of them are ready." % (repo_b.name, p_b.number)), ] - assert p_b.comments == [] + assert p_b.comments == [seen(env, p_b, users)] def test_two_of_three_unready(self, env, project, repo_a, repo_b, repo_c, users, config): """ In a 3-batch, if two of the PRs are not ready both should be @@ -531,15 +535,16 @@ class TestCompanionsNotReady: ) env.run_crons() - assert pr_a.comments == [] + assert pr_a.comments == [seen(env, pr_a, users)] assert pr_b.comments == [ (users['reviewer'], 'hansen r+'), + seen(env, pr_b, users), (users['user'], "Linked pull request(s) %s#%d, %s#%d not ready. Linked PRs are not staged until all of them are ready." % ( repo_a.name, pr_a.number, repo_c.name, pr_c.number )) ] - assert pr_c.comments == [] + assert pr_c.comments == [seen(env, pr_c, users)] def test_one_of_three_unready(self, env, project, repo_a, repo_b, repo_c, users, config): """ In a 3-batch, if one PR is not ready it should be linked on the @@ -569,15 +574,17 @@ class TestCompanionsNotReady: ) env.run_crons() - assert pr_a.comments == [] + assert pr_a.comments == [seen(env, pr_a, users)] assert pr_b.comments == [ (users['reviewer'], 'hansen r+'), + seen(env, pr_b, users), (users['user'], "Linked pull request(s) %s#%d not ready. Linked PRs are not staged until all of them are ready." % ( repo_a.name, pr_a.number )) ] assert pr_c.comments == [ (users['reviewer'], 'hansen r+'), + seen(env, pr_c, users), (users['user'], "Linked pull request(s) %s#%d not ready. Linked PRs are not staged until all of them are ready." % ( repo_a.name, pr_a.number @@ -616,6 +623,7 @@ def test_other_failed(env, project, repo_a, repo_b, users, config): assert pr.state == 'error' assert pr_a.comments == [ (users['reviewer'], 'hansen r+'), + seen(env, pr_a, users), (users['user'], 'Staging failed: ci/runbot on %s (view more at http://example.org/b)' % sth) ] diff --git a/runbot_merge/tests/test_oddities.py b/runbot_merge/tests/test_oddities.py index 39526d9f..d071be84 100644 --- a/runbot_merge/tests/test_oddities.py +++ b/runbot_merge/tests/test_oddities.py @@ -1,8 +1,3 @@ -import json - -from utils import Commit - - def test_partner_merge(env): p_src = env['res.partner'].create({ 'name': 'kfhsf', diff --git a/runbot_merge/tests/test_status_overrides.py b/runbot_merge/tests/test_status_overrides.py index 6fe50a74..506b821d 100644 --- a/runbot_merge/tests/test_status_overrides.py +++ b/runbot_merge/tests/test_status_overrides.py @@ -2,7 +2,7 @@ import json import pytest -from utils import Commit +from utils import seen, Commit def test_no_duplicates(env): @@ -87,6 +87,7 @@ def test_basic(env, project, make_repo, users, setreviewers, config): comments = pr.comments assert comments == [ (users['reviewer'], 'hansen r+'), + seen(env, pr, users), (users['reviewer'], 'hansen override=l/int'), (users['user'], "I'm sorry, @{}. You are not allowed to override this status.".format(users['reviewer'])), (users['other'], "hansen override=l/int"), @@ -137,6 +138,7 @@ def test_no_repository(env, project, make_repo, users, setreviewers, config): comments = pr.comments assert comments == [ (users['reviewer'], 'hansen r+'), + seen(env, pr, users), (users['other'], "hansen override=l/int"), ] assert pr_id.statuses == '{}' diff --git a/runbot_merge/tests/test_utils.py b/runbot_merge/tests/test_utils.py deleted file mode 100644 index 5f52b1bb..00000000 --- a/runbot_merge/tests/test_utils.py +++ /dev/null @@ -1,27 +0,0 @@ -# -*- coding: utf-8 -*- -import re - - -class re_matches: - def __init__(self, pattern, flags=0): - self._r = re.compile(pattern, flags) - - def __eq__(self, text): - return self._r.match(text) - - def __repr__(self): - return '~' + self._r.pattern + '~' - -def get_partner(env, gh_login): - return env['res.partner'].search([('github_login', '=', gh_login)]) - -def _simple_init(repo): - """ Creates a very simple initialisation: a master branch with a commit, - and a PR by 'user' with two commits, targeted to the master branch - """ - m = repo.make_commit(None, 'initial', None, tree={'m': 'm'}) - repo.make_ref('heads/master', m) - c1 = repo.make_commit(m, 'first', None, tree={'m': 'c1'}) - c2 = repo.make_commit(c1, 'second', None, tree={'m': 'c2'}) - prx = repo.make_pr(title='title', body='body', target='master', head=c2) - return prx diff --git a/runbot_merge/views/templates.xml b/runbot_merge/views/templates.xml index 6ea86076..13902623 100644 --- a/runbot_merge/views/templates.xml +++ b/runbot_merge/views/templates.xml @@ -338,7 +338,6 @@
  • CI -