From f6dc176f0e186b1bf49644dd1b1ff1e5fe4326a4 Mon Sep 17 00:00:00 2001 From: David Shrewsbury Date: Mon, 2 Oct 2017 13:34:37 -0400 Subject: [PATCH] Fix branch matching logic Based on Jim's feedback, change the branch matching logic to always have priority over ref matching. And v3 will always have refs, so no need to check if that attribute exists. Also adds a test that checks the current breakage of branch matching logic. Change-Id: Iba148b73a77b3300ad84db1c05c083d2c82cd950 --- tests/base.py | 7 +++ tests/fixtures/layouts/matcher-test.yaml | 63 ++++++++++++++++++++++++ tests/unit/test_change_matcher.py | 7 +-- tests/unit/test_scheduler.py | 27 ++++++++++ zuul/change_matcher.py | 12 +++-- 5 files changed, 105 insertions(+), 11 deletions(-) create mode 100644 tests/fixtures/layouts/matcher-test.yaml diff --git a/tests/base.py b/tests/base.py index 2e3d682141..c240a1bd72 100755 --- a/tests/base.py +++ b/tests/base.py @@ -2620,6 +2620,13 @@ class ZuulTestCase(BaseTestCase): return build raise Exception("Unable to find build %s" % name) + def assertJobNotInHistory(self, name, project=None): + for job in self.history: + if (project is None or + job.parameters['zuul']['project']['name'] == project): + self.assertNotEqual(job.name, name, + 'Job %s found in history' % name) + def getJobFromHistory(self, name, project=None): for job in self.history: if (job.name == name and diff --git a/tests/fixtures/layouts/matcher-test.yaml b/tests/fixtures/layouts/matcher-test.yaml new file mode 100644 index 0000000000..b511a2f769 --- /dev/null +++ b/tests/fixtures/layouts/matcher-test.yaml @@ -0,0 +1,63 @@ +- pipeline: + name: check + manager: independent + trigger: + gerrit: + - event: patchset-created + success: + gerrit: + Verified: 1 + failure: + gerrit: + Verified: -1 + +- pipeline: + name: gate + manager: dependent + success-message: Build succeeded (gate). + trigger: + gerrit: + - event: comment-added + approval: + - Approved: 1 + success: + gerrit: + Verified: 2 + submit: true + failure: + gerrit: + Verified: -2 + start: + gerrit: + Verified: 0 + precedence: high + +- job: + name: base + parent: null + +- job: + name: project-test1 + nodeset: + nodes: + - name: controller + label: label1 + +- job: + name: ignore-branch + branches: ^(?!featureA).*$ + nodeset: + nodes: + - name: controller + label: label2 + +- project: + name: org/project + check: + jobs: + - project-test1 + - ignore-branch + gate: + jobs: + - project-test1 + - ignore-branch diff --git a/tests/unit/test_change_matcher.py b/tests/unit/test_change_matcher.py index 6b161a1e57..3d5345f5a9 100644 --- a/tests/unit/test_change_matcher.py +++ b/tests/unit/test_change_matcher.py @@ -60,7 +60,7 @@ class TestBranchMatcher(BaseTestMatcher): self.assertTrue(self.matcher.matches(self.change)) def test_matches_returns_true_on_matching_ref(self): - self.change.branch = 'bar' + delattr(self.change, 'branch') self.change.ref = 'foo' self.assertTrue(self.matcher.matches(self.change)) @@ -69,11 +69,6 @@ class TestBranchMatcher(BaseTestMatcher): self.change.ref = 'baz' self.assertFalse(self.matcher.matches(self.change)) - def test_matches_returns_false_for_missing_attrs(self): - delattr(self.change, 'branch') - # ref is by default not an attribute - self.assertFalse(self.matcher.matches(self.change)) - class TestFileMatcher(BaseTestMatcher): diff --git a/tests/unit/test_scheduler.py b/tests/unit/test_scheduler.py index 9e5e36cafd..c15e62cf54 100755 --- a/tests/unit/test_scheduler.py +++ b/tests/unit/test_scheduler.py @@ -5642,3 +5642,30 @@ class TestSemaphoreInRepo(ZuulTestCase): self.assertEqual(A.reported, 2) self.assertEqual(B.reported, 2) self.assertEqual(C.reported, 2) + + +class TestSchedulerBranchMatcher(ZuulTestCase): + + @simple_layout('layouts/matcher-test.yaml') + def test_job_branch_ignored(self): + ''' + Test that branch matching logic works. + + The 'ignore-branch' job has a branch matcher that is supposed to + match every branch except for the 'featureA' branch, so it should + not be run on a change to that branch. + ''' + self.create_branch('org/project', 'featureA') + A = self.fake_gerrit.addFakeChange('org/project', 'featureA', 'A') + A.addApproval('Code-Review', 2) + self.fake_gerrit.addEvent(A.addApproval('Approved', 1)) + self.waitUntilSettled() + self.printHistory() + self.assertEqual(self.getJobFromHistory('project-test1').result, + 'SUCCESS') + self.assertJobNotInHistory('ignore-branch') + self.assertEqual(A.data['status'], 'MERGED') + self.assertEqual(A.reported, 2, + "A should report start and success") + self.assertIn('gate', A.messages[1], + "A should transit gate") diff --git a/zuul/change_matcher.py b/zuul/change_matcher.py index baea21753b..7f6673d9ee 100644 --- a/zuul/change_matcher.py +++ b/zuul/change_matcher.py @@ -60,11 +60,13 @@ class ProjectMatcher(AbstractChangeMatcher): class BranchMatcher(AbstractChangeMatcher): def matches(self, change): - return ( - (hasattr(change, 'branch') and self.regex.match(change.branch)) or - (hasattr(change, 'ref') and - change.ref is not None and self.regex.match(change.ref)) - ) + if hasattr(change, 'branch'): + if self.regex.match(change.branch): + return True + return False + if self.regex.match(change.ref): + return True + return False class FileMatcher(AbstractChangeMatcher):