Merge lp:~salgado/launchpad/tech-debt into lp:launchpad

[Guilherme Salgado]

[bug=888010] [r=wallyworld][no-qa] Remove setSlaveForTesting() and some more testing-specific code from Builder

Remove Builder.setSlaveForTesting and some more testing-specific code

[Ian Booth (wallyworld)]

Thank you, this is a great change.

review: Approve

=== modified file 'lib/lp/buildmaster/interfaces/builder.py'
--- lib/lp/buildmaster/interfaces/builder.py	2012-01-11 08:52:22 +0000
+++ lib/lp/buildmaster/interfaces/builder.py	2012-02-10 05:25:23 +0000
@@ -193,9 +193,6 @@
     def failBuilder(reason):
         """Mark builder as failed for a given reason."""
 
-    def setSlaveForTesting(proxy):
-        """Sets the RPC proxy through which to operate the build slave."""
-
     def verifySlaveBuildCookie(slave_build_id):
         """Verify that a slave's build cookie is consistent.
 
@@ -337,11 +334,9 @@
             or immediately if it's a non-virtual slave.
         """
 
-    def findAndStartJob(buildd_slave=None):
+    def findAndStartJob():
         """Find a job to run and send it to the buildd slave.
 
-        :param buildd_slave: An optional buildd slave that this builder should
-            talk to.
         :return: A Deferred whose value is the `IBuildQueue` instance
             found or None if no job was found.
         """
 
=== modified file 'lib/lp/buildmaster/model/builder.py'
--- lib/lp/buildmaster/model/builder.py	2012-01-24 10:09:47 +0000
+++ lib/lp/buildmaster/model/builder.py	2012-02-10 05:25:23 +0000
@@ -540,25 +540,11 @@
 
         return d.addCallback(got_resume_ok).addErrback(got_resume_bad)
 
-    _testing_slave = None
-
     @cachedproperty
     def slave(self):
         """See IBuilder."""
-        # When testing it's possible to substitute the slave object, which is
-        # usually an XMLRPC client, with a stub object that removes the need
-        # to actually create a buildd slave in various states - which can be
-        # hard to create. We cannot use the property cache because it is
-        # cleared on transaction boundaries, hence the low tech approach.
-        if self._testing_slave is not None:
-            return self._testing_slave
         return BuilderSlave.makeBuilderSlave(self.url, self.vm_host)
 
-    def setSlaveForTesting(self, proxy):
-        """See IBuilder."""
-        self._testing_slave = proxy
-        del get_property_cache(self).slave
-
     def startBuild(self, build_queue_item, logger):
         """See IBuilder."""
         self.current_build_behavior = build_queue_item.required_build_behavior
@@ -845,7 +831,7 @@
             self.failBuilder(error_message)
             return defer.succeed(None)
 
-    def findAndStartJob(self, buildd_slave=None):
+    def findAndStartJob(self):
         """See IBuilder."""
         # XXX This method should be removed in favour of two separately
         # called methods that find and dispatch the job.  It will
@@ -857,9 +843,6 @@
             logger.debug("No build candidates available for builder.")
             return defer.succeed(None)
 
-        if buildd_slave is not None:
-            self.setSlaveForTesting(buildd_slave)
-
         d = self._dispatchBuildCandidate(candidate)
         return d.addCallback(lambda ignored: candidate)
 
 
=== modified file 'lib/lp/buildmaster/tests/test_builder.py'
--- lib/lp/buildmaster/tests/test_builder.py	2012-01-11 08:52:22 +0000
+++ lib/lp/buildmaster/tests/test_builder.py	2012-02-10 05:25:23 +0000
@@ -205,7 +205,7 @@
         processor = self.factory.makeProcessor(name="i386")
         builder = self.factory.makeBuilder(
             processor=processor, virtualized=True, vm_host="bladh")
