From d249417cebcc6b822b66f90c2bb89de44e35d71c Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Mon, 2 Aug 2021 09:18:30 +0200 Subject: [PATCH] [FIX] forwardport: fix deduplication of authorship in multi-pr conflict a45f7260fa36acfdcca1e03c3f388f4f93e96e07 had intended to use the original authorship information for conflict commit even if there were multiple commits, as long as there was only one author (/ committer) for the entire sequence. Sadly the deduplication was buggy as it took the *authorship date* in account, which basically ensured commits would never be considered as having the same authorship outside of tests (where it was possible for commits to be created at the same second). Related to #505 --- conftest.py | 10 +- forwardport/models/project.py | 10 +- forwardport/tests/test_conflicts.py | 321 ++++++++++++++++++++++++++++ forwardport/tests/test_simple.py | 217 +------------------ mergebot_test_utils/utils.py | 6 +- 5 files changed, 336 insertions(+), 228 deletions(-) create mode 100644 forwardport/tests/test_conflicts.py 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