Merge lp:~james-w/launchpad/fix-publishing-getbyid into lp:launchpad

[James Westby]

Fix the API of IPublishingSet.getByIdAndArchive.

Hi,

This fixes IPublishingSet.getByIdAndArchive to have a more
usual API. It used to return a ResultSet, now we just call
.one() on it and return None if not found.

We also fix the one caller, which was not using .one() either.

In addition I added a bunch of tests for the method.

In the course of doing that I added some methods to the factory
for binary-type soyuz-stuff that wasn't in the factory before.
It's not complete, but when someone needs the extra arguments
they can add them.

While doing that I made a few cleanups elsewhere to consolidate
code.

Thanks,

James

[Jelmer Vernooij (jelmer)]

Hi James,

Thanks for the cleanups.

Was changing
lib/lp/soyuz/scripts/tests/upload_test_files/drdsl_1.2.0.orig.tar.gz intentional ?

The class definition of PublishingSetTests needs to be preceded by two empty lines.

There seems to be quite a bit of duplication in the various methods in PublishingSetTests, perhaps some of that code could be moved into a setUp() method.

I'm bothered by the size of the test factory and its lack of tests. Seeing it extended further worries me a bit, though I don't have any easy alternatives.

review: Needs Information (code)

[Jonathan Lange (jml)]

On Thu, Jun 24, 2010 at 10:40 AM, Jelmer Vernooij <email address hidden> wrote:
...
> I'm bothered by the size of the test factory and its lack of tests. Seeing it extended further worries me a bit, though I don't have any easy alternatives.

I feel guilty about that. The test factory is roughly speaking my
fault. When I made the thing that would later become it, it lacked
tests, since then it has grown incrementally.

jml

[James Westby (james-w)]

> Hi James,
>
> Thanks for the cleanups.
>
> Was changing
> lib/lp/soyuz/scripts/tests/upload_test_files/drdsl_1.2.0.orig.tar.gz
> intentional ?

No, some make target or something seems to modify it here.

> The class definition of PublishingSetTests needs to be preceded by two empty
> lines.

Changed, thanks.

> There seems to be quite a bit of duplication in the various methods in
> PublishingSetTests, perhaps some of that code could be moved into a setUp()
> method.

Changed, thanks.

> I'm bothered by the size of the test factory and its lack of tests. Seeing it
> extended further worries me a bit, though I don't have any easy alternatives.

It's possible, but tedious to test the factory itself. We could start doing that?

Thanks,

James

[Robert Collins (lifeless)]

Jelmer, the test fixture discussion in bzr may help in terms of giving
you some ideas.

launchpad already has a fixture supporting thing; it might need tweaking.

But basically, rather than one big factory, many small ones, to the
extent of one per thing you want to create, not one to create many
things.

-Rob

[Jelmer Vernooij (jelmer)]

review: Approve (code)

[Jelmer Vernooij (jelmer)]

> > I'm bothered by the size of the test factory and its lack of tests. Seeing
> it
> > extended further worries me a bit, though I don't have any easy
> alternatives.
>
> It's possible, but tedious to test the factory itself. We could start doing
> that?
Yeah, it would be nice to do some more testing of the factory, perhaps in preparation of refactoring it into smaller bits as Rob suggests. The success of the test factory and the fact that it's used everywhere will probably make refactoring tedious though...

=== modified file 'lib/lp/registry/tests/test_distroseries.py'
--- lib/lp/registry/tests/test_distroseries.py	2010-05-13 19:24:12 +0000
+++ lib/lp/registry/tests/test_distroseries.py	2010-07-15 14:23:42 +0000
@@ -233,7 +233,7 @@
         sourcepackagename = self.factory.makeSourcePackageName(name)
         self.factory.makeSourcePackagePublishingHistory(
             sourcepackagename=sourcepackagename, distroseries=self.series,
-            component=component, section=section)
+            component=component, section_name=section)
         source_package = self.factory.makeSourcePackage(
             sourcepackagename=sourcepackagename, distroseries=self.series)
         if heat is not None:

=== modified file 'lib/lp/soyuz/browser/archive.py'
--- lib/lp/soyuz/browser/archive.py	2010-06-30 15:58:24 +0000
+++ lib/lp/soyuz/browser/archive.py	2010-07-15 14:23:42 +0000
@@ -201,12 +201,8 @@
 
         # The ID is not enough on its own to identify the publication,
         # we need to make sure it matches the context archive as well.
-        results = getUtility(IPublishingSet).getByIdAndArchive(
+        return getUtility(IPublishingSet).getByIdAndArchive(
             pub_id, self.context, source)
-        if results.count() == 1:
-            return results[0]
-
-        return None
 
     @stepthrough('+binaryhits')
     def traverse_binaryhits(self, name_str):

=== modified file 'lib/lp/soyuz/model/publishing.py'
--- lib/lp/soyuz/model/publishing.py	2010-06-22 16:08:05 +0000
+++ lib/lp/soyuz/model/publishing.py	2010-07-15 14:23:42 +0000
@@ -1266,7 +1266,7 @@
         return Store.of(archive).find(
             baseclass,
             baseclass.id == id,
-            baseclass.archive == archive.id)
+            baseclass.archive == archive.id).one()
 
     def _extractIDs(self, one_or_more_source_publications):
         """Return a list of database IDs for the given list or single object.

=== modified file 'lib/lp/soyuz/tests/test_publishing.py'
--- lib/lp/soyuz/tests/test_publishing.py	2010-05-05 14:50:42 +0000
+++ lib/lp/soyuz/tests/test_publishing.py	2010-07-15 14:23:42 +0000
@@ -19,7 +19,8 @@
 from canonical.database.constants import UTC_NOW
 from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
 from canonical.launchpad.webapp.interfaces import NotFoundError
-from canonical.testing import LaunchpadZopelessLayer
+from canonical.testing import (
+    DatabaseFunctionalLayer, LaunchpadZopelessLayer)
 from lp.archivepublisher.config import Config
 from lp.archivepublisher.diskpool import DiskPool
 from lp.buildmaster.interfaces.buildbase import BuildStatus
@@ -38,7 +39,7 @@
 from lp.soyuz.interfaces.component import IComponentSet
 from lp.soyuz.interfaces.section import ISectionSet
 from lp.soyuz.interfaces.publishing import (
-    PackagePublishingPriority, PackagePublishingStatus)
+    IPublishingSet, PackagePublishingPriority, PackagePublishingStatus)
 from lp.soyuz.interfaces.queue import PackageUploadStatus
 from canonical.launchpad.scripts import FakeLogger
 from lp.testing import TestCaseWithFactory
@@ -975,5 +976,61 @@
         self.assertEquals(self.sparc_distroarch, builds[1].distro_arch_series)
 
 
+class PublishingSetTests(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(PublishingSetTests, self).setUp()
+        self.distroseries = self.factory.makeDistroSeries()
+        self.archive = self.factory.makeArchive(
+            distribution=self.distroseries.distribution)
+        self.publishing = self.factory.makeSourcePackagePublishingHistory(
+            distroseries=self.distroseries, archive=self.archive)
+        self.publishing_set = getUtility(IPublishingSet)
+
+    def test_getByIdAndArchive_finds_record(self):
+        record = self.publishing_set.getByIdAndArchive(
+            self.publishing.id, self.archive)
+        self.assertEqual(self.publishing, record)
+
+    def test_getByIdAndArchive_finds_record_explicit_source(self):
+        record = self.publishing_set.getByIdAndArchive(
+            self.publishing.id, self.archive, source=True)
+        self.assertEqual(self.publishing, record)
+
+    def test_getByIdAndArchive_wrong_archive(self):
+        wrong_archive = self.factory.makeArchive()
+        record = self.publishing_set.getByIdAndArchive(
+            self.publishing.id, wrong_archive)
+        self.assertEqual(None, record)
+
+    def makeBinaryPublishing(self):
+        distroarchseries = self.factory.makeDistroArchSeries(
+            distroseries=self.distroseries)
+        binary_publishing = self.factory.makeBinaryPackagePublishingHistory(
+            archive=self.archive, distroarchseries=distroarchseries)
+        return binary_publishing
+
+    def test_getByIdAndArchive_wrong_type(self):
+        self.makeBinaryPublishing()
+        record = self.publishing_set.getByIdAndArchive(
+            self.publishing.id, self.archive, source=False)
+        self.assertEqual(None, record)
+
+    def test_getByIdAndArchive_finds_binary(self):
+        binary_publishing = self.makeBinaryPublishing()
+        record = self.publishing_set.getByIdAndArchive(
+            binary_publishing.id, self.archive, source=False)
+        self.assertEqual(binary_publishing, record)
+
+    def test_getByIdAndArchive_binary_wrong_archive(self):
+        binary_publishing = self.makeBinaryPublishing()
+        wrong_archive = self.factory.makeArchive()
+        record = self.publishing_set.getByIdAndArchive(
+            binary_publishing.id, wrong_archive, source=False)
+        self.assertEqual(None, record)
+
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2010-07-14 19:33:16 +0000
+++ lib/lp/testing/factory.py	2010-07-15 14:23:42 +0000
@@ -132,17 +132,22 @@
 from lp.services.worlddata.interfaces.country import ICountrySet
 from lp.services.worlddata.interfaces.language import ILanguageSet
 
+from lp.soyuz.adapters.packagelocation import PackageLocation
 from lp.soyuz.interfaces.archive import (
     default_name_by_purpose, IArchiveSet, ArchivePurpose)
-from lp.soyuz.adapters.packagelocation import PackageLocation
 from lp.soyuz.interfaces.binarypackagebuild import IBinaryPackageBuildSet
+from lp.soyuz.interfaces.binarypackagerelease import BinaryPackageFormat
 from lp.soyuz.interfaces.component import IComponentSet
 from lp.soyuz.interfaces.packageset import IPackagesetSet
 from lp.soyuz.interfaces.processor import IProcessorFamilySet
-from lp.soyuz.interfaces.publishing import PackagePublishingStatus
+from lp.soyuz.interfaces.publishing import (
+    PackagePublishingPriority, PackagePublishingStatus)
 from lp.soyuz.interfaces.section import ISectionSet
+from lp.soyuz.model.binarypackagename import BinaryPackageName
+from lp.soyuz.model.binarypackagerelease import BinaryPackageRelease
 from lp.soyuz.model.processor import ProcessorFamily, ProcessorFamilySet
-from lp.soyuz.model.publishing import SourcePackagePublishingHistory
+from lp.soyuz.model.publishing import (
+    BinaryPackagePublishingHistory, SourcePackagePublishingHistory)
 
 from lp.testing import run_with_login, time_counter, login, logout, temp_dir
 
@@ -2199,9 +2204,10 @@
 
     def makeSourcePackageRelease(self, archive=None, sourcepackagename=None,
                                  distroseries=None, maintainer=None,
-                                 creator=None, component=None, section=None,
-                                 urgency=None, version=None,
-                                 builddepends=None, builddependsindep=None,
+                                 creator=None, component=None,
+                                 section_name=None, urgency=None,
+                                 version=None, builddepends=None,
+                                 builddependsindep=None,
                                  build_conflicts=None,
                                  build_conflicts_indep=None,
                                  architecturehintlist='all',
@@ -2236,9 +2242,7 @@
         if urgency is None:
             urgency = self.getAnySourcePackageUrgency()
 
-        if section is None:
-            section = self.getUniqueString('section')
-        section = getUtility(ISectionSet).ensure(section)
+        section = self.makeSection(name=section_name)
 
         if maintainer is None:
             maintainer = self.makePerson()
@@ -2324,8 +2328,9 @@
     def makeSourcePackagePublishingHistory(self, sourcepackagename=None,
                                            distroseries=None, maintainer=None,
                                            creator=None, component=None,
-                                           section=None, urgency=None,
-                                           version=None, archive=None,
+                                           section_name=None,
+                                           urgency=None, version=None,
+                                           archive=None,
                                            builddepends=None,
                                            builddependsindep=None,
                                            build_conflicts=None,
@@ -2365,7 +2370,7 @@
             distroseries=distroseries,
             maintainer=maintainer,
             creator=creator, component=component,
-            section=section,
+            section_name=section_name,
             urgency=urgency,
             version=version,
             builddepends=builddepends,
@@ -2394,6 +2399,97 @@
         # of SSPPH and other useful attributes.
         return SourcePackagePublishingHistory.get(sspph.id)
 
+    def makeBinaryPackagePublishingHistory(self, binarypackagerelease=None,
+                                           distroarchseries=None,
+                                           component=None, section_name=None,
+                                           priority=None, status=None,
+                                           scheduleddeletiondate=None,
+                                           dateremoved=None,
+                                           pocket=None, archive=None):
+        """Make a `BinaryPackagePublishingHistory`."""
+        if distroarchseries is None:
+            if archive is None:
+                distribution = None
+            else:
+                distribution = archive.distribution
+            distroseries = self.makeDistroSeries(distribution=distribution)
+            distroarchseries = self.makeDistroArchSeries(
+                distroseries=distroseries)
+
+        if archive is None:
+            archive = self.makeArchive(
+                distribution=distroarchseries.distroseries.distribution,
+                purpose=ArchivePurpose.PRIMARY)
+
+        if pocket is None:
+            pocket = self.getAnyPocket()
+
+        if status is None:
+            status = PackagePublishingStatus.PENDING
+
+        if priority is None:
+            priority = PackagePublishingPriority.OPTIONAL
+
+        bpr = self.makeBinaryPackageRelease(
+            component=component,
+            section_name=section_name,
+            priority=priority)
+
+        return BinaryPackagePublishingHistory(
+            distroarchseries=distroarchseries,
+            binarypackagerelease=bpr,
+            component=bpr.component,
+            section=bpr.section,
+            status=status,
+            dateremoved=dateremoved,
+            scheduleddeletiondate=scheduleddeletiondate,
+            pocket=pocket,
+            priority=priority,
+            archive=archive)
+
+    def makeBinaryPackageName(self, name=None):
+        if name is None:
+            name = self.getUniqueString("binarypackage")
+        return BinaryPackageName(name=name)
+
+    def makeBinaryPackageRelease(self, binarypackagename=None,
+                                 version=None, build=None,
+                                 binpackageformat=None, component=None,
+                                 section_name=None, priority=None,
+                                 architecturespecific=False,
+                                 summary=None, description=None):
+        """Make a `BinaryPackageRelease`."""
+        if binarypackagename is None:
+            binarypackagename = self.makeBinaryPackageName()
+        if version is None:
+            version = self.getUniqueString("version")
+        if build is None:
+            build = self.makeBinaryPackageBuild()
+        if binpackageformat is None:
+            binpackageformat = BinaryPackageFormat.DEB
+        if component is None:
+            component = self.makeComponent()
+        section = self.makeSection(name=section_name)
+        if priority is None:
+            priority = PackagePublishingPriority.OPTIONAL
+        if summary is None:
+            summary = self.getUniqueString("summary")
+        if description is None:
+            description = self.getUniqueString("description")
+        return BinaryPackageRelease(binarypackagename=binarypackagename,
+                                    version=version, build=build,
+                                    binpackageformat=binpackageformat,
+                                    component=component, section=section,
+                                    priority=priority, summary=summary,
+                                    description=description,
+                                    architecturespecific=architecturespecific)
+
+    def makeSection(self, name=None):
+        """Make a `Section`."""
+        if name is None:
+            name = self.getUniqueString('section')
+        return getUtility(ISectionSet).ensure(name)
+
     def makePackageset(self, name=None, description=None, owner=None,
                        packages=(), distroseries=None):
         """Make an `IPackageset`."""