Refresh dependencies for changes in pipelines

Commit 549103881899751e84b411212f7820cde502b42b appears to have
had an error.  It attempted to only refresh changes if they are
in the pipeline or are dependencies of changes in the pipeline,
but it only did the latter, which has an adverse effect for the
GitHub driver.  We can change dependencies of GitHub changes
without creating new patchsets, which means that this error
(not updating dependencies of changes in the pipeline) would cause
Zuul not to eject a GitHub change from a pipeline when its
dependencies changed via a PR message update.

The error went unnoticed with Gerrit since dependency updates
happen with new patchsets, which themselves cause changes to be
ejected.

This change corrects the behavior.

Repetitive calls to updateCommitDependencies are also eliminated
by creating a set of changes to update across the whole pipeline.

Finally, two tests are added, one for Gerrit (which previously would
have passed) and one for GitHub (which previously would have failed).

Change-Id: I005820223471e8f6372ef70730f381918ec9c1af

2 files changed, 99 insertions, 6 deletions

diff --git a/tests/unit/test_circular_dependencies.py b/tests/unit/test_circular_dependencies.py
index 74d2cedf0..f534b2596 100644
--- a/tests/unit/test_circular_dependencies.py
+++ b/tests/unit/test_circular_dependencies.py
@@ -2332,6 +2332,52 @@ class TestGerritCircularDependencies(ZuulTestCase):
             dict(name="project6-job-t2", result="SUCCESS", changes="1,1 2,1"),
         ], ordered=False)
 
+    def test_dependency_refresh(self):
+        # Test that when two changes are put into a cycle, the
+        # dependencies are refreshed and items already in pipelines
+        # are updated.
+        self.executor_server.hold_jobs_in_build = True
+
+        # This simulates the typical workflow where a developer only
+        # knows the change id of changes one at a time.
+        # The first change:
+        A = self.fake_gerrit.addFakeChange("org/project", "master", "A")
+        self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
+        self.waitUntilSettled()
+
+        # Now that it has been uploaded, upload the second change and
+        # point it at the first.
+        # B -> A
+        B = self.fake_gerrit.addFakeChange("org/project", "master", "B")
+        B.data["commitMessage"] = "{}\n\nDepends-On: {}\n".format(
+            B.subject, A.data["url"]
+        )
+        self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
+        self.waitUntilSettled()
+
+        # Now that the second change is known, update the first change
+        # B <-> A
+        A.addPatchset()
+        A.data["commitMessage"] = "{}\n\nDepends-On: {}\n".format(
+            A.subject, B.data["url"]
+        )
+        self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(2))
+        self.waitUntilSettled()
+
+        self.executor_server.hold_jobs_in_build = False
+        self.executor_server.release()
+        self.waitUntilSettled()
+
+        # A quirk: at the end of this process, the first change in
+        # Gerrit has a complete run because the process of updating it
+        # involves a new patchset that is enqueued.  Compare to the
+        # same test in GitHub.
+        self.assertHistory([
+            dict(name="project-job", result="ABORTED", changes="1,1"),
+            dict(name="project-job", result="ABORTED", changes="1,1 2,1"),
+            dict(name="project-job", result="SUCCESS", changes="2,1 1,2"),
+        ], ordered=False)
+
 
 class TestGithubCircularDependencies(ZuulTestCase):
     config_file = "zuul-gerrit-github.conf"
@@ -2524,3 +2570,50 @@ class TestGithubCircularDependencies(ZuulTestCase):
                       B.comments[-1]))
         self.assertFalse(re.search('Change .*? is needed',
                                    B.comments[-1]))
+
+    def test_dependency_refresh(self):
+        # Test that when two changes are put into a cycle, the
+        # dependencies are refreshed and items already in pipelines
+        # are updated.
+        self.executor_server.hold_jobs_in_build = True
+
+        # This simulates the typical workflow where a developer only
+        # knows the PR id of changes one at a time.
+        # The first change:
+        A = self.fake_github.openFakePullRequest("gh/project", "master", "A")
+        self.fake_github.emitEvent(A.getPullRequestOpenedEvent())
+        self.waitUntilSettled()
+
+        # Now that it has been uploaded, upload the second change and
+        # point it at the first.
+        # B -> A
+        B = self.fake_github.openFakePullRequest("gh/project", "master", "B")
+        B.body = "{}\n\nDepends-On: {}\n".format(
+            B.subject, A.url
+        )
+        self.fake_github.emitEvent(B.getPullRequestOpenedEvent())
+        self.waitUntilSettled()
+
+        # Now that the second change is known, update the first change
+        # B <-> A
+        A.body = "{}\n\nDepends-On: {}\n".format(
+            A.subject, B.url
+        )
+
+        self.fake_github.emitEvent(A.getPullRequestEditedEvent(A.subject))
+        self.waitUntilSettled()
+
+        self.executor_server.hold_jobs_in_build = False
+        self.executor_server.release()
+        self.waitUntilSettled()
+
+        # A quirk: at the end of this process, the second PR in GitHub
+        # has a complete run because the process of updating the first
+        # change is not disruptive to the second.  Compare to the same
+        # test in Gerrit.
+        self.assertHistory([
+            dict(name="project-job", result="ABORTED",
+                 changes=f"{A.number},{A.head_sha}"),
+            dict(name="project-job", result="SUCCESS",
+                 changes=f"{A.number},{A.head_sha} {B.number},{B.head_sha}"),
+        ], ordered=False)
diff --git a/zuul/manager/__init__.py b/zuul/manager/__init__.py
index 60eb479e0..832be780a 100644
--- a/zuul/manager/__init__.py
+++ b/zuul/manager/__init__.py
@@ -276,19 +276,19 @@ class PipelineManager(metaclass=ABCMeta):
         if not isinstance(change, model.Change):
             return
 
-        change_in_pipeline = False
+        to_refresh = set()
         for item in self.pipeline.getAllItems():
             if not isinstance(item.change, model.Change):
                 continue
+            if item.change.equals(change):
+                to_refresh.add(item.change)
             for dep_change_ref in item.change.commit_needs_changes:
-                if item.change.equals(change):
-                    change_in_pipeline = True
                 dep_change_key = ChangeKey.fromReference(dep_change_ref)
                 if dep_change_key.isSameChange(change.cache_stat.key):
-                    self.updateCommitDependencies(item.change, None, event)
+                    to_refresh.add(item.change)
 
-        if change_in_pipeline:
-            self.updateCommitDependencies(change, None, event)
+        for existing_change in to_refresh:
+            self.updateCommitDependencies(existing_change, None, event)
 
     def reportEnqueue(self, item):
         if not self.pipeline.state.disabled: