Launchpad itself

Merge lp://staging/~michael.nelson/launchpad/536700-present-packagebuilds-in-ppa-context into lp://staging/launchpad/db-devel

536700-present-packagebuilds-in-ppa-context
Merge into db-devel

Proposed by Michael Nelson on 2010-06-14

Status:

Merged

Merged at revision:

9485

Proposed branch:

lp://staging/~michael.nelson/launchpad/536700-present-packagebuilds-in-ppa-context

Merge into:

lp://staging/launchpad/db-devel

Diff against target:

1137 lines (+424/-145) (has conflicts)

28 files modified

lib/canonical/buildd/apply-ogre-model (+0/-39)
lib/canonical/buildd/binarypackage.py (+7/-17)
lib/canonical/buildd/debian.py (+1/-40)
lib/canonical/buildd/debian/changelog (+7/-0)
lib/canonical/buildd/debian/rules (+2/-2)
lib/canonical/buildd/debian/upgrade-config (+11/-0)
lib/canonical/buildd/template-buildd-slave.conf (+0/-1)
lib/canonical/buildd/tests/buildd-slave-test.conf (+0/-1)
lib/canonical/launchpad/templates/launchpad-form.pt (+6/-3)
lib/canonical/widgets/product.py (+3/-1)
lib/lp/buildmaster/configure.zcml (+6/-0)
lib/lp/buildmaster/interfaces/packagebuild.py (+26/-1)
lib/lp/buildmaster/model/packagebuild.py (+45/-2)
lib/lp/buildmaster/tests/test_packagebuild.py (+49/-6)
lib/lp/code/browser/sourcepackagerecipe.py (+12/-3)
lib/lp/code/browser/tests/test_sourcepackagerecipe.py (+56/-7)
lib/lp/code/model/sourcepackagerecipedata.py (+2/-0)
lib/lp/registry/doc/product-widgets.txt (+45/-2)
lib/lp/registry/model/sourcepackage.py (+5/-3)
lib/lp/soyuz/configure.zcml (+5/-0)
lib/lp/soyuz/interfaces/archive.py (+6/-0)
lib/lp/soyuz/interfaces/buildrecords.py (+11/-6)
lib/lp/soyuz/model/archive.py (+14/-4)
lib/lp/soyuz/model/binarypackagebuild.py (+21/-0)
lib/lp/soyuz/tests/test_archive.py (+1/-1)
lib/lp/soyuz/tests/test_binarypackagebuild.py (+37/-0)
lib/lp/soyuz/tests/test_hasbuildrecords.py (+31/-0)
lib/lp/testing/factory.py (+15/-6)

Text conflict in lib/lp/code/browser/tests/test_sourcepackagerecipe.py

To merge this branch:

bzr merge lp://staging/~michael.nelson/launchpad/536700-present-packagebuilds-in-ppa-context

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Henning Eggers (community)	code	2010-06-14	Approve on 2010-06-16
Review via email: mp+27477@code.staging.launchpad.net

Description of the change

Please review my branch:

bzr+ssh://bazaar.launchpad.net/~michael.nelson/launchpad/536700-present-packagebuilds-in-ppa-context

revision 11006.

Test command: bin/test -vv -m test_packagebuild -m test_binarypackagebuild -m
test_hasbuildrecords

This branch does some initial work to enable PPA's to present general
IPackageBuilds rather than IBinaryPackageBuilds (bug 536700).

It adds and tests an IPackageBuildSet.getBuildsForArchive(). It adds an
optional param to the getBuildRecords() method - binary_only - which defaults
to true (current behaivour). It then updates IArchive.getBuildRecords() to
support binary_only=False.

Additionally, it adds an adapter for IBuildFarmJob->IBinaryPackageBuild in
preparation for the UI (so each build can present its title correctly).

Revision history for this message

Henning Eggers (henninge) wrote on 2010-06-15:

Download full text (19.4 KiB)

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Michael,
thank you for your fix and sorry for the delay...

Am 14.06.2010 10:35, schrieb Michael Nelson:
> Test command: bin/test -vv -m test_packagebuild -m test_binarypackagebuild -m
> test_hasbuildrecords

Yup, they pass. Thanks.

> This branch does some initial work to enable PPA's to present general
> IPackageBuilds rather than IBinaryPackageBuilds (bug 536700).

So, I assume that is the reason that no bug is attached to this branch?

>
> It adds and tests an IPackageBuildSet.getBuildsForArchive(). It adds an
> optional param to the getBuildRecords() method - binary_only - which defaults
> to true (current behaivour). It then updates IArchive.getBuildRecords() to
> support binary_only=False.
>
> Additionally, it adds an adapter for IBuildFarmJob->IBinaryPackageBuild in
> preparation for the UI (so each build can present its title correctly).
>

Very clever. ;-)

Here are my comments:

> === modified file 'lib/lp/buildmaster/configure.zcml'

OK.

> === modified file 'lib/lp/buildmaster/interfaces/packagebuild.py'

OK.

> === modified file 'lib/lp/buildmaster/model/packagebuild.py'
> --- lib/lp/buildmaster/model/packagebuild.py 2010-06-07 10:43:01 +0000
> +++ lib/lp/buildmaster/model/packagebuild.py 2010-06-14 08:35:42 +0000
> @@ -11,6 +11,7 @@
> from lazr.delegates import delegates
>
> from storm.locals import Int, Reference, Storm, Unicode
> +from storm.expr import Desc
>
> from zope.component import getUtility
> from zope.interface import classProvides, implements
> @@ -19,13 +20,16 @@
> from canonical.launchpad.browser.librarian import (
> ProxiedLibraryFileAlias)
> from canonical.launchpad.interfaces.lpstorm import IMasterStore
> +from canonical.launchpad.webapp.interfaces import (
> + IStoreSelector, MAIN_STORE, DEFAULT_FLAVOR)
>
> from lp.buildmaster.interfaces.buildbase import BuildStatus
> from lp.buildmaster.interfaces.buildfarmjob import IBuildFarmJobSource
> from lp.buildmaster.interfaces.packagebuild import (
> - IPackageBuild, IPackageBuildSource)
> + IPackageBuild, IPackageBuildSet, IPackageBuildSource)
> from lp.buildmaster.model.buildbase import handle_status_for_build, BuildBase
> -from lp.buildmaster.model.buildfarmjob import BuildFarmJobDerived
> +from lp.buildmaster.model.buildfarmjob import (
> + BuildFarmJob, BuildFarmJobDerived)
> from lp.registry.interfaces.pocket import PackagePublishingPocket
> from lp.soyuz.adapters.archivedependencies import (
> default_component_dependency_name)
> @@ -214,3 +218,40 @@
> def _handleStatus_GIVENBACK(self, librarian, slave_status, logger):
> return BuildBase._handleStatus_GIVENBACK(
> self, librarian, slave_status, logger)
> +
> +
> +class PackageBuildSet:
> + implements(IPackageBuildSet)
> +
> + def getBuildsForArchive(self, archive, status=None, pocket=None):
> + """See `IPackageBuildSet`."""
> +
> + extra_exprs = []
> +
> + # Add query clause that filters on build status if the latter is
> + # provided.

