Merge pull request #1681 from sa2ajj/fix-properties-eight

change.properties is a IProperties, so api is getProperty()

master/buildbot/changes/filter.py

@@ -83,7 +83,7 @@ def filter_change(self, change):
             return False
         for chg_attr, (filt_list, filt_re, filt_fn) in self.checks.items():
             if chg_attr.startswith("prop:"):
-                chg_val = change.properties.get(chg_attr.split(":", 1)[1], '')
+                chg_val = change.properties.getProperty(chg_attr.split(":", 1)[1], '')
             else:
                 chg_val = getattr(change, chg_attr, '')
             if filt_list is not None and chg_val not in filt_list:

master/buildbot/changes/gerritchangesource.py

@@ -42,6 +42,21 @@ def __init__(self,
             del self.checks["branch"]
 
 
+def _gerrit_user_to_author(props, username=u"unknown"):
+    """
+    Convert Gerrit account properties to Buildbot format
+
+    Take into account missing values
+    """
+    username = props.get("username", username)
+
+    result = [props.get("name", username)]
+    if "email" in props:
+        result.append(u"<%s>" % props["email"])
+
+    return u" ".join(result)
+
+
 class GerritChangeSource(base.ChangeSource):
 
     """This source will maintain a connection to gerrit ssh server
@@ -182,11 +197,9 @@ def addChangeFromEvent(self, properties, event):
         if "change" in event and "patchSet" in event:
             event_change = event["change"]
             return self.addChange({
-                'author': "%s <%s>" % (
-                    event_change["owner"]["name"],
-                    event_change["owner"]["email"]),
-                'project': event_change["project"],
-                'repository': "ssh://%s@%s:%s/%s" % (
+                'author': _gerrit_user_to_author(event_change["owner"]),
+                'project': util.ascii2unicode(event_change["project"]),
+                'repository': u"ssh://%s@%s:%s/%s" % (
                     self.username, self.gerritserver,
                     self.gerritport, event_change["project"]),
                 'branch': self.getGroupingPolicyFromEvent(event),
@@ -199,11 +212,10 @@ def addChangeFromEvent(self, properties, event):
 
     def eventReceived_ref_updated(self, properties, event):
         ref = event["refUpdate"]
-        author = "gerrit"
+        author = u"gerrit"
 
         if "submitter" in event:
-            author = "%s <%s>" % (
-                event["submitter"]["name"], event["submitter"]["email"])
+            author = _gerrit_user_to_author(event["submitter"], author)
 
         return self.addChange(dict(
             author=author,

master/buildbot/test/fake/change.py

@@ -0,0 +1,34 @@
+# This file is part of Buildbot.  Buildbot is free software: you can
+# redistribute it and/or modify it under the terms of the GNU General Public
+# License as published by the Free Software Foundation, version 2.
+#
+# This program is distributed in the hope that it will be useful, but WITHOUT
+# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
+# FOR A PARTICULAR PURPOSE.  See the GNU General Public License for more
+# details.
+#
+# You should have received a copy of the GNU General Public License along with
+# this program; if not, write to the Free Software Foundation, Inc., 51
+# Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+#
+# Copyright Buildbot Team Members
+
+from buildbot.process.properties import Properties
+from buildbot.test.fake.state import State
+
+
+class Change(State):
+
+    project = ''
+    repository = ''
+    branch = ''
+    category = ''
+    codebase = ''
+    properties = {}
+
+    def __init__(self, **kw):
+        State.__init__(self, **kw)
+        # change.properties is a IProperties
+        props = Properties()
+        props.update(self.properties, "test")
+        self.properties = props

master/buildbot/test/unit/test_changes_filter.py

@@ -18,15 +18,7 @@
 from twisted.trial import unittest
 
 from buildbot.changes import filter
-from buildbot.test.fake.state import State
-
-
-class Change(State):
-    project = ''
-    repository = ''
-    branch = ''
-    category = ''
-    codebase = ''
+from buildbot.test.fake.change import Change
 
 
 class ChangeFilter(unittest.TestCase):
@@ -112,7 +104,7 @@ def test_filter_change_combination(self):
         self.no(Change(project='p', repository='r', branch='b', category='x'),
                 "three match -> False")
         self.no(Change(project='p', repository='r', branch='b', category='c',
-                codebase='x'), "four match -> False")
+                       codebase='x'), "four match -> False")
         self.yes(Change(project='p', repository='r', branch='b', category='c',
                         codebase='cb'), "all match -> True")
         self.check()
@@ -129,3 +121,14 @@ def test_filter_change_combination_filter_fn(self):
         self.yes(Change(project='p', repository='r', branch='b', category='c', ff=True),
                  "all match and fn returns True -> False")
         self.check()
+
+    def test_filter_props(self):
+        self.setfilter()
+        self.filt.checks.update(
+            self.filt.createChecks(
+                ("ref-updated", None, None, "prop:event.type"),
+            ))
+        self.yes(Change(properties={'event.type': 'ref-updated'}), "matching property")
+        self.no(Change(properties={'event.type': 'patch-uploaded'}), "non matching property")
+        self.no(Change(properties={}), "no property")
+        self.check()

master/buildbot/test/unit/test_changes_gerritchangesource.py

@@ -16,11 +16,65 @@
 import types
 
 from buildbot.changes import gerritchangesource
+from buildbot.test.fake.change import Change
 from buildbot.test.util import changesource
 from buildbot.util import json
 from twisted.trial import unittest
 
 
+class TestGerritHelpers(unittest.TestCase):
+    def test_proper_json(self):
+        self.assertEqual(u"Justin Case <justin.case@example.com>",
+                         gerritchangesource._gerrit_user_to_author({
+                             "username": "justincase",
+                             "name": "Justin Case",
+                             "email": "justin.case@example.com"
+                         }))
+
+    def test_missing_username(self):
+        self.assertEqual(u"Justin Case <justin.case@example.com>",
+                         gerritchangesource._gerrit_user_to_author({
+                             "name": "Justin Case",
+                             "email": "justin.case@example.com"
+                         }))
+
+    def test_missing_name(self):
+        self.assertEqual(u"unknown <justin.case@example.com>",
+                         gerritchangesource._gerrit_user_to_author({
+                             "email": "justin.case@example.com"
+                         }))
+        self.assertEqual(u"gerrit <justin.case@example.com>",
+                         gerritchangesource._gerrit_user_to_author({
+                             "email": "justin.case@example.com"
+                         }, u"gerrit"))
+        self.assertEqual(u"justincase <justin.case@example.com>",
+                         gerritchangesource._gerrit_user_to_author({
+                             "username": "justincase",
+                             "email": "justin.case@example.com"
+                         }, u"gerrit"))
+
+    def test_missing_email(self):
+        self.assertEqual(u"Justin Case",
+                         gerritchangesource._gerrit_user_to_author({
+                             "username": "justincase",
+                             "name": "Justin Case"
+                         }))
+        self.assertEqual(u"Justin Case",
+                         gerritchangesource._gerrit_user_to_author({
+                             "name": "Justin Case"
+                         }))
+        self.assertEqual(u"justincase",
+                         gerritchangesource._gerrit_user_to_author({
+                             "username": "justincase"
+                         }))
+        self.assertEqual(u"unknown",
+                         gerritchangesource._gerrit_user_to_author({
+                         }))
+        self.assertEqual(u"gerrit",
+                         gerritchangesource._gerrit_user_to_author({
+                         }, u"gerrit"))
+
+
 class TestGerritChangeSource(changesource.ChangeSourceMixin,
                              unittest.TestCase):
 
@@ -144,15 +198,17 @@ def check(_):
 
 class TestGerritChangeFilter(unittest.TestCase):
     def test_basic(self):
-        class Change(object):
-            def __init__(self, chdict):
-                self.__dict__ = chdict
+        ch = Change(**TestGerritChangeSource.expected_change)
 
-        ch = Change(TestGerritChangeSource.expected_change)
-        f = gerritchangesource.GerritChangeFilter(branch=["br"], eventtype=["patchset-created"])
+        f = gerritchangesource.GerritChangeFilter(branch=["br"],
+                                                  eventtype=["patchset-created"])
         self.assertTrue(f.filter_change(ch))
-        f = gerritchangesource.GerritChangeFilter(branch="br2", eventtype=["patchset-created"])
+
+        f = gerritchangesource.GerritChangeFilter(branch="br2",
+                                                  eventtype=["patchset-created"])
         self.assertFalse(f.filter_change(ch))
-        f = gerritchangesource.GerritChangeFilter(branch="br", eventtype="ref-updated")
+
+        f = gerritchangesource.GerritChangeFilter(branch="br",
+                                                  eventtype="ref-updated")
         self.assertFalse(f.filter_change(ch))
         self.assertEqual(repr(f), '<GerritChangeFilter on prop:event.change.branch == br and prop:event.type == ref-updated>')