-        builder.setSlaveForTesting(OkSlave())
+        self.patch(BuilderSlave, 'makeBuilderSlave', FakeMethod(OkSlave()))
         distroseries = self.factory.makeDistroSeries()
         das = self.factory.makeDistroArchSeries(
             distroseries=distroseries, architecturetag="i386",
@@ -370,7 +370,8 @@
         builder, build = self._setupBinaryBuildAndBuilder()
         candidate = build.queueBuild()
         building_slave = BuildingSlave()
-        builder.setSlaveForTesting(building_slave)
+        self.patch(
+            BuilderSlave, 'makeBuilderSlave', FakeMethod(building_slave))
         candidate.markAsBuilding(builder)
 
         # At this point we should see a valid behaviour on the builder:
@@ -405,7 +406,7 @@
                      build_status=None, logtail=False, filemap=None,
                      dependencies=None):
         builder = self.factory.makeBuilder()
-        builder.setSlaveForTesting(slave)
+        self.patch(BuilderSlave, 'makeBuilderSlave', FakeMethod(slave))
         d = builder.slaveStatus()
 
         def got_status(status_dict):
@@ -458,13 +459,15 @@
 
     def test_isAvailable_with_slave_fault(self):
         builder = self.factory.makeBuilder()
-        builder.setSlaveForTesting(BrokenSlave())
+        self.patch(
+            BuilderSlave, 'makeBuilderSlave', FakeMethod(BrokenSlave()))
         d = builder.isAvailable()
         return d.addCallback(self.assertFalse)
 
     def test_isAvailable_with_slave_idle(self):
         builder = self.factory.makeBuilder()
-        builder.setSlaveForTesting(OkSlave())
+        self.patch(
+            BuilderSlave, 'makeBuilderSlave', FakeMethod(OkSlave()))
         d = builder.isAvailable()
         return d.addCallback(self.assertTrue)
 
 
=== modified file 'lib/lp/buildmaster/tests/test_manager.py'
--- lib/lp/buildmaster/tests/test_manager.py	2012-01-20 15:42:44 +0000
+++ lib/lp/buildmaster/tests/test_manager.py	2012-02-10 05:25:23 +0000
@@ -33,7 +33,10 @@
     NewBuildersScanner,
     SlaveScanner,
     )
-from lp.buildmaster.model.builder import Builder
+from lp.buildmaster.model.builder import (
+    Builder,
+    BuilderSlave,
+    )
 from lp.buildmaster.tests.harness import BuilddManagerTestSetup
 from lp.buildmaster.tests.mock_slaves import (
     BrokenSlave,
@@ -134,7 +137,7 @@
         # Reset sampledata builder.
         builder = getUtility(IBuilderSet)[BOB_THE_BUILDER_NAME]
         self._resetBuilder(builder)
-        builder.setSlaveForTesting(OkSlave())
+        self.patch(BuilderSlave, 'makeBuilderSlave', FakeMethod(OkSlave()))
         # Set this to 1 here so that _checkDispatch can make sure it's
         # reset to 0 after a successful dispatch.
         builder.failure_count = 1
@@ -246,7 +249,8 @@
 
         login('[e-mail]')
         builder.builderok = True
-        builder.setSlaveForTesting(BuildingSlave(build_id='8-1'))
+        self.patch(BuilderSlave, 'makeBuilderSlave',
+                   FakeMethod(BuildingSlave(build_id='8-1')))
         transaction.commit()
         login(ANONYMOUS)
 
@@ -263,7 +267,7 @@
     def test_scan_with_nothing_to_dispatch(self):
         factory = LaunchpadObjectFactory()
         builder = factory.makeBuilder()
-        builder.setSlaveForTesting(OkSlave())
+        self.patch(BuilderSlave, 'makeBuilderSlave', FakeMethod(OkSlave()))
         scanner = self._getScanner(builder_name=builder.name)
         d = scanner.scan()
         return d.addCallback(self._checkNoDispatch, builder)
@@ -272,7 +276,7 @@
         # Reset sampledata builder.
         builder = getUtility(IBuilderSet)[BOB_THE_BUILDER_NAME]
         self._resetBuilder(builder)
-        builder.setSlaveForTesting(OkSlave())
+        self.patch(BuilderSlave, 'makeBuilderSlave', FakeMethod(OkSlave()))
         builder.manual = True
         scanner = self._getScanner()
         d = scanner.scan()
@@ -283,7 +287,7 @@
         # Reset sampledata builder.
         builder = getUtility(IBuilderSet)[BOB_THE_BUILDER_NAME]
         self._resetBuilder(builder)
-        builder.setSlaveForTesting(OkSlave())
+        self.patch(BuilderSlave, 'makeBuilderSlave', FakeMethod(OkSlave()))
         builder.builderok = False
         scanner = self._getScanner()
         d = scanner.scan()
@@ -295,7 +299,8 @@
     def test_scan_of_broken_slave(self):
         builder = getUtility(IBuilderSet)[BOB_THE_BUILDER_NAME]
         self._resetBuilder(builder)
-        builder.setSlaveForTesting(BrokenSlave())
+        self.patch(
+            BuilderSlave, 'makeBuilderSlave', FakeMethod(BrokenSlave()))
         builder.failure_count = 0
         scanner = self._getScanner(builder_name=builder.name)
         d = scanner.scan()
@@ -390,7 +395,7 @@
             getUtility(IBuilderSet)[BOB_THE_BUILDER_NAME])
         self._resetBuilder(builder)
         self.assertEqual(0, builder.failure_count)
