mirror of
https://github.com/odoo/runbot.git
synced 2025-03-16 16:05:42 +07:00
cabab210de
Merge errors are logical failures, not technical, it doesn't make sense to log them out because there's nothing to be done technically, a PR having consistency issues or a conflict is "normal". As such those messages are completely useless and just take unnecessary space in the logs, making their use more difficult. Instead of sending them to logging, log staging attempts to the PR itself, and only do normal logging of the operation as an indicative item. And remove a bunch of `expect_log_errors` which don't stand anymore. While at it, fix a missed issue in forward porting: if the `root.head` doesn't exist in the repo its `fetch` will immediately fail before `cat-file` can even run, so the second one is redundant and the first one needs to be handled properly. Do that. And leave checking for *that* specific condition as a logged-out error as it should mean there's something very odd with the repository (how can a pull request have a head we can't fetch?)
496 lines
20 KiB
Python
496 lines
20 KiB
Python
import pytest
|
|
|
|
from utils import seen, Commit, make_basic, to_pr
|
|
|
|
|
|
@pytest.mark.parametrize('source,limit,count', [
|
|
pytest.param('a', 'b', 1, id='not-last'),
|
|
pytest.param('b', 'b', 0, id='current'),
|
|
pytest.param('b', 'a', 0, id='earlier'),
|
|
])
|
|
def test_configure_fp_limit(env, config, make_repo, source, limit, count, page):
|
|
prod, other = make_basic(env, config, make_repo, statuses="default")
|
|
with prod:
|
|
[c] = prod.make_commits(
|
|
source, Commit('c', tree={'f': 'g'}),
|
|
ref='heads/branch',
|
|
)
|
|
pr = prod.make_pr(target=source, head='branch')
|
|
prod.post_status(c, 'success')
|
|
pr.post_comment(f'hansen r+ up to {limit}', config['role_reviewer']['token'])
|
|
env.run_crons()
|
|
with prod:
|
|
prod.post_status(f'staging.{source}', 'success')
|
|
env.run_crons()
|
|
|
|
pr_id = to_pr(env, pr)
|
|
descendants = env['runbot_merge.pull_requests'].search([
|
|
('source_id', '=', pr_id.id)
|
|
])
|
|
assert len(descendants) == count
|
|
limit_id = env['runbot_merge.branch'].search([('name', '=', limit)])
|
|
|
|
assert pr_id.limit_id == limit_id
|
|
assert not descendants.limit_id, "descendant should not inherit the limit explicitly"
|
|
|
|
# check that the basic thingie works
|
|
page(f'/{prod.name}/pull/{pr.number}.png')
|
|
|
|
if descendants:
|
|
c = env['runbot_merge.branch'].search([('name', '=', 'c')])
|
|
descendants.limit_id = c.id
|
|
|
|
page(f'/{prod.name}/pull/{pr.number}.png')
|
|
|
|
def test_ignore(env, config, make_repo, users):
|
|
""" Provide an "ignore" command which is equivalent to setting the limit
|
|
to target
|
|
"""
|
|
prod, _ = make_basic(env, config, make_repo, statuses="default")
|
|
branch_a = env['runbot_merge.branch'].search([('name', '=', 'a')])
|
|
with prod:
|
|
[c] = prod.make_commits('a', Commit('c', tree={'0': '0'}), ref='heads/mybranch')
|
|
pr = prod.make_pr(target='a', head='mybranch')
|
|
prod.post_status(c, 'success')
|
|
env.run_crons()
|
|
with prod:
|
|
pr.post_comment('hansen ignore', config['role_reviewer']['token'])
|
|
pr.post_comment('hansen r+ fw=no', config['role_reviewer']['token'])
|
|
env.run_crons()
|
|
pr_id = env['runbot_merge.pull_requests'].search([('number', '=', pr.number)])
|
|
assert pr_id.limit_id == branch_a
|
|
|
|
with prod:
|
|
prod.post_status('staging.a', 'success')
|
|
env.run_crons()
|
|
|
|
assert env['runbot_merge.pull_requests'].search([]) == pr_id,\
|
|
"should not have created a forward port"
|
|
|
|
assert pr.comments == [
|
|
seen(env, pr, users),
|
|
(users['reviewer'], "hansen ignore"),
|
|
(users['reviewer'], "hansen r+ fw=no"),
|
|
(users['user'], "'ignore' is deprecated, use 'fw=no' to disable forward porting."),
|
|
(users['user'], "Forward-port disabled (via limit)."),
|
|
(users['user'], "Disabled forward-porting."),
|
|
]
|
|
|
|
def test_disable(env, config, make_repo, users):
|
|
""" Checks behaviour if the limit target is disabled:
|
|
|
|
* disable target while FP is ongoing -> skip over (and stop there so no FP)
|
|
* forward-port over a disabled branch
|
|
* request a disabled target as limit
|
|
"""
|
|
prod, other = make_basic(env, config, make_repo)
|
|
project = env['runbot_merge.project'].search([])
|
|
with prod:
|
|
[c] = prod.make_commits('a', Commit('c 0', tree={'0': '0'}), ref='heads/branch0')
|
|
pr = prod.make_pr(target='a', head='branch0')
|
|
prod.post_status(c, 'success', 'legal/cla')
|
|
prod.post_status(c, 'success', 'ci/runbot')
|
|
pr.post_comment('hansen r+ up to b', config['role_reviewer']['token'])
|
|
|
|
[c] = prod.make_commits('a', Commit('c 1', tree={'1': '1'}), ref='heads/branch1')
|
|
pr = prod.make_pr(target='a', head='branch1')
|
|
prod.post_status(c, 'success', 'legal/cla')
|
|
prod.post_status(c, 'success', 'ci/runbot')
|
|
pr.post_comment('hansen r+', config['role_reviewer']['token'])
|
|
env.run_crons()
|
|
|
|
with prod:
|
|
prod.post_status('staging.a', 'success', 'legal/cla')
|
|
prod.post_status('staging.a', 'success', 'ci/runbot')
|
|
# disable branch b
|
|
env['runbot_merge.branch'].search([('name', '=', 'b')]).active = False
|
|
env.run_crons()
|
|
|
|
# should have created a single PR (to branch c, for pr 1)
|
|
_0, _1, p = env['runbot_merge.pull_requests'].search([], order='number')
|
|
assert p.parent_id == _1
|
|
assert p.target.name == 'c'
|
|
|
|
with prod:
|
|
[c] = prod.make_commits('a', Commit('c 2', tree={'2': '2'}), ref='heads/branch2')
|
|
pr = prod.make_pr(target='a', head='branch2')
|
|
prod.post_status(c, 'success', 'legal/cla')
|
|
prod.post_status(c, 'success', 'ci/runbot')
|
|
pr.post_comment('hansen r+ up to', config['role_reviewer']['token'])
|
|
pr.post_comment('hansen up to b', config['role_reviewer']['token'])
|
|
pr.post_comment('hansen up to foo', config['role_reviewer']['token'])
|
|
pr.post_comment('hansen up to c', config['role_reviewer']['token'])
|
|
env.run_crons()
|
|
|
|
# use a set because git webhooks delays might lead to mis-ordered
|
|
# responses and we don't care that much
|
|
assert set(pr.comments) == {
|
|
seen(env, pr, users),
|
|
(users['reviewer'], "hansen r+ up to"),
|
|
(users['user'], """\
|
|
@{reviewer} please provide a branch to forward-port to.
|
|
|
|
For your own safety I've ignored *everything in your entire comment*.
|
|
|
|
Currently available commands:
|
|
|
|
|command||
|
|
|-|-|
|
|
|`help`|displays this help|
|
|
|`r(eview)+`|approves the PR, if it's a forwardport also approves all non-detached parents|
|
|
|`r(eview)=<number>`|only approves the specified parents|
|
|
|`fw=no`|does not forward-port this PR|
|
|
|`fw=default`|forward-ports this PR normally|
|
|
|`fw=skipci`|does not wait for a forward-port's statuses to succeed before creating the next one|
|
|
|`up to <branch>`|only ports this PR forward to the specified branch (included)|
|
|
|`merge`|integrate the PR with a simple merge commit, using the PR description as message|
|
|
|`rebase-merge`|rebases the PR on top of the target branch the integrates with a merge commit, using the PR description as message|
|
|
|`rebase-ff`|rebases the PR on top of the target branch, then fast-forwards|
|
|
|`squash`|squashes the PR as a single commit on the target branch, using the PR description as message|
|
|
|`delegate+`|grants approval rights to the PR author|
|
|
|`delegate=<...>`|grants approval rights on this PR to the specified github users|
|
|
|`default`|stages the PR normally|
|
|
|`priority`|tries to stage this PR first, then adds `default` PRs if the staging has room|
|
|
|`alone`|stages this PR only with other PRs of the same priority|
|
|
|`cancel=staging`|automatically cancels the current staging when this PR becomes ready|
|
|
|`check`|fetches or refreshes PR metadata, resets mergebot state|
|
|
|
|
Note: this help text is dynamic and will change with the state of the PR.
|
|
""".format_map(users)),
|
|
(users['reviewer'], "hansen up to b"),
|
|
(users['user'], "@{reviewer} branch 'b' is disabled, it can't be used as a forward port target.".format_map(users)),
|
|
(users['reviewer'], "hansen up to foo"),
|
|
(users['user'], "@{reviewer} there is no branch 'foo', it can't be used as a forward port target.".format_map(users)),
|
|
(users['reviewer'], "hansen up to c"),
|
|
(users['user'], "Forward-porting to 'c'."),
|
|
}
|
|
|
|
|
|
def test_limit_after_merge(env, config, make_repo, users):
|
|
prod, other = make_basic(env, config, make_repo)
|
|
reviewer = config['role_reviewer']['token']
|
|
branch_b = env['runbot_merge.branch'].search([('name', '=', 'b')])
|
|
branch_c = env['runbot_merge.branch'].search([('name', '=', 'c')])
|
|
with prod:
|
|
[c] = prod.make_commits('a', Commit('c', tree={'0': '0'}), ref='heads/abranch')
|
|
pr1 = prod.make_pr(target='a', head='abranch')
|
|
prod.post_status(c, 'success', 'legal/cla')
|
|
prod.post_status(c, 'success', 'ci/runbot')
|
|
pr1.post_comment('hansen r+', reviewer)
|
|
env.run_crons()
|
|
|
|
with prod:
|
|
prod.post_status('staging.a', 'success', 'legal/cla')
|
|
prod.post_status('staging.a', 'success', 'ci/runbot')
|
|
env.run_crons()
|
|
|
|
p1, p2 = env['runbot_merge.pull_requests'].search([], order='number')
|
|
assert p1.limit_id == p2.limit_id == env['runbot_merge.branch'].browse(())
|
|
pr2 = prod.get_pr(p2.number)
|
|
with prod:
|
|
pr1.post_comment('hansen up to b', reviewer)
|
|
pr2.post_comment('hansen up to b', reviewer)
|
|
env.run_crons()
|
|
|
|
assert p1.limit_id == p2.limit_id == branch_b
|
|
assert pr1.comments == [
|
|
(users['reviewer'], "hansen r+"),
|
|
seen(env, pr1, users),
|
|
(users['reviewer'], 'hansen up to b'),
|
|
(users['user'], "Forward-porting to 'b'."),
|
|
(users['user'], f"Forward-porting to 'b' (from {p2.display_name})."),
|
|
]
|
|
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.
|
|
|
|
More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
|
|
"""),
|
|
(users['reviewer'], 'hansen up to b'),
|
|
(users['user'], f"Forward-porting {p1.display_name} to 'b'."),
|
|
]
|
|
|
|
# update pr2 to detach it from pr1
|
|
with other:
|
|
other.make_commits(
|
|
p2.target.name,
|
|
Commit('updated', tree={'1': '1'}),
|
|
ref=pr2.ref,
|
|
make=False
|
|
)
|
|
env.run_crons()
|
|
assert not p2.parent_id
|
|
assert p2.source_id == p1
|
|
|
|
with prod:
|
|
pr2.post_comment('hansen up to c', reviewer)
|
|
env.run_crons()
|
|
|
|
assert pr2.comments[4:] == [
|
|
(users['user'], f"@{users['user']} @{users['reviewer']} this PR was modified / updated and has become a normal PR. It must be merged directly."),
|
|
(users['reviewer'], 'hansen up to c'),
|
|
(users['user'], "Forward-porting to 'c'."),
|
|
]
|
|
with prod:
|
|
prod.post_status(p2.head, 'success', 'legal/cla')
|
|
prod.post_status(p2.head, 'success', 'ci/runbot')
|
|
pr2.post_comment('hansen r+', reviewer)
|
|
env.run_crons()
|
|
with prod:
|
|
prod.post_status('staging.b', 'success', 'legal/cla')
|
|
prod.post_status('staging.b', 'success', 'ci/runbot')
|
|
env.run_crons()
|
|
|
|
_, _, p3 = env['runbot_merge.pull_requests'].search([], order='number')
|
|
assert p3
|
|
pr3 = prod.get_pr(p3.number)
|
|
with prod:
|
|
pr3.post_comment("hansen up to c", reviewer)
|
|
env.run_crons()
|
|
assert pr3.comments == [
|
|
seen(env, pr3, users),
|
|
(users['user'], f"""\
|
|
@{users['user']} @{users['reviewer']} this PR targets c and is the last of the forward-port chain.
|
|
|
|
To merge the full chain, use
|
|
> @hansen r+
|
|
|
|
More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
|
|
"""),
|
|
(users['reviewer'], "hansen up to c"),
|
|
(users['user'], f"Forward-porting {p2.display_name} to 'c'."),
|
|
]
|
|
# 7 of previous check, plus r+
|
|
assert pr2.comments[8:] == [
|
|
(users['user'], f"Forward-porting to 'c' (from {p3.display_name}).")
|
|
]
|
|
|
|
|
|
|
|
@pytest.mark.parametrize("update_from", [
|
|
pytest.param(lambda source: [('id', '=', source)], id='source'),
|
|
pytest.param(lambda source: [('source_id', '=', source), ('target', '=', '2')], id='child'),
|
|
pytest.param(lambda source: [('source_id', '=', source), ('target', '=', '3')], id='root'),
|
|
pytest.param(lambda source: [('source_id', '=', source), ('target', '=', '4')], id='parent'),
|
|
pytest.param(lambda source: [('source_id', '=', source), ('target', '=', '5')], id='current'),
|
|
# pytest.param(id='tip'), # doesn't exist
|
|
])
|
|
@pytest.mark.parametrize("limit", range(1, 6+1))
|
|
def test_post_merge(
|
|
env, post_merge, users, config, branches,
|
|
update_from: callable,
|
|
limit: int,
|
|
):
|
|
PRs = env['runbot_merge.pull_requests']
|
|
project, prod, _ = post_merge
|
|
reviewer = config['role_reviewer']['token']
|
|
|
|
# fetch source PR
|
|
[source] = PRs.search([('source_id', '=', False)])
|
|
|
|
# validate the forward ports for "child", "root", and "parent" so "current"
|
|
# exists and we have one more target
|
|
for branch in map(str, range(2, 4+1)):
|
|
setci(source=source, repo=prod, target=branch)
|
|
env.run_crons()
|
|
# update 3 to make it into a root
|
|
root = PRs.search([('source_id', '=', source.id), ('target.name', '=', '3')])
|
|
root.write({'parent_id': False, 'detach_reason': 'testing'})
|
|
# send detach messages so they're not part of the limit stuff batch
|
|
env.run_crons()
|
|
|
|
# cheat: we know PR numbers are assigned sequentially
|
|
prs = list(map(prod.get_pr, range(1, 6)))
|
|
before = {p.number: len(p.comments) for p in prs}
|
|
|
|
from_id = PRs.search(update_from(source.id))
|
|
from_ = prod.get_pr(from_id.number)
|
|
with prod:
|
|
from_.post_comment(f'hansen up to {limit}', reviewer)
|
|
env.run_crons()
|
|
|
|
# there should always be a comment on the source and root indicating how
|
|
# far we port
|
|
# the PR we post on should have a comment indicating the correction
|
|
current_id = PRs.search([('number', '=', '5')])
|
|
actual_limit = max(limit, 5)
|
|
for p in prs:
|
|
# case for the PR on which we posted the comment
|
|
if p.number == from_.number:
|
|
root_opt = '' if p.number == root.number else f' {root.display_name}'
|
|
trailer = '' if actual_limit == limit else f" (instead of the requested '{limit}' because {current_id.display_name} already exists)"
|
|
assert p.comments[before[p.number] + 1:] == [
|
|
(users['user'], f"Forward-porting{root_opt} to '{actual_limit}'{trailer}.")
|
|
]
|
|
# case for reference PRs source and root (which get their own notifications)
|
|
elif p.number in (source.number, root.number):
|
|
assert p.comments[before[p.number]:] == [
|
|
(users['user'], f"Forward-porting to '{actual_limit}' (from {from_id.display_name}).")
|
|
]
|
|
|
|
@pytest.mark.parametrize('mode', [
|
|
None,
|
|
# last forward port should fail ci, and only be validated after target bump
|
|
'failbump',
|
|
# last forward port should fail ci, then be validated, then target bump
|
|
'failsucceed',
|
|
# last forward port should be merged before bump
|
|
'mergetip',
|
|
# every forward port should be merged before bump
|
|
'mergeall',
|
|
])
|
|
def test_resume_fw(env, post_merge, users, config, branches, mode):
|
|
"""Singleton version of test_post_merge: completes the forward porting
|
|
including validation then tries to increase the limit, which should resume
|
|
forward porting
|
|
"""
|
|
|
|
PRs = env['runbot_merge.pull_requests']
|
|
project, prod, _ = post_merge
|
|
reviewer = config['role_reviewer']['token']
|
|
|
|
# fetch source PR
|
|
[source] = PRs.search([('source_id', '=', False)])
|
|
with prod:
|
|
prod.get_pr(source.number).post_comment('hansen up to 5', reviewer)
|
|
# validate the forward ports for "child", "root", and "parent" so "current"
|
|
# exists and we have one more target
|
|
for branch in map(str, range(2, 5+1)):
|
|
setci(
|
|
source=source, repo=prod, target=branch,
|
|
status='failure' if branch == '5' and mode in ('failbump', 'failsucceed') else 'success'
|
|
)
|
|
env.run_crons()
|
|
# cheat: we know PR numbers are assigned sequentially
|
|
prs = list(map(prod.get_pr, range(1, 6)))
|
|
before = {p.number: len(p.comments) for p in prs}
|
|
|
|
if mode == 'failsucceed':
|
|
setci(source=source, repo=prod, target=5)
|
|
# sees the success, limit is still 5, considers the porting finished
|
|
env.run_crons()
|
|
|
|
if mode and mode.startswith('merge'):
|
|
numbers = range(5 if mode == 'mergetip' else 2, 5 + 1)
|
|
with prod:
|
|
for number in numbers:
|
|
prod.get_pr(number).post_comment('hansen r+', reviewer)
|
|
env.run_crons()
|
|
with prod:
|
|
for target in numbers:
|
|
pr = PRs.search([('target.name', '=', str(target))])
|
|
prod.post_status(f'staging.{target}', 'success')
|
|
env.run_crons()
|
|
for number in numbers:
|
|
assert PRs.search([('number', '=', number)]).state == 'merged'
|
|
|
|
from_ = prod.get_pr(source.number)
|
|
with prod:
|
|
from_.post_comment('hansen up to 6', reviewer)
|
|
env.run_crons()
|
|
|
|
if mode == 'failbump':
|
|
setci(source=source, repo=prod, target=5)
|
|
# setci moved the PR from opened to validated, so *now* it can be
|
|
# forward-ported, but that still needs to actually happen
|
|
env.run_crons()
|
|
|
|
# since PR5 CI succeeded and we've increased the limit there should be a
|
|
# new PR
|
|
assert PRs.search([('source_id', '=', source.id), ('target.name', '=', 6)])
|
|
pr5_id = PRs.search([('source_id', '=', source.id), ('target.name', '=', 5)])
|
|
if mode == 'failbump':
|
|
# because the initial forward porting was never finished as the PR CI
|
|
# failed until *after* we bumped the limit, so it's not *resuming* per se.
|
|
assert prs[0].comments[before[1]+1:] == [
|
|
(users['user'], f"Forward-porting to '6'.")
|
|
]
|
|
else:
|
|
assert prs[0].comments[before[1]+1:] == [
|
|
(users['user'], f"Forward-porting to '6', resuming forward-port stopped at {pr5_id.display_name}.")
|
|
]
|
|
|
|
def setci(*, source, repo, target, status='success'):
|
|
"""Validates (CI success) the descendant of ``source`` targeting ``target``
|
|
in ``repo``.
|
|
"""
|
|
pr = source.search([('source_id', '=', source.id), ('target.name', '=', str(target))])
|
|
assert pr, f"could not find forward port of {source.display_name} to {target}"
|
|
with repo:
|
|
repo.post_status(pr.head, status)
|
|
|
|
|
|
@pytest.fixture(scope='session')
|
|
def branches():
|
|
"""Need enough branches to make space for:
|
|
|
|
- a source
|
|
- an ancestor (before and separated from the root, but not the source)
|
|
- a root (break in the parent chain
|
|
- a parent (between "current" and root)
|
|
- "current"
|
|
- the tip branch
|
|
"""
|
|
return range(1, 6 + 1)
|
|
|
|
@pytest.fixture
|
|
def post_merge(env, config, users, make_repo, branches):
|
|
"""Create a setup for the post-merge limits test which is both simpler and
|
|
more complicated than the standard test setup(s): it doesn't need more
|
|
variety in code, but it needs a lot more "depth" in terms of number of
|
|
branches it supports. Branches are fixture-ed to make it easier to share
|
|
between this fixture and the actual test.
|
|
|
|
All the branches are set to the same commit because that basically
|
|
shouldn't matter.
|
|
"""
|
|
prod = make_repo("post-merge-test")
|
|
with prod:
|
|
[c] = prod.make_commits(None, Commit('base', tree={'f': ''}))
|
|
for i in branches:
|
|
prod.make_ref(f'heads/{i}', c)
|
|
dev = prod.fork()
|
|
|
|
proj = env['runbot_merge.project'].create({
|
|
'name': prod.name,
|
|
'github_token': config['github']['token'],
|
|
'github_prefix': 'hansen',
|
|
'fp_github_token': config['github']['token'],
|
|
'fp_github_name': 'herbert',
|
|
'branch_ids': [
|
|
(0, 0, {'name': str(i), 'sequence': 1000 - (i * 10)})
|
|
for i in branches
|
|
],
|
|
'repo_ids': [
|
|
(0, 0, {
|
|
'name': prod.name,
|
|
'required_statuses': 'default',
|
|
'fp_remote_target': dev.name,
|
|
})
|
|
]
|
|
})
|
|
env['runbot_merge.events_sources'].create({'repository': prod.name})
|
|
|
|
env['res.partner'].search([
|
|
('github_login', '=', config['role_reviewer']['user'])
|
|
]).write({
|
|
'review_rights': [(0, 0, {'repository_id': proj.repo_ids.id, 'review': True})]
|
|
})
|
|
|
|
reviewer = config['role_reviewer']['token']
|
|
# merge the source PR
|
|
source_target = str(branches[0])
|
|
with prod:
|
|
[c] = prod.make_commits(source_target, Commit('my pr', tree={'x': ''}), ref='heads/mypr')
|
|
pr1 = prod.make_pr(target=source_target, head=c, title="a title")
|
|
|
|
prod.post_status(c, 'success')
|
|
pr1.post_comment('hansen r+', reviewer)
|
|
env.run_crons()
|
|
with prod:
|
|
prod.post_status(f'staging.{source_target}', 'success')
|
|
env.run_crons()
|
|
|
|
return proj, prod, dev
|