I had been told that comments are not there to reiterate what the code is
doing - the code should speak for itself. Thi...

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Michael,
thank you for your fix and sorry for the delay...

Am 14.06.2010 10:35, schrieb Michael Nelson:
> Test command: bin/test -vv -m test_packagebuild -m test_binarypackagebuild -m
> test_hasbuildrecords

Yup, they pass. Thanks.

> This branch does some initial work to enable PPA's to present general
> IPackageBuilds rather than IBinaryPackageBuilds (bug 536700).

So, I assume that is the reason that no bug is attached to this branch?

> 
> It adds and tests an IPackageBuildSet.getBuildsForArchive(). It adds an
> optional param to the getBuildRecords() method - binary_only - which defaults
> to true (current behaivour). It then updates IArchive.getBuildRecords() to
> support binary_only=False.
> 
> Additionally, it adds an adapter for IBuildFarmJob->IBinaryPackageBuild in
> preparation for the UI (so each build can present its title correctly).
>

Very clever. ;-)

Here are my comments:

> === modified file 'lib/lp/buildmaster/configure.zcml'

OK.

> === modified file 'lib/lp/buildmaster/interfaces/packagebuild.py'

OK.

> === modified file 'lib/lp/buildmaster/model/packagebuild.py'
> --- lib/lp/buildmaster/model/packagebuild.py	2010-06-07 10:43:01 +0000
> +++ lib/lp/buildmaster/model/packagebuild.py	2010-06-14 08:35:42 +0000
> @@ -11,6 +11,7 @@
>  from lazr.delegates import delegates
>  
>  from storm.locals import Int, Reference, Storm, Unicode
> +from storm.expr import Desc
>  
>  from zope.component import getUtility
>  from zope.interface import classProvides, implements
> @@ -19,13 +20,16 @@
>  from canonical.launchpad.browser.librarian import (
>      ProxiedLibraryFileAlias)
>  from canonical.launchpad.interfaces.lpstorm import IMasterStore
> +from canonical.launchpad.webapp.interfaces import (
> +        IStoreSelector, MAIN_STORE, DEFAULT_FLAVOR)
>  
>  from lp.buildmaster.interfaces.buildbase import BuildStatus
>  from lp.buildmaster.interfaces.buildfarmjob import IBuildFarmJobSource
>  from lp.buildmaster.interfaces.packagebuild import (
> -    IPackageBuild, IPackageBuildSource)
> +    IPackageBuild, IPackageBuildSet, IPackageBuildSource)
>  from lp.buildmaster.model.buildbase import handle_status_for_build, BuildBase
> -from lp.buildmaster.model.buildfarmjob import BuildFarmJobDerived
> +from lp.buildmaster.model.buildfarmjob import (
> +    BuildFarmJob, BuildFarmJobDerived)
>  from lp.registry.interfaces.pocket import PackagePublishingPocket
>  from lp.soyuz.adapters.archivedependencies import (
>      default_component_dependency_name)
> @@ -214,3 +218,40 @@
>      def _handleStatus_GIVENBACK(self, librarian, slave_status, logger):
>          return BuildBase._handleStatus_GIVENBACK(
>              self, librarian, slave_status, logger)
> +
> +
> +class PackageBuildSet:
> +    implements(IPackageBuildSet)
> +
> +    def getBuildsForArchive(self, archive, status=None, pocket=None):
> +        """See `IPackageBuildSet`."""
> +
> +        extra_exprs = []
> +
> +        # Add query clause that filters on build status if the latter is
> +        # provided.

I had been told that comments are not there to reiterate what the code is
doing - the code should speak for itself. This looks like such a case, does it
not?

> +        if status is not None:
> +            extra_exprs.append(BuildFarmJob.status == status)
> +
> +        # Add query clause that filters on pocket if the latter is provided.

Same here. The comment is just chatter. Maybe its pseudo code that you forgot
in here?

> +        if pocket:
> +            extra_exprs.append(PackageBuild.pocket == pocket)
> +
> +        store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
> +        result_set = store.find(PackageBuild,
> +            PackageBuild.archive == archive,
> +            PackageBuild.build_farm_job == BuildFarmJob.id,
> +            *extra_exprs)
> +
> +        # Ordering according status
> +        # * SUPERSEDED & All by -datecreated

What does "& All" refer to"?

> +        # * FULLYBUILT & FAILURES by -datebuilt
I would put this near the "else" to explain which you expect there.

> +        # It should present the builds in a more natural order.
This is a really useful comment line because it explains the "why" while the
"how" can be read from the code directly.

> +        if status == BuildStatus.SUPERSEDED or status is None:

What does it mean when status is None? That might be worth a comment, don't
you think?

> +            result_set.order_by(
> +                Desc(BuildFarmJob.date_created), BuildFarmJob.id)
> +        else:

Or maybe even "elif FULLYBUILT & FAILURES"? Then add an "else" that raises an
exception? But maybe that is too paranoid ...