-        builder.setSlaveForTesting(slave)
+        self.patch(BuilderSlave, 'makeBuilderSlave', FakeMethod(slave))
         builder.vm_host = "fake_vm_host"
 
         scanner = self._getScanner()
@@ -434,7 +439,7 @@
         # For now, we can only cancel virtual builds.
         builder.virtualized = True
         builder.vm_host = "fake_vm_host"
-        builder.setSlaveForTesting(slave)
+        self.patch(BuilderSlave, 'makeBuilderSlave', FakeMethod(slave))
         transaction.commit()
         login(ANONYMOUS)
         buildqueue = builder.currentjob
@@ -508,7 +513,7 @@
         slave = OkSlave()
         slave.resume = fake_resume
         self.builder.vm_host = "fake_vm_host"
-        self.builder.setSlaveForTesting(slave)
+        self.patch(BuilderSlave, 'makeBuilderSlave', FakeMethod(slave))
         buildqueue = self.builder.currentjob
         build = getUtility(IBinaryPackageBuildSet).getByQueueEntry(buildqueue)
         build.status = BuildStatus.CANCELLING
 
=== modified file 'lib/lp/buildmaster/tests/test_packagebuild.py'
--- lib/lp/buildmaster/tests/test_packagebuild.py	2012-01-11 08:52:22 +0000
+++ lib/lp/buildmaster/tests/test_packagebuild.py	2012-02-10 05:25:23 +0000
@@ -26,6 +26,7 @@
     IPackageBuildSet,
     IPackageBuildSource,
     )
+from lp.buildmaster.model.builder import BuilderSlave
 from lp.buildmaster.model.buildfarmjob import BuildFarmJob
 from lp.buildmaster.model.packagebuild import PackageBuild
 from lp.buildmaster.tests.mock_slaves import WaitingSlave
@@ -303,7 +304,7 @@
         self.build.buildqueue_record.setDateStarted(UTC_NOW)
         self.slave = WaitingSlave('BuildStatus.OK')
         self.slave.valid_file_hashes.append('test_file_hash')
-        builder.setSlaveForTesting(self.slave)
+        self.patch(BuilderSlave, 'makeBuilderSlave', FakeMethod(self.slave))
 
         # We overwrite the buildmaster root to use a temp directory.
         tempdir = tempfile.mkdtemp()
 
=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipebuild.py'
--- lib/lp/code/model/tests/test_sourcepackagerecipebuild.py	2012-01-11 08:52:22 +0000
+++ lib/lp/code/model/tests/test_sourcepackagerecipebuild.py	2012-02-10 05:25:23 +0000
@@ -21,6 +21,7 @@
 from lp.app.errors import NotFoundError
 from lp.buildmaster.enums import BuildStatus
 from lp.buildmaster.interfaces.buildqueue import IBuildQueue
+from lp.buildmaster.model.builder import BuilderSlave
 from lp.buildmaster.model.buildfarmjob import BuildFarmJob
 from lp.buildmaster.model.packagebuild import PackageBuild
 from lp.buildmaster.tests.mock_slaves import WaitingSlave
