Launchpad itself

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

  1. fix-publishing-getbyid
  2. Merge into devel
Proposed by James Westby
Status: Merged
Approved by: Jelmer Vernooij
Approved revision: no longer in the source branch.
Merged at revision: 11135
Proposed branch: lp:~james-w/launchpad/fix-publishing-getbyid
Merge into: lp:launchpad
Diff against target: 302 lines (+170/-21)
5 files modified
lib/lp/registry/tests/test_distroseries.py (+1/-1)
lib/lp/soyuz/browser/archive.py (+1/-5)
lib/lp/soyuz/model/publishing.py (+1/-1)
lib/lp/soyuz/tests/test_publishing.py (+59/-2)
lib/lp/testing/factory.py (+108/-12)
To merge this branch: bzr merge lp:~james-w/launchpad/fix-publishing-getbyid
Reviewer Review Type Date Requested Status
Jelmer Vernooij (community) code Approve
Review via email: mp+28359@code.launchpad.net

Commit message

Fix the API of IPublishingSet.getByIdAndArchive.

Description of the change

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

To post a comment you must log in.
Revision history for this message
Jelmer Vernooij (jelmer) wrote :

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)
Revision history for this message
Jonathan Lange (jml) wrote :

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

Revision history for this message
James Westby (james-w) wrote :

> 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

Revision history for this message
Robert Collins (lifeless) wrote :

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

