Merge lp:~wgrant/launchpad/bfj-mixin into lp:launchpad

[William Grant]

Extract all methods from PackageBuild(Derived)/BuildFarmJob(Derived) to separate mixins. The BFJ/PB classes are now basically free of logic, so we can begin the flattening.

More preparation for the BFJ+PB flattening. BuildFarmJob and PackageBuild are now extraordinarily boring objects, just containing the minimal methods for creating and destroying the basic objects. All the notable logic is now in BuildFarmJobMixin and PackageBuildMixin, permitting us to soon redirect attribute writes on BPB/SPRB/TTB to multiple columns for the migration.

BuildFarmJob(Derived) and PackageBuild(Derived) will die once the migration is complete. *Mixin will be the only remnants.

I had to shuffle some tests around to make them test *Mixin methods on a concrete implementation.

[Steve Kowalik (stevenk)]

review: Approve (code)

=== removed file 'lib/lp/buildmaster/doc/buildfarmjob.txt'
--- lib/lp/buildmaster/doc/buildfarmjob.txt	2013-01-04 06:20:06 +0000
+++ lib/lp/buildmaster/doc/buildfarmjob.txt	1970-01-01 00:00:00 +0000
@@ -1,15 +0,0 @@
-BuildFarmJob
-============
-
-    >>> from lp.buildmaster.interfaces.buildfarmjob import (
-    ...     IBuildFarmJob)
-    >>> from lp.buildmaster.enums import BuildFarmJobType
-    >>> from lp.buildmaster.model.buildfarmjob import BuildFarmJob
-
-BuildFarmJob provides a basic implementation of IBuildFarmJob.  The
-specific build farm job classes will automatically delegate to
-BuildFarmJob by inheriting BuildFarmJobDerived.
-
-    >>> buildfarmjob = BuildFarmJob(BuildFarmJobType.PACKAGEBUILD)
-    >>> verifyObject(IBuildFarmJob, buildfarmjob)
-    True

=== modified file 'lib/lp/buildmaster/model/buildfarmjob.py'
--- lib/lp/buildmaster/model/buildfarmjob.py	2013-01-04 07:52:40 +0000
+++ lib/lp/buildmaster/model/buildfarmjob.py	2013-01-07 04:25:24 +0000
@@ -5,6 +5,7 @@
 __all__ = [
     'BuildFarmJob',
     'BuildFarmJobDerived',
+    'BuildFarmJobMixin',
     'BuildFarmJobOld',
     'BuildFarmJobOldDerived',
     ]
@@ -278,6 +279,34 @@
         store.add(build_farm_job)
         return build_farm_job
 
+    def getSpecificJob(self):
+        """See `IBuild`"""
+        # Adapt ourselves based on our job type.
+        try:
+            source = getUtility(
+                ISpecificBuildFarmJobSource, self.job_type.name)
+        except ComponentLookupError:
+            raise InconsistentBuildFarmJobError(
+                "No source was found for the build farm job type %s." % (
+                    self.job_type.name))
+
+        build = source.getByBuildFarmJob(self)
+
+        if build is None:
+            raise InconsistentBuildFarmJobError(
+                "There is no related specific job for the build farm "
+                "job with id %d." % self.id)
+
+        # Just to be on the safe side, make sure the build is still
+        # proxied before returning it.
+        assert isProxy(build), (
+            "Unproxied result returned from ISpecificBuildFarmJobSource.")
+
+        return build
+
+
+class BuildFarmJobMixin:
+
     @property
     def title(self):
         """See `IBuildFarmJob`."""
@@ -327,31 +356,6 @@
                                    BuildStatus.UPLOADING,
                                    BuildStatus.SUPERSEDED]
 
-    def getSpecificJob(self):
-        """See `IBuild`"""
-        # Adapt ourselves based on our job type.
-        try:
-            source = getUtility(
-                ISpecificBuildFarmJobSource, self.job_type.name)
-        except ComponentLookupError:
-            raise InconsistentBuildFarmJobError(
-                "No source was found for the build farm job type %s." % (
-                    self.job_type.name))
-
-        build = source.getByBuildFarmJob(self)
-
-        if build is None:
-            raise InconsistentBuildFarmJobError(
-                "There is no related specific job for the build farm "
-                "job with id %d." % self.id)
-
-        # Just to be on the safe side, make sure the build is still
-        # proxied before returning it.
-        assert isProxy(build), (
-            "Unproxied result returned from ISpecificBuildFarmJobSource.")
-
-        return build
-
     def gotFailure(self):
         """See `IBuildFarmJob`."""
         self.failure_count += 1

=== modified file 'lib/lp/buildmaster/model/packagebuild.py'
--- lib/lp/buildmaster/model/packagebuild.py	2012-10-31 23:03:26 +0000
+++ lib/lp/buildmaster/model/packagebuild.py	2013-01-07 04:25:24 +0000
@@ -5,6 +5,7 @@
 __all__ = [
     'PackageBuild',
     'PackageBuildDerived',
+    'PackageBuildMixin',
     'PackageBuildSet',
     ]
 
@@ -43,6 +44,7 @@
     )
 from lp.buildmaster.model.buildfarmjob import (
     BuildFarmJob,
+    BuildFarmJobMixin,
     BuildFarmJobDerived,
     )
 from lp.buildmaster.model.buildqueue import BuildQueue
@@ -128,6 +130,15 @@
         store.remove(self)
         store.remove(build_farm_job)
 
+
+class PackageBuildMixin(BuildFarmJobMixin):
+
+    # The list of build status values for which email notifications are
+    # allowed to be sent. It is up to each callback as to whether it will
+    # consider sending a notification but it won't do so if the status is not
+    # in this list.
+    ALLOWED_STATUS_NOTIFICATIONS = ['OK', 'PACKAGEFAIL', 'CHROOTFAIL']
+
     @property
     def current_component(self):
         """See `IPackageBuild`."""
@@ -234,37 +245,10 @@
         """See `IPackageBuild`."""
         raise NotImplementedError
 
-    def handleStatus(self, status, librarian, slave_status):
-        """See `IPackageBuild`."""
-        raise NotImplementedError
-
-    def queueBuild(self, suspended=False):
-        """See `IPackageBuild`."""
-        raise NotImplementedError
-
-    def getBuildCookie(self):
-        """See `IPackageBuild`."""
-        raise NotImplementedError
-
     def getUploader(self, changes):
         """See `IPackageBuild`."""
         raise NotImplementedError
 
-
-class PackageBuildDerived:
-    """Setup the delegation for package build.
-
-    This class also provides some common implementation for handling
-    build status.
-    """
-    delegates(IPackageBuild, context="package_build")
-
-    # The list of build status values for which email notifications are
-    # allowed to be sent. It is up to each callback as to whether it will
-    # consider sending a notification but it won't do so if the status is not
-    # in this list.
-    ALLOWED_STATUS_NOTIFICATIONS = ['OK', 'PACKAGEFAIL', 'CHROOTFAIL']
-
     def getBuildCookie(self):
         """See `IPackageBuild`."""
         return '%s-%s' % (self.job_type.name, self.id)
@@ -520,6 +504,15 @@
         return d.addCallback(build_info_stored)
 
 
+class PackageBuildDerived:
+    """Setup the delegation for package build.
+
+    This class also provides some common implementation for handling
+    build status.
+    """
+    delegates(IPackageBuild, context="package_build")
+
+
 class PackageBuildSet:
     implements(IPackageBuildSet)
 

=== modified file 'lib/lp/buildmaster/tests/test_buildfarmjob.py'
--- lib/lp/buildmaster/tests/test_buildfarmjob.py	2013-01-04 06:25:25 +0000
+++ lib/lp/buildmaster/tests/test_buildfarmjob.py	2013-01-07 04:25:24 +0000
@@ -40,13 +40,13 @@
     )
 
 
