Merge branch 'buildbot-0.8.6'

* buildbot-0.8.6:
  Cope if releaseLocks is called multiple times
  Capture and log errors during cleanup
  Add tests for #2280.
  Create a FakeMaster class.
  Unregister slaver from PBManager when removing slave.

master/buildbot/buildslave.py

@@ -233,6 +233,8 @@ def reconfigService(self, new_config):
         return d
 
     def stopService(self):
+        if self.registration:
+            self.registration.unregister()
         self.stopMissingTimer()
         return service.MultiService.stopService(self)
 

master/buildbot/locks.py

@@ -101,7 +101,9 @@ def release(self, owner, access):
 
         debuglog("%s release(%s, %s)" % (self, owner, access.mode))
         entry = (owner, access)
-        assert entry in self.owners
+        if not entry in self.owners:
+            debuglog("%s already released" % self)
+            return
         self.owners.remove(entry)
         # who can we wake up?
         # After an exclusive access, we may need to wake up several waiting.

master/buildbot/process/builder.py

@@ -260,9 +260,12 @@ def _startBuildFor(self, slavebuilder, buildrequests):
         # into a list so that, at any point, we can abort this operation.
         cleanups = []
         def run_cleanups():
-            while cleanups:
-                fn = cleanups.pop()
-                fn()
+            try:
+                while cleanups:
+                    fn = cleanups.pop()
+                    fn()
+            except:
+                log.err(failure.Failure(), "while running %r" % (run_cleanups,))
 
         # the last cleanup we want to perform is to update the big
         # status based on any other cleanup

master/buildbot/test/fake/fakemaster.py

@@ -16,6 +16,7 @@
 import weakref
 from twisted.internet import defer
 from buildbot.test.fake import fakedb
+from buildbot.test.fake.pbmanager import FakePBManager
 from buildbot import config
 import mock
 
@@ -36,23 +37,27 @@ def mkref(x):
         return d
 
 
-def make_master(master_id=fakedb.FakeBuildRequestsComponent.MASTER_ID):
+class FakeMaster(mock.Mock):
     """
     Create a fake Master instance: a Mock with some convenience
     implementations:
 
     - Non-caching implementation for C{self.caches}
     """
 
-    fakemaster = mock.Mock(name="fakemaster")
+    def __init__(self, master_id=fakedb.FakeBuildRequestsComponent.MASTER_ID):
+        mock.Mock.__init__(self, name="fakemaster")
+        self._master_id = master_id
+        self.config = config.MasterConfig()
+        self.caches.get_cache = FakeCache
+        self.pbmanager = FakePBManager()
 
-    # set up caches
-    fakemaster.caches.get_cache = FakeCache
+    def getObjectId(self):
+        return defer.succeed(self._master_id)
 
-    # and a getObjectId method
-    fakemaster.getObjectId = (lambda : defer.succeed(master_id))
+    # work around http://code.google.com/p/mock/issues/detail?id=105
+    def _get_child_mock(self, **kw):
+        return mock.Mock(**kw)
 
-    # and some config - this class's constructor is good enough to trust
-    fakemaster.config = config.MasterConfig()
-
-    return fakemaster
+# Leave this alias, in case we want to add more behavior later
+make_master = FakeMaster

master/buildbot/test/fake/pbmanager.py

@@ -0,0 +1,47 @@
+# 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 twisted.application import service
+from twisted.internet import defer
+
+class FakePBManager(service.MultiService):
+
+    def __init__(self):
+        service.MultiService.__init__(self)
+        self.setName("fake-pbmanager")
+        self._registrations = []
+        self._unregistrations = []
+
+    def register(self, portstr, username, password, pfactory):
+        if (portstr, username) not in self._registrations:
+            reg = FakeRegistration(self, portstr, username)
+            self._registrations.append((portstr,username,password))
+            return reg
+        else:
+            raise KeyError, ("username '%s' is already registered on port %s"
+                    % (username, portstr))
+
+    def _unregister(self, portstr, username):
+        self._unregistrations.append((portstr, username))
+        return defer.succeed(None)
+
+class FakeRegistration(object):
+    def __init__(self, pbmanager, portstr, username):
+        self._portstr = portstr
+        self._username = username
+        self._pbmanager = pbmanager
+
+    def unregister(self):
+        self._pbmanager._unregister(self._portstr, self._username)