@@ -608,7 +609,7 @@
                 result=True)
         queue_record.builder = self.factory.makeBuilder()
         slave = WaitingSlave('BuildStatus.OK')
-        queue_record.builder.setSlaveForTesting(slave)
+        self.patch(BuilderSlave, 'makeBuilderSlave', FakeMethod(slave))
         return build
 
     def assertDeferredNotifyCount(self, status, build, expected_count):
 
=== modified file 'lib/lp/soyuz/tests/test_binarypackagebuild.py'
--- lib/lp/soyuz/tests/test_binarypackagebuild.py	2012-01-11 08:52:22 +0000
+++ lib/lp/soyuz/tests/test_binarypackagebuild.py	2012-02-10 05:25:23 +0000
@@ -18,6 +18,7 @@
 from lp.buildmaster.interfaces.builder import IBuilderSet
 from lp.buildmaster.interfaces.buildqueue import IBuildQueue
 from lp.buildmaster.interfaces.packagebuild import IPackageBuild
+from lp.buildmaster.model.builder import BuilderSlave
 from lp.buildmaster.model.buildqueue import BuildQueue
 from lp.buildmaster.tests.mock_slaves import WaitingSlave
 from lp.buildmaster.tests.test_packagebuild import (
@@ -48,6 +49,7 @@
     logout,
     TestCaseWithFactory,
     )
