From 3ccbc89b26708883c1ee13c40b97dd1cfb959622 Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Mon, 28 Mar 2022 14:52:27 -0700 Subject: [PATCH] De-duplicate change dependencies Gerrit will return git parents in a submitted together response, regardless of whether submitWholeTopic is set. Make sure we emulate that in our tests and validate the outcome. Adding these tests has shown that Zuul is unable to enqueue dependency cycles when dependencies are duplicated, so deduplicate them in the model class. This is done as late as possible so that dependencies from as many sources as are available are de-duplicated. Change-Id: Iaf94fc8947975ecbb615dbbc1c20a885835f69cd --- tests/base.py | 10 ++++-- tests/unit/test_circular_dependencies.py | 43 ++++++++++++++++++++++++ tests/unit/test_gerrit.py | 42 +++++++++++++++++++++++ zuul/model.py | 12 +++++-- 4 files changed, 102 insertions(+), 5 deletions(-) diff --git a/tests/base.py b/tests/base.py index 18880010e3..28981b5ddf 100644 --- a/tests/base.py +++ b/tests/base.py @@ -815,8 +815,9 @@ class FakeGerritChange(object): d['isCurrentPatchSet'] = False return json.loads(json.dumps(self.data)) - def queryHTTP(self): - self.queried += 1 + def queryHTTP(self, internal=False): + if not internal: + self.queried += 1 labels = {} for cat in self.categories: labels[cat] = {} @@ -1155,6 +1156,11 @@ class GerritWebServer(object): f'topic:{topic}', http=True) else: results = [] + for dep in change.data.get('dependsOn', []): + dep_change = fake_gerrit.changes.get(int(dep['number'])) + r = dep_change.queryHTTP(internal=True) + if r not in results: + results.append(r) self.send_data(results) self.end_headers() diff --git a/tests/unit/test_circular_dependencies.py b/tests/unit/test_circular_dependencies.py index 288d1ccbd0..be8004d850 100644 --- a/tests/unit/test_circular_dependencies.py +++ b/tests/unit/test_circular_dependencies.py @@ -1539,6 +1539,43 @@ class TestGerritCircularDependencies(ZuulTestCase): def test_submitted_together_git(self): self.fake_gerrit._fake_submit_whole_topic = True + A = self.fake_gerrit.addFakeChange('org/project1', "master", "A") + B = self.fake_gerrit.addFakeChange('org/project1', "master", "B") + C = self.fake_gerrit.addFakeChange('org/project1', "master", "C") + D = self.fake_gerrit.addFakeChange('org/project1', "master", "D") + E = self.fake_gerrit.addFakeChange('org/project1', "master", "E") + F = self.fake_gerrit.addFakeChange('org/project1', "master", "F") + G = self.fake_gerrit.addFakeChange('org/project1', "master", "G") + G.setDependsOn(F, 1) + F.setDependsOn(E, 1) + E.setDependsOn(D, 1) + D.setDependsOn(C, 1) + C.setDependsOn(B, 1) + B.setDependsOn(A, 1) + + self.fake_gerrit.addEvent(C.getPatchsetCreatedEvent(1)) + self.waitUntilSettled() + + self.assertEqual(len(C.patchsets[-1]["approvals"]), 1) + self.assertEqual(C.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(C.patchsets[-1]["approvals"][0]["value"], "1") + self.assertEqual(A.queried, 1) + self.assertEqual(B.queried, 1) + self.assertEqual(C.queried, 1) + self.assertEqual(D.queried, 1) + self.assertEqual(E.queried, 1) + self.assertEqual(F.queried, 1) + self.assertEqual(G.queried, 1) + self.assertHistory([ + dict(name="project1-job", result="SUCCESS", + changes="1,1 2,1 3,1"), + dict(name="project-vars-job", result="SUCCESS", + changes="1,1 2,1 3,1"), + ], ordered=False) + + def test_submitted_together_git_topic(self): + self.fake_gerrit._fake_submit_whole_topic = True + A = self.fake_gerrit.addFakeChange('org/project1', "master", "A", topic='test-topic') B = self.fake_gerrit.addFakeChange('org/project1', "master", "B", @@ -1573,6 +1610,12 @@ class TestGerritCircularDependencies(ZuulTestCase): self.assertEqual(E.queried, 8) self.assertEqual(F.queried, 8) self.assertEqual(G.queried, 8) + self.assertHistory([ + dict(name="project1-job", result="SUCCESS", + changes="7,1 6,1 5,1 4,1 1,1 2,1 3,1"), + dict(name="project-vars-job", result="SUCCESS", + changes="7,1 6,1 5,1 4,1 1,1 2,1 3,1"), + ], ordered=False) @simple_layout('layouts/submitted-together-per-branch.yaml') def test_submitted_together_per_branch(self): diff --git a/tests/unit/test_gerrit.py b/tests/unit/test_gerrit.py index 23a6a0cb57..bcb8f7bc8c 100644 --- a/tests/unit/test_gerrit.py +++ b/tests/unit/test_gerrit.py @@ -297,6 +297,48 @@ class TestGerritWeb(ZuulTestCase): self.waitUntilSettled() self.assertEqual(9, len(self.history)) + def test_submitted_together_git(self): + # This tests that the circular dependency handling for submit + # whole topic doesn't activate for changes which are only in a + # git dependency. + A = self.fake_gerrit.addFakeChange('org/project1', "master", "A") + B = self.fake_gerrit.addFakeChange('org/project1', "master", "B") + C = self.fake_gerrit.addFakeChange('org/project1', "master", "C") + D = self.fake_gerrit.addFakeChange('org/project1', "master", "D") + E = self.fake_gerrit.addFakeChange('org/project1', "master", "E") + F = self.fake_gerrit.addFakeChange('org/project1', "master", "F") + G = self.fake_gerrit.addFakeChange('org/project1', "master", "G") + G.setDependsOn(F, 1) + F.setDependsOn(E, 1) + E.setDependsOn(D, 1) + D.setDependsOn(C, 1) + C.setDependsOn(B, 1) + B.setDependsOn(A, 1) + + self.fake_gerrit.addEvent(C.getPatchsetCreatedEvent(1)) + self.waitUntilSettled() + + self.assertEqual(len(C.patchsets[-1]["approvals"]), 1) + self.assertEqual(C.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(C.patchsets[-1]["approvals"][0]["value"], "1") + self.assertEqual(A.queried, 1) + self.assertEqual(B.queried, 1) + self.assertEqual(C.queried, 1) + self.assertEqual(D.queried, 1) + self.assertEqual(E.queried, 1) + self.assertEqual(F.queried, 1) + self.assertEqual(G.queried, 1) + self.assertHistory([ + dict(name="project-merge", result="SUCCESS", + changes="1,1 2,1 3,1"), + dict(name="project-test1", result="SUCCESS", + changes="1,1 2,1 3,1"), + dict(name="project-test2", result="SUCCESS", + changes="1,1 2,1 3,1"), + dict(name="project1-project2-integration", result="SUCCESS", + changes="1,1 2,1 3,1"), + ], ordered=False) + class TestFileComments(AnsibleZuulTestCase): config_file = 'zuul-gerrit-web.conf' diff --git a/zuul/model.py b/zuul/model.py index 450191df2f..cb2a8ccd70 100644 --- a/zuul/model.py +++ b/zuul/model.py @@ -5523,12 +5523,18 @@ class Change(Branch): def needs_changes(self): commit_needs_changes = self.commit_needs_changes or [] topic_needs_changes = self.topic_needs_changes or [] - return (self.git_needs_changes + self.compat_needs_changes + - commit_needs_changes + topic_needs_changes) + r = OrderedDict() + for x in (self.git_needs_changes + self.compat_needs_changes + + commit_needs_changes + topic_needs_changes): + r[x] = None + return tuple(r.keys()) @property def needed_by_changes(self): - return (self.git_needed_by_changes + self.compat_needed_by_changes) + r = OrderedDict() + for x in (self.git_needed_by_changes + self.compat_needed_by_changes): + r[x] = None + return tuple(r.keys()) def isUpdateOf(self, other): if (self.project == other.project and