diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 9964e6d0..c1a95425 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -692,18 +692,27 @@ class PullRequests(models.Model): # targets for pr in self: required = pr.repository.project_id.required_statuses.split(',') - if all(state_(statuses, r) == 'success' for r in required): + + for ci in required: + st = state_(statuses, ci) or 'pending' + if st == 'success': + continue + + # only report an issue of the PR is already approved (r+'d) + if st in ('error', 'failure') and pr.state == 'approved': + self.env['runbot_merge.pull_requests.feedback'].create({ + 'repository': pr.repository.id, + 'pull_request': pr.number, + 'message': "%r failed on this reviewed PR." % ci, + }) + break + else: # all success (no break) oldstate = pr.state if oldstate == 'opened': pr.state = 'validated' elif oldstate == 'approved': pr.state = 'ready' - # _logger.info("CI+ (%s) for PR %s:%s: %s -> %s", - # statuses, pr.repository.name, pr.number, oldstate, pr.state) - # else: - # _logger.info("CI- (%s) for PR %s:%s", statuses, pr.repository.name, pr.number) - def _auto_init(self): res = super(PullRequests, self)._auto_init() tools.create_unique_index( diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index 3b6c1bba..051d8ed0 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -10,7 +10,7 @@ from lxml import html import odoo -from test_utils import re_matches, run_crons, get_partner +from test_utils import re_matches, run_crons, get_partner, _simple_init @pytest.fixture def repo(make_repo): @@ -759,15 +759,29 @@ def test_rebase_failure(env, repo, users, remote_p): 'b': b'b', } +def test_ci_failure_after_review(env, repo, users): + """ If a PR is r+'d but the CI ends up failing afterwards, ping the user + so they're aware. This is useful for the more "fire and forget" approach + especially small / simple PRs where you assume they're going to pass and + just r+ immediately. + """ + prx = _simple_init(repo) + prx.post_comment('hansen r+', "reviewer") + run_crons(env) + + repo.post_status(prx.head, 'failure', 'ci/runbot') + repo.post_status(prx.head, 'success', 'legal/cla') + run_crons(env) + + assert prx.comments == [ + (users['reviewer'], 'hansen r+'), + (users['user'], "'ci/runbot' failed on this reviewed PR.".format_map(users)), + ] + class TestRetry: @pytest.mark.xfail(reason="This may not be a good idea as it could lead to tons of rebuild spam") def test_auto_retry_push(self, env, repo): - 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', 'body', target='master', ctid=c2, user='user') + prx = _simple_init(repo) repo.post_status(prx.head, 'success', 'ci/runbot') repo.post_status(prx.head, 'success', 'legal/cla') prx.post_comment('hansen r+', "reviewer") @@ -787,7 +801,7 @@ class TestRetry: ]) assert pr.state == 'error' - prx.push(repo.make_commit(c2, 'third', None, tree={'m': 'c3'})) + prx.push(repo.make_commit(prx.head, 'third', None, tree={'m': 'c3'})) assert pr.state == 'approved' env['runbot_merge.project']._check_progress() assert pr.state == 'approved' @@ -808,12 +822,7 @@ class TestRetry: """ An accepted but failed PR should be re-tried when the author or a reviewer asks for it """ - 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', 'body', target='master', ctid=c2, user='user') + prx = _simple_init(repo) repo.post_status(prx.head, 'success', 'ci/runbot') repo.post_status(prx.head, 'success', 'legal/cla') prx.post_comment('hansen r+ delegate=%s rebase-merge' % users['other'], "reviewer") @@ -852,12 +861,7 @@ class TestRetry: def test_retry_ignored(self, env, repo, users): """ Check feedback in case of ignored retry command on a non-error PR. """ - 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', 'body', target='master', ctid=c2, user='user') + prx = _simple_init(repo) prx.post_comment('hansen r+', 'reviewer') prx.post_comment('hansen retry', 'reviewer') @@ -870,12 +874,7 @@ class TestRetry: @pytest.mark.parametrize('disabler', ['user', 'other', 'reviewer']) def test_retry_disable(self, env, repo, disabler, users): - 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', 'body', target='master', ctid=c2, user='user') + prx = _simple_init(repo) repo.post_status(prx.head, 'success', 'ci/runbot') repo.post_status(prx.head, 'success', 'legal/cla') prx.post_comment('hansen r+ delegate=%s rebase-merge' % users['other'], "reviewer") @@ -897,7 +896,7 @@ class TestRetry: prx.post_comment('hansen r-', user=disabler) assert pr.state == 'validated' - prx.push(repo.make_commit(c2, 'third', None, tree={'m': 'c3'})) + prx.push(repo.make_commit(prx.head, 'third', None, tree={'m': 'c3'})) repo.post_status(prx.head, 'success', 'ci/runbot') repo.post_status(prx.head, 'success', 'legal/cla') run_crons(env) diff --git a/runbot_merge/tests/test_utils.py b/runbot_merge/tests/test_utils.py index 30405d95..432c48f9 100644 --- a/runbot_merge/tests/test_utils.py +++ b/runbot_merge/tests/test_utils.py @@ -21,3 +21,14 @@ def run_crons(env): 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', 'body', target='master', ctid=c2, user='user') + return prx