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
1=== modified file 'lib/lp/registry/tests/test_distroseries.py'
2--- lib/lp/registry/tests/test_distroseries.py 2010-05-13 19:24:12 +0000
3+++ lib/lp/registry/tests/test_distroseries.py 2010-07-15 14:23:42 +0000
4@@ -233,7 +233,7 @@
5 sourcepackagename = self.factory.makeSourcePackageName(name)
6 self.factory.makeSourcePackagePublishingHistory(
7 sourcepackagename=sourcepackagename, distroseries=self.series,
8- component=component, section=section)
9+ component=component, section_name=section)
10 source_package = self.factory.makeSourcePackage(
11 sourcepackagename=sourcepackagename, distroseries=self.series)
12 if heat is not None:
13
14=== modified file 'lib/lp/soyuz/browser/archive.py'
15--- lib/lp/soyuz/browser/archive.py 2010-06-30 15:58:24 +0000
16+++ lib/lp/soyuz/browser/archive.py 2010-07-15 14:23:42 +0000
17@@ -201,12 +201,8 @@
18
19 # The ID is not enough on its own to identify the publication,
20 # we need to make sure it matches the context archive as well.
21- results = getUtility(IPublishingSet).getByIdAndArchive(
22+ return getUtility(IPublishingSet).getByIdAndArchive(
23 pub_id, self.context, source)
24- if results.count() == 1:
25- return results[0]
26-
27- return None
28
29 @stepthrough('+binaryhits')
30 def traverse_binaryhits(self, name_str):
31
32=== modified file 'lib/lp/soyuz/model/publishing.py'
33--- lib/lp/soyuz/model/publishing.py 2010-06-22 16:08:05 +0000
34+++ lib/lp/soyuz/model/publishing.py 2010-07-15 14:23:42 +0000
35@@ -1266,7 +1266,7 @@
36 return Store.of(archive).find(
37 baseclass,
38 baseclass.id == id,
39- baseclass.archive == archive.id)
40+ baseclass.archive == archive.id).one()
41
42 def _extractIDs(self, one_or_more_source_publications):
43 """Return a list of database IDs for the given list or single object.
44
45=== modified file 'lib/lp/soyuz/tests/test_publishing.py'
46--- lib/lp/soyuz/tests/test_publishing.py 2010-05-05 14:50:42 +0000
47+++ lib/lp/soyuz/tests/test_publishing.py 2010-07-15 14:23:42 +0000
48@@ -19,7 +19,8 @@
49 from canonical.database.constants import UTC_NOW
50 from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
51 from canonical.launchpad.webapp.interfaces import NotFoundError
52-from canonical.testing import LaunchpadZopelessLayer
53+from canonical.testing import (
54+ DatabaseFunctionalLayer, LaunchpadZopelessLayer)
55 from lp.archivepublisher.config import Config
56 from lp.archivepublisher.diskpool import DiskPool
57 from lp.buildmaster.interfaces.buildbase import BuildStatus
58@@ -38,7 +39,7 @@
59 from lp.soyuz.interfaces.component import IComponentSet
60 from lp.soyuz.interfaces.section import ISectionSet
61 from lp.soyuz.interfaces.publishing import (
62- PackagePublishingPriority, PackagePublishingStatus)
63+ IPublishingSet, PackagePublishingPriority, PackagePublishingStatus)
64 from lp.soyuz.interfaces.queue import PackageUploadStatus
65 from canonical.launchpad.scripts import FakeLogger
66 from lp.testing import TestCaseWithFactory
67@@ -975,5 +976,61 @@
68 self.assertEquals(self.sparc_distroarch, builds[1].distro_arch_series)
69
70
71+class PublishingSetTests(TestCaseWithFactory):
72+
73+ layer = DatabaseFunctionalLayer
74+
75+ def setUp(self):
76+ super(PublishingSetTests, self).setUp()
77+ self.distroseries = self.factory.makeDistroSeries()
78+ self.archive = self.factory.makeArchive(
79+ distribution=self.distroseries.distribution)
80+ self.publishing = self.factory.makeSourcePackagePublishingHistory(
81+ distroseries=self.distroseries, archive=self.archive)
82+ self.publishing_set = getUtility(IPublishingSet)
83+
84+ def test_getByIdAndArchive_finds_record(self):
85+ record = self.publishing_set.getByIdAndArchive(
86+ self.publishing.id, self.archive)
87+ self.assertEqual(self.publishing, record)
88+
89+ def test_getByIdAndArchive_finds_record_explicit_source(self):
90+ record = self.publishing_set.getByIdAndArchive(
91+ self.publishing.id, self.archive, source=True)
92+ self.assertEqual(self.publishing, record)
93+
94+ def test_getByIdAndArchive_wrong_archive(self):
95+ wrong_archive = self.factory.makeArchive()
96+ record = self.publishing_set.getByIdAndArchive(
97+ self.publishing.id, wrong_archive)
98+ self.assertEqual(None, record)
99+
100+ def makeBinaryPublishing(self):
101+ distroarchseries = self.factory.makeDistroArchSeries(
102+ distroseries=self.distroseries)
103+ binary_publishing = self.factory.makeBinaryPackagePublishingHistory(
104+ archive=self.archive, distroarchseries=distroarchseries)
105+ return binary_publishing
106+
107+ def test_getByIdAndArchive_wrong_type(self):
108+ self.makeBinaryPublishing()
109+ record = self.publishing_set.getByIdAndArchive(
110+ self.publishing.id, self.archive, source=False)
111+ self.assertEqual(None, record)
112+
113+ def test_getByIdAndArchive_finds_binary(self):
114+ binary_publishing = self.makeBinaryPublishing()
115+ record = self.publishing_set.getByIdAndArchive(
116+ binary_publishing.id, self.archive, source=False)
117+ self.assertEqual(binary_publishing, record)
118+
119+ def test_getByIdAndArchive_binary_wrong_archive(self):
120+ binary_publishing = self.makeBinaryPublishing()
121+ wrong_archive = self.factory.makeArchive()
122+ record = self.publishing_set.getByIdAndArchive(
123+ binary_publishing.id, wrong_archive, source=False)
124+ self.assertEqual(None, record)
125+
126+
127 def test_suite():
128 return unittest.TestLoader().loadTestsFromName(__name__)
129
130=== modified file 'lib/lp/testing/factory.py'
131--- lib/lp/testing/factory.py 2010-07-14 19:33:16 +0000
132+++ lib/lp/testing/factory.py 2010-07-15 14:23:42 +0000
133@@ -132,17 +132,22 @@
134 from lp.services.worlddata.interfaces.country import ICountrySet
135 from lp.services.worlddata.interfaces.language import ILanguageSet
136
137+from lp.soyuz.adapters.packagelocation import PackageLocation
138 from lp.soyuz.interfaces.archive import (
139 default_name_by_purpose, IArchiveSet, ArchivePurpose)
140-from lp.soyuz.adapters.packagelocation import PackageLocation
141 from lp.soyuz.interfaces.binarypackagebuild import IBinaryPackageBuildSet
142+from lp.soyuz.interfaces.binarypackagerelease import BinaryPackageFormat
143 from lp.soyuz.interfaces.component import IComponentSet
144 from lp.soyuz.interfaces.packageset import IPackagesetSet
145 from lp.soyuz.interfaces.processor import IProcessorFamilySet
146-from lp.soyuz.interfaces.publishing import PackagePublishingStatus
147+from lp.soyuz.interfaces.publishing import (
148+ PackagePublishingPriority, PackagePublishingStatus)
149 from lp.soyuz.interfaces.section import ISectionSet
150+from lp.soyuz.model.binarypackagename import BinaryPackageName
151+from lp.soyuz.model.binarypackagerelease import BinaryPackageRelease
152 from lp.soyuz.model.processor import ProcessorFamily, ProcessorFamilySet
153-from lp.soyuz.model.publishing import SourcePackagePublishingHistory
154+from lp.soyuz.model.publishing import (
155+ BinaryPackagePublishingHistory, SourcePackagePublishingHistory)
156
157 from lp.testing import run_with_login, time_counter, login, logout, temp_dir
158
159@@ -2199,9 +2204,10 @@
160
161 def makeSourcePackageRelease(self, archive=None, sourcepackagename=None,
162 distroseries=None, maintainer=None,
163- creator=None, component=None, section=None,
164- urgency=None, version=None,
165- builddepends=None, builddependsindep=None,
166+ creator=None, component=None,
167+ section_name=None, urgency=None,
168+ version=None, builddepends=None,
169+ builddependsindep=None,
170 build_conflicts=None,
171 build_conflicts_indep=None,
172 architecturehintlist='all',
173@@ -2236,9 +2242,7 @@
174 if urgency is None:
175 urgency = self.getAnySourcePackageUrgency()
176
177- if section is None:
178- section = self.getUniqueString('section')
179- section = getUtility(ISectionSet).ensure(section)
180+ section = self.makeSection(name=section_name)
181
182 if maintainer is None:
183 maintainer = self.makePerson()
184@@ -2324,8 +2328,9 @@
185 def makeSourcePackagePublishingHistory(self, sourcepackagename=None,
186 distroseries=None, maintainer=None,
187 creator=None, component=None,
188- section=None, urgency=None,
189- version=None, archive=None,
190+ section_name=None,
191+ urgency=None, version=None,
192+ archive=None,
193 builddepends=None,
194 builddependsindep=None,
195 build_conflicts=None,
196@@ -2365,7 +2370,7 @@
197 distroseries=distroseries,
198 maintainer=maintainer,
199 creator=creator, component=component,
200- section=section,
201+ section_name=section_name,
202 urgency=urgency,
203 version=version,
204 builddepends=builddepends,
205@@ -2394,6 +2399,97 @@
206 # of SSPPH and other useful attributes.
207 return SourcePackagePublishingHistory.get(sspph.id)
208
209+ def makeBinaryPackagePublishingHistory(self, binarypackagerelease=None,
210+ distroarchseries=None,
211+ component=None, section_name=None,
212+ priority=None, status=None,
213+ scheduleddeletiondate=None,
214+ dateremoved=None,
215+ pocket=None, archive=None):
216+ """Make a `BinaryPackagePublishingHistory`."""
217+ if distroarchseries is None:
218+ if archive is None:
219+ distribution = None
220+ else:
221+ distribution = archive.distribution
222+ distroseries = self.makeDistroSeries(distribution=distribution)
223+ distroarchseries = self.makeDistroArchSeries(
224+ distroseries=distroseries)
225+
226+ if archive is None:
227+ archive = self.makeArchive(
228+ distribution=distroarchseries.distroseries.distribution,
229+ purpose=ArchivePurpose.PRIMARY)
230+
231+ if pocket is None:
232+ pocket = self.getAnyPocket()
233+
234+ if status is None:
235+ status = PackagePublishingStatus.PENDING
236+
237+ if priority is None:
238+ priority = PackagePublishingPriority.OPTIONAL
239+
240+ bpr = self.makeBinaryPackageRelease(
241+ component=component,
242+ section_name=section_name,
243+ priority=priority)
244+
245+ return BinaryPackagePublishingHistory(
246+ distroarchseries=distroarchseries,
247+ binarypackagerelease=bpr,
248+ component=bpr.component,
249+ section=bpr.section,
250+ status=status,
251+ dateremoved=dateremoved,
252+ scheduleddeletiondate=scheduleddeletiondate,
253+ pocket=pocket,
254+ priority=priority,
255+ archive=archive)
256+
257+ def makeBinaryPackageName(self, name=None):
258+ if name is None:
259+ name = self.getUniqueString("binarypackage")
260+ return BinaryPackageName(name=name)
261+
262+ def makeBinaryPackageRelease(self, binarypackagename=None,
263+ version=None, build=None,
264+ binpackageformat=None, component=None,
265+ section_name=None, priority=None,
266+ architecturespecific=False,
267+ summary=None, description=None):
268+ """Make a `BinaryPackageRelease`."""
269+ if binarypackagename is None:
270+ binarypackagename = self.makeBinaryPackageName()
271+ if version is None:
272+ version = self.getUniqueString("version")
273+ if build is None:
274+ build = self.makeBinaryPackageBuild()
275+ if binpackageformat is None:
276+ binpackageformat = BinaryPackageFormat.DEB
277+ if component is None:
278+ component = self.makeComponent()
279+ section = self.makeSection(name=section_name)
280+ if priority is None:
281+ priority = PackagePublishingPriority.OPTIONAL
282+ if summary is None:
283+ summary = self.getUniqueString("summary")
284+ if description is None:
285+ description = self.getUniqueString("description")
286+ return BinaryPackageRelease(binarypackagename=binarypackagename,
287+ version=version, build=build,
288+ binpackageformat=binpackageformat,
289+ component=component, section=section,
290+ priority=priority, summary=summary,
291+ description=description,
292+ architecturespecific=architecturespecific)
293+
294+ def makeSection(self, name=None):
295+ """Make a `Section`."""
296+ if name is None:
297+ name = self.getUniqueString('section')
298+ return getUtility(ISectionSet).ensure(name)
299+
300 def makePackageset(self, name=None, description=None, owner=None,
301 packages=(), distroseries=None):
302 """Make an `IPackageset`."""