Merge lp:~michael.nelson/launchpad/587113-buildbase-handleStatus into lp:launchpad

[Michael Nelson]

This branch fixes the core issue of bug 587113.

It refactors the tests so that handleStatus() is called for both BinaryPackageBuilds and SourcePackageRecipeBuilds.

It also adds other tests to ensure log files and other attributes are correctly added for both build types.

To enable the shared implementation to work for both classes (until SourcePackageRecipeBuild is also updated to use the new table structure) I had to alias 3 attributes of IBuildBase.

To test:
bin/test -vv -m test_sourcepackagerecipebuild -m test_binarypackagebuild

[Graham Binns (gmb)]

review: Approve (code)

=== modified file 'lib/canonical/launchpad/webapp/configure.zcml'
--- lib/canonical/launchpad/webapp/configure.zcml	2010-06-05 05:05:43 +0000
+++ lib/canonical/launchpad/webapp/configure.zcml	2010-06-08 16:04:27 +0000
@@ -421,7 +421,7 @@
     <adapter
         for="lp.buildmaster.interfaces.packagebuild.IPackageBuild"
         provides="zope.traversing.interfaces.IPathAdapter"
-        factory="canonical.launchpad.webapp.tales.BuildBaseFormatterAPI"
+        factory="canonical.launchpad.webapp.tales.PackageBuildFormatterAPI"
         name="fmt"
         />
 

=== modified file 'lib/canonical/launchpad/webapp/tales.py'
--- lib/canonical/launchpad/webapp/tales.py	2010-06-05 05:05:43 +0000
+++ lib/canonical/launchpad/webapp/tales.py	2010-06-08 16:04:27 +0000
@@ -1515,8 +1515,8 @@
         return url
 
 
-class BuildBaseFormatterAPI(ObjectFormatterAPI):
-    """Adapter providing fmt support for `IBuildBase` objects."""
+class PackageBuildFormatterAPI(ObjectFormatterAPI):
+    """Adapter providing fmt support for `IPackageBuild` objects."""
     def _composeArchiveReference(self, archive):
         if archive.is_ppa:
             return " [%s/%s]" % (

=== modified file 'lib/lp/buildmaster/interfaces/buildbase.py'
--- lib/lp/buildmaster/interfaces/buildbase.py	2010-05-19 15:39:52 +0000
+++ lib/lp/buildmaster/interfaces/buildbase.py	2010-06-08 16:04:27 +0000
@@ -116,7 +116,22 @@
     """Common interface shared by farm jobs that build a package."""
     # XXX 2010-04-21 michael.nelson bug=567922. This interface
     # can be removed once all *Build classes inherit from
-    # IBuildFarmJob/IPackageBuild.
+    # IBuildFarmJob/IPackageBuild. Until that time, to allow the shared
+    # implementation of handling build status, IBuildBase needs to
+    # provide aliases for buildstate, buildlog and datebuilt as follows:
+    # status => buildstate
+    # log => buildlog
+    # date_finished => datebuilt
+    status = Choice(
+            title=_('State'), required=True, vocabulary=BuildStatus,
+            description=_("The current build state."))
+    log = Object(
+        schema=ILibraryFileAlias, required=False,
+        title=_("The LibraryFileAlias containing the entire buildlog."))
+    date_finished = Datetime(
+            title=_('Date built'), required=False,
+            description=_("The time when the build result got collected."))
+
 
     build_farm_job_type = Choice(
         title=_("Job type"), required=True, readonly=True,
@@ -130,10 +145,7 @@
             title=_('Date created'), required=True, readonly=True,
             description=_("The time when the build request was created.")))
 