+from lp.testing.fakemethod import FakeMethod
 from lp.testing.layers import (
     DatabaseFunctionalLayer,
     LaunchpadZopelessLayer,
@@ -522,7 +524,8 @@
         self.build = gedit_src_hist.createMissingBuilds()[0]
 
         self.builder = self.factory.makeBuilder()
-        self.builder.setSlaveForTesting(WaitingSlave('BuildStatus.OK'))
+        self.patch(BuilderSlave, 'makeBuilderSlave',
+                   FakeMethod(WaitingSlave('BuildStatus.OK')))
         self.build.buildqueue_record.markAsBuilding(self.builder)
 
     def testDependencies(self):
 
=== modified file 'lib/lp/soyuz/tests/test_binarypackagebuildbehavior.py'
--- lib/lp/soyuz/tests/test_binarypackagebuildbehavior.py	2012-01-20 15:42:44 +0000
+++ lib/lp/soyuz/tests/test_binarypackagebuildbehavior.py	2012-02-10 05:25:23 +0000
@@ -18,6 +18,7 @@
 from zope.security.proxy import removeSecurityProxy
 
 from lp.buildmaster.enums import BuildStatus
+from lp.buildmaster.model.builder import BuilderSlave
 from lp.buildmaster.tests.mock_slaves import (
     AbortedSlave,
     AbortingSlave,
@@ -40,6 +41,7 @@
 from lp.soyuz.enums import ArchivePurpose
 from lp.testing import TestCaseWithFactory
 from lp.testing.dbuser import switch_dbuser
+from lp.testing.fakemethod import FakeMethod
 from lp.testing.layers import LaunchpadZopelessLayer
 
 
@@ -138,7 +140,7 @@
         archive = self.factory.makeArchive(virtualized=False)
         slave = OkSlave()
         builder = self.factory.makeBuilder(virtualized=False)
-        builder.setSlaveForTesting(slave)
+        self.patch(BuilderSlave, 'makeBuilderSlave', FakeMethod(slave))
         build = self.factory.makeBinaryPackageBuild(
             builder=builder, archive=archive)
         lf = self.factory.makeLibraryFileAlias()
@@ -158,7 +160,7 @@
         slave = OkSlave()
         builder = self.factory.makeBuilder(
             virtualized=True, vm_host="foohost")
-        builder.setSlaveForTesting(slave)
+        self.patch(BuilderSlave, 'makeBuilderSlave', FakeMethod(slave))
         build = self.factory.makeBinaryPackageBuild(
             builder=builder, archive=archive)
         lf = self.factory.makeLibraryFileAlias()
@@ -181,7 +183,7 @@
             virtualized=False, purpose=ArchivePurpose.PARTNER)
         slave = OkSlave()
         builder = self.factory.makeBuilder(virtualized=False)
-        builder.setSlaveForTesting(slave)
+        self.patch(BuilderSlave, 'makeBuilderSlave', FakeMethod(slave))
         build = self.factory.makeBinaryPackageBuild(
             builder=builder, archive=archive)
         lf = self.factory.makeLibraryFileAlias()
@@ -309,7 +311,8 @@
         # When a package fails to build, make sure the builder notes are
         # stored and the build status is set as failed.
         waiting_slave = WaitingSlave('BuildStatus.PACKAGEFAIL')
-        self.builder.setSlaveForTesting(waiting_slave)
+        self.patch(BuilderSlave, 'makeBuilderSlave',
+                   FakeMethod(waiting_slave))
 
         def got_update(ignored):
             self.assertBuildProperties(self.build)
@@ -322,7 +325,8 @@
         # Package build was left in dependency wait.
         DEPENDENCIES = 'baz (>= 1.0.1)'
         waiting_slave = WaitingSlave('BuildStatus.DEPFAIL', DEPENDENCIES)
-        self.builder.setSlaveForTesting(waiting_slave)
+        self.patch(BuilderSlave, 'makeBuilderSlave',
+                   FakeMethod(waiting_slave))
 
         def got_update(ignored):
             self.assertBuildProperties(self.build)
@@ -335,7 +339,8 @@
     def test_chrootfail_collection(self):
         # There was a chroot problem for this build.
         waiting_slave = WaitingSlave('BuildStatus.CHROOTFAIL')
-        self.builder.setSlaveForTesting(waiting_slave)
+        self.patch(BuilderSlave, 'makeBuilderSlave',
+                   FakeMethod(waiting_slave))
 
         def got_update(ignored):
             self.assertBuildProperties(self.build)
@@ -347,7 +352,8 @@
     def test_builderfail_collection(self):
         # The builder failed after we dispatched the build.
         waiting_slave = WaitingSlave('BuildStatus.BUILDERFAIL')
-        self.builder.setSlaveForTesting(waiting_slave)
+        self.patch(BuilderSlave, 'makeBuilderSlave',
+                   FakeMethod(waiting_slave))
 
         def got_update(ignored):
             self.assertEqual(
@@ -363,7 +369,8 @@
 
     def test_building_collection(self):
         # The builder is still building the package.
-        self.builder.setSlaveForTesting(BuildingSlave())
+        self.patch(BuilderSlave, 'makeBuilderSlave',
+                   FakeMethod(BuildingSlave()))
 
         def got_update(ignored):
             # The fake log is returned from the BuildingSlave() mock.
@@ -374,7 +381,8 @@
 
     def test_aborted_collection(self):
         # The builder aborted the job.
-        self.builder.setSlaveForTesting(AbortedSlave())
+        self.patch(BuilderSlave, 'makeBuilderSlave',
+                   FakeMethod(AbortedSlave()))
 
         def got_update(ignored):
             self.assertEqual(BuildStatus.NEEDSBUILD, self.build.status)
@@ -384,7 +392,8 @@
 
     def test_aborting_collection(self):
         # The builder is in the process of aborting.
-        self.builder.setSlaveForTesting(AbortingSlave())
+        self.patch(BuilderSlave, 'makeBuilderSlave',
+                   FakeMethod(AbortingSlave()))
 
         def got_update(ignored):
             self.assertEqual(
@@ -398,7 +407,8 @@
         # If we collected a build for a superseded/deleted source then
         # the build should get marked superseded as the build results
         # get discarded.
-        self.builder.setSlaveForTesting(WaitingSlave('BuildStatus.OK'))
+        self.patch(BuilderSlave, 'makeBuilderSlave',
+                   FakeMethod(WaitingSlave('BuildStatus.OK')))
         spr = removeSecurityProxy(self.build.source_package_release)
         pub = self.build.current_source_publication
         pub.requestDeletion(spr.creator)
@@ -412,7 +422,8 @@
 
     def test_uploading_collection(self):
         # After a successful build, the status should be UPLOADING.
-        self.builder.setSlaveForTesting(WaitingSlave('BuildStatus.OK'))
+        self.patch(BuilderSlave, 'makeBuilderSlave',
+                   FakeMethod(WaitingSlave('BuildStatus.OK')))
 
         def got_update(ignored):
             self.assertEqual(self.build.status, BuildStatus.UPLOADING)
@@ -425,7 +436,8 @@
 
     def test_givenback_collection(self):
         waiting_slave = WaitingSlave('BuildStatus.GIVENBACK')
-        self.builder.setSlaveForTesting(waiting_slave)
+        self.patch(BuilderSlave, 'makeBuilderSlave',
+                   FakeMethod(waiting_slave))
         score = self.candidate.lastscore
 
         def got_update(ignored):
@@ -440,7 +452,8 @@
         return d.addCallback(got_update)
 
     def test_log_file_collection(self):
-        self.builder.setSlaveForTesting(WaitingSlave('BuildStatus.OK'))
+        self.patch(BuilderSlave, 'makeBuilderSlave',
+                   FakeMethod(WaitingSlave('BuildStatus.OK')))
         self.build.status = BuildStatus.FULLYBUILT
         old_tmps = sorted(os.listdir('/tmp'))
 
@@ -488,7 +501,8 @@
     def test_private_build_log_storage(self):
         # Builds in private archives should have their log uploaded to
         # the restricted librarian.
-        self.builder.setSlaveForTesting(WaitingSlave('BuildStatus.OK'))
+        self.patch(BuilderSlave, 'makeBuilderSlave',
+                   FakeMethod(WaitingSlave('BuildStatus.OK')))
 
         # Go behind Storm's back since the field validator on
         # Archive.private prevents us from setting it to True with
 
=== modified file 'lib/lp/translations/stories/buildfarm/xx-build-summary.txt'
--- lib/lp/translations/stories/buildfarm/xx-build-summary.txt	2012-01-15 13:32:27 +0000
+++ lib/lp/translations/stories/buildfarm/xx-build-summary.txt	2012-02-10 05:25:23 +0000
@@ -9,12 +9,14 @@
 
 Create a builder working on a TranslationTemplatesBuild for a branch.
 
+    >>> from testtools.monkey import patch
     >>> from zope.component import getUtility
     >>> from lp.app.interfaces.launchpad import ILaunchpadCelebrities
     >>> from lp.services.librarian.interfaces import (
     ...     ILibraryFileAliasSet)
     >>> from lp.app.enums import ServiceUsage
     >>> from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet
+    >>> from lp.buildmaster.model.builder import BuilderSlave
     >>> from lp.testing.factory import (
     ...     remove_security_proxy_and_shout_at_engineer)
     >>> from lp.testing.fakemethod import FakeMethod
@@ -51,7 +53,7 @@
     ...     fake_chroot)
 
     >>> builder = factory.makeBuilder(vm_host=factory.getUniqueString())
-    >>> builder.setSlaveForTesting(FakeSlave())
+    >>> _ = patch(BuilderSlave, 'makeBuilderSlave', FakeMethod(FakeSlave()))
     >>> buildqueue.markAsBuilding(builder)
 
     >>> builder_page = canonical_url(builder)
 
=== modified file 'lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py'
--- lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py	2012-02-01 05:57:16 +0000
+++ lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py	2012-02-10 05:25:23 +0000
@@ -20,6 +20,7 @@
     IBuildFarmJobBehavior,
     )
 from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet
+from lp.buildmaster.model.builder import BuilderSlave
 from lp.buildmaster.tests.mock_slaves import (
     SlaveTestHelpers,
     WaitingSlave,
@@ -79,7 +80,7 @@
         behavior = IBuildFarmJobBehavior(specific_job)
         slave = WaitingSlave()
         behavior._builder = removeSecurityProxy(self.factory.makeBuilder())
-        behavior._builder.setSlaveForTesting(slave)
+        self.patch(BuilderSlave, 'makeBuilderSlave', FakeMethod(slave))
         if use_fake_chroot:
             lf = self.factory.makeLibraryFileAlias()
             self.layer.txn.commit()