From 401787b7aebde7cbf80e59bc3da07dc272e23c59 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Tue, 15 Oct 2019 08:54:25 +0200 Subject: [PATCH] [FIX] forwardport: co-dependent FPs where one PR is updated In the case where we have a co-dependent forward port (co-dependent PRs got forward-ported, which they should be together) where *one* of the PRs got explicitly updated, the batch would "fall into a hole" being handled as neither "this is part of a forward-port sequence" nor "this is a new merge to forward-port" (the latter being the proper one). Modify & remove guards which checked that either no or all PRs in a batch have parents: should be either all or not all. Fixes #231 --- forwardport/models/forwardport.py | 5 -- forwardport/models/project.py | 11 ++-- forwardport/tests/test_batches.py | 89 +++++++++++++++++++++++++++++++ 3 files changed, 96 insertions(+), 9 deletions(-) create mode 100644 forwardport/tests/test_batches.py diff --git a/forwardport/models/forwardport.py b/forwardport/models/forwardport.py index cb63c5fe..ced89b7c 100644 --- a/forwardport/models/forwardport.py +++ b/forwardport/models/forwardport.py @@ -35,11 +35,6 @@ class BatchQueue(models.Model, Queue): def _process_item(self): batch = self.batch_id - # only some prs of the batch have a parent, that's weird - with_parent = batch.prs.filtered(lambda p: p.parent_id) - if with_parent and with_parent != batch.prs: - _logger.warning("Found a subset of batch %s (%s) with parents: %s, should probably investigate (normally either they're all parented or none are)", batch, batch.prs, with_parent) - newbatch = batch.prs._port_forward() if newbatch: _logger.info( diff --git a/forwardport/models/project.py b/forwardport/models/project.py index d1ecfb2c..7e9c5769 100644 --- a/forwardport/models/project.py +++ b/forwardport/models/project.py @@ -828,11 +828,14 @@ class Stagings(models.Model): def write(self, vals): r = super().write(vals) - if vals.get('active') == False and self.state == 'success': + # we've just deactivated a successful staging (so it got ~merged) + if vals.get('active') is False and self.state == 'success': + # check al batches to see if they should be forward ported for b in self.with_context(active_test=False).batch_ids: - # if batch PRs have parents they're part of an FP sequence and - # thus handled separately - if not b.mapped('prs.parent_id'): + # if all PRs of a batch have parents they're part of an FP + # sequence and thus handled separately, otherwise they're + # considered regular merges + if not all(p.parent_id for p in b.prs): self.env['forwardport.batches'].create({ 'batch_id': b.id, 'source': 'merge', diff --git a/forwardport/tests/test_batches.py b/forwardport/tests/test_batches.py new file mode 100644 index 00000000..ebce4eb8 --- /dev/null +++ b/forwardport/tests/test_batches.py @@ -0,0 +1,89 @@ +from utils import * + + +def test_single_updated(env, config, make_repo): + """ Given co-dependent PRs getting merged, one of them being modified should + lead to a restart of the merge & forward port process. + + See test_update_pr for a simpler (single-PR) version + """ + r1, _ = make_basic(env, config, make_repo, reponame='repo-1') + r2, _ = make_basic(env, config, make_repo, reponame='repo-2') + + with r1: + r1.make_commits('a', Commit('1', tree={'1': '0'}), ref='heads/aref') + pr1 = r1.make_pr(target='a', head='aref') + r1.post_status('aref', 'success', 'legal/cla') + r1.post_status('aref', 'success', 'ci/runbot') + pr1.post_comment('hansen r+', config['role_reviewer']['token']) + with r2: + r2.make_commits('a', Commit('2', tree={'2': '0'}), ref='heads/aref') + pr2 = r2.make_pr(target='a', head='aref') + r2.post_status('aref', 'success', 'legal/cla') + r2.post_status('aref', 'success', 'ci/runbot') + pr2.post_comment('hansen r+', config['role_reviewer']['token']) + env.run_crons() + + with r1, r2: + r1.post_status('staging.a', 'success', 'legal/cla') + r1.post_status('staging.a', 'success', 'ci/runbot') + r2.post_status('staging.a', 'success', 'legal/cla') + r2.post_status('staging.a', 'success', 'ci/runbot') + env.run_crons() + + pr1_id, pr11_id, pr2_id, pr21_id = pr_ids = env['runbot_merge.pull_requests'].search([]).sorted('display_name') + assert pr1_id.number == pr1.number + assert pr2_id.number == pr2.number + assert pr1_id.state == pr2_id.state == 'merged' + + assert pr11_id.parent_id == pr1_id + assert pr11_id.repository.name == pr1_id.repository.name == r1.name + + assert pr21_id.parent_id == pr2_id + assert pr21_id.repository.name == pr2_id.repository.name == r2.name + + assert pr11_id.target.name == pr21_id.target.name == 'b' + + # don't even bother faking CI failure, straight update pr21_id + repo, ref = r2.get_pr(pr21_id.number).branch + with repo: + repo.make_commits( + pr21_id.target.name, + Commit('Whops', tree={'2': '1'}), + ref='heads/' + ref, + make=False + ) + env.run_crons() + + assert not pr21_id.parent_id + + with r1, r2: + r1.post_status(pr11_id.head, 'success', 'legal/cla') + r1.post_status(pr11_id.head, 'success', 'ci/runbot') + r1.get_pr(pr11_id.number).post_comment('hansen r+', config['role_reviewer']['token']) + r2.post_status(pr21_id.head, 'success', 'legal/cla') + r2.post_status(pr21_id.head, 'success', 'ci/runbot') + r2.get_pr(pr21_id.number).post_comment('hansen r+', config['role_reviewer']['token']) + env.run_crons() + + prs_again = env['runbot_merge.pull_requests'].search([]) + assert prs_again == pr_ids,\ + "should not have created FP PRs as we're now in a detached (iso new PR) state " \ + "(%s)" % prs_again.mapped('display_name') + + with r1, r2: + r1.post_status('staging.b', 'success', 'legal/cla') + r1.post_status('staging.b', 'success', 'ci/runbot') + r2.post_status('staging.b', 'success', 'legal/cla') + r2.post_status('staging.b', 'success', 'ci/runbot') + env.run_crons() + + new_prs = env['runbot_merge.pull_requests'].search([]).sorted('display_name') - pr_ids + assert len(new_prs) == 2, "should have created the new FP PRs" + pr12_id, pr22_id = new_prs + + assert pr12_id.source_id == pr1_id + assert pr12_id.parent_id == pr11_id + + assert pr22_id.source_id == pr2_id + assert pr22_id.parent_id == pr21_id \ No newline at end of file