-    buildstate = exported(
-        Choice(
-            title=_('State'), required=True, vocabulary=BuildStatus,
-            description=_("The current build state.")))
+    buildstate = exported(status)
 
     date_first_dispatched = exported(
         Datetime(
@@ -146,19 +158,14 @@
         title=_("Builder"), schema=IBuilder, required=False,
         description=_("The Builder which address this build request."))
 
-    datebuilt = exported(
-        Datetime(
-            title=_('Date built'), required=False,
-            description=_("The time when the build result got collected.")))
+    datebuilt = exported(date_finished)
 
     buildduration = Timedelta(
         title=_("Build Duration"), required=False,
         description=_("Build duration interval, calculated when the "
                       "build result gets collected."))
 
-    buildlog = Object(
-        schema=ILibraryFileAlias, required=False,
-        title=_("The LibraryFileAlias containing the entire buildlog."))
+    buildlog = log
 
     build_log_url = exported(
         TextLine(
@@ -225,6 +232,14 @@
             executable.
         """
 
+    def getUploadLogContent(root, leaf):
+        """Retrieve the upload log contents.
+
+        :param root: Root directory for the uploads
+        :param leaf: Leaf for this particular upload
+        :return: Contents of log file or message saying no log file was found.
+        """
+
     def handleStatus(build, status, librarian, slave_status):
         """Handle a finished build status from a slave.
 

=== modified file 'lib/lp/buildmaster/model/buildbase.py'
--- lib/lp/buildmaster/model/buildbase.py	2010-05-21 09:42:21 +0000
+++ lib/lp/buildmaster/model/buildbase.py	2010-06-08 16:04:27 +0000
@@ -9,7 +9,10 @@
 
 __metaclass__ = type
 
-__all__ = ['BuildBase']
+__all__ = [
+    'handle_status_for_build',
+    'BuildBase',
+    ]
 
 import datetime
 import logging
@@ -37,6 +40,24 @@
 UPLOAD_LOG_FILENAME = 'uploader.log'
 
 
+def handle_status_for_build(build, status, librarian, slave_status):
+    """Find and call the correct method for handling the build status.
+
+    This is extracted from build base so that the implementation
+    can be shared by the newer IPackageBuild class.
+    """
+    logger = logging.getLogger(BUILDD_MANAGER_LOG_NAME)
+
+    method = getattr(BuildBase, '_handleStatus_' + status, None)
+
+    if method is None:
+        logger.critical("Unknown BuildStatus '%s' for builder '%s'"
+                        % (status, build.buildqueue_record.builder.url))
+        return
+
+    method(build, librarian, slave_status, logger)
+
+
 class BuildBase:
     """A mixin class providing functionality for farm jobs that build a
     package.
@@ -127,19 +148,9 @@
             return None
         return self._getProxiedFileURL(self.upload_log)
 
-    @staticmethod
-    def handleStatus(build, status, librarian, slave_status):
+    def handleStatus(self, status, librarian, slave_status):
         """See `IBuildBase`."""
-        logger = logging.getLogger(BUILDD_MANAGER_LOG_NAME)
-
-        method = getattr(build, '_handleStatus_' + status, None)
-
-        if method is None:
-            logger.critical("Unknown BuildStatus '%s' for builder '%s'"
-                            % (status, build.buildqueue_record.builder.url))
-            return
-
-        method(librarian, slave_status, logger)
+        return handle_status_for_build(self, status, librarian, slave_status)
 
     @staticmethod
     def _handleStatus_OK(build, librarian, slave_status, logger):

=== modified file 'lib/lp/buildmaster/model/packagebuild.py'
--- lib/lp/buildmaster/model/packagebuild.py	2010-05-21 09:42:21 +0000
+++ lib/lp/buildmaster/model/packagebuild.py	2010-06-08 16:04:27 +0000
@@ -24,7 +24,7 @@
 from lp.buildmaster.interfaces.buildfarmjob import IBuildFarmJobSource
 from lp.buildmaster.interfaces.packagebuild import (
     IPackageBuild, IPackageBuildSource)
-from lp.buildmaster.model.buildbase import BuildBase
+from lp.buildmaster.model.buildbase import handle_status_for_build, BuildBase
 from lp.buildmaster.model.buildfarmjob import BuildFarmJobDerived
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.soyuz.adapters.archivedependencies import (
@@ -185,7 +185,7 @@
 
     def handleStatus(self, status, librarian, slave_status):
         """See `IPackageBuild`."""
-        return BuildBase.handleStatus(self, status, librarian, slave_status)
+        return handle_status_for_build(self, status, librarian, slave_status)
 
     # The following private handlers currently re-use the BuildBase
     # implementation until it is no longer in use. If we find in the

=== modified file 'lib/lp/buildmaster/tests/test_buildbase.py'
--- lib/lp/buildmaster/tests/test_buildbase.py	2010-05-21 09:42:21 +0000
+++ lib/lp/buildmaster/tests/test_buildbase.py	2010-06-08 16:04:27 +0000
@@ -15,17 +15,16 @@
 from datetime import datetime
 import os
 import unittest
+from zope.security.proxy import removeSecurityProxy
 
 from canonical.config import config
 from canonical.database.constants import UTC_NOW
-from canonical.testing.layers import (
-    DatabaseFunctionalLayer, LaunchpadZopelessLayer)
+from canonical.testing.layers import LaunchpadZopelessLayer
 from lp.buildmaster.interfaces.buildbase import BuildStatus
 from lp.buildmaster.model.buildbase import BuildBase
 from lp.registry.interfaces.pocket import pocketsuffix
 from lp.soyuz.tests.soyuzbuilddhelpers import WaitingSlave
-from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
-from lp.testing import TestCase, TestCaseWithFactory
+from lp.testing import TestCase
 from lp.testing.fakemethod import FakeMethod
 
 
@@ -62,27 +61,41 @@
         self.package_build = BuildBase()
 
 
-class TestBuildBaseWithDatabase(TestCaseWithFactory):
+class TestGetUploadMethodsMixin:
     """Tests for `IBuildBase` that need objects from the rest of Launchpad."""
 
-    layer = DatabaseFunctionalLayer
+    layer = LaunchpadZopelessLayer
+
+    def makeBuild(self):
+        """Allow classes to override the build with which the test runs.
+
+        XXX michaeln 2010-06-03 bug=567922
+        Until buildbase is removed, we need to ensure these tests
+        run against new IPackageBuild builds (BinaryPackageBuild)
+        and the IBuildBase builds (SPRecipeBuild). They assume the build
+        is successfully built and check that incorrect upload paths will
+        set the status to FAILEDTOUPLOAD.
+        """
+        raise NotImplemented
+
+    def setUp(self):
+        super(TestGetUploadMethodsMixin, self).setUp()
+        self.build = self.makeBuild()
 
     def test_getUploadLogContent_nolog(self):
         """If there is no log file there, a string explanation is returned.
         """
         self.useTempDir()
-        build_base = BuildBase()
         self.assertEquals('Could not find upload log file',
-            build_base.getUploadLogContent(os.getcwd(), "myleaf"))
+            self.build.getUploadLogContent(os.getcwd(), "myleaf"))
 
     def test_getUploadLogContent_only_dir(self):
         """If there is a directory but no log file, expect the error string,
         not an exception."""
         self.useTempDir()
         os.makedirs("accepted/myleaf")
-        build_base = BuildBase()
         self.assertEquals('Could not find upload log file',
-            build_base.getUploadLogContent(os.getcwd(), "myleaf"))
+            self.build.getUploadLogContent(os.getcwd(), "myleaf"))
 
     def test_getUploadLogContent_readsfile(self):
         """If there is a log file, return its contents."""
@@ -90,47 +103,53 @@
         os.makedirs("accepted/myleaf")
         with open('accepted/myleaf/uploader.log', 'w') as f:
             f.write('foo')
-        build_base = BuildBase()
         self.assertEquals('foo',
-            build_base.getUploadLogContent(os.getcwd(), "myleaf"))
+            self.build.getUploadLogContent(os.getcwd(), "myleaf"))
 
     def test_getUploaderCommand(self):
-        build_base = BuildBase()
         upload_leaf = self.factory.getUniqueString('upload-leaf')
-        build_base.distro_series = self.factory.makeDistroSeries()
-        build_base.distribution = build_base.distro_series.distribution
-        build_base.pocket = self.factory.getAnyPocket()
-        build_base.id = self.factory.getUniqueInteger()
-        build_base.policy_name = self.factory.getUniqueString('policy-name')
         config_args = list(config.builddmaster.uploader.split())
         log_file = self.factory.getUniqueString('logfile')
         config_args.extend(
             ['--log-file', log_file,
-             '-d', build_base.distribution.name,
-             '-s', (build_base.distro_series.name
-                    + pocketsuffix[build_base.pocket]),
-             '-b', str(build_base.id),
+             '-d', self.build.distribution.name,
+             '-s', (self.build.distro_series.name
+                    + pocketsuffix[self.build.pocket]),
+             '-b', str(self.build.id),
              '-J', upload_leaf,
-             '--context=%s' % build_base.policy_name,
+             '--context=%s' % self.build.policy_name,
              os.path.abspath(config.builddmaster.root),
              ])
-        uploader_command = build_base.getUploaderCommand(
-            build_base, upload_leaf, log_file)
+        uploader_command = self.build.getUploaderCommand(
+            self.build, upload_leaf, log_file)
         self.assertEqual(config_args, uploader_command)
 
 
-class TestBuildBaseHandleStatus(TestCaseWithFactory):
-    """Tests for `IBuildBase`s handleStatus method."""
+class TestHandleStatusMixin:
+    """Tests for `IBuildBase`s handleStatus method.
+
+    Note: these tests do *not* test the updating of the build
+    status to FULLYBUILT as this happens during the upload which
+    is stubbed out by a mock function.
+    """
 
     layer = LaunchpadZopelessLayer
 
+    def makeBuild(self):
+        """Allow classes to override the build with which the test runs.
+
+        XXX michaeln 2010-06-03 bug=567922
+        Until buildbase is removed, we need to ensure these tests
+        run against new IPackageBuild builds (BinaryPackageBuild)
+        and the IBuildBase builds (SPRecipeBuild). They assume the build
+        is successfully built and check that incorrect upload paths will
+        set the status to FAILEDTOUPLOAD.
+        """
+        raise NotImplementedError
+
     def setUp(self):
-        super(TestBuildBaseHandleStatus, self).setUp()
-        test_publisher = SoyuzTestPublisher()
-        test_publisher.prepareBreezyAutotest()
-        binaries = test_publisher.getPubBinaries()
-        self.build = binaries[0].binarypackagerelease.build
-
+        super(TestHandleStatusMixin, self).setUp()
+        self.build = self.makeBuild()
         # For the moment, we require a builder for the build so that
         # handleStatus_OK can get a reference to the slave.
         builder = self.factory.makeBuilder()
@@ -149,10 +168,14 @@
         config.push('tmp_builddmaster_root', tmp_builddmaster_root)
 
         # We stub out our builds getUploaderCommand() method so
-        # we can check whether it was called.
+        # we can check whether it was called as well as
+        # verifySuccessfulUpload().
         self.fake_getUploaderCommand = FakeMethod(
             result=['echo', 'noop'])
-        self.build.getUploaderCommand = self.fake_getUploaderCommand
+        removeSecurityProxy(self.build).getUploaderCommand = (
+            self.fake_getUploaderCommand)
+        removeSecurityProxy(self.build).verifySuccessfulUpload = FakeMethod(
+            result=True)
 
     def test_handleStatus_OK_normal_file(self):
         # A filemap with plain filenames should not cause a problem.
@@ -183,6 +206,24 @@
         self.assertEqual(BuildStatus.FAILEDTOUPLOAD, self.build.status)
         self.assertEqual(0, self.fake_getUploaderCommand.call_count)
 
+    def test_handleStatus_OK_sets_build_log(self):
+        # The build log is set during handleStatus.
+        removeSecurityProxy(self.build).log = None
+        self.assertEqual(None, self.build.log)
+        self.build.handleStatus('OK', None, {
+                'filemap': { 'myfile.py': 'test_file_hash'},
+                })
+        self.assertNotEqual(None, self.build.log)
+
+    def test_date_finished_set(self):
+        # The date finished is updated during handleStatus_OK.
+        removeSecurityProxy(self.build).date_finished = None
+        self.assertEqual(None, self.build.date_finished)
+        self.build.handleStatus('OK', None, {
+                'filemap': { 'myfile.py': 'test_file_hash'},
+                })
+        self.assertNotEqual(None, self.build.date_finished)
+
 
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/code/model/sourcepackagerecipebuild.py'
--- lib/lp/code/model/sourcepackagerecipebuild.py	2010-05-27 22:18:16 +0000
+++ lib/lp/code/model/sourcepackagerecipebuild.py	2010-06-08 16:04:27 +0000
@@ -75,7 +75,6 @@
     buildlog = Reference(buildlog_id, 'LibraryFileAlias.id')
 
     buildstate = DBEnum(enum=BuildStatus, name='build_state')
-
     dependencies = Unicode(allow_none=True)
 
     upload_log_id = Int(name='upload_log', allow_none=True)
@@ -88,6 +87,13 @@
     datecreated = UtcDateTimeCol(notNull=True, dbName='date_created')
     datebuilt = UtcDateTimeCol(notNull=False, dbName='date_built')
 
+    # See `IBuildBase` - the following attributes are aliased
+    # to allow a shared implementation of the handleStatus methods
+    # until IBuildBase is removed.
+    status = buildstate
+    date_finished = datebuilt
+    log = buildlog
+
     @property
     def datestarted(self):
         """See `IBuild`."""

=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipebuild.py'
--- lib/lp/code/model/tests/test_sourcepackagerecipebuild.py	2010-05-27 22:18:16 +0000
+++ lib/lp/code/model/tests/test_sourcepackagerecipebuild.py	2010-06-08 16:04:27 +0000
@@ -20,11 +20,14 @@
 
 from canonical.launchpad.interfaces.launchpad import NotFoundError
 from canonical.launchpad.webapp.authorization import check_permission
-from lp.buildmaster.interfaces.buildbase import IBuildBase
+from lp.buildmaster.interfaces.buildbase import BuildStatus, IBuildBase
 from lp.buildmaster.interfaces.buildqueue import IBuildQueue
+from lp.buildmaster.tests.test_buildbase import (
+    TestGetUploadMethodsMixin, TestHandleStatusMixin)
 from lp.code.interfaces.sourcepackagerecipebuild import (
     ISourcePackageRecipeBuildJob, ISourcePackageRecipeBuild,
     ISourcePackageRecipeBuildSource)
+from lp.soyuz.interfaces.processor import IProcessorFamilySet
 from lp.soyuz.model.processor import ProcessorFamily
 from lp.testing import ANONYMOUS, login, person_logged_in, TestCaseWithFactory
 
@@ -193,6 +196,33 @@
         self.assertEqual([binary], list(spb.binary_builds))
 
 
+class MakeSPRecipeBuildMixin:
+    """Provide the common makeBuild method returning a queued build."""
+
+    def makeBuild(self):
+        person = self.factory.makePerson()
+        distroseries = self.factory.makeDistroSeries()
+        processor_fam = getUtility(IProcessorFamilySet).getByName('x86')
+        distroseries_i386 = distroseries.newArch(
+            'i386', processor_fam, False, person,
+            supports_virtualized=True)
+        distroseries.nominatedarchindep = distroseries_i386
+        build = self.factory.makeSourcePackageRecipeBuild(
+            distroseries=distroseries,
+            status=BuildStatus.FULLYBUILT)
+        build.queueBuild(build)
+        return build
+
+
+class TestGetUploadMethodsForSPRecipeBuild(
+    MakeSPRecipeBuildMixin, TestGetUploadMethodsMixin, TestCaseWithFactory):
+    """IBuildBase.getUpload-related methods work with SPRecipe builds."""
+
+
+class TestHandleStatusForSPRBuild(
+    MakeSPRecipeBuildMixin, TestHandleStatusMixin, TestCaseWithFactory):
+    """IBuildBase.handleStatus works with SPRecipe builds."""
+
 
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/soyuz/tests/test_binarypackagebuild.py'
--- lib/lp/soyuz/tests/test_binarypackagebuild.py	2010-06-07 20:21:02 +0000
+++ lib/lp/soyuz/tests/test_binarypackagebuild.py	2010-06-08 16:04:27 +0000
@@ -17,6 +17,8 @@
 from lp.buildmaster.interfaces.buildqueue import IBuildQueue
 from lp.buildmaster.interfaces.packagebuild import IPackageBuild
 from lp.buildmaster.model.buildqueue import BuildQueue
+from lp.buildmaster.tests.test_buildbase import (
+    TestGetUploadMethodsMixin, TestHandleStatusMixin)
 from lp.soyuz.interfaces.binarypackagebuild import (
     IBinaryPackageBuild, IBinaryPackageBuildSet)
 from lp.soyuz.interfaces.buildpackagejob import IBuildPackageJob
@@ -345,5 +347,26 @@
         self.assertIsNot(None, self.build.date_finished)
 
 
+class MakeBinaryPackageBuildMixin:
+    """Provide the makeBuild method returning a queud build."""
+
+    def makeBuild(self):
+        test_publisher = SoyuzTestPublisher()
+        test_publisher.prepareBreezyAutotest()
+        binaries = test_publisher.getPubBinaries()
+        return binaries[0].binarypackagerelease.build
+
+
+class TestGetUploadMethodsForBinaryPackageBuild(
+    MakeBinaryPackageBuildMixin, TestGetUploadMethodsMixin,
+    TestCaseWithFactory):
+    """IBuildBase.getUpload-related methods work with binary builds."""
+
+
+class TestHandleStatusForBinaryPackageBuild(
+    MakeBinaryPackageBuildMixin, TestHandleStatusMixin, TestCaseWithFactory):
+    """IBuildBase.handleStatus works with binary builds."""
+
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2010-05-30 04:06:48 +0000
+++ lib/lp/testing/factory.py	2010-06-08 16:04:27 +0000
@@ -1789,7 +1789,8 @@
     def makeSourcePackageRecipeBuild(self, sourcepackage=None, recipe=None,
                                      requester=None, archive=None,
                                      sourcename=None, distroseries=None,
-                                     pocket=None):
+                                     pocket=None,
+                                     status=BuildStatus.NEEDSBUILD):
         """Make a new SourcePackageRecipeBuild."""
         if recipe is None:
             recipe = self.makeSourcePackageRecipe(name=sourcename)
@@ -1800,12 +1801,14 @@
                 distribution=archive.distribution)
         if requester is None:
             requester = self.makePerson()
-        return getUtility(ISourcePackageRecipeBuildSource).new(
+        spr_build = getUtility(ISourcePackageRecipeBuildSource).new(
             distroseries=distroseries,
             recipe=recipe,
             archive=archive,
             requester=requester,
             pocket=pocket)
+        removeSecurityProxy(spr_build).buildstate = status
+        return spr_build
 
     def makeSourcePackageRecipeBuildJob(
         self, score=9876, virtualized=True, estimated_duration=64,