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):