Fix dependency cycle false positive
This corrects a false-positive in the dependency cycle detection,
but only for the new URL-style depends-on headers. It does not
do so for the legacy gerrit headers.
We used a single history list to store all the changes we enqueued
ahead of a given change, but this meant that if there was more
than one path to a change, we would see it in the history on the
second traversal. Instead, when traversing the tree, use copies
of the history list at each stage so that it can be rewound when
going back up the tree. The second path to a change will not
trip the cycle detection, and will proceed on to the point where
it notices the change is already in the queue and return harmlessly.
Also, check whether the exact change is in the history, not just
the number, since numbers are no longer unique with multiple sources.
Also, fix a bug in the test_crd_cycle test which was causing the
test to always pass since the changes were never enqueued due to
missing approval requirements.
Change-Id: I3241f90a1d7469d433cfa176e719322203d4d089
Story: 2001427
Task: 6133
diff --git a/tests/unit/test_gerrit_crd.py b/tests/unit/test_gerrit_crd.py
index 732bc3d..a8924b9 100644
--- a/tests/unit/test_gerrit_crd.py
+++ b/tests/unit/test_gerrit_crd.py
@@ -90,6 +90,40 @@
'project-merge', 'org/project1').changes
self.assertEqual(changes, '2,1 1,1')
+ def test_crd_gate_triangle(self):
+ A = self.fake_gerrit.addFakeChange('org/project1', 'master', 'A')
+ B = self.fake_gerrit.addFakeChange('org/project2', 'master', 'B')
+ C = self.fake_gerrit.addFakeChange('org/project2', 'master', 'C')
+ A.addApproval('Code-Review', 2)
+ B.addApproval('Code-Review', 2)
+ C.addApproval('Code-Review', 2)
+ A.addApproval('Approved', 1)
+ B.addApproval('Approved', 1)
+
+ # C-->B
+ # \ /
+ # v
+ # A
+
+ # C Depends-On: A
+ C.data['commitMessage'] = '%s\n\nDepends-On: %s\n' % (
+ C.subject, A.data['url'])
+ # B Depends-On: A
+ B.data['commitMessage'] = '%s\n\nDepends-On: %s\n' % (
+ B.subject, A.data['url'])
+ # C git-depends on B
+ C.setDependsOn(B, 1)
+ self.fake_gerrit.addEvent(C.addApproval('Approved', 1))
+ self.waitUntilSettled()
+
+ self.assertEqual(A.reported, 2)
+ self.assertEqual(B.reported, 2)
+ self.assertEqual(C.reported, 2)
+ self.assertEqual(A.data['status'], 'MERGED')
+ self.assertEqual(B.data['status'], 'MERGED')
+ self.assertEqual(C.data['status'], 'MERGED')
+ self.assertEqual(self.history[-1].changes, '1,1 2,1 3,1')
+
def test_crd_branch(self):
"Test cross-repo dependencies in multiple branches"
@@ -257,6 +291,7 @@
B = self.fake_gerrit.addFakeChange('org/project2', 'master', 'B')
A.addApproval('Code-Review', 2)
B.addApproval('Code-Review', 2)
+ B.addApproval('Approved', 1)
# A -> B -> A (via commit-depends)
@@ -522,6 +557,30 @@
for job in self.history:
self.assertEqual(len(job.changes.split()), 1)
+ def test_crd_check_triangle(self):
+ A = self.fake_gerrit.addFakeChange('org/project1', 'master', 'A')
+ B = self.fake_gerrit.addFakeChange('org/project2', 'master', 'B')
+ C = self.fake_gerrit.addFakeChange('org/project2', 'master', 'C')
+
+ # C-->B
+ # \ /
+ # v
+ # A
+
+ # C Depends-On: A
+ C.data['commitMessage'] = '%s\n\nDepends-On: %s\n' % (
+ C.subject, A.data['url'])
+ # B Depends-On: A
+ B.data['commitMessage'] = '%s\n\nDepends-On: %s\n' % (
+ B.subject, A.data['url'])
+ # C git-depends on B
+ C.setDependsOn(B, 1)
+ self.fake_gerrit.addEvent(C.getPatchsetCreatedEvent(1))
+ self.waitUntilSettled()
+
+ self.assertEqual(C.reported, 1)
+ self.assertEqual(self.history[0].changes, '1,1 2,1 3,1')
+
@simple_layout('layouts/three-projects.yaml')
def test_crd_check_transitive(self):
"Test transitive cross-repo dependencies"
diff --git a/tests/unit/test_gerrit_legacy_crd.py b/tests/unit/test_gerrit_legacy_crd.py
index c711e4d..90c93ec 100644
--- a/tests/unit/test_gerrit_legacy_crd.py
+++ b/tests/unit/test_gerrit_legacy_crd.py
@@ -260,6 +260,7 @@
B = self.fake_gerrit.addFakeChange('org/project2', 'master', 'B')
A.addApproval('Code-Review', 2)
B.addApproval('Code-Review', 2)
+ B.addApproval('Approved', 1)
# A -> B -> A (via commit-depends)