Revision history for this message
Jelmer Vernooij (jelmer) :
review: Approve (code)
Revision history for this message
Jelmer Vernooij (jelmer) 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.
>
> 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...

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
233 sourcepackagename = self.factory.makeSourcePackageName(name)233 sourcepackagename = self.factory.makeSourcePackageName(name)
234 self.factory.makeSourcePackagePublishingHistory(234 self.factory.makeSourcePackagePublishingHistory(
235 sourcepackagename=sourcepackagename, distroseries=self.series,235 sourcepackagename=sourcepackagename, distroseries=self.series,
236 component=component, section=section)236 component=component, section_name=section)
237 source_package = self.factory.makeSourcePackage(237 source_package = self.factory.makeSourcePackage(
238 sourcepackagename=sourcepackagename, distroseries=self.series)238 sourcepackagename=sourcepackagename, distroseries=self.series)
239 if heat is not None:239 if heat is not None:
240240
=== 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 @@
201201
202 # The ID is not enough on its own to identify the publication,202 # The ID is not enough on its own to identify the publication,
203 # we need to make sure it matches the context archive as well.203 # we need to make sure it matches the context archive as well.
204 results = getUtility(IPublishingSet).getByIdAndArchive(204 return getUtility(IPublishingSet).getByIdAndArchive(
205 pub_id, self.context, source)205 pub_id, self.context, source)
206 if results.count() == 1:
207 return results[0]
208
209 return None
210206
211 @stepthrough('+binaryhits')207 @stepthrough('+binaryhits')
212 def traverse_binaryhits(self, name_str):208 def traverse_binaryhits(self, name_str):
213209
=== 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 @@
1266 return Store.of(archive).find(1266 return Store.of(archive).find(
1267 baseclass,1267 baseclass,
1268 baseclass.id == id,1268 baseclass.id == id,
1269 baseclass.archive == archive.id)1269 baseclass.archive == archive.id).one()
12701270
1271 def _extractIDs(self, one_or_more_source_publications):1271 def _extractIDs(self, one_or_more_source_publications):
1272 """Return a list of database IDs for the given list or single object.1272 """Return a list of database IDs for the given list or single object.
12731273
=== 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 @@
19from canonical.database.constants import UTC_NOW19from canonical.database.constants import UTC_NOW
20from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet20from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
21from canonical.launchpad.webapp.interfaces import NotFoundError21from canonical.launchpad.webapp.interfaces import NotFoundError
22from canonical.testing import LaunchpadZopelessLayer22from canonical.testing import (
23 DatabaseFunctionalLayer, LaunchpadZopelessLayer)
23from lp.archivepublisher.config import Config24from lp.archivepublisher.config import Config
24from lp.archivepublisher.diskpool import DiskPool25from lp.archivepublisher.diskpool import DiskPool
25from lp.buildmaster.interfaces.buildbase import BuildStatus26from lp.buildmaster.interfaces.buildbase import BuildStatus
@@ -38,7 +39,7 @@
38from lp.soyuz.interfaces.component import IComponentSet39from lp.soyuz.interfaces.component import IComponentSet
39from lp.soyuz.interfaces.section import ISectionSet40from lp.soyuz.interfaces.section import ISectionSet
40from lp.soyuz.interfaces.publishing import (41from lp.soyuz.interfaces.publishing import (
41 PackagePublishingPriority, PackagePublishingStatus)42 IPublishingSet, PackagePublishingPriority, PackagePublishingStatus)
42from lp.soyuz.interfaces.queue import PackageUploadStatus43from lp.soyuz.interfaces.queue import PackageUploadStatus
43from canonical.launchpad.scripts import FakeLogger44from canonical.launchpad.scripts import FakeLogger
44from lp.testing import TestCaseWithFactory45from lp.testing import TestCaseWithFactory
@@ -975,5 +976,61 @@
975 self.assertEquals(self.sparc_distroarch, builds[1].distro_arch_series)976 self.assertEquals(self.sparc_distroarch, builds[1].distro_arch_series)
976977
977978
979class PublishingSetTests(TestCaseWithFactory):
980
981 layer = DatabaseFunctionalLayer
982
983 def setUp(self):
984 super(PublishingSetTests, self).setUp()
985 self.distroseries = self.factory.makeDistroSeries()
986 self.archive = self.factory.makeArchive(
987 distribution=self.distroseries.distribution)
988 self.publishing = self.factory.makeSourcePackagePublishingHistory(
989 distroseries=self.distroseries, archive=self.archive)
990 self.publishing_set = getUtility(IPublishingSet)
991
992 def test_getByIdAndArchive_finds_record(self):
993 record = self.publishing_set.getByIdAndArchive(
994 self.publishing.id, self.archive)
995 self.assertEqual(self.publishing, record)
996
997 def test_getByIdAndArchive_finds_record_explicit_source(self):
998 record = self.publishing_set.getByIdAndArchive(
999 self.publishing.id, self.archive, source=True)
1000 self.assertEqual(self.publishing, record)
1001
1002 def test_getByIdAndArchive_wrong_archive(self):
1003 wrong_archive = self.factory.makeArchive()
1004 record = self.publishing_set.getByIdAndArchive(
1005 self.publishing.id, wrong_archive)
1006 self.assertEqual(None, record)
1007
1008 def makeBinaryPublishing(self):
1009 distroarchseries = self.factory.makeDistroArchSeries(
1010 distroseries=self.distroseries)
1011 binary_publishing = self.factory.makeBinaryPackagePublishingHistory(
1012 archive=self.archive, distroarchseries=distroarchseries)
1013 return binary_publishing
1014
1015 def test_getByIdAndArchive_wrong_type(self):
1016 self.makeBinaryPublishing()
1017 record = self.publishing_set.getByIdAndArchive(
1018 self.publishing.id, self.archive, source=False)
1019 self.assertEqual(None, record)
1020
1021 def test_getByIdAndArchive_finds_binary(self):
1022 binary_publishing = self.makeBinaryPublishing()
1023 record = self.publishing_set.getByIdAndArchive(
1024 binary_publishing.id, self.archive, source=False)
1025 self.assertEqual(binary_publishing, record)
1026
1027 def test_getByIdAndArchive_binary_wrong_archive(self):
1028 binary_publishing = self.makeBinaryPublishing()
1029 wrong_archive = self.factory.makeArchive()
1030 record = self.publishing_set.getByIdAndArchive(
1031 binary_publishing.id, wrong_archive, source=False)
1032 self.assertEqual(None, record)
1033
1034
978def test_suite():1035def test_suite():
979 return unittest.TestLoader().loadTestsFromName(__name__)1036 return unittest.TestLoader().loadTestsFromName(__name__)
9801037
=== 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 @@
132from lp.services.worlddata.interfaces.country import ICountrySet132from lp.services.worlddata.interfaces.country import ICountrySet
133from lp.services.worlddata.interfaces.language import ILanguageSet133from lp.services.worlddata.interfaces.language import ILanguageSet
134134
135from lp.soyuz.adapters.packagelocation import PackageLocation
135from lp.soyuz.interfaces.archive import (136from lp.soyuz.interfaces.archive import (
136 default_name_by_purpose, IArchiveSet, ArchivePurpose)137 default_name_by_purpose, IArchiveSet, ArchivePurpose)
137from lp.soyuz.adapters.packagelocation import PackageLocation
138from lp.soyuz.interfaces.binarypackagebuild import IBinaryPackageBuildSet138from lp.soyuz.interfaces.binarypackagebuild import IBinaryPackageBuildSet
139from lp.soyuz.interfaces.binarypackagerelease import BinaryPackageFormat
139from lp.soyuz.interfaces.component import IComponentSet140from lp.soyuz.interfaces.component import IComponentSet
140from lp.soyuz.interfaces.packageset import IPackagesetSet141from lp.soyuz.interfaces.packageset import IPackagesetSet
141from lp.soyuz.interfaces.processor import IProcessorFamilySet142from lp.soyuz.interfaces.processor import IProcessorFamilySet
142from lp.soyuz.interfaces.publishing import PackagePublishingStatus143from lp.soyuz.interfaces.publishing import (
144 PackagePublishingPriority, PackagePublishingStatus)
143from lp.soyuz.interfaces.section import ISectionSet145from lp.soyuz.interfaces.section import ISectionSet
146from lp.soyuz.model.binarypackagename import BinaryPackageName
147from lp.soyuz.model.binarypackagerelease import BinaryPackageRelease
144from lp.soyuz.model.processor import ProcessorFamily, ProcessorFamilySet148from lp.soyuz.model.processor import ProcessorFamily, ProcessorFamilySet
145from lp.soyuz.model.publishing import SourcePackagePublishingHistory149from lp.soyuz.model.publishing import (
150 BinaryPackagePublishingHistory, SourcePackagePublishingHistory)
146151
147from lp.testing import run_with_login, time_counter, login, logout, temp_dir152from lp.testing import run_with_login, time_counter, login, logout, temp_dir
148153
@@ -2199,9 +2204,10 @@
21992204
2200 def makeSourcePackageRelease(self, archive=None, sourcepackagename=None,2205 def makeSourcePackageRelease(self, archive=None, sourcepackagename=None,
2201 distroseries=None, maintainer=None,2206 distroseries=None, maintainer=None,
2202 creator=None, component=None, section=None,2207 creator=None, component=None,
2203 urgency=None, version=None,2208 section_name=None, urgency=None,
2204 builddepends=None, builddependsindep=None,2209 version=None, builddepends=None,
2210 builddependsindep=None,
2205 build_conflicts=None,2211 build_conflicts=None,
2206 build_conflicts_indep=None,2212 build_conflicts_indep=None,
2207 architecturehintlist='all',2213 architecturehintlist='all',
@@ -2236,9 +2242,7 @@
2236 if urgency is None:2242 if urgency is None:
2237 urgency = self.getAnySourcePackageUrgency()2243 urgency = self.getAnySourcePackageUrgency()
22382244
2239 if section is None:2245 section = self.makeSection(name=section_name)
2240 section = self.getUniqueString('section')
2241 section = getUtility(ISectionSet).ensure(section)
22422246
2243 if maintainer is None:2247 if maintainer is None:
2244 maintainer = self.makePerson()2248 maintainer = self.makePerson()
@@ -2324,8 +2328,9 @@
2324 def makeSourcePackagePublishingHistory(self, sourcepackagename=None,2328 def makeSourcePackagePublishingHistory(self, sourcepackagename=None,
2325 distroseries=None, maintainer=None,2329 distroseries=None, maintainer=None,
2326 creator=None, component=None,2330 creator=None, component=None,
2327 section=None, urgency=None,2331 section_name=None,
2328 version=None, archive=None,2332 urgency=None, version=None,
2333 archive=None,
2329 builddepends=None,2334 builddepends=None,
2330 builddependsindep=None,2335 builddependsindep=None,
2331 build_conflicts=None,2336 build_conflicts=None,
@@ -2365,7 +2370,7 @@
2365 distroseries=distroseries,2370 distroseries=distroseries,
2366 maintainer=maintainer,2371 maintainer=maintainer,
2367 creator=creator, component=component,2372 creator=creator, component=component,
2368 section=section,2373 section_name=section_name,
2369 urgency=urgency,2374 urgency=urgency,
2370 version=version,2375 version=version,
2371 builddepends=builddepends,2376 builddepends=builddepends,
@@ -2394,6 +2399,97 @@
2394 # of SSPPH and other useful attributes.2399 # of SSPPH and other useful attributes.
2395 return SourcePackagePublishingHistory.get(sspph.id)2400 return SourcePackagePublishingHistory.get(sspph.id)
23962401
2402 def makeBinaryPackagePublishingHistory(self, binarypackagerelease=None,
2403 distroarchseries=None,
2404 component=None, section_name=None,
2405 priority=None, status=None,
2406 scheduleddeletiondate=None,
2407 dateremoved=None,
2408 pocket=None, archive=None):
2409 """Make a `BinaryPackagePublishingHistory`."""
2410 if distroarchseries is None:
2411 if archive is None:
2412 distribution = None
2413 else:
2414 distribution = archive.distribution
2415 distroseries = self.makeDistroSeries(distribution=distribution)
2416 distroarchseries = self.makeDistroArchSeries(
2417 distroseries=distroseries)
2418
2419 if archive is None:
2420 archive = self.makeArchive(
2421 distribution=distroarchseries.distroseries.distribution,
2422 purpose=ArchivePurpose.PRIMARY)
2423
2424 if pocket is None:
2425 pocket = self.getAnyPocket()
2426
2427 if status is None:
2428 status = PackagePublishingStatus.PENDING
2429
2430 if priority is None:
2431 priority = PackagePublishingPriority.OPTIONAL
2432
2433 bpr = self.makeBinaryPackageRelease(
2434 component=component,
2435 section_name=section_name,
2436 priority=priority)
2437
2438 return BinaryPackagePublishingHistory(
2439 distroarchseries=distroarchseries,
2440 binarypackagerelease=bpr,
2441 component=bpr.component,
2442 section=bpr.section,
2443 status=status,
2444 dateremoved=dateremoved,
2445 scheduleddeletiondate=scheduleddeletiondate,
2446 pocket=pocket,
2447 priority=priority,
2448 archive=archive)
2449
2450 def makeBinaryPackageName(self, name=None):
2451 if name is None:
2452 name = self.getUniqueString("binarypackage")
2453 return BinaryPackageName(name=name)
2454
2455 def makeBinaryPackageRelease(self, binarypackagename=None,
2456 version=None, build=None,
2457 binpackageformat=None, component=None,
2458 section_name=None, priority=None,
2459 architecturespecific=False,
2460 summary=None, description=None):
2461 """Make a `BinaryPackageRelease`."""
2462 if binarypackagename is None:
2463 binarypackagename = self.makeBinaryPackageName()
2464 if version is None:
2465 version = self.getUniqueString("version")
2466 if build is None:
2467 build = self.makeBinaryPackageBuild()
2468 if binpackageformat is None:
2469 binpackageformat = BinaryPackageFormat.DEB
2470 if component is None:
2471 component = self.makeComponent()
2472 section = self.makeSection(name=section_name)
2473 if priority is None:
2474 priority = PackagePublishingPriority.OPTIONAL
2475 if summary is None:
2476 summary = self.getUniqueString("summary")
2477 if description is None:
2478 description = self.getUniqueString("description")
2479 return BinaryPackageRelease(binarypackagename=binarypackagename,
2480 version=version, build=build,
2481 binpackageformat=binpackageformat,
2482 component=component, section=section,
2483 priority=priority, summary=summary,
2484 description=description,
2485 architecturespecific=architecturespecific)
2486
2487 def makeSection(self, name=None):
2488 """Make a `Section`."""
2489 if name is None:
2490 name = self.getUniqueString('section')
2491 return getUtility(ISectionSet).ensure(name)
2492
2397 def makePackageset(self, name=None, description=None, owner=None,2493 def makePackageset(self, name=None, description=None, owner=None,
2398 packages=(), distroseries=None):2494 packages=(), distroseries=None):
2399 """Make an `IPackageset`."""2495 """Make an `IPackageset`."""