> +            result_set.order_by(
> +                Desc(BuildFarmJob.date_finished), BuildFarmJob.id)
> +
> +        return result_set
> 
> === modified file 'lib/lp/buildmaster/tests/test_packagebuild.py'
> --- lib/lp/buildmaster/tests/test_packagebuild.py	2010-05-21 12:48:44 +0000
> +++ lib/lp/buildmaster/tests/test_packagebuild.py	2010-06-14 08:35:42 +0000
> @@ -15,9 +15,10 @@
>  
>  from canonical.testing.layers import LaunchpadFunctionalLayer
>  
> +from lp.buildmaster.interfaces.buildbase import BuildStatus
>  from lp.buildmaster.interfaces.buildfarmjob import BuildFarmJobType
>  from lp.buildmaster.interfaces.packagebuild import (
> -    IPackageBuild, IPackageBuildSource)
> +    IPackageBuild, IPackageBuildSet, IPackageBuildSource)
>  from lp.buildmaster.model.packagebuild import PackageBuild
>  from lp.buildmaster.tests.test_buildbase import TestBuildBaseMixin
>  from lp.registry.interfaces.pocket import PackagePublishingPocket
> @@ -31,15 +32,16 @@
>      only classes deriving from PackageBuild should be used.
>      """
>  
> -    def makePackageBuild(self, archive=None):
> +    def makePackageBuild(

Please move this into the factory.

> +        self, archive=None, job_type=BuildFarmJobType.PACKAGEBUILD,
> +        status=BuildStatus.NEEDSBUILD,
> +        pocket=PackagePublishingPocket.RELEASE):
>          if archive is None:
>              archive = self.factory.makeArchive()
>  
>          return getUtility(IPackageBuildSource).new(
> -            job_type=BuildFarmJobType.PACKAGEBUILD,
> -            virtualized=True,
> -            archive=archive,
> -            pocket=PackagePublishingPocket.RELEASE)
> +            job_type=job_type, virtualized=True, archive=archive,
> +            status=status, pocket=pocket)
>  
>  
>  class TestBuildBaseMethods(TestPackageBuildBase, TestBuildBaseMixin):
> @@ -176,5 +178,46 @@
>              u'My deps', self.package_build.dependencies)
>  
>  
> +class TestPackageBuildSet(TestPackageBuildBase):

It's missing from __all__. Is that intentional?

> +
> +    layer = LaunchpadFunctionalLayer
> +
> +    def setUp(self):
> +        super(TestPackageBuildSet, self).setUp()
> +        joe = self.factory.makePerson(name="joe")
> +        self.archive = self.factory.makeArchive(owner=joe, name="ppa")

What are the "joe" and "ppa" names for? You never mention them again.

> +        self.package_builds = []
> +        self.package_builds.append(
> +            self.makePackageBuild(archive=self.archive,
> +                                  pocket=PackagePublishingPocket.UPDATES))
> +        self.package_builds.append(
> +            self.makePackageBuild(archive=self.archive,
> +                                  status=BuildStatus.BUILDING))
> +        self.package_build_set = getUtility(IPackageBuildSet)
> +
> +    def test_getBuildsForArchive_all(self):
> +        # The default call without arguments returns all builds for the
> +        # archive.
> +        self.assertContentEqual(
> +            self.package_builds, self.package_build_set.getBuildsForArchive(
> +                self.archive))
> +
> +    def test_getBuildsForArchive_by_status(self):
> +        # If the status arg is used, the results will be filtered by
> +        # status.
> +        self.assertContentEqual(
> +            self.package_builds[1:],
> +            self.package_build_set.getBuildsForArchive(
> +                self.archive, status=BuildStatus.BUILDING))
> +
> +    def test_getBuildsForArchive_by_pocket(self):
> +        # If the pocket arg is used, the results will be filtered by
> +        # pocket.
> +        self.assertContentEqual(
> +            self.package_builds[:1],
> +            self.package_build_set.getBuildsForArchive(
> +                self.archive, pocket=PackagePublishingPocket.UPDATES))
> +
> +
>  def test_suite():
>      return unittest.TestLoader().loadTestsFromName(__name__)
> 
> === modified file 'lib/lp/registry/model/sourcepackage.py'
> --- lib/lp/registry/model/sourcepackage.py	2010-06-09 08:26:26 +0000
> +++ lib/lp/registry/model/sourcepackage.py	2010-06-14 08:35:42 +0000
> @@ -508,13 +508,15 @@
>          return not self.__eq__(other)
>  
>      def getBuildRecords(self, build_state=None, name=None, pocket=None,
> -                        arch_tag=None, user=None):
> +                        arch_tag=None, user=None, binary_only=True):
>          # Ignore "user", since it would not make any difference to the
>          # records returned here (private builds are only in PPA right
>          # now and this method only returns records for SPRs in a
>          # distribution).
>          # We also ignore the name parameter (required as part of the
> -        # IHasBuildRecords interface) and use our own name.
> +        # IHasBuildRecords interface) and use our own name and the
> +        # binary_only parameter as a source package can only have
> +        # binary builds.

Thanks for updating the comment.

>  
>          """See `IHasBuildRecords`"""

I know you did not introduce this but shouldn't the doc string come directly
after the definition line? I have just never seen it this way round ...

>          clauseTables = ['SourcePackageRelease',
>

> === modified file 'lib/lp/soyuz/configure.zcml'

OK. Adapters are kinda cool. ;)

> === modified file 'lib/lp/soyuz/interfaces/archive.py'

OK.

> === modified file 'lib/lp/soyuz/interfaces/buildrecords.py'

OK.

> === modified file 'lib/lp/soyuz/model/archive.py'
> --- lib/lp/soyuz/model/archive.py	2010-06-08 11:30:29 +0000
> +++ lib/lp/soyuz/model/archive.py	2010-06-14 08:35:42 +0000
> @@ -34,6 +34,7 @@
>      cursor, quote, quote_like, sqlvalues, SQLBase)
>  from canonical.launchpad.interfaces.lpstorm import ISlaveStore
>  from lp.buildmaster.interfaces.buildbase import BuildStatus
> +from lp.buildmaster.interfaces.packagebuild import IPackageBuildSet
>  from lp.buildmaster.model.buildfarmjob import BuildFarmJob
>  from lp.buildmaster.model.packagebuild import PackageBuild
>  from lp.services.job.interfaces.job import JobStatus
> @@ -65,7 +66,7 @@
>      ArchiveNotPrivate, ArchivePurpose, ArchiveStatus, CannotCopy,
>      CannotSwitchPrivacy, CannotUploadToPPA, CannotUploadToPocket,
>      DistroSeriesNotFound, IArchive, IArchiveSet, IDistributionArchive,
> -    InsufficientUploadRights, InvalidPocketForPPA,
> +    IncompatibleArguments, InsufficientUploadRights, InvalidPocketForPPA,
>      InvalidPocketForPartnerArchive, InvalidComponent, IPPA,
>      MAIN_ARCHIVE_PURPOSES, NoRightsForArchive, NoRightsForComponent,
>      NoSuchPPA, NoTokensForTeams, PocketNotFound, VersionRequiresName,
> @@ -366,12 +367,21 @@
>          return None
>  
>      def getBuildRecords(self, build_state=None, name=None, pocket=None,
> -                        arch_tag=None, user=None):
> +                        arch_tag=None, user=None, binary_only=True):
>          """See IHasBuildRecords"""
>          # Ignore "user", since anyone already accessing this archive
>          # will implicitly have permission to see it.
> -        return getUtility(IBinaryPackageBuildSet).getBuildsForArchive(
> -            self, build_state, name, pocket, arch_tag)
> +
> +        if binary_only:
> +            return getUtility(IBinaryPackageBuildSet).getBuildsForArchive(
> +                self, build_state, name, pocket, arch_tag)
> +        else:
> +            if arch_tag is not None or name is not None:
> +                raise IncompatibleArguments(
> +                    "The 'arch_tag' and 'name' parameters can be used only "
> +                    "with binary_only=True.")

I am not very good at webservice design but couldn't you also just ignore
those parameters that are not used? And document that in the interface.

Of course, this makes the webservice easier to debug, so maybe this is great...

> +            return getUtility(IPackageBuildSet).getBuildsForArchive(
> +                self, status=build_state, pocket=pocket)
>  
>      def getPublishedSources(self, name=None, version=None, status=None,
>                              distroseries=None, pocket=None,
> 
> === modified file 'lib/lp/soyuz/model/binarypackagebuild.py'
> --- lib/lp/soyuz/model/binarypackagebuild.py	2010-06-09 12:13:53 +0000
> +++ lib/lp/soyuz/model/binarypackagebuild.py	2010-06-14 08:35:42 +0000
> @@ -65,6 +65,26 @@
>      PackageUpload, PackageUploadBuild)
>  
>  
> +def get_binary_build_for_build_farm_job(build_farm_job):
> +    """Factory method to returning a binary for a build farm job."""
> +    # No need to query the db if the build_farm_job doesn't have
> +    # the correct job type.
> +    if build_farm_job.job_type != BuildFarmJobType.PACKAGEBUILD:
> +        return None
> +
> +    store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
> +    resulting_tuple =  store.find(
> +        (BinaryPackageBuild, PackageBuild, BuildFarmJob),

I think you mean this:
    resulting_tuple =  store.using(
        BinaryPackageBuild, PackageBuild, BuildFarmJob).find(
        BinaryPackageBuild,

Saves some serialization...

> +        BinaryPackageBuild.package_build == PackageBuild.id,
> +        PackageBuild.build_farm_job == BuildFarmJob.id,
> +        BuildFarmJob.id == build_farm_job.id).one()
> +
> +    if resulting_tuple is None:
> +        return None
> +
> +    return resulting_tuple[0]

... and the [0] can go away.

> +
> +
>  class BinaryPackageBuild(PackageBuildDerived, SQLBase):
>      implements(IBinaryPackageBuild)
>      _table = 'BinaryPackageBuild'
> 
> === modified file 'lib/lp/soyuz/tests/test_binarypackagebuild.py'
> --- lib/lp/soyuz/tests/test_binarypackagebuild.py	2010-06-09 12:07:58 +0000
> +++ lib/lp/soyuz/tests/test_binarypackagebuild.py	2010-06-14 08:35:42 +0000
> @@ -15,6 +15,7 @@
>  from canonical.testing import LaunchpadZopelessLayer
>  from lp.services.job.model.job import Job
>  from lp.buildmaster.interfaces.buildbase import BuildStatus
> +from lp.buildmaster.interfaces.buildfarmjob import BuildFarmJobType
>  from lp.buildmaster.interfaces.builder import IBuilderSet
>  from lp.buildmaster.interfaces.buildqueue import IBuildQueue
>  from lp.buildmaster.interfaces.packagebuild import IPackageBuild
> @@ -31,7 +32,7 @@
>  from lp.soyuz.model.processor import ProcessorFamilySet
>  from lp.soyuz.tests.soyuzbuilddhelpers import WaitingSlave
>  from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
> -from lp.testing import TestCaseWithFactory
> +from lp.testing import record_statements, TestCaseWithFactory

Ah, I see you may not know about "assertStatementCount" ?

>  
>  
>  class TestBinaryPackageBuild(TestCaseWithFactory):
> @@ -120,6 +121,45 @@
>                  self.build.build_farm_job.id),
>              self.build.log_url)
>  
> +    def test_adapt_from_build_farm_job(self):
> +        # An `IBuildFarmJob` can be adapted to an IBinaryPackageBuild
> +        # if it has the correct job type.
> +        build_farm_job = self.build.build_farm_job
> +        store = Store.of(build_farm_job)
> +        store.flush()
> +
> +        binary_package_build = IBinaryPackageBuild(build_farm_job)
> +        self.failUnlessEqual(self.build, binary_package_build)
> +
> +    def test_adapt_from_build_farm_job_wrong_type(self):
> +        # An `IBuildFarmJob` of the wrong type results is None.
> +        build_farm_job = self.build.build_farm_job
> +        removeSecurityProxy(build_farm_job).job_type = (
> +            BuildFarmJobType.RECIPEBRANCHBUILD)
> +
> +        binary_package_build = IBinaryPackageBuild(build_farm_job)
> +        self.failUnlessEqual(None, binary_package_build)
> +
> +    def test_adapt_from_build_farm_job_prefetching(self):
> +        # The package_build is prefetched for efficiency.
> +        build_farm_job = self.build.build_farm_job
> +
> +        # We clear the cache to avoid getting cached objects where
> +        # they would normally be queries.
> +        store = Store.of(build_farm_job)
> +        store.flush()
> +        store.reset()
> +
> +        binary_package_build = IBinaryPackageBuild(build_farm_job)
> +
> +        package_build, statements = record_statements(
> +            getattr, binary_package_build, 'package_build')
> +        self.failUnlessEqual(0, len(statements))

package_build = self.assertStatementCount(0,
           getattr, binary_package_build, 'package_build')

> +
> +        build_farm_job, statements = record_statements(
> +            getattr, package_build, 'build_farm_job')
> +        self.failUnlessEqual(0, len(statements))
> +
>  
>  class TestBuildUpdateDependencies(TestCaseWithFactory):
>  
> 
> === modified file 'lib/lp/soyuz/tests/test_hasbuildrecords.py'
> --- lib/lp/soyuz/tests/test_hasbuildrecords.py	2010-05-06 10:35:17 +0000
> +++ lib/lp/soyuz/tests/test_hasbuildrecords.py	2010-06-14 08:35:42 +0000
> @@ -12,6 +12,11 @@
>  
>  from lp.registry.model.sourcepackage import SourcePackage
>  from lp.buildmaster.interfaces.builder import IBuilderSet
> +from lp.buildmaster.interfaces.buildfarmjob import BuildFarmJobType
> +from lp.buildmaster.interfaces.packagebuild import (
> +    IPackageBuildSource)
> +from lp.registry.interfaces.pocket import PackagePublishingPocket
> +from lp.soyuz.interfaces.archive import IncompatibleArguments
>  from lp.soyuz.interfaces.buildrecords import IHasBuildRecords
>  from lp.soyuz.model.processor import ProcessorFamilySet
>  from lp.soyuz.tests.test_binarypackagebuild import (
> @@ -81,6 +86,28 @@
>  
>          self.context = self.publisher.distroseries.main_archive
>  
> +    def test_binary_only_false(self):
> +        # An archive can optionally return the more general
> +        # package build objects.
> +        spr_build = getUtility(IPackageBuildSource).new(
> +            job_type=BuildFarmJobType.RECIPEBRANCHBUILD, virtualized=True,
> +            archive=self.context, pocket=PackagePublishingPocket.RELEASE)

Good thing you have a method in LaunchpadObjectFactory for this now, is it
not? :-P

> +
> +        builds = self.context.getBuildRecords(binary_only=True)
> +        self.failUnlessEqual(3, builds.count())
> +
> +        builds = self.context.getBuildRecords(binary_only=False)
> +        self.failUnlessEqual(4, builds.count())
> +
> +    def test_incompatible_arguments(self):
> +        # binary_only=False is incompatible with arch_tag and name.
> +        self.failUnlessRaises(
> +            IncompatibleArguments, self.context.getBuildRecords,
> +            binary_only=False, arch_tag="anything")
> +        self.failUnlessRaises(
> +            IncompatibleArguments, self.context.getBuildRecords,
> +            binary_only=False, name="anything")
> +
>  
>  class TestBuilderHasBuildRecords(TestHasBuildRecordsInterface):
>      """Test the Builder implementation of IHasBuildRecords."""
>

This is a good branch but as I am really interested in your reply I am not
marking this as approved yet. ;-)

review needs-fixing code

Cheers,
Henning

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkwXSIQACgkQBT3oW1L17iiXggCdHIb1cM044UWvLzIaPGbHdhS4
axsAnj5XhZibyCFHghnIyw7Ttias9lnr
=jMJ3
-----END PGP SIGNATURE-----

review: Needs Fixing (code)

Revision history for this message

Michael Nelson (michael.nelson) wrote on 2010-06-15:

Download full text (19.3 KiB)

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hi Michael,
> thank you for your fix and sorry for the delay...

No problem... it was worth all the improvements below :)

>
> Am 14.06.2010 10:35, schrieb Michael Nelson:
> > Test command: bin/test -vv -m test_packagebuild -m test_binarypackagebuild
> -m
> > test_hasbuildrecords
>
> Yup, they pass. Thanks.
>
> > This branch does some initial work to enable PPA's to present general
> > IPackageBuilds rather than IBinaryPackageBuilds (bug 536700).
>
> So, I assume that is the reason that no bug is attached to this branch?

There are two now :P

>
> >
> > It adds and tests an IPackageBuildSet.getBuildsForArchive(). It adds an
> > optional param to the getBuildRecords() method - binary_only - which
> defaults
> > to true (current behaivour). It then updates IArchive.getBuildRecords() to
> > support binary_only=False.
> >
> > Additionally, it adds an adapter for IBuildFarmJob->IBinaryPackageBuild in
> > preparation for the UI (so each build can present its title correctly).
> >
>
> Very clever. ;-)
>
> Here are my comments:
>
...
> > === modified file 'lib/lp/buildmaster/model/packagebuild.py'
> > --- lib/lp/buildmaster/model/packagebuild.py 2010-06-07 10:43:01 +0000
> > +++ lib/lp/buildmaster/model/packagebuild.py 2010-06-14 08:35:42 +0000
> > @@ -214,3 +218,40 @@
> > def _handleStatus_GIVENBACK(self, librarian, slave_status, logger):
> > return BuildBase._handleStatus_GIVENBACK(
> > self, librarian, slave_status, logger)
> > +
> > +
> > +class PackageBuildSet:
> > + implements(IPackageBuildSet)
> > +
> > + def getBuildsForArchive(self, archive, status=None, pocket=None):
> > + """See `IPackageBuildSet`."""
> > +
> > + extra_exprs = []
> > +
> > + # Add query clause that filters on build status if the latter is
> > + # provided.
>
> I had been told that comments are not there to reiterate what the code is
> doing - the code should speak for itself. This looks like such a case, does it
> not?

/me must stop doing that. Removed.

>
> > + if status is not None:
> > + extra_exprs.append(BuildFarmJob.status == status)
> > +
> > + # Add query clause that filters on pocket if the latter is
> provided.
>
> Same here. The comment is just chatter. Maybe its pseudo code that you forgot
> in here?

Ditto.

>
> > + if pocket:
> > + extra_exprs.append(PackageBuild.pocket == pocket)
> > +
> > + store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
> > + result_set = store.find(PackageBuild,
> > + PackageBuild.archive == archive,
> > + PackageBuild.build_farm_job == BuildFarmJob.id,
> > + *extra_exprs)
> > +
> > + # Ordering according status
> > + # * SUPERSEDED & All by -datecreated
>
> What does "& All" refer to"?

I updated this to: When we have a set of builds that may include pending or superseded builds, we order by -date_created (as we won't always have a date_finished), otherwise we can order by -date_finished.

>
> > + # * FULLYBUILT & FAILURES by -datebuilt
> I would put this near the "else" to expla...

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Hi Michael,
> thank you for your fix and sorry for the delay...

No problem... it was worth all the improvements below :)

> 
> Am 14.06.2010 10:35, schrieb Michael Nelson:
> > Test command: bin/test -vv -m test_packagebuild -m test_binarypackagebuild
> -m
> > test_hasbuildrecords
> 
> Yup, they pass. Thanks.
> 
> > This branch does some initial work to enable PPA's to present general
> > IPackageBuilds rather than IBinaryPackageBuilds (bug 536700).
> 
> So, I assume that is the reason that no bug is attached to this branch?

There are two now :P

> 
> >
> > It adds and tests an IPackageBuildSet.getBuildsForArchive(). It adds an
> > optional param to the getBuildRecords() method - binary_only - which
> defaults
> > to true (current behaivour). It then updates IArchive.getBuildRecords() to
> > support binary_only=False.
> >
> > Additionally, it adds an adapter for IBuildFarmJob->IBinaryPackageBuild in
> > preparation for the UI (so each build can present its title correctly).
> >
> 
> Very clever. ;-)
> 
> Here are my comments:
> 
...
> > === modified file 'lib/lp/buildmaster/model/packagebuild.py'
> > --- lib/lp/buildmaster/model/packagebuild.py  2010-06-07 10:43:01 +0000
> > +++ lib/lp/buildmaster/model/packagebuild.py  2010-06-14 08:35:42 +0000
> > @@ -214,3 +218,40 @@
> >      def _handleStatus_GIVENBACK(self, librarian, slave_status, logger):
> >          return BuildBase._handleStatus_GIVENBACK(
> >              self, librarian, slave_status, logger)
> > +
> > +
> > +class PackageBuildSet:
> > +    implements(IPackageBuildSet)
> > +
> > +    def getBuildsForArchive(self, archive, status=None, pocket=None):
> > +        """See `IPackageBuildSet`."""
> > +
> > +        extra_exprs = []
> > +
> > +        # Add query clause that filters on build status if the latter is
> > +        # provided.
> 
> I had been told that comments are not there to reiterate what the code is
> doing - the code should speak for itself. This looks like such a case, does it
> not?

/me must stop doing that. Removed.

> 
> > +        if status is not None:
> > +            extra_exprs.append(BuildFarmJob.status == status)
> > +
> > +        # Add query clause that filters on pocket if the latter is
> provided.
> 
> Same here. The comment is just chatter. Maybe its pseudo code that you forgot
> in here?

Ditto.

> 
> > +        if pocket:
> > +            extra_exprs.append(PackageBuild.pocket == pocket)
> > +
> > +        store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
> > +        result_set = store.find(PackageBuild,
> > +            PackageBuild.archive == archive,
> > +            PackageBuild.build_farm_job == BuildFarmJob.id,
> > +            *extra_exprs)
> > +
> > +        # Ordering according status
> > +        # * SUPERSEDED & All by -datecreated
> 
> What does "& All" refer to"?

> 
> > +        # * FULLYBUILT & FAILURES by -datebuilt
> I would put this near the "else" to explain which you expect there.

See what you think of the above comment - it seems like one statement, but happy to change it if you disagree.

> 
> > +        # It should present the builds in a more natural order.
> This is a really useful comment line because it explains the "why" while the
> "how" can be read from the code directly.
> 
> > +        if status == BuildStatus.SUPERSEDED or status is None:
> 
> What does it mean when status is None? That might be worth a comment, don't
> you think?

Is it clearer now with the above comment? (ie. if the user does not specify a status, we find builds of all statuses).

Also, I've updated the implementation to be more specific (and hopefully readable):

# When we have a set of builds that may include pending or
        # superseded builds, we order by -date_created (as we won't                                              
        # always have a date_finished). Otherwise we can order by                                                
        # -date_finished. 
        if status is None or status in [                                                                         
            BuildStatus.NEEDSBUILD,                                                                              
            BuildStatus.BUILDING,
            BuildStatus.SUPERSEDED]:

> 
> > +            result_set.order_by(
> > +                Desc(BuildFarmJob.date_created), BuildFarmJob.id)
> > +        else:
> 
> Or maybe even "elif FULLYBUILT & FAILURES"? Then add an "else" that raises an
> exception? But maybe that is too paranoid ...

Yeah, given that status=None (ie. find all builds, irregardless of status), I don't think we need to verify with an elif here.

> 
> 
> > +            result_set.order_by(
> > +                Desc(BuildFarmJob.date_finished), BuildFarmJob.id)
> > +
> > +        return result_set
> >
> > === modified file 'lib/lp/buildmaster/tests/test_packagebuild.py'
> > --- lib/lp/buildmaster/tests/test_packagebuild.py     2010-05-21 12:48:44
> +0000
> > +++ lib/lp/buildmaster/tests/test_packagebuild.py     2010-06-14 08:35:42
> +0000
> > @@ -15,9 +15,10 @@
> >
> >  from canonical.testing.layers import LaunchpadFunctionalLayer
> >
> > +from lp.buildmaster.interfaces.buildbase import BuildStatus
> >  from lp.buildmaster.interfaces.buildfarmjob import BuildFarmJobType
> >  from lp.buildmaster.interfaces.packagebuild import (
> > -    IPackageBuild, IPackageBuildSource)
> > +    IPackageBuild, IPackageBuildSet, IPackageBuildSource)
> >  from lp.buildmaster.model.packagebuild import PackageBuild
> >  from lp.buildmaster.tests.test_buildbase import TestBuildBaseMixin
> >  from lp.registry.interfaces.pocket import PackagePublishingPocket
> > @@ -31,15 +32,16 @@
> >      only classes deriving from PackageBuild should be used.
> >      """
> >
> > -    def makePackageBuild(self, archive=None):
> > +    def makePackageBuild(
> 
> Please move this into the factory.

Hrm... did you take into account the comment above that method? It's not in the diff so you may not have seen it:

"This is not included in the launchpad test factory because only classes deriving from PackageBuild should be used."

Or maybe that's not a good reason to omit it from the factory? My reasoning was that there shouldn't be other tests outside of PackageBuild that need to create one - it should always be a derived class.

> 
> > +        self, archive=None, job_type=BuildFarmJobType.PACKAGEBUILD,
> > +        status=BuildStatus.NEEDSBUILD,
> > +        pocket=PackagePublishingPocket.RELEASE):
> >          if archive is None:
> >              archive = self.factory.makeArchive()
> >
> >          return getUtility(IPackageBuildSource).new(
> > -            job_type=BuildFarmJobType.PACKAGEBUILD,
> > -            virtualized=True,
> > -            archive=archive,
> > -            pocket=PackagePublishingPocket.RELEASE)
> > +            job_type=job_type, virtualized=True, archive=archive,
> > +            status=status, pocket=pocket)
> >
> >
> >  class TestBuildBaseMethods(TestPackageBuildBase, TestBuildBaseMixin):
> > @@ -176,5 +178,46 @@
> >              u'My deps', self.package_build.dependencies)
> >
> >
> > +class TestPackageBuildSet(TestPackageBuildBase):
> 
> It's missing from __all__. Is that intentional?

Because it's not intended to be exported (none of the test cases in this module are exported). Similar to lp.code.model.tests.test_sourcepackagerecipebuild. Or did I miss something?

> 
> > +
> > +    layer = LaunchpadFunctionalLayer
> > +
> > +    def setUp(self):
> > +        super(TestPackageBuildSet, self).setUp()
> > +        joe = self.factory.makePerson(name="joe")
> > +        self.archive = self.factory.makeArchive(owner=joe, name="ppa")
> 
> What are the "joe" and "ppa" names for? You never mention them again.

Removed (re-used from previous test case that tested urls).

> > === modified file 'lib/lp/registry/model/sourcepackage.py'
> > --- lib/lp/registry/model/sourcepackage.py    2010-06-09 08:26:26 +0000
> > +++ lib/lp/registry/model/sourcepackage.py    2010-06-14 08:35:42 +0000
> > @@ -508,13 +508,15 @@
> >          return not self.__eq__(other)
> >
> >      def getBuildRecords(self, build_state=None, name=None, pocket=None,
> > -                        arch_tag=None, user=None):
> > +                        arch_tag=None, user=None, binary_only=True):
> >          # Ignore "user", since it would not make any difference to the
> >          # records returned here (private builds are only in PPA right
> >          # now and this method only returns records for SPRs in a
> >          # distribution).
> >          # We also ignore the name parameter (required as part of the
> > -        # IHasBuildRecords interface) and use our own name.
> > +        # IHasBuildRecords interface) and use our own name and the
> > +        # binary_only parameter as a source package can only have
> > +        # binary builds.
> 
> Thanks for updating the comment.
> 
> >
> >          """See `IHasBuildRecords`"""
> 
> I know you did not introduce this but shouldn't the doc string come directly
> after the definition line? I have just never seen it this way round ...

Me either. Updated.

> 
> >          clauseTables = ['SourcePackageRelease',
> >
> 
> > === modified file 'lib/lp/soyuz/configure.zcml'
> 
> OK. Adapters are kinda cool. ;)
> 
> > === modified file 'lib/lp/soyuz/interfaces/archive.py'
> 
> OK.
> 
> > === modified file 'lib/lp/soyuz/interfaces/buildrecords.py'
> 
> OK.
> 
> > === modified file 'lib/lp/soyuz/model/archive.py'
> > --- lib/lp/soyuz/model/archive.py     2010-06-08 11:30:29 +0000
> > +++ lib/lp/soyuz/model/archive.py     2010-06-14 08:35:42 +0000
> > @@ -34,6 +34,7 @@
> >      cursor, quote, quote_like, sqlvalues, SQLBase)
> >  from canonical.launchpad.interfaces.lpstorm import ISlaveStore
> >  from lp.buildmaster.interfaces.buildbase import BuildStatus
> > +from lp.buildmaster.interfaces.packagebuild import IPackageBuildSet
> >  from lp.buildmaster.model.buildfarmjob import BuildFarmJob
> >  from lp.buildmaster.model.packagebuild import PackageBuild
> >  from lp.services.job.interfaces.job import JobStatus
> > @@ -65,7 +66,7 @@
> >      ArchiveNotPrivate, ArchivePurpose, ArchiveStatus, CannotCopy,
> >      CannotSwitchPrivacy, CannotUploadToPPA, CannotUploadToPocket,
> >      DistroSeriesNotFound, IArchive, IArchiveSet, IDistributionArchive,
> > -    InsufficientUploadRights, InvalidPocketForPPA,
> > +    IncompatibleArguments, InsufficientUploadRights, InvalidPocketForPPA,
> >      InvalidPocketForPartnerArchive, InvalidComponent, IPPA,
> >      MAIN_ARCHIVE_PURPOSES, NoRightsForArchive, NoRightsForComponent,
> >      NoSuchPPA, NoTokensForTeams, PocketNotFound, VersionRequiresName,
> > @@ -366,12 +367,21 @@
> >          return None
> >
> >      def getBuildRecords(self, build_state=None, name=None, pocket=None,
> > -                        arch_tag=None, user=None):
> > +                        arch_tag=None, user=None, binary_only=True):
> >          """See IHasBuildRecords"""
> >          # Ignore "user", since anyone already accessing this archive
> >          # will implicitly have permission to see it.
> > -        return getUtility(IBinaryPackageBuildSet).getBuildsForArchive(
> > -            self, build_state, name, pocket, arch_tag)
> > +
> > +        if binary_only:
> > +            return getUtility(IBinaryPackageBuildSet).getBuildsForArchive(
> > +                self, build_state, name, pocket, arch_tag)
> > +        else:
> > +            if arch_tag is not None or name is not None:
> > +                raise IncompatibleArguments(
> > +                    "The 'arch_tag' and 'name' parameters can be used only
> "
> > +                    "with binary_only=True.")
> 
> I am not very good at webservice design but couldn't you also just ignore
> those parameters that are not used? And document that in the interface.

