Fix change history cycle detection
We were short-circuiting loading some changes which appeared in
our dependent change query history because we were only looking
at change numbers, not change+patchset. Record both in the history
to avoid this problem.
Note, this could likely be simplified further, but rather than doing
so now, I'll leave that for when we implement cross-source dependencies,
which is very likely to change this further.
This also corrects a defect in the fake gerrit change test fixture.
Change-Id: Ie1d9fe0dfd32032caf4ff8c90bded30d5e7bf886
diff --git a/tests/base.py b/tests/base.py
index fb94638..2568188 100755
--- a/tests/base.py
+++ b/tests/base.py
@@ -410,8 +410,8 @@
needed = other.data.get('neededBy', [])
d = {'id': self.data['id'],
'number': self.data['number'],
- 'ref': self.patchsets[patchset - 1]['ref'],
- 'revision': self.patchsets[patchset - 1]['revision']
+ 'ref': self.patchsets[-1]['ref'],
+ 'revision': self.patchsets[-1]['revision']
}
needed.append(d)
other.data['neededBy'] = needed
diff --git a/tests/unit/test_scheduler.py b/tests/unit/test_scheduler.py
index d9cf839..0229d65 100755
--- a/tests/unit/test_scheduler.py
+++ b/tests/unit/test_scheduler.py
@@ -1178,6 +1178,34 @@
repo.heads.master.commit = repo.commit('init')
self.test_build_configuration()
+ def test_dependent_changes_rebase(self):
+ # Test that no errors occur when we walk a dependency tree
+ # with an unused leaf node due to a rebase.
+ # Start by constructing: C -> B -> A
+ A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A')
+ B = self.fake_gerrit.addFakeChange('org/project', 'master', 'B')
+ B.setDependsOn(A, 1)
+
+ C = self.fake_gerrit.addFakeChange('org/project', 'master', 'C')
+ C.setDependsOn(B, 1)
+
+ # Then rebase to form: D -> C -> A
+ C.addPatchset() # C,2
+ C.setDependsOn(A, 1)
+
+ D = self.fake_gerrit.addFakeChange('org/project', 'master', 'D')
+ D.setDependsOn(C, 2)
+
+ # Walk the entire tree
+ self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
+ self.waitUntilSettled()
+ self.assertEqual(len(self.history), 3)
+
+ # Verify that walking just part of the tree still works
+ self.fake_gerrit.addEvent(D.getPatchsetCreatedEvent(1))
+ self.waitUntilSettled()
+ self.assertEqual(len(self.history), 6)
+
def test_dependent_changes_dequeue(self):
"Test that dependent patches are not needlessly tested"
diff --git a/zuul/driver/gerrit/gerritconnection.py b/zuul/driver/gerrit/gerritconnection.py
index 924a42f..a5eb12e 100644
--- a/zuul/driver/gerrit/gerritconnection.py
+++ b/zuul/driver/gerrit/gerritconnection.py
@@ -371,10 +371,14 @@
def _updateChange(self, change, history=None):
- # In case this change is already in the history we have a cyclic
- # dependency and don't need to update ourselves again as this gets
- # done in a previous frame of the call stack.
- if history and change.number in history:
+ # In case this change is already in the history we have a
+ # cyclic dependency and don't need to update ourselves again
+ # as this gets done in a previous frame of the call stack.
+ # NOTE(jeblair): I don't think it's possible to hit this case
+ # anymore as all paths hit the change cache first.
+ if (history and change.number and change.patchset and
+ (change.number, change.patchset) in history):
+ self.log.debug("Change %s is in history" % (change,))
return change
self.log.info("Updating %s" % (change,))
@@ -420,7 +424,7 @@
history = []
else:
history = history[:]
- history.append(change.number)
+ history.append((change.number, change.patchset))
needs_changes = []
if 'dependsOn' in data:
@@ -465,7 +469,7 @@
# reference the latest patchset of its Depends-On (this
# change). In case the dep is already in history we already
# refreshed this change so refresh is not needed in this case.
- refresh = dep_num not in history
+ refresh = (dep_num, dep_ps) not in history
dep = self._getChange(
dep_num, dep_ps, refresh=refresh, history=history)
if (not dep.is_merged) and dep.is_current_patchset: