Fix gerrit errors from production

This corrects the following errors.

Test fixture errors:

* Unlike the SSH API, the Gerrit REST API does not include
  COMMIT_MSG in the list of files.
* We weren't checking all elements of the (change id, branch,
  project) tuple when searching for change ids
* I observed a race around poll synchronization; I can't figure it out
  so I've added more debug log lines.

Production errors:

* Break after the first successful submit rather than attemping the
  call multiple times in all cases.  This was non-fatal.
* Only attempt to get the remote version if using HTTP.  This was
  non-fatal.
* URLquote search queries (they may contain '#').  The additional CRD
  tests cover this now.
* Coerce the list of files to a list rather than a dict_keys instance.
  The new single file test covers this.

Change-Id: I89a68b2d189b82f0bc64e78fbc875d5628cc2cf0

7 files changed, 71 insertions, 4 deletions

diff --git a/tests/base.py b/tests/base.py
index 9d49f4019..7cfdc2d78 100644
--- a/tests/base.py
+++ b/tests/base.py
@@ -552,6 +552,8 @@ class FakeGerritChange(object):
         num = len(self.patchsets)
         files = {}
         for f in rev['files']:
+            if f['file'] == '/COMMIT_MSG':
+                continue
             files[f['file']] = {"status": f['type'][0]}  # ADDED -> A
         parent = '0000000000000000000000000000000000000000'
         if self.depends_on_change:
@@ -711,9 +713,12 @@ class GerritWebServer(object):
                 self.end_headers()
 
             def _get_change(self, change_id):
+                change_id = urllib.parse.unquote(change_id)
                 project, branch, change = change_id.split('~')
                 for c in fake_gerrit.changes.values():
-                    if c.data['id'] == change:
+                    if (c.data['id'] == change and
+                        c.data['branch'] == branch and
+                        c.data['project'] == project):
                         return c
 
             def review(self, change_id, revision, data):
@@ -3916,8 +3921,11 @@ class ZuulTestCase(BaseTestCase):
             self.sched.wake_event.wait(0.1)
 
     def waitForPoll(self, poller, timeout=30):
+        self.log.debug("Wait for poll on %s", poller)
         self.poller_events[poller].clear()
-        self.poller_events[poller].wait(30)
+        self.log.debug("Waiting for poll on %s", poller)
+        self.poller_events[poller].wait(timeout)
+        self.log.debug("Done waiting for poll on %s", poller)
 
     def logState(self):
         """ Log the current state of the system """
diff --git a/tests/fixtures/layouts/single-file-matcher.yaml b/tests/fixtures/layouts/single-file-matcher.yaml
new file mode 100644
index 000000000..0e7275aba
--- /dev/null
+++ b/tests/fixtures/layouts/single-file-matcher.yaml
@@ -0,0 +1,28 @@
+- pipeline:
+    name: check
+    manager: independent
+    trigger:
+      gerrit:
+        - event: patchset-created
+    success:
+      gerrit:
+        Verified: 1
+    failure:
+      gerrit:
+        Verified: -1
+
+- job:
+    name: base
+    parent: null
+    run: playbooks/base.yaml
+
+- job:
+    name: irr-job
+    irrelevant-files:
+      - README
+
+- project:
+    name: org/project
+    check:
+      jobs:
+        - irr-job
diff --git a/tests/unit/test_gerrit.py b/tests/unit/test_gerrit.py
index f784cdfcc..af8e1cc3d 100644
--- a/tests/unit/test_gerrit.py
+++ b/tests/unit/test_gerrit.py
@@ -191,6 +191,26 @@ class TestGerritWeb(ZuulTestCase):
                       B.messages[0])
         self.assertEqual(B.comments, [])
 
+    @simple_layout('layouts/single-file-matcher.yaml')
+    def test_single_file(self):
+        # HTTP requests don't return a commit_msg entry in the files
+        # list, but the rest of zuul always expects one.  This test
+        # returns a single file to exercise the single-file code path
+        # in the files matcher.
+        files = {'README': 'please!\n'}
+
+        change = self.fake_gerrit.addFakeChange('org/project',
+                                                'master',
+                                                'test irrelevant-files',
+                                                files=files)
+        self.fake_gerrit.addEvent(change.getPatchsetCreatedEvent(1))
+        self.waitUntilSettled()
+
+        tested_change_ids = [x.changes[0] for x in self.history
+                             if x.name == 'project-test-irrelevant-files']
+
+        self.assertEqual([], tested_change_ids)
+
 
 class TestFileComments(AnsibleZuulTestCase):
     config_file = 'zuul-gerrit-web.conf'
diff --git a/tests/unit/test_gerrit_crd.py b/tests/unit/test_gerrit_crd.py
index 544e3c750..032d5d4e2 100644
--- a/tests/unit/test_gerrit_crd.py
+++ b/tests/unit/test_gerrit_crd.py
@@ -770,3 +770,7 @@ class TestGerritCRDAltBaseUrl(ZuulTestCase):
         self.assertEqual(self.history[0].changes, '2,1 1,1')
         tenant = self.sched.abide.tenants.get('tenant-one')
         self.assertEqual(len(tenant.layout.pipelines['check'].queues), 0)
+
+
+class TestGerritCRDWeb(TestGerritCRD):
+    config_file = 'zuul-gerrit-web.conf'
diff --git a/tests/unit/test_gerrit_legacy_crd.py b/tests/unit/test_gerrit_legacy_crd.py
index 515e2fa95..1ae2f862b 100644
--- a/tests/unit/test_gerrit_legacy_crd.py
+++ b/tests/unit/test_gerrit_legacy_crd.py
@@ -626,3 +626,7 @@ class TestGerritLegacyCRD(ZuulTestCase):
         self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(2))
         self.waitUntilSettled()
         self.assertEqual(B.reported, 2)
+
+
+class TestGerritLegacyCRDWeb(TestGerritLegacyCRD):
+    config_file = 'zuul-gerrit-web.conf'
diff --git a/zuul/driver/gerrit/gerritconnection.py b/zuul/driver/gerrit/gerritconnection.py
index 8a64bad7c..2350efb96 100644
--- a/zuul/driver/gerrit/gerritconnection.py
+++ b/zuul/driver/gerrit/gerritconnection.py
@@ -1055,6 +1055,7 @@ class GerritConnection(BaseConnection):
             for x in range(1, 4):
                 try:
                     self.post('changes/%s/submit' % (changeid,), {})
+                    break
                 except Exception:
                     log.exception(
                         "Error submitting data to gerrit, attempt %s", x)
@@ -1153,6 +1154,7 @@ class GerritConnection(BaseConnection):
         sortkey = ''
         done = False
         offset = 0
+        query = urllib.parse.quote(query, safe='')
         while not done:
             # We don't actually want to limit to 500, but that's the
             # server-side default, and if we don't specify this, we
@@ -1326,7 +1328,8 @@ class GerritConnection(BaseConnection):
     def onLoad(self):
         self.log.debug("Starting Gerrit Connection/Watchers")
         try:
-            self._getRemoteVersion()
+            if self.session:
+                self._getRemoteVersion()
         except Exception:
             self.log.exception("Unable to determine remote Gerrit version")
 
diff --git a/zuul/driver/gerrit/gerritmodel.py b/zuul/driver/gerrit/gerritmodel.py
index 3967fbf82..1a715c392 100644
--- a/zuul/driver/gerrit/gerritmodel.py
+++ b/zuul/driver/gerrit/gerritmodel.py
@@ -110,7 +110,7 @@ class GerritChange(Change):
         if str(current_revision['_number']) == self.patchset:
             self.ref = current_revision['ref']
             self.commit = data['current_revision']
-            files = current_revision.get('files', []).keys()
+            files = list(current_revision.get('files', []).keys())
             self.is_current_patchset = True
         else:
             self.is_current_patchset = False