Yes you can, but here the inteface (and API) is defined once in IHasBuildRecords.getBuildRecords() and included for multiple implementations.

> 
> Of course, this makes the webservice easier to debug, so maybe this is
> great...
> 
> > +            return getUtility(IPackageBuildSet).getBuildsForArchive(
> > +                self, status=build_state, pocket=pocket)
> >
> >      def getPublishedSources(self, name=None, version=None, status=None,
> >                              distroseries=None, pocket=None,
> >
> > === modified file 'lib/lp/soyuz/model/binarypackagebuild.py'
> > --- lib/lp/soyuz/model/binarypackagebuild.py  2010-06-09 12:13:53 +0000
> > +++ lib/lp/soyuz/model/binarypackagebuild.py  2010-06-14 08:35:42 +0000
> > @@ -65,6 +65,26 @@
> >      PackageUpload, PackageUploadBuild)
> >
> >
> > +def get_binary_build_for_build_farm_job(build_farm_job):
> > +    """Factory method to returning a binary for a build farm job."""
> > +    # No need to query the db if the build_farm_job doesn't have
> > +    # the correct job type.
> > +    if build_farm_job.job_type != BuildFarmJobType.PACKAGEBUILD:
> > +        return None
> > +
> > +    store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
> > +    resulting_tuple =  store.find(
> > +        (BinaryPackageBuild, PackageBuild, BuildFarmJob),
> 
> I think you mean this:
>     resulting_tuple =  store.using(
>         BinaryPackageBuild, PackageBuild, BuildFarmJob).find(
>         BinaryPackageBuild,
> 
> Saves some serialization...

They are actually different. I'm specifying the find-spec above (ie. the tables/expressions that are *returned* by the find, where as aiui 'using' is for explicitly declaring joins (usually when you need a LeftJoin or similar). At least, that's the only time I've needed to use using.

Anyway, to make this difference clearer, I've done:

find_spec = (BinaryPackageBuild, PackageBuild, BuildFarmJob)                                                 
    resulting_tuple = store.find(
        find_spec,                                                                                               
        BinaryPackageBuild.package_build == PackageBuild.id,                                                     
        PackageBuild.build_farm_job == BuildFarmJob.id,                                                          
        BuildFarmJob.id == build_farm_job.id).one()

See what you think.

> 
> > +        BinaryPackageBuild.package_build == PackageBuild.id,
> > +        PackageBuild.build_farm_job == BuildFarmJob.id,
> > +        BuildFarmJob.id == build_farm_job.id).one()
> > +
> > +    if resulting_tuple is None:
> > +        return None
> > +
> > +    return resulting_tuple[0]
> 
> ... and the [0] can go away.

erm, really? I'm confused now. The whole point is to pre-load the data... see the test:

test_adapt_from_build_farm_job_prefetching

If there's a way to get that test to past without that find_spec, I'm keen to hear about it! :)