master/buildbot/test/unit/test_buildslave.py

@@ -17,7 +17,7 @@
 from twisted.trial import unittest
 from twisted.internet import defer
 from buildbot import buildslave, config
-from buildbot.test.fake import fakemaster
+from buildbot.test.fake import fakemaster, pbmanager
 
 class AbstractBuildSlave(unittest.TestCase):
 
@@ -68,15 +68,11 @@ def do_test_reconfigService(self, old, old_port, new, new_port):
         old.master = master
         if old_port:
             self.old_registration = old.registration = \
-                    mock.Mock(name='old_registration')
+                    pbmanager.FakeRegistration(master.pbmanager, old_port, old.slavename)
             old.registered_port = old_port
         old.missing_timer = mock.Mock(name='missing_timer')
         old.startService()
 
-        self.new_registration = mock.Mock(name='new_registration')
-        master.pbmanager.register = mock.Mock(
-                side_effect=lambda *args : self.new_registration)
-
         new_config = mock.Mock()
         new_config.slavePortnum = new_port
         new_config.slaves = [ new ]
@@ -107,7 +103,7 @@ def test_reconfigService_attrs(self):
         self.assertEqual(old.missing_timeout, 121)
         self.assertEqual(old.properties.getProperty('a'), 'c')
         self.assertEqual(old.keepalive_interval, 61)
-        self.assertFalse(self.master.pbmanager.register.called)
+        self.assertEqual(self.master.pbmanager._registrations, [])
         self.assertTrue(old.updateSlave.called)
 
     @defer.deferredGenerator
@@ -130,7 +126,7 @@ def test_reconfigService_initial_registration(self):
         yield wfd
         wfd.getResult()
 
-        self.assertTrue(self.master.pbmanager.register.called)
+        self.assertEqual(self.master.pbmanager._registrations, [('tcp:1234', 'bot', 'pass')])
 
     @defer.inlineCallbacks
     def test_reconfigService_reregister_password(self):
@@ -140,8 +136,8 @@ def test_reconfigService_reregister_password(self):
         yield self.do_test_reconfigService(old, 'tcp:1234', new, 'tcp:1234')
 
         self.assertEqual(old.password, 'newpass')
-        self.assertTrue(self.old_registration.unregister.called)
-        self.assertTrue(self.master.pbmanager.register.called)
+        self.assertEqual(self.master.pbmanager._unregistrations, [('tcp:1234', 'bot')])
+        self.assertEqual(self.master.pbmanager._registrations, [('tcp:1234', 'bot', 'newpass')])
 
     @defer.inlineCallbacks
     def test_reconfigService_reregister_port(self):
@@ -150,8 +146,25 @@ def test_reconfigService_reregister_port(self):
 
         yield self.do_test_reconfigService(old, 'tcp:1234', new, 'tcp:5678')
 
-        self.assertTrue(self.old_registration.unregister.called)
-        self.assertTrue(self.master.pbmanager.register.called)
+        self.assertEqual(self.master.pbmanager._unregistrations, [('tcp:1234', 'bot')])
+        self.assertEqual(self.master.pbmanager._registrations, [('tcp:5678', 'bot', 'pass')])
+
+    @defer.inlineCallbacks
+    def test_stopService(self):
+        master = self.master = fakemaster.make_master()
+        slave = self.ConcreteBuildSlave('bot', 'pass')
+        slave.master = master
+        slave.startService()
+
+        config = mock.Mock()
+        config.slavePortnum = "tcp:1234"
+        config.slaves = [ slave ]
+
+        yield slave.reconfigService(config)
+        yield slave.stopService()
+
+        self.assertEqual(self.master.pbmanager._unregistrations, [('tcp:1234', 'bot')])
+        self.assertEqual(self.master.pbmanager._registrations, [('tcp:1234', 'bot', 'pass')])
 
     # FIXME: Test that reconfig properly deals with
     #   1) locks