launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06307
[Merge] lp:~salgado/launchpad/tech-debt into lp:launchpad
Guilherme Salgado has proposed merging lp:~salgado/launchpad/tech-debt into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #888010 in Launchpad itself: "Builder.setSlaveForTesting is now a liability"
https://bugs.launchpad.net/launchpad/+bug/888010
For more details, see:
https://code.launchpad.net/~salgado/launchpad/tech-debt/+merge/92390
Remove Builder.setSlaveForTesting and some more testing-specific code
--
https://code.launchpad.net/~salgado/launchpad/tech-debt/+merge/92390
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~salgado/launchpad/tech-debt into lp:launchpad.
=== 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-09 23:17:21 +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-09 23:17:21 +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-09 23:17:21 +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-09 23:17:21 +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('foo.bar@xxxxxxxxxxxxx')
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-09 23:17:21 +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-09 23:17:21 +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-09 23:17:21 +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-09 23:17:21 +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-09 23:17:21 +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-09 23:17:21 +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()
