From f30367443409d2d34c15a959dba4e8288aa09ecf Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 21 Jun 2024 10:27:01 +0200 Subject: [PATCH] [FIX] *: re-enable notification on status failure If a PR gets approved *then* fails CI, there should be a notification warning the author & reviewer since 48e08b657b23760e6d45f397cf59e7710bc7f89a, it even has a test, which passes (in fact it has *two*, one of which is redundant, so merge `test_ci_failure_after_review` into the later `test_ci_approved`). *However* this is in runbot_merge, turns out in fafa7ef437d75d72e34be91af72e16c52135b7fa some refactoring was done in order to override the notification and customise it for *forward ports* with a failed status... except that override never called its `super()`, so as soon as forwardport is installed the base notification stops working, and that's been that since October 2019 (had been added in March that year, ignoring deployment lag). This can be revealed by adding the corresponding check in the *forwardport* tests, revealing the failure. This was a pain to track down, thankfully it reproduced relatively easily locally. While this could be resolved in the override, might as well fold it into the base method in furtherance of #789: the mergebot is only used by odoo, and only with both modules combined, so splitting them is not useful. And furthermore it things should work fine with the forwardport installed but unused. Fixes #894 --- forwardport/models/project.py | 12 ----- forwardport/tests/test_updates.py | 15 +++++- runbot_merge/models/pull_requests.py | 8 +++ runbot_merge/tests/test_basic.py | 74 +++++++++------------------- 4 files changed, 45 insertions(+), 64 deletions(-) diff --git a/forwardport/models/project.py b/forwardport/models/project.py index 892c2d58..abca0e7d 100644 --- a/forwardport/models/project.py +++ b/forwardport/models/project.py @@ -275,18 +275,6 @@ class PullRequests(models.Model): }) return r - def _notify_ci_failed(self, ci): - # only care about FP PRs which are not staged / merged yet - # NB: probably ignore approved PRs as normal message will handle them? - if not (self.state == 'opened' and self.parent_id): - return - - self.env.ref('runbot_merge.forwardport.ci.failed')._send( - repository=self.repository, - pull_request=self.number, - token_field='fp_github_token', - format_args={'pr': self, 'ci': ci}, - ) def _validate(self, statuses): failed = super()._validate(statuses) diff --git a/forwardport/tests/test_updates.py b/forwardport/tests/test_updates.py index 0ee884d2..d68c6c92 100644 --- a/forwardport/tests/test_updates.py +++ b/forwardport/tests/test_updates.py @@ -33,11 +33,22 @@ def test_update_pr(env, config, make_repo, users, merge_parent) -> None: ref='heads/hugechange' ) pr = prod.make_pr(target='a', head='hugechange') - prod.post_status(p_1, 'success', 'legal/cla') - prod.post_status(p_1, 'success', 'ci/runbot') pr.post_comment('hansen r+', config['role_reviewer']['token']) + prod.post_status(p_1, 'success', 'legal/cla') + prod.post_status(p_1, 'failure', 'ci/runbot') env.run_crons() + + assert pr.comments == [ + (users['reviewer'], 'hansen r+'), + seen(env, pr, users), + (users['user'], "@{user} @{reviewer} 'ci/runbot' failed on this reviewed PR.".format_map(users)), + ] + + with prod: + prod.post_status(p_1, 'success', 'ci/runbot') + env.run_crons() + with prod: prod.post_status('staging.a', 'success', 'legal/cla') prod.post_status('staging.a', 'success', 'ci/runbot') diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index e4676bdb..983ce2f1 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -1168,6 +1168,14 @@ class PullRequests(models.Model): pull_request=self.number, format_args={'pr': self, 'status': ci} ) + elif self.state == 'opened' and self.parent_id: + # only care about FP PRs which are not approved / staged / merged yet + self.env.ref('runbot_merge.forwardport.ci.failed')._send( + repository=self.repository, + pull_request=self.number, + token_field='fp_github_token', + format_args={'pr': self, 'ci': ci}, + ) def _auto_init(self): for field in self._fields.values(): diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index 40b26eda..0bf57dd3 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -1042,38 +1042,6 @@ def test_rebase_failure(env, repo, users, config): 'b': 'b', } -def test_ci_failure_after_review(env, repo, users, config): - """ 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. - """ - with repo: - prx = _simple_init(repo) - prx.post_comment('hansen r+ rebase-ff', config['role_reviewer']['token']) - env.run_crons() - - for ctx, url in [ - ('ci/runbot', 'https://a'), - ('ci/runbot', 'https://a'), - ('legal/cla', 'https://b'), - ('foo/bar', 'https://c'), - ('ci/runbot', 'https://a'), - ('legal/cla', 'https://d'), # url changes so different from the previous - ]: - with repo: - repo.post_status(prx.head, 'failure', ctx, target_url=url) - env.run_crons() - - assert prx.comments == [ - (users['reviewer'], 'hansen r+ rebase-ff'), - seen(env, prx, users), - (users['user'], "Merge method set to rebase and fast-forward."), - (users['user'], "@{user} @{reviewer} 'ci/runbot' failed on this reviewed PR.".format_map(users)), - (users['user'], "@{user} @{reviewer} 'legal/cla' failed on this reviewed PR.".format_map(users)), - (users['user'], "@{user} @{reviewer} 'legal/cla' failed on this reviewed PR.".format_map(users)), - ] - def test_reopen_merged_pr(env, repo, config, users): """ Reopening a *merged* PR should cause us to immediately close it again, and insult whoever did it @@ -3902,28 +3870,34 @@ class TestFeedback: def test_ci_approved(self, repo, env, users, config): """CI failing on an r+'d PR sends feedback""" with repo: - m = repo.make_commit(None, 'initial', None, tree={'m': 'm'}) - repo.make_ref('heads/master', m) + [m] = repo.make_commits(None, Commit('initial', tree={'m': 'm'}), ref="heads/master") - c1 = repo.make_commit(m, 'first', None, tree={'m': 'c1'}) - prx = repo.make_pr(title='title', body='body', target='master', head=c1) - pr = env['runbot_merge.pull_requests'].search([ - ('repository.name', '=', repo.name), - ('number', '=', prx.number) - ]) - - with repo: - prx.post_comment('hansen r+', config['role_reviewer']['token']) - assert pr.state == 'approved' - - with repo: - repo.post_status(prx.head, 'failure', 'ci/runbot') + [c1] = repo.make_commits(m, Commit('first', tree={'m': 'c1'})) + pr = repo.make_pr(title='title', body='body', target='master', head=c1) + pr.post_comment('hansen r+', config['role_reviewer']['token']) env.run_crons() - assert prx.comments == [ + pr_id = to_pr(env, pr) + assert pr_id.state == 'approved' + + for ctx, url in [ + ('ci/runbot', 'https://a'), + ('ci/runbot', 'https://a'), + ('legal/cla', 'https://b'), + ('foo/bar', 'https://c'), + ('ci/runbot', 'https://a'), + ('legal/cla', 'https://d'), # url changes so different from the previous + ]: + with repo: + repo.post_status(pr_id.head, 'failure', ctx, target_url=url) + env.run_crons() + + assert pr.comments == [ (users['reviewer'], 'hansen r+'), - seen(env, prx, users), - (users['user'], "@%(user)s @%(reviewer)s 'ci/runbot' failed on this reviewed PR." % users) + seen(env, pr, users), + (users['user'], "@{user} @{reviewer} 'ci/runbot' failed on this reviewed PR.".format_map(users)), + (users['user'], "@{user} @{reviewer} 'legal/cla' failed on this reviewed PR.".format_map(users)), + (users['user'], "@{user} @{reviewer} 'legal/cla' failed on this reviewed PR.".format_map(users)), ] def test_review_failed(self, repo, env, users, config):