mirror of
https://github.com/odoo/runbot.git
synced 2025-03-15 23:45:44 +07:00
[FIX] runbot_message: error on PR page
The page of PRs in "error" is currently kinda broken: it does not show
any feedback aside from the PR being in error which is not very
useful.
The intent was always to show an explanation, but when adding the page
I just deref'd `staging_id` which always fails though in two different
ways:
* when the PR can not be staged at all (because of a conflict) there
is no staging at all with a reason to show, so there should be
a fallback that the PR could not even be staged
* `staging_id` is a related field which deref's to the staging_ids
of the first *active* batch, except when a staging completes
(successfully or not) both staging and batch are disabled.
Plus the first batch will be the one for the first staging so if the
PR is retried and fails again the wrong reason may be displayed.
So update the section to show what we want: the reason of the
staging of the *last* batch attached to the PR.
NOTE: there's one failure mode remaining, namely if a staging fails
then on retry the PR conflicts with the new state of the
repository (so it can't be staged at all), the "reason" will
remain that of the staging. This could be mitigated by attaching
a "nonsense" batch on failure to stage (similar to the
forwardport stuff), that batch would have no staging, therefore
no staging reason, therefore fallback.
Closes #525
This commit is contained in:
xmo-odoo
GitHub
parent
e5f84dc380
commit
874719870d
@ -6,7 +6,7 @@ import textwrap
|
||||
from unittest import mock
|
||||
|
||||
import pytest
|
||||
from lxml import html
|
||||
from lxml import html, etree
|
||||
|
||||
import odoo
|
||||
from utils import _simple_init, seen, re_matches, get_partner, Commit, pr_page, to_pr, part_of
|
||||
@ -416,7 +416,7 @@ def test_staging_concurrent(env, repo, config):
|
||||
])
|
||||
assert pr2.staging_id
|
||||
|
||||
def test_staging_confict_first(env, repo, users, config, page):
|
||||
def test_staging_conflict_first(env, repo, users, config, page):
|
||||
""" If the first batch of a staging triggers a conflict, the PR should be
|
||||
marked as in error
|
||||
"""
|
||||
@ -427,25 +427,25 @@ def test_staging_confict_first(env, repo, users, config, page):
|
||||
|
||||
c1 = repo.make_commit(m1, 'other second', None, tree={'f': 'c1'})
|
||||
c2 = repo.make_commit(c1, 'third', None, tree={'f': 'c2'})
|
||||
prx = repo.make_pr(title='title', body='body', target='master', head=c2)
|
||||
repo.post_status(prx.head, 'success', 'ci/runbot')
|
||||
repo.post_status(prx.head, 'success', 'legal/cla')
|
||||
prx.post_comment('hansen r+ rebase-merge', config['role_reviewer']['token'])
|
||||
pr = repo.make_pr(title='title', body='body', target='master', head=c2)
|
||||
repo.post_status(pr.head, 'success', 'ci/runbot')
|
||||
repo.post_status(pr.head, 'success', 'legal/cla')
|
||||
pr.post_comment('hansen r+ rebase-merge', config['role_reviewer']['token'])
|
||||
env.run_crons()
|
||||
|
||||
pr1 = env['runbot_merge.pull_requests'].search([
|
||||
('repository.name', '=', repo.name),
|
||||
('number', '=', prx.number)
|
||||
])
|
||||
assert pr1.state == 'error'
|
||||
assert pr_page(page, prx).cssselect('.alert-danger')
|
||||
assert prx.comments == [
|
||||
pr_id = to_pr(env, pr)
|
||||
assert pr_id.state == 'error'
|
||||
assert pr.comments == [
|
||||
(users['reviewer'], 'hansen r+ rebase-merge'),
|
||||
seen(env, prx, users),
|
||||
seen(env, pr, 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')),
|
||||
]
|
||||
|
||||
dangerbox = pr_page(page, pr).cssselect('.alert-danger span')
|
||||
assert dangerbox
|
||||
assert dangerbox[0].text.strip() == 'Unable to stage PR'
|
||||
|
||||
def test_staging_conflict_second(env, repo, users, config):
|
||||
""" If the non-first batch of a staging triggers a conflict, the PR should
|
||||
just be skipped: it might be a conflict with an other PR which could fail
|
||||
@ -467,17 +467,10 @@ def test_staging_conflict_second(env, repo, users, config):
|
||||
repo.post_status(pr1.head, 'success', 'ci/runbot')
|
||||
repo.post_status(pr1.head, 'success', 'legal/cla')
|
||||
pr1.post_comment('hansen r+', config['role_reviewer']['token'])
|
||||
|
||||
env.run_crons()
|
||||
PRs = env['runbot_merge.pull_requests']
|
||||
pr0_id = PRs.search([
|
||||
('repository.name', '=', repo.name),
|
||||
('number', '=', pr0.number),
|
||||
])
|
||||
pr1_id = PRs.search([
|
||||
('repository.name', '=', repo.name),
|
||||
('number', '=', pr1.number),
|
||||
])
|
||||
|
||||
pr0_id = to_pr(env, pr0)
|
||||
pr1_id = to_pr(env, pr1)
|
||||
assert pr0_id.staging_id, "pr0 should have been staged"
|
||||
assert not pr1_id.staging_id, "pr1 should not have been staged (due to conflict)"
|
||||
assert pr1_id.state == 'ready', "pr1 should not be in error yet"
|
||||
@ -492,7 +485,7 @@ def test_staging_conflict_second(env, repo, users, config):
|
||||
assert pr1_id.state == 'error', "now pr1 should be in error"
|
||||
|
||||
|
||||
def test_staging_ci_timeout(env, repo, config):
|
||||
def test_staging_ci_timeout(env, repo, config, page):
|
||||
"""If a staging timeouts (~ delay since staged greater than
|
||||
configured)... requeue?
|
||||
"""
|
||||
@ -502,22 +495,23 @@ def test_staging_ci_timeout(env, repo, config):
|
||||
|
||||
c1 = repo.make_commit(m, 'first', None, tree={'f': 'c1'})
|
||||
c2 = repo.make_commit(c1, 'second', None, tree={'f': 'c2'})
|
||||
prx = repo.make_pr(title='title', body='body', target='master', head=c2)
|
||||
repo.post_status(prx.head, 'success', 'ci/runbot')
|
||||
repo.post_status(prx.head, 'success', 'legal/cla')
|
||||
prx.post_comment('hansen r+ rebase-merge', config['role_reviewer']['token'])
|
||||
pr = repo.make_pr(title='title', body='body', target='master', head=c2)
|
||||
repo.post_status(pr.head, 'success', 'ci/runbot')
|
||||
repo.post_status(pr.head, 'success', 'legal/cla')
|
||||
pr.post_comment('hansen r+ rebase-merge', config['role_reviewer']['token'])
|
||||
env.run_crons()
|
||||
|
||||
pr1 = env['runbot_merge.pull_requests'].search([
|
||||
('repository.name', '=', repo.name),
|
||||
('number', '=', prx.number)
|
||||
])
|
||||
assert pr1.staging_id
|
||||
pr_id = to_pr(env, pr)
|
||||
assert pr_id.staging_id
|
||||
timeout = env['runbot_merge.project'].search([]).ci_timeout
|
||||
|
||||
pr1.staging_id.staged_at = odoo.fields.Datetime.to_string(datetime.datetime.now() - datetime.timedelta(minutes=2*timeout))
|
||||
pr_id.staging_id.staged_at = odoo.fields.Datetime.to_string(datetime.datetime.now() - datetime.timedelta(minutes=2*timeout))
|
||||
env.run_crons('runbot_merge.merge_cron', 'runbot_merge.staging_cron')
|
||||
assert pr1.state == 'error', "timeout should fail the PR"
|
||||
assert pr_id.state == 'error', "timeout should fail the PR"
|
||||
|
||||
dangerbox = pr_page(page, pr).cssselect('.alert-danger span')
|
||||
assert dangerbox
|
||||
assert dangerbox[0].text == 'timed out (>60 minutes)'
|
||||
|
||||
def test_timeout_bump_on_pending(env, repo, config):
|
||||
with repo:
|
||||
@ -539,7 +533,7 @@ def test_timeout_bump_on_pending(env, repo, config):
|
||||
env.run_crons('runbot_merge.process_updated_commits')
|
||||
assert st.timeout_limit > old_timeout
|
||||
|
||||
def test_staging_ci_failure_single(env, repo, users, config):
|
||||
def test_staging_ci_failure_single(env, repo, users, config, page):
|
||||
""" on failure of single-PR staging, mark & notify failure
|
||||
"""
|
||||
with repo:
|
||||
@ -548,15 +542,13 @@ def test_staging_ci_failure_single(env, repo, users, config):
|
||||
|
||||
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)
|
||||
repo.post_status(prx.head, 'success', 'ci/runbot')
|
||||
repo.post_status(prx.head, 'success', 'legal/cla')
|
||||
prx.post_comment('hansen r+ rebase-merge', config['role_reviewer']['token'])
|
||||
pr = repo.make_pr(title='title', body='body', target='master', head=c2)
|
||||
repo.post_status(pr.head, 'success', 'ci/runbot')
|
||||
repo.post_status(pr.head, 'success', 'legal/cla')
|
||||
pr.post_comment('hansen r+ rebase-merge', config['role_reviewer']['token'])
|
||||
env.run_crons()
|
||||
assert env['runbot_merge.pull_requests'].search([
|
||||
('repository.name', '=', repo.name),
|
||||
('number', '=', prx.number)
|
||||
]).staging_id
|
||||
pr_id = to_pr(env, pr)
|
||||
assert pr_id.staging_id
|
||||
|
||||
staging_head = repo.commit('heads/staging.master')
|
||||
with repo:
|
||||
@ -564,18 +556,19 @@ def test_staging_ci_failure_single(env, repo, users, config):
|
||||
repo.post_status(staging_head.id, 'success', 'legal/cla')
|
||||
repo.post_status(staging_head.id, 'failure', 'ci/runbot') # stable genius
|
||||
env.run_crons()
|
||||
assert env['runbot_merge.pull_requests'].search([
|
||||
('repository.name', '=', repo.name),
|
||||
('number', '=', prx.number)
|
||||
]).state == 'error'
|
||||
assert pr_id.state == 'error'
|
||||
|
||||
assert prx.comments == [
|
||||
assert pr.comments == [
|
||||
(users['reviewer'], 'hansen r+ rebase-merge'),
|
||||
seen(env, prx, users),
|
||||
seen(env, pr, users),
|
||||
(users['user'], "Merge method set to rebase and merge, using the PR as merge commit message"),
|
||||
(users['user'], 'Staging failed: ci/runbot')
|
||||
]
|
||||
|
||||
dangerbox = pr_page(page, pr).cssselect('.alert-danger span')
|
||||
assert dangerbox
|
||||
assert dangerbox[0].text == 'ci/runbot'
|
||||
|
||||
def test_ff_failure(env, repo, config):
|
||||
""" target updated while the PR is being staged => redo staging """
|
||||
with repo:
|
||||
@ -1144,6 +1137,42 @@ class TestRetry:
|
||||
('number', '=', prx.number)
|
||||
]).state == 'merged'
|
||||
|
||||
def test_retry_again_message(self, env, repo, users, config, page):
|
||||
""" For a retried PR, the error message on the PR's page should be the
|
||||
later staging
|
||||
"""
|
||||
with repo:
|
||||
pr = _simple_init(repo)
|
||||
repo.post_status(pr.head, 'success', 'ci/runbot')
|
||||
repo.post_status(pr.head, 'success', 'legal/cla')
|
||||
pr.post_comment('hansen r+ delegate=%s rebase-merge' % users['other'],
|
||||
config["role_reviewer"]['token'])
|
||||
env.run_crons()
|
||||
pr_id = to_pr(env, pr)
|
||||
assert pr_id.staging_id
|
||||
|
||||
with repo:
|
||||
repo.post_status('staging.master', 'success', 'legal/cla')
|
||||
repo.post_status('staging.master', 'failure', 'ci/runbot',
|
||||
target_url='https://example.com/whocares')
|
||||
env.run_crons()
|
||||
assert pr_id.state == 'error'
|
||||
|
||||
with repo:
|
||||
pr.post_comment('hansen retry', config['role_reviewer']['token'])
|
||||
env.run_crons('runbot_merge.merge_cron', 'runbot_merge.staging_cron')
|
||||
|
||||
with repo:
|
||||
repo.post_status('staging.master', 'success', 'legal/cla')
|
||||
repo.post_status('staging.master', 'failure', 'ci/runbot',
|
||||
target_url='https://example.com/ohno')
|
||||
env.run_crons()
|
||||
assert pr_id.state == 'error'
|
||||
|
||||
dangerbox = pr_page(page, pr).cssselect('.alert-danger span')
|
||||
assert dangerbox
|
||||
assert dangerbox[0].text == 'ci/runbot (view more at https://example.com/ohno)'
|
||||
|
||||
def test_retry_ignored(self, env, repo, users, config):
|
||||
""" Check feedback in case of ignored retry command on a non-error PR.
|
||||
"""
|
||||
|
||||
@ -327,7 +327,9 @@
|
||||
<template id="view_pull_request_info_error">
|
||||
<div class="alert alert-danger">
|
||||
Error:
|
||||
<p t-field="pr.staging_id.reason"/>
|
||||
<span t-esc="pr.with_context(active_test=False).batch_ids[-1:].staging_id.reason">
|
||||
Unable to stage PR
|
||||
</span>
|
||||
</div>
|
||||
</template>
|
||||
<template id="view_pull_request_info_staging">
|
||||
|
||||
Loading…
Reference in New Issue
Block a user