-class TestBuildFarmJobMixin:
+class TestBuildFarmJobBase:
 
     layer = DatabaseFunctionalLayer
 
     def setUp(self):
         """Create a build farm job with which to test."""
-        super(TestBuildFarmJobMixin, self).setUp()
+        super(TestBuildFarmJobBase, self).setUp()
         self.build_farm_job = self.makeBuildFarmJob()
 
     def makeBuildFarmJob(self, builder=None,
@@ -68,13 +68,9 @@
         return build_farm_job
 
 
-class TestBuildFarmJob(TestBuildFarmJobMixin, TestCaseWithFactory):
+class TestBuildFarmJob(TestBuildFarmJobBase, TestCaseWithFactory):
     """Tests for the build farm job object."""
 
-    def test_providesInterface(self):
-        # BuildFarmJob provides IBuildFarmJob
-        self.assertProvides(self.build_farm_job, IBuildFarmJob)
-
     def test_saves_record(self):
         # A build farm job can be stored in the database.
         flush_database_updates()
@@ -105,19 +101,46 @@
         self.assertEqual(None, self.build_farm_job.date_first_dispatched)
         self.assertEqual(None, self.build_farm_job.builder)
         self.assertEqual(None, self.build_farm_job.log)
-        self.assertEqual(None, self.build_farm_job.log_url)
-        self.assertEqual(None, self.build_farm_job.buildqueue_record)
-
-    def test_unimplemented_methods(self):
-        # A build farm job leaves the implementation of various
-        # methods for derived classes.
-        self.assertRaises(NotImplementedError, self.build_farm_job.makeJob)
-
-    def test_title(self):
-        # The default title simply uses the job type's title.
-        self.assertEqual(
-            self.build_farm_job.job_type.title,
-            self.build_farm_job.title)
+
+    def test_getSpecificJob_none(self):
+        # An exception is raised if there is no related specific job.
+        self.assertRaises(
+            InconsistentBuildFarmJobError, self.build_farm_job.getSpecificJob)
+
+    def test_getSpecificJob_unimplemented_type(self):
+        # An `IBuildFarmJob` with an unimplemented type results in an
+        # exception.
+        removeSecurityProxy(self.build_farm_job).job_type = (
+            BuildFarmJobType.RECIPEBRANCHBUILD)
+
+        self.assertRaises(
+            InconsistentBuildFarmJobError, self.build_farm_job.getSpecificJob)
+
+    def test_date_created(self):
+        # date_created can be passed optionally when creating a
+        # bulid farm job to ensure we don't get identical timestamps
+        # when transactions are committed.
+        ten_years_ago = datetime.now(pytz.UTC) - timedelta(365 * 10)
+        build_farm_job = getUtility(IBuildFarmJobSource).new(
+            job_type=BuildFarmJobType.PACKAGEBUILD,
+            date_created=ten_years_ago)
+        self.failUnlessEqual(ten_years_ago, build_farm_job.date_created)
+
+
+class TestBuildFarmJobMixin(TestCaseWithFactory):
+    """Test methods provided by BuildFarmJobMixin."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestBuildFarmJobMixin, self).setUp()
+        # BuildFarmJobMixin only operates as part of a concrete
+        # IBuildFarmJob implementation. Here we use BinaryPackageBuild.
+        self.build_farm_job = self.factory.makeBinaryPackageBuild()
+
+    def test_providesInterface(self):
+        # BuildFarmJobMixin derivatives provide IBuildFarmJob
+        self.assertProvides(self.build_farm_job, IBuildFarmJob)
 
     def test_duration_none(self):
         # If either start or finished is none, the duration will be
@@ -141,32 +164,8 @@
         naked_bfj.date_finished = now + duration
         self.failUnlessEqual(duration, self.build_farm_job.duration)
 
-    def test_date_created(self):
-        # date_created can be passed optionally when creating a
-        # bulid farm job to ensure we don't get identical timestamps
-        # when transactions are committed.
-        ten_years_ago = datetime.now(pytz.UTC) - timedelta(365 * 10)
-        build_farm_job = getUtility(IBuildFarmJobSource).new(
-            job_type=BuildFarmJobType.PACKAGEBUILD,
-            date_created=ten_years_ago)
-        self.failUnlessEqual(ten_years_ago, build_farm_job.date_created)
-
-    def test_getSpecificJob_none(self):
-        # An exception is raised if there is no related specific job.
-        self.assertRaises(
-            InconsistentBuildFarmJobError, self.build_farm_job.getSpecificJob)
-
-    def test_getSpecificJob_unimplemented_type(self):
-        # An `IBuildFarmJob` with an unimplemented type results in an
-        # exception.
-        removeSecurityProxy(self.build_farm_job).job_type = (
-            BuildFarmJobType.RECIPEBRANCHBUILD)
-
-        self.assertRaises(
-            InconsistentBuildFarmJobError, self.build_farm_job.getSpecificJob)
-
-
-class TestBuildFarmJobSecurity(TestBuildFarmJobMixin, TestCaseWithFactory):
+
+class TestBuildFarmJobSecurity(TestBuildFarmJobBase, TestCaseWithFactory):
 
     def test_view_build_farm_job(self):
         # Anonymous access can read public builds, but not edit.
@@ -184,7 +183,7 @@
             BuildStatus.FULLYBUILT, self.build_farm_job.status)
 
 
-class TestBuildFarmJobSet(TestBuildFarmJobMixin, TestCaseWithFactory):
+class TestBuildFarmJobSet(TestBuildFarmJobBase, TestCaseWithFactory):
 
     layer = LaunchpadFunctionalLayer
 

=== modified file 'lib/lp/buildmaster/tests/test_packagebuild.py'
--- lib/lp/buildmaster/tests/test_packagebuild.py	2013-01-04 06:25:25 +0000
+++ lib/lp/buildmaster/tests/test_packagebuild.py	2013-01-07 04:25:24 +0000
@@ -78,10 +78,6 @@
         joes_ppa = self.factory.makeArchive(owner=joe, name="ppa")
         self.package_build = self.makePackageBuild(archive=joes_ppa)
 
-    def test_providesInterface(self):
-        # PackageBuild provides IPackageBuild
-        self.assertProvides(self.package_build, IPackageBuild)
-
     def test_saves_record(self):
         # A package build can be stored in the database.
         store = Store.of(self.package_build)
@@ -91,25 +87,74 @@
             PackageBuild.id == self.package_build.id).one()
         self.assertEqual(self.package_build, retrieved_build)
 
-    def test_unimplemented_methods(self):
-        # Classes deriving from PackageBuild must provide various
-        # methods.
-        self.assertRaises(
-            NotImplementedError, self.package_build.estimateDuration)
-        self.assertRaises(
-            NotImplementedError, self.package_build.verifySuccessfulUpload)
-        self.assertRaises(NotImplementedError, self.package_build.notify)
-        self.assertRaises(
-            NotImplementedError, self.package_build.handleStatus,
-            None, None, None)
-
     def test_default_values(self):
         # PackageBuild has a number of default values.
-        self.failUnlessEqual(
-            'multiverse', self.package_build.current_component.name)
         self.failUnlessEqual(None, self.package_build.distribution)
         self.failUnlessEqual(None, self.package_build.distro_series)
 
+    def test_destroySelf_removes_BuildFarmJob(self):
+        # Destroying a packagebuild also destroys the BuildFarmJob it
+        # references.
+        naked_build = removeSecurityProxy(self.package_build)
+        store = Store.of(self.package_build)
+        # Ensure build_farm_job_id is set.
+        store.flush()
+        build_farm_job_id = naked_build.build_farm_job_id
+        naked_build.destroySelf()
+        result = store.find(
+            BuildFarmJob, BuildFarmJob.id == build_farm_job_id)
+        self.assertIs(None, result.one())
+
+    def test_view_package_build(self):
+        # Anonymous access can read public builds, but not edit.
+        self.failUnlessEqual(
+            None, self.package_build.dependencies)
+        self.assertRaises(
+            Unauthorized, setattr, self.package_build,
+            'dependencies', u'my deps')
+
+    def test_edit_package_build(self):
+        # An authenticated user who belongs to the owning archive team
+        # can edit the build.
+        login_person(self.package_build.archive.owner)
+        self.package_build.dependencies = u'My deps'
+        self.failUnlessEqual(
+            u'My deps', self.package_build.dependencies)
+
+        # But other users cannot.
+        other_person = self.factory.makePerson()
+        login_person(other_person)
+        self.assertRaises(
+            Unauthorized, setattr, self.package_build,
+            'dependencies', u'my deps')
+
+    def test_admin_package_build(self):
+        # Users with edit access can update attributes.
+        login('[e-mail]')
+        self.package_build.dependencies = u'My deps'
+        self.failUnlessEqual(
+            u'My deps', self.package_build.dependencies)
+
+
+class TestPackageBuildMixin(TestCaseWithFactory):
+    """Test methods provided by PackageBuildMixin."""
+
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        super(TestPackageBuildMixin, self).setUp()
+        # BuildFarmJobMixin only operates as part of a concrete
+        # IBuildFarmJob implementation. Here we use
+        # SourcePackageRecipeBuild.
+        joe = self.factory.makePerson(name="joe")
+        joes_ppa = self.factory.makeArchive(owner=joe, name="ppa")
+        self.package_build = self.factory.makeSourcePackageRecipeBuild(
+            archive=joes_ppa)
+
+    def test_providesInterface(self):
+        # PackageBuild provides IPackageBuild
+        self.assertProvides(self.package_build, IPackageBuild)
+
     def test_log_url(self):
         # The url of the build log file is determined by the PackageBuild.
         lfa = self.factory.makeLibraryFileAlias('mybuildlog.txt')
@@ -117,8 +162,8 @@
         log_url = self.package_build.log_url
         self.failUnlessEqual(
             'http://launchpad.dev/~joe/'
-            '+archive/ppa/+build/%d/+files/mybuildlog.txt' % (
-                self.package_build.build_farm_job.id),
+            '+archive/ppa/+recipebuild/%d/+files/mybuildlog.txt' % (
+                self.package_build.id),
             log_url)
 
     def test_storeUploadLog(self):
@@ -152,45 +197,14 @@
     def test_upload_log_url(self):
         # The url of the upload log file is determined by the PackageBuild.
         Store.of(self.package_build).flush()
-        build_id = self.package_build.build_farm_job.id
         self.package_build.storeUploadLog("Some content")
         log_url = self.package_build.upload_log_url
         self.failUnlessEqual(
             'http://launchpad.dev/~joe/'
-            '+archive/ppa/+build/%d/+files/upload_%d_log.txt' % (
-                build_id, build_id),
+            '+archive/ppa/+recipebuild/%d/+files/upload_%d_log.txt' % (
+                self.package_build.id, self.package_build.build_farm_job.id),
             log_url)
 
-    def test_view_package_build(self):
-        # Anonymous access can read public builds, but not edit.
-        self.failUnlessEqual(
-            None, self.package_build.dependencies)
-        self.assertRaises(
-            Unauthorized, setattr, self.package_build,
-            'dependencies', u'my deps')
-
-    def test_edit_package_build(self):
-        # An authenticated user who belongs to the owning archive team
-        # can edit the build.
-        login_person(self.package_build.archive.owner)
-        self.package_build.dependencies = u'My deps'
-        self.failUnlessEqual(
-            u'My deps', self.package_build.dependencies)
-
-        # But other users cannot.
-        other_person = self.factory.makePerson()
-        login_person(other_person)
-        self.assertRaises(
-            Unauthorized, setattr, self.package_build,
-            'dependencies', u'my deps')
-
-    def test_admin_package_build(self):
-        # Users with edit access can update attributes.
-        login('[e-mail]')
-        self.package_build.dependencies = u'My deps'
-        self.failUnlessEqual(
-            u'My deps', self.package_build.dependencies)
-
     def test_getUploadDirLeaf(self):
         # getUploadDirLeaf returns the current time, followed by the build
         # cookie.
@@ -202,19 +216,6 @@
             '%s-%s' % (now.strftime("%Y%m%d-%H%M%S"), build_cookie),
             upload_leaf)
 
-    def test_destroySelf_removes_BuildFarmJob(self):
-        # Destroying a packagebuild also destroys the BuildFarmJob it
-        # references.
-        naked_build = removeSecurityProxy(self.package_build)
-        store = Store.of(self.package_build)
-        # Ensure build_farm_job_id is set.
-        store.flush()
-        build_farm_job_id = naked_build.build_farm_job_id
-        naked_build.destroySelf()
-        result = store.find(
-            BuildFarmJob, BuildFarmJob.id == build_farm_job_id)
-        self.assertIs(None, result.one())
-
 
 class TestPackageBuildSet(TestPackageBuildBase):
 

=== modified file 'lib/lp/code/model/sourcepackagerecipebuild.py'
--- lib/lp/code/model/sourcepackagerecipebuild.py	2013-01-07 02:40:55 +0000
+++ lib/lp/code/model/sourcepackagerecipebuild.py	2013-01-07 04:25:24 +0000
@@ -41,6 +41,7 @@
 from lp.buildmaster.model.packagebuild import (
     PackageBuild,
     PackageBuildDerived,
+    PackageBuildMixin,
     )
 from lp.code.errors import (
     BuildAlreadyPending,
@@ -74,7 +75,7 @@
 from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
 
 
-class SourcePackageRecipeBuild(PackageBuildDerived, Storm):
+class SourcePackageRecipeBuild(PackageBuildMixin, PackageBuildDerived, Storm):
 
     __storm_table__ = 'SourcePackageRecipeBuild'
 

=== modified file 'lib/lp/soyuz/model/binarypackagebuild.py'
--- lib/lp/soyuz/model/binarypackagebuild.py	2013-01-02 06:05:27 +0000
+++ lib/lp/soyuz/model/binarypackagebuild.py	2013-01-07 04:25:24 +0000
@@ -45,6 +45,7 @@
 from lp.buildmaster.model.packagebuild import (
     PackageBuild,
     PackageBuildDerived,
+    PackageBuildMixin,
     )
 from lp.services.config import config
 from lp.services.database.bulk import load_related
@@ -92,7 +93,7 @@
     )
 
 
-class BinaryPackageBuild(PackageBuildDerived, SQLBase):
+class BinaryPackageBuild(PackageBuildMixin, PackageBuildDerived, SQLBase):
     implements(IBinaryPackageBuild)
     _table = 'BinaryPackageBuild'
     _defaultOrder = 'id'

=== modified file 'lib/lp/translations/model/translationtemplatesbuild.py'
--- lib/lp/translations/model/translationtemplatesbuild.py	2012-01-02 18:22:20 +0000
+++ lib/lp/translations/model/translationtemplatesbuild.py	2013-01-07 04:25:24 +0000
@@ -18,7 +18,10 @@
     implements,
     )
 
-from lp.buildmaster.model.buildfarmjob import BuildFarmJobDerived
+from lp.buildmaster.model.buildfarmjob import (
+    BuildFarmJobDerived,
+    BuildFarmJobMixin,
+    )
 from lp.code.model.branch import Branch
 from lp.code.model.branchcollection import GenericBranchCollection
 from lp.code.model.branchjob import (
@@ -38,7 +41,7 @@
     )
 
 
-class TranslationTemplatesBuild(BuildFarmJobDerived, Storm):
+class TranslationTemplatesBuild(BuildFarmJobMixin, BuildFarmJobDerived, Storm):
     """A `BuildFarmJob` extension for translation templates builds."""
 
     implements(ITranslationTemplatesBuild)