From f82ef0882cbbbab468bf0b5551cdf91d715f0321 Mon Sep 17 00:00:00 2001
From: "James E. Blair" <jim@acmegating.com>
Date: Wed, 4 Jan 2023 15:28:38 -0800
Subject: [PATCH] Further avoid unnecessary change dependency updates

When adding a unit test for change I4fd6c0d4cf2839010ddf7105a7db12da06ef1074
I noticed that we were still querying the dependent change 4 times instead of
the expected 2.  This was due to an indentation error which caused all 3
query retry attempts to execute.

This change corrects that and adds a unit test that covers this as well as
the previous optimization.

Change-Id: I798d8d713b8303abcebc32d5f9ccad84bd4a28b0
---
 tests/fixtures/layouts/two-check.yaml | 47 +++++++++++++++++++++++++++
 tests/unit/test_scheduler.py          | 16 +++++++++
 zuul/source/__init__.py               |  2 +-
 3 files changed, 64 insertions(+), 1 deletion(-)
 create mode 100644 tests/fixtures/layouts/two-check.yaml

diff --git a/tests/fixtures/layouts/two-check.yaml b/tests/fixtures/layouts/two-check.yaml
new file mode 100644
index 0000000000..bc3e598182
--- /dev/null
+++ b/tests/fixtures/layouts/two-check.yaml
@@ -0,0 +1,47 @@
+- pipeline:
+    name: check1
+    manager: independent
+    trigger:
+      gerrit:
+        - event: patchset-created
+    success:
+      gerrit:
+        Verified: 1
+    failure:
+      gerrit:
+        Verified: -1
+
+- pipeline:
+    name: check2
+    manager: independent
+    trigger:
+      gerrit:
+        - event: patchset-created
+    success:
+      gerrit:
+        Verified: 1
+    failure:
+      gerrit:
+        Verified: -1
+
+- job:
+    name: base
+    parent: null
+    run: playbooks/base.yaml
+    nodeset:
+      nodes:
+        - label: ubuntu-xenial
+          name: controller
+
+- job:
+    name: check-job
+    run: playbooks/check.yaml
+
+- project:
+    name: org/project
+    check1:
+      jobs:
+        - check-job
+    check2:
+      jobs:
+        - check-job
diff --git a/tests/unit/test_scheduler.py b/tests/unit/test_scheduler.py
index 6a0da1279f..5c36e66210 100644
--- a/tests/unit/test_scheduler.py
+++ b/tests/unit/test_scheduler.py
@@ -3713,6 +3713,22 @@ class TestScheduler(ZuulTestCase):
         self.assertEqual(self.history[4].pipeline, 'check')
         self.assertEqual(self.history[5].pipeline, 'check')
 
+    @simple_layout('layouts/two-check.yaml')
+    def test_query_dependency_count(self):
+        # Test that we efficiently query dependent changes
+        A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A')
+        B = self.fake_gerrit.addFakeChange('org/project', 'master', 'B')
+        B.data['commitMessage'] = '%s\n\nDepends-On: %s\n' % (
+            B.subject, A.data['url'])
+        self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
+        self.waitUntilSettled()
+        # 1. The query to find the change id
+        #      from the Depends-On string "change:1" (simpleQuery)
+        # 2. The query to populate the change once we know the id
+        #      (queryChange)
+        self.assertEqual(A.queried, 2)
+        self.assertEqual(B.queried, 1)
+
     def test_reconfigure_merge(self):
         """Test that two reconfigure events are merged"""
         # Wrap the recofiguration handler so we can count how many
diff --git a/zuul/source/__init__.py b/zuul/source/__init__.py
index dd11aa9b63..01a683b236 100644
--- a/zuul/source/__init__.py
+++ b/zuul/source/__init__.py
@@ -109,7 +109,7 @@ class BaseSource(object, metaclass=abc.ABCMeta):
                     time.sleep(1)
                 else:
                     raise
-        return dep
+            return dep
 
     @abc.abstractmethod
     def getChangesDependingOn(self, change, projects, tenant):