>
> > +
> > +
> >  class BinaryPackageBuild(PackageBuildDerived, SQLBase):
> >      implements(IBinaryPackageBuild)
> >      _table = 'BinaryPackageBuild'
> >
> > === modified file 'lib/lp/soyuz/tests/test_binarypackagebuild.py'
> > --- lib/lp/soyuz/tests/test_binarypackagebuild.py     2010-06-09 12:07:58
> +0000
> > +++ lib/lp/soyuz/tests/test_binarypackagebuild.py     2010-06-14 08:35:42
> +0000
> > @@ -15,6 +15,7 @@
> >  from canonical.testing import LaunchpadZopelessLayer
> >  from lp.services.job.model.job import Job
> >  from lp.buildmaster.interfaces.buildbase import BuildStatus
> > +from lp.buildmaster.interfaces.buildfarmjob import BuildFarmJobType
> >  from lp.buildmaster.interfaces.builder import IBuilderSet
> >  from lp.buildmaster.interfaces.buildqueue import IBuildQueue
> >  from lp.buildmaster.interfaces.packagebuild import IPackageBuild
> > @@ -31,7 +32,7 @@
> >  from lp.soyuz.model.processor import ProcessorFamilySet
> >  from lp.soyuz.tests.soyuzbuilddhelpers import WaitingSlave
> >  from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
> > -from lp.testing import TestCaseWithFactory
> > +from lp.testing import record_statements, TestCaseWithFactory
> 
> Ah, I see you may not know about "assertStatementCount" ?

No - I'd only used record_statements... great, and updated.

> > === modified file 'lib/lp/soyuz/tests/test_hasbuildrecords.py'
> > --- lib/lp/soyuz/tests/test_hasbuildrecords.py        2010-05-06 10:35:17
> +0000
> > +++ lib/lp/soyuz/tests/test_hasbuildrecords.py        2010-06-14 08:35:42
> +0000
> > @@ -12,6 +12,11 @@
> >
> >  from lp.registry.model.sourcepackage import SourcePackage
> >  from lp.buildmaster.interfaces.builder import IBuilderSet
> > +from lp.buildmaster.interfaces.buildfarmjob import BuildFarmJobType
> > +from lp.buildmaster.interfaces.packagebuild import (
> > +    IPackageBuildSource)
> > +from lp.registry.interfaces.pocket import PackagePublishingPocket
> > +from lp.soyuz.interfaces.archive import IncompatibleArguments
> >  from lp.soyuz.interfaces.buildrecords import IHasBuildRecords
> >  from lp.soyuz.model.processor import ProcessorFamilySet
> >  from lp.soyuz.tests.test_binarypackagebuild import (
> > @@ -81,6 +86,28 @@
> >
> >          self.context = self.publisher.distroseries.main_archive
> >
> > +    def test_binary_only_false(self):
> > +        # An archive can optionally return the more general
> > +        # package build objects.
> > +        spr_build = getUtility(IPackageBuildSource).new(
> > +            job_type=BuildFarmJobType.RECIPEBRANCHBUILD, virtualized=True,
> > +            archive=self.context, pocket=PackagePublishingPocket.RELEASE)
> 
> Good thing you have a method in LaunchpadObjectFactory for this now, is it
> not? :-P

Gar - sprung! I'm still not sure that it belongs in the factory as we shouldn't ever be creating package builds on their own (without their related BinaryPackageBuild or SPRecipeBuild) (for the same reason, there's no makeBuildFarmJob in the factory. The reason this test does so is because currently there are no other implementations to use. I've added a comment to that effect:

# Until we have different IBuildFarmJob types implemented, we                                            
        # can only test this by creating a lone PackageBuild of a                                                
        # different type.

But let me know if you're still keen for it to be a factory method.

> 
> > +
> > +        builds = self.context.getBuildRecords(binary_only=True)
> > +        self.failUnlessEqual(3, builds.count())
> > +
> > +        builds = self.context.getBuildRecords(binary_only=False)
> > +        self.failUnlessEqual(4, builds.count())
> > +
> > +    def test_incompatible_arguments(self):
> > +        # binary_only=False is incompatible with arch_tag and name.
> > +        self.failUnlessRaises(
> > +            IncompatibleArguments, self.context.getBuildRecords,
> > +            binary_only=False, arch_tag="anything")
> > +        self.failUnlessRaises(
> > +            IncompatibleArguments, self.context.getBuildRecords,
> > +            binary_only=False, name="anything")
> > +
> >
> >  class TestBuilderHasBuildRecords(TestHasBuildRecordsInterface):
> >      """Test the Builder implementation of IHasBuildRecords."""
> >
> 
> This is a good branch but as I am really interested in your reply I am not
> marking this as approved yet. ;-)

OK, let me know what you think. Thanks Henning!

Revision history for this message

Henning Eggers (henninge) wrote on 2010-06-16:

As discussed on mumble:

- Please make sure the indention is less confusing by using an intermediate variable for the states, e.g. "unfinished_states".

+ if status is None or status in [
+ BuildStatus.NEEDSBUILD,
+ BuildStatus.BUILDING,
+ BuildStatus.SUPERSEDED]:
result_set.order_by(
Desc(BuildFarmJob.date_created), BuildFarmJob.id)