From 37cf6accb7e7ce6433b5b86b46e5f83ef68ba253 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Tue, 1 Oct 2019 20:51:31 +0200 Subject: [PATCH] [FIX] forwardport: FP PR getting CI'd after initial FP check Test is probably more complex than necessary (thinking about it, the failed staging is probably unnecessary) but that triggers the issue and matches the original scenario. The problem was really with new CI events being received on the last forward-port PR of a sequence: previous PRs would have a child PR so the check would abort, but for the last PR it would go through, fail to find an active batch, then blow up as it tries to create a forwardport.batch without an actual batch. Change this to use the existence of an inactive batch not linked to a staging as a flag that the PR has been processed and forward-ported. Closes #218 --- forwardport/models/project.py | 16 +++++---- forwardport/tests/test_weird.py | 63 +++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 6 deletions(-) diff --git a/forwardport/models/project.py b/forwardport/models/project.py index 8c55dac4..2c84fccf 100644 --- a/forwardport/models/project.py +++ b/forwardport/models/project.py @@ -266,14 +266,18 @@ class PullRequests(models.Model): }) continue - # if it already has a child, bail - if self.search_count([('parent_id', '=', self.id)]): - _logger.info('-> already has a child') + # check if we've already forward-ported this branch: + # it has a batch without a staging + batch = self.env['runbot_merge.batch'].with_context(active_test=False).search([ + ('staging_id', '=', False), + ('prs', 'in', pr.id), + ], limit=1) + # if the batch is inactive, the forward-port has been done *or* + # the PR's own forward port is in error, so bail + if not batch.active: continue - # check if we've already selected this PR's batch for forward - # porting and just haven't come around to it yet - batch = pr.batch_id + # otherwise check if we already have a pending forward port _logger.info("%s %s %s", pr, batch, batch.prs) if self.env['forwardport.batches'].search_count([('batch_id', '=', batch.id)]): _logger.warn('-> already recorded') diff --git a/forwardport/tests/test_weird.py b/forwardport/tests/test_weird.py index 7d851cb8..9feff8bc 100644 --- a/forwardport/tests/test_weird.py +++ b/forwardport/tests/test_weird.py @@ -141,3 +141,66 @@ def test_no_target(env, config, make_repo): env.run_crons() assert len(env['runbot_merge.pull_requests'].search([], order='number')) == 1,\ "should not have created forward port" + +def test_failed_staging(env, config, make_repo): + proj, prod, _ = make_basic(env, config, make_repo, fp_token=True, fp_remote=True) + + reviewer = config['role_reviewer']['token'] + with prod: + prod.make_commits('a', Commit('c', tree={'a': '0'}), ref='heads/abranch') + pr1 = prod.make_pr(target='a', head='abranch') + prod.post_status(pr1.head, 'success', 'legal/cla') + prod.post_status(pr1.head, '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() + + pr1_id, pr2_id = env['runbot_merge.pull_requests'].search([], order='number') + assert pr2_id.parent_id == pr2_id.source_id == pr1_id + with prod: + prod.post_status(pr2_id.head, 'success', 'legal/cla') + prod.post_status(pr2_id.head, 'success', 'ci/runbot') + env.run_crons() + + pr1_id, pr2_id, pr3_id = env['runbot_merge.pull_requests'].search([], order='number') + pr2 = prod.get_pr(pr2_id.number) + pr3 = prod.get_pr(pr3_id.number) + with prod: + prod.post_status(pr3_id.head, 'success', 'legal/cla') + prod.post_status(pr3_id.head, 'success', 'ci/runbot') + pr3.post_comment('%s r+' % proj.fp_github_name, reviewer) + env.run_crons() + + prod.commit('staging.c') + + with prod: + prod.post_status('staging.b', 'success', 'legal/cla') + prod.post_status('staging.b', 'success', 'ci/runbot') + prod.post_status('staging.c', 'failure', 'ci/runbot') + env.run_crons() + + pr3_head = env['runbot_merge.commit'].search([ + ('sha', '=', pr3_id.head), + ]) + assert len(pr3_head) == 1 + + assert not pr3_id.batch_id, "check that the PR indeed has no batch anymore" + assert not pr3_id.batch_ids + + assert len(env['runbot_merge.batch'].search([ + ('prs', 'in', pr3_id.id), + '|', ('active', '=', True), + ('active', '=', False), + ])) == 2, "check that there do exist batches" + + # send a new status to the PR, as if somebody had rebuilt it or something + with prod: + pr3.post_comment('hansen retry', reviewer) + prod.post_status(pr3_id.head, 'success', 'foo/bar') + prod.post_status(pr3_id.head, 'success', 'legal/cla') + assert pr3_head.to_check, "check that the commit was updated as to process" + env.run_crons() + assert not pr3_head.to_check, "check that the commit was processed"