diff --git a/conftest.py b/conftest.py index 83d88889..678deec3 100644 --- a/conftest.py +++ b/conftest.py @@ -130,16 +130,18 @@ def rolemap(request, config): r = _rate_limited(lambda: requests.get('https://api.github.com/user', headers={'Authorization': 'token %s' % data['token']})) r.raise_for_status() - rolemap[role] = data['user'] = r.json()['login'] + user = rolemap[role] = r.json() + data['user'] = user['login'] return rolemap @pytest.fixture def partners(env, config, rolemap): m = dict.fromkeys(rolemap.keys(), env['res.partner']) - for role, login in rolemap.items(): + for role, u in rolemap.items(): if role in ('user', 'other'): continue + login = u['login'] m[role] = env['res.partner'].create({ 'name': config['role_' + role].get('name', login), 'github_login': login, @@ -165,7 +167,7 @@ def setreviewers(partners): @pytest.fixture def users(partners, rolemap): - return rolemap + return {k: v['login'] for k, v in rolemap.items()} @pytest.fixture(scope='session') def tunnel(pytestconfig, port): @@ -419,7 +421,7 @@ def _rate_limited(req): q = req() if not q.ok and q.headers.get('X-RateLimit-Remaining') == '0': reset = int(q.headers['X-RateLimit-Reset']) - delay = round(reset - time.time() + 1.0) + delay = max(0, round(reset - time.time() + 1.0)) print("Hit rate limit, sleeping for", delay, "seconds") time.sleep(delay) continue diff --git a/forwardport/models/project.py b/forwardport/models/project.py index 2204cf16..3fd0ee42 100644 --- a/forwardport/models/project.py +++ b/forwardport/models/project.py @@ -860,8 +860,10 @@ This PR targets %s and is part of the forward-port chain. Further PRs will be cr root_branch = 'origin/pull/%d' % root.number working_copy.checkout('-bsquashed', root_branch) root_commits = root.commits() + # commits returns oldest first, so youngest (head) last + head_commit = root_commits[-1]['commit'] - to_tuple = operator.itemgetter('name', 'email', 'date') + to_tuple = operator.itemgetter('name', 'email') to_dict = lambda term, vals: { 'GIT_%s_NAME' % term: vals[0], 'GIT_%s_EMAIL' % term: vals[1], @@ -872,8 +874,10 @@ This PR targets %s and is part of the forward-port chain. Further PRs will be cr authors.add(to_tuple(c['author'])) committers.add(to_tuple(c['committer'])) fp_authorship = (project_id.fp_github_name, project_id.fp_github_email, '') - author = authors.pop() if len(authors) == 1 else fp_authorship - committer = committers.pop() if len(committers) == 1 else fp_authorship + author = fp_authorship if len(authors) != 1\ + else authors.pop() + (head_commit['author']['date'],) + committer = fp_authorship if len(committers) != 1 \ + else committers.pop() + (head_commit['committer']['date'],) conf = working_copy.with_config(env={ **to_dict('AUTHOR', author), **to_dict('COMMITTER', committer), diff --git a/forwardport/tests/test_conflicts.py b/forwardport/tests/test_conflicts.py new file mode 100644 index 00000000..3b13a210 --- /dev/null +++ b/forwardport/tests/test_conflicts.py @@ -0,0 +1,321 @@ +import re +import time +from operator import itemgetter + +from utils import make_basic, Commit, validate_all, re_matches, seen, REF_PATTERN, to_pr + + +def test_conflict(env, config, make_repo, users): + """ Create a PR to A which will (eventually) conflict with C when + forward-ported. + """ + prod, other = make_basic(env, config, make_repo) + # create a d branch + with prod: + prod.make_commits('c', Commit('1111', tree={'i': 'a'}), ref='heads/d') + project = env['runbot_merge.project'].search([]) + project.write({ + 'branch_ids': [ + (0, 0, {'name': 'd', 'fp_sequence': 4, 'fp_target': True}) + ] + }) + + # generate a conflict: create a h file in a PR to a + with prod: + [p_0] = prod.make_commits( + 'a', Commit('p_0', tree={'h': 'xxx'}), + ref='heads/conflicting' + ) + pr = prod.make_pr(target='a', head='conflicting') + prod.post_status(p_0, 'success', 'legal/cla') + prod.post_status(p_0, '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') + env.run_crons() + pra_id, prb_id = env['runbot_merge.pull_requests'].search([], order='number') + # mark pr b as OK so it gets ported to c + with prod: + validate_all([prod], [prb_id.head]) + env.run_crons() + + pra_id, prb_id, prc_id = env['runbot_merge.pull_requests'].search([], order='number') + # should have created a new PR + # but it should not have a parent, and there should be conflict markers + assert not prc_id.parent_id + assert prc_id.source_id == pra_id + assert prc_id.state == 'opened' + + p = prod.commit(p_0) + c = prod.commit(prc_id.head) + assert c.author == p.author + # ignore date as we're specifically not keeping the original's + without_date = itemgetter('name', 'email') + assert without_date(c.committer) == without_date(p.committer) + assert prod.read_tree(c) == { + 'f': 'c', + 'g': 'a', + 'h': re_matches(r'''<<<\x3c<<< HEAD +a +======= +xxx +>>>\x3e>>> [0-9a-f]{7,}.* +'''), + } + prb = prod.get_pr(prb_id.number) + assert prb.comments == [ + seen(env, prb, users), + (users['user'], '''\ +This PR targets b and is part of the forward-port chain. Further PRs will be created up to d. + +More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port +'''), + (users['user'], """Ping @%s, @%s +The next pull request (%s) is in conflict. You can merge the chain up to here by saying +> @%s r+ + +More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port +""" % ( + users['user'], users['reviewer'], + prc_id.display_name, + project.fp_github_name + )) + ] + + # check that CI passing does not create more PRs + with prod: + validate_all([prod], [prc_id.head]) + env.run_crons() + time.sleep(5) + env.run_crons() + assert pra_id | prb_id | prc_id == env['runbot_merge.pull_requests'].search([], order='number'),\ + "CI passing should not have resumed the FP process on a conflicting / draft PR" + + # fix the PR, should behave as if this were a normal PR + prc = prod.get_pr(prc_id.number) + pr_repo, pr_ref = prc.branch + with pr_repo: + pr_repo.make_commits( + # if just given a branch name, goes and gets it from pr_repo whose + # "b" was cloned before that branch got rolled back + 'c', + Commit('h should indeed be xxx', tree={'h': 'xxx'}), + ref='heads/%s' % pr_ref, + make=False, + ) + env.run_crons() + assert prod.read_tree(prod.commit(prc_id.head)) == { + 'f': 'c', + 'g': 'a', + 'h': 'xxx', + } + assert prc_id.state == 'opened', "state should be open still" + assert ('#%d' % pra_id.number) in prc_id.message + + # check that merging the fixed PR fixes the flow and restarts a forward + # port process + with prod: + prod.post_status(prc.head, 'success', 'legal/cla') + prod.post_status(prc.head, 'success', 'ci/runbot') + prc.post_comment('hansen r+', config['role_reviewer']['token']) + env.run_crons() + + assert prc_id.staging_id + with prod: + prod.post_status('staging.c', 'success', 'legal/cla') + prod.post_status('staging.c', 'success', 'ci/runbot') + env.run_crons() + + *_, prd_id = env['runbot_merge.pull_requests'].search([], order='number') + assert ('#%d' % pra_id.number) in prd_id.message, \ + "check that source / PR A is referenced by resume PR" + assert ('#%d' % prc_id.number) in prd_id.message, \ + "check that parent / PR C is referenced by resume PR" + assert prd_id.parent_id == prc_id + assert prd_id.source_id == pra_id + assert re.match( + REF_PATTERN.format(target='d', source='conflicting'), + prd_id.refname + ) + assert prod.read_tree(prod.commit(prd_id.head)) == { + 'f': 'c', + 'g': 'a', + 'h': 'xxx', + 'i': 'a', + } + +def test_conflict_deleted(env, config, make_repo): + prod, other = make_basic(env, config, make_repo) + # remove f from b + with prod: + prod.make_commits( + 'b', Commit('33', tree={'g': 'c'}, reset=True), + ref='heads/b' + ) + + # generate a conflict: update f in a + with prod: + [p_0] = prod.make_commits( + 'a', Commit('p_0', tree={'f': 'xxx'}), + ref='heads/conflicting' + ) + pr = prod.make_pr(target='a', head='conflicting') + prod.post_status(p_0, 'success', 'legal/cla') + prod.post_status(p_0, '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') + + env.run_crons() + # wait a bit for PR webhook... ? + time.sleep(5) + env.run_crons() + + # should have created a new PR + pr0, pr1 = env['runbot_merge.pull_requests'].search([], order='number') + # but it should not have a parent + assert not pr1.parent_id + assert pr1.source_id == pr0 + assert prod.read_tree(prod.commit('b')) == { + 'g': 'c', + } + assert pr1.state == 'opened' + # NOTE: no actual conflict markers because pr1 essentially adds f de-novo + assert prod.read_tree(prod.commit(pr1.head)) == { + 'f': 'xxx', + 'g': 'c', + } + + # check that CI passing does not create more PRs + with prod: + validate_all([prod], [pr1.head]) + env.run_crons() + time.sleep(5) + env.run_crons() + assert pr0 | pr1 == env['runbot_merge.pull_requests'].search([], order='number'),\ + "CI passing should not have resumed the FP process on a conflicting / draft PR" + + # fix the PR, should behave as if this were a normal PR + get_pr = prod.get_pr(pr1.number) + pr_repo, pr_ref = get_pr.branch + with pr_repo: + pr_repo.make_commits( + # if just given a branch name, goes and gets it from pr_repo whose + # "b" was cloned before that branch got rolled back + prod.commit('b').id, + Commit('f should indeed be removed', tree={'g': 'c'}, reset=True), + ref='heads/%s' % pr_ref, + make=False, + ) + env.run_crons() + assert prod.read_tree(prod.commit(pr1.head)) == { + 'g': 'c', + } + assert pr1.state == 'opened', "state should be open still" + +def test_multiple_commits_same_authorship(env, config, make_repo): + """ When a PR has multiple commits by the same author and its + forward-porting triggers a conflict, the resulting (squashed) conflict + commit should have the original author (same with the committer). + """ + author = {'name': 'George Pearce', 'email': 'gp@example.org'} + committer = {'name': 'G. P. W. Meredith', 'email': 'gpwm@example.org'} + prod, _ = make_basic(env, config, make_repo) + with prod: + # conflict: create `g` in `a`, using two commits + prod.make_commits( + 'a', + Commit('c0', tree={'g': '1'}, + author={**author, 'date': '1932-10-18T12:00:00Z'}, + committer={**committer, 'date': '1932-11-02T12:00:00Z'}), + Commit('c1', tree={'g': '2'}, + author={**author, 'date': '1932-11-12T12:00:00Z'}, + committer={**committer, 'date': '1932-11-13T12:00:00Z'}), + ref='heads/conflicting' + ) + pr = prod.make_pr(target='a', head='conflicting') + prod.post_status('conflicting', 'success', 'legal/cla') + prod.post_status('conflicting', 'success', 'ci/runbot') + pr.post_comment('hansen r+ rebase-ff', config['role_reviewer']['token']) + env.run_crons() + + pr_id = to_pr(env, pr) + assert pr_id.state == 'ready' + assert pr_id.staging_id + + with prod: + prod.post_status('staging.a', 'success', 'legal/cla') + prod.post_status('staging.a', 'success', 'ci/runbot') + env.run_crons() + + for _ in range(20): + pr_ids = env['runbot_merge.pull_requests'].search([], order='number') + if len(pr_ids) == 2: + _ , pr2_id = pr_ids + break + time.sleep(0.5) + else: + assert 0, "timed out" + + c = prod.commit(pr2_id.head) + get = itemgetter('name', 'email') + assert get(c.author) == get(author) + assert get(c.committer) == get(committer) + + +def test_multiple_commits_different_authorship(env, config, make_repo, users, rolemap): + """ When a PR has multiple commits by the same author and its + forward-porting triggers a conflict, the resulting (squashed) conflict + commit should have the original author (same with the committer). + """ + author = {'name': 'George Pearce', 'email': 'gp@example.org'} + committer = {'name': 'G. P. W. Meredith', 'email': 'gpwm@example.org'} + prod, _ = make_basic(env, config, make_repo) + with prod: + # conflict: create `g` in `a`, using two commits + # just swap author and committer in the commits + prod.make_commits( + 'a', + Commit('c0', tree={'g': '1'}, + author={**author, 'date': '1932-10-18T12:00:00Z'}, + committer={**committer, 'date': '1932-11-02T12:00:00Z'}), + Commit('c1', tree={'g': '2'}, + author={**committer, 'date': '1932-11-12T12:00:00Z'}, + committer={**author, 'date': '1932-11-13T12:00:00Z'}), + ref='heads/conflicting' + ) + pr = prod.make_pr(target='a', head='conflicting') + prod.post_status('conflicting', 'success', 'legal/cla') + prod.post_status('conflicting', 'success', 'ci/runbot') + pr.post_comment('hansen r+ rebase-ff', config['role_reviewer']['token']) + env.run_crons() + + pr_id = to_pr(env, pr) + assert pr_id.state == 'ready' + assert pr_id.staging_id + + with prod: + prod.post_status('staging.a', 'success', 'legal/cla') + prod.post_status('staging.a', 'success', 'ci/runbot') + env.run_crons() + + for _ in range(20): + pr_ids = env['runbot_merge.pull_requests'].search([], order='number') + if len(pr_ids) == 2: + _ , pr2_id = pr_ids + break + time.sleep(0.5) + else: + assert 0, "timed out" + + c = prod.commit(pr2_id.head) + get = itemgetter('name', 'email') + rm = rolemap['user'] + assert get(c.author) == (rm['login'], rm['email']) + assert get(c.committer) == (rm['login'], rm['email']) diff --git a/forwardport/tests/test_simple.py b/forwardport/tests/test_simple.py index a5efb8a3..d10d5fcb 100644 --- a/forwardport/tests/test_simple.py +++ b/forwardport/tests/test_simple.py @@ -3,11 +3,10 @@ import collections import re import time from datetime import datetime, timedelta -from operator import itemgetter import pytest -from utils import seen, re_matches, Commit, make_basic, REF_PATTERN, MESSAGE_TEMPLATE, validate_all +from utils import seen, Commit, make_basic, REF_PATTERN, MESSAGE_TEMPLATE, validate_all FMT = '%Y-%m-%d %H:%M:%S' FAKE_PREV_WEEK = (datetime.now() + timedelta(days=1)).strftime(FMT) @@ -233,220 +232,6 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port with pytest.raises(AssertionError, match="Not Found"): other.get_ref(pr2_ref) -def test_conflict(env, config, make_repo, users): - """ Create a PR to A which will (eventually) conflict with C when - forward-ported. - """ - prod, other = make_basic(env, config, make_repo) - # create a d branch - with prod: - prod.make_commits('c', Commit('1111', tree={'i': 'a'}), ref='heads/d') - project = env['runbot_merge.project'].search([]) - project.write({ - 'branch_ids': [ - (0, 0, {'name': 'd', 'fp_sequence': 4, 'fp_target': True}) - ] - }) - - # generate a conflict: create a h file in a PR to a - with prod: - [p_0] = prod.make_commits( - 'a', Commit('p_0', tree={'h': 'xxx'}), - ref='heads/conflicting' - ) - pr = prod.make_pr(target='a', head='conflicting') - prod.post_status(p_0, 'success', 'legal/cla') - prod.post_status(p_0, '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') - env.run_crons() - pra_id, prb_id = env['runbot_merge.pull_requests'].search([], order='number') - # mark pr b as OK so it gets ported to c - with prod: - validate_all([prod], [prb_id.head]) - env.run_crons() - - pra_id, prb_id, prc_id = env['runbot_merge.pull_requests'].search([], order='number') - # should have created a new PR - # but it should not have a parent, and there should be conflict markers - assert not prc_id.parent_id - assert prc_id.source_id == pra_id - assert prc_id.state == 'opened' - - p = prod.commit(p_0) - c = prod.commit(prc_id.head) - assert c.author == p.author - # ignore date as we're specifically not keeping the original's - without_date = itemgetter('name', 'email') - assert without_date(c.committer) == without_date(p.committer) - assert prod.read_tree(c) == { - 'f': 'c', - 'g': 'a', - 'h': re_matches(r'''<<<\x3c<<< HEAD -a -======= -xxx ->>>\x3e>>> [0-9a-f]{7,}.* -'''), - } - prb = prod.get_pr(prb_id.number) - assert prb.comments == [ - seen(env, prb, users), - (users['user'], '''\ -This PR targets b and is part of the forward-port chain. Further PRs will be created up to d. - -More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port -'''), - (users['user'], """Ping @%s, @%s -The next pull request (%s) is in conflict. You can merge the chain up to here by saying -> @%s r+ - -More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port -""" % ( - users['user'], users['reviewer'], - prc_id.display_name, - project.fp_github_name - )) - ] - - # check that CI passing does not create more PRs - with prod: - validate_all([prod], [prc_id.head]) - env.run_crons() - time.sleep(5) - env.run_crons() - assert pra_id | prb_id | prc_id == env['runbot_merge.pull_requests'].search([], order='number'),\ - "CI passing should not have resumed the FP process on a conflicting / draft PR" - - # fix the PR, should behave as if this were a normal PR - prc = prod.get_pr(prc_id.number) - pr_repo, pr_ref = prc.branch - with pr_repo: - pr_repo.make_commits( - # if just given a branch name, goes and gets it from pr_repo whose - # "b" was cloned before that branch got rolled back - 'c', - Commit('h should indeed be xxx', tree={'h': 'xxx'}), - ref='heads/%s' % pr_ref, - make=False, - ) - env.run_crons() - assert prod.read_tree(prod.commit(prc_id.head)) == { - 'f': 'c', - 'g': 'a', - 'h': 'xxx', - } - assert prc_id.state == 'opened', "state should be open still" - assert ('#%d' % pra_id.number) in prc_id.message - - # check that merging the fixed PR fixes the flow and restarts a forward - # port process - with prod: - prod.post_status(prc.head, 'success', 'legal/cla') - prod.post_status(prc.head, 'success', 'ci/runbot') - prc.post_comment('hansen r+', config['role_reviewer']['token']) - env.run_crons() - - assert prc_id.staging_id - with prod: - prod.post_status('staging.c', 'success', 'legal/cla') - prod.post_status('staging.c', 'success', 'ci/runbot') - env.run_crons() - - *_, prd_id = env['runbot_merge.pull_requests'].search([], order='number') - assert ('#%d' % pra_id.number) in prd_id.message, \ - "check that source / PR A is referenced by resume PR" - assert ('#%d' % prc_id.number) in prd_id.message, \ - "check that parent / PR C is referenced by resume PR" - assert prd_id.parent_id == prc_id - assert prd_id.source_id == pra_id - assert re.match( - REF_PATTERN.format(target='d', source='conflicting'), - prd_id.refname - ) - assert prod.read_tree(prod.commit(prd_id.head)) == { - 'f': 'c', - 'g': 'a', - 'h': 'xxx', - 'i': 'a', - } - -def test_conflict_deleted(env, config, make_repo): - prod, other = make_basic(env, config, make_repo) - # remove f from b - with prod: - prod.make_commits( - 'b', Commit('33', tree={'g': 'c'}, reset=True), - ref='heads/b' - ) - - # generate a conflict: update f in a - with prod: - [p_0] = prod.make_commits( - 'a', Commit('p_0', tree={'f': 'xxx'}), - ref='heads/conflicting' - ) - pr = prod.make_pr(target='a', head='conflicting') - prod.post_status(p_0, 'success', 'legal/cla') - prod.post_status(p_0, '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') - - env.run_crons() - # wait a bit for PR webhook... ? - time.sleep(5) - env.run_crons() - - # should have created a new PR - pr0, pr1 = env['runbot_merge.pull_requests'].search([], order='number') - # but it should not have a parent - assert not pr1.parent_id - assert pr1.source_id == pr0 - assert prod.read_tree(prod.commit('b')) == { - 'g': 'c', - } - assert pr1.state == 'opened' - # NOTE: no actual conflict markers because pr1 essentially adds f de-novo - assert prod.read_tree(prod.commit(pr1.head)) == { - 'f': 'xxx', - 'g': 'c', - } - - # check that CI passing does not create more PRs - with prod: - validate_all([prod], [pr1.head]) - env.run_crons() - time.sleep(5) - env.run_crons() - assert pr0 | pr1 == env['runbot_merge.pull_requests'].search([], order='number'),\ - "CI passing should not have resumed the FP process on a conflicting / draft PR" - - # fix the PR, should behave as if this were a normal PR - get_pr = prod.get_pr(pr1.number) - pr_repo, pr_ref = get_pr.branch - with pr_repo: - pr_repo.make_commits( - # if just given a branch name, goes and gets it from pr_repo whose - # "b" was cloned before that branch got rolled back - prod.commit('b').id, - Commit('f should indeed be removed', tree={'g': 'c'}, reset=True), - ref='heads/%s' % pr_ref, - make=False, - ) - env.run_crons() - assert prod.read_tree(prod.commit(pr1.head)) == { - 'g': 'c', - } - assert pr1.state == 'opened', "state should be open still" - def test_empty(env, config, make_repo, users): """ Cherrypick of an already cherrypicked (or separately implemented) commit -> create draft PR. diff --git a/mergebot_test_utils/utils.py b/mergebot_test_utils/utils.py index 57610839..ba37fbc1 100644 --- a/mergebot_test_utils/utils.py +++ b/mergebot_test_utils/utils.py @@ -52,11 +52,7 @@ class re_matches: return '~' + self._r.pattern + '~' def seen(env, pr, users): - pr_id = env['runbot_merge.pull_requests'].search([ - ('repository.name', '=', pr.repo.name), - ('number', '=', pr.number) - ]) - return users['user'], f'[Pull request status dashboard]({pr_id.url}).' + return users['user'], f'[Pull request status dashboard]({to_pr(env, pr).url}).' def make_basic(env, config, make_repo, *, reponame='proj', project_name='myproject'): """ Creates a basic repo with 3 forking branches