Merge lp:~james-w/launchpad/copy-archive-test-refactor into lp:launchpad
- copy-archive-test-refactor
- Merge into devel
Proposed by
James Westby
| Status: | Merged |
|---|---|
| Approved by: | Jelmer Vernooij |
| Approved revision: | no longer in the source branch. |
| Merged at revision: | 11050 |
| Proposed branch: | lp:~james-w/launchpad/copy-archive-test-refactor |
| Merge into: | lp:launchpad |
| Diff against target: |
762 lines (+436/-150) 2 files modified
lib/lp/soyuz/scripts/populate_archive.py (+1/-1) lib/lp/soyuz/scripts/tests/test_populatearchive.py (+435/-149) |
| To merge this branch: | bzr merge lp:~james-w/launchpad/copy-archive-test-refactor |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Julian Edwards (community) | Approve | ||
| Guilherme Salgado (community) | Approve | ||
|
Review via email:
|
|||
Commit message
Move some copy archive tests to use the factory, and add some more specific tests.
Description of the change
Summary
The copy-archive tests are hard to understand as they use
the sampledata and test a lot of things in some methods.
There is even a method to check that the sampledata doesn't
change.
Proposed fix
Use the factory to create the data needed for a specific test,
and then break the tests down in to unit tests that test one
thing only.
Pre-implementation notes
None.
Implementation details
There is some duplication here, because I wasn't comfortable deleting
some of the existing tests as I wasn't sure what I hadn't tested with
new tests.
There are also two methods for running the script now, I left the original
one as it handled checking error messages, and that's what most of the
tests that use it now do.
I also created a small helper class to save passing around lots of values
between methods.
Tests
None as it is just test changes. You can run the tests in that file
with
./bin/test -cvv -s lp.soyuz.
Demo and Q/A
N/A
lint
None.
Thanks,
James
To post a comment you must log in.
Revision history for this message
| James Westby (james-w) wrote : | # |
On Mon, 21 Jun 2010 18:01:25 -0000, Guilherme Salgado <email address hidden> wrote:
> Review: Approve
> Hi James,
>
> These changes look good to me; I only have a few comments.
>
> review approve
Great, thanks.
> Maybe you can mark the old tests with XXXs for someone who knows more
> about this check whether or not they can be removed?
Good idea, I'll look up the XXX policy and do this.
> > + def __init__(self, name, version,
> > + status=
>
> I think the style guide says that when you have a line break in the
> middle of a list of arguments you should indent all arguments at the
> same level, so
>
> def foo(self, bar, baz,
> etc...):
Will fix, thanks.
> I'd rather use self.assertIs(None, copy_archive) here because when that
> fails you'll get a helpful failure message instead of the "True is not
> False" one that assertTrue() gives.
Good idea, thanks.
> > + def createSourceDis
> > + """Create a distribution to be the source of a copy archive."""
> > + distroseries = self.createSour
> > + self.createSour
> > + self.layer.commit()
>
> Why do you need to commit here? If the commit is really necessary, I
> think it deserves a comment.
Because we are execing a script that will query the db from a different
transaction. I'll add the comment.
> When we remove the security proxy of something we should assign it to a
> different variable and name it appropriately (e.g. naked_build), to make
> sure it stands out wherever it's used. This is not a big deal in test
> code, but it's caused us some problems in production code.
Will fix, thanks.
> I wonder why a build.source_
> you know?
I don't know.
> Does this one deserve a comment as well?
Yes, will add it.
Thanks,
James
Revision history for this message
| James Westby (james-w) wrote : | # |
All comments addressed.
Julian, would you take a look to check you are happy with the proposed
changes?
Thanks,
James
Revision history for this message
| Julian Edwards (julian-edwards) wrote : | # |
James, thanks for making the code better!
>> Why do you need to commit here? If the commit is really necessary, I
>> think it deserves a comment.
>
>Because we are execing a script that will query the db from a different
>transaction. I'll add the comment.
Using commit() should be a last resort, as well as exec-ing scripts, as they are both very slow.
I don't have the time right now to delve very deeply into the code so I'll just raise some food for thought:
* Will store.flush() work instead of commit() ?
* Is the external script being invoked every test? Ideally it should be called once and only once with a simple case to make sure it's executable, then the bulk of the tests done via a script hook.
* It would be great if someone fixed the fucking hideous command line args to that script
Other than that, I expect as you guys use it more you'll see if it's buggy and needs more tests.
Cheers
J
review:
Needs Information
Revision history for this message
| James Westby (james-w) wrote : | # |
On Tue, 22 Jun 2010 08:32:29 -0000, Julian Edwards <email address hidden> wrote:
> Review: Needs Information
> James, thanks for making the code better!
>
> >> Why do you need to commit here? If the commit is really necessary, I
> >> think it deserves a comment.
> >
> >Because we are execing a script that will query the db from a different
> >transaction. I'll add the comment.
>
> Using commit() should be a last resort, as well as exec-ing scripts, as they are both very slow.
>
> I don't have the time right now to delve very deeply into the code so I'll just raise some food for thought:
> * Will store.flush() work instead of commit() ?
No.
> * Is the external script being invoked every test? Ideally it should
> be called once and only once with a simple case to make sure it's
> executable, then the bulk of the tests done via a script hook.
Done.
> * It would be great if someone fixed the fucking hideous command line args to that script
Another time.
Please review the changes.
Thanks,
James
Revision history for this message
| Julian Edwards (julian-edwards) wrote : | # |
Loads better, thanks James.
review:
Approve
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
| 1 | === modified file 'lib/lp/soyuz/scripts/populate_archive.py' |
| 2 | --- lib/lp/soyuz/scripts/populate_archive.py 2009-06-25 04:06:00 +0000 |
| 3 | +++ lib/lp/soyuz/scripts/populate_archive.py 2010-06-22 11:23:29 +0000 |
| 4 | @@ -386,7 +386,7 @@ |
| 5 | :param distroseries: the distro series for which to create builds. |
| 6 | :param archive: the archive for which to create builds. |
| 7 | :param proc_families: the list of processor families for |
| 8 | - which to create builds (optional). |
| 9 | + which to create builds. |
| 10 | """ |
| 11 | # Avoid circular imports. |
| 12 | from lp.soyuz.interfaces.publishing import active_publishing_status |
| 13 | |
| 14 | === modified file 'lib/lp/soyuz/scripts/tests/test_populatearchive.py' |
| 15 | --- lib/lp/soyuz/scripts/tests/test_populatearchive.py 2010-05-20 15:27:12 +0000 |
| 16 | +++ lib/lp/soyuz/scripts/tests/test_populatearchive.py 2010-06-22 11:23:29 +0000 |
| 17 | @@ -14,19 +14,21 @@ |
| 18 | from zope.security.proxy import removeSecurityProxy |
| 19 | |
| 20 | from canonical.config import config |
| 21 | -from canonical.launchpad.scripts import BufferLogger |
| 22 | +from canonical.launchpad.scripts import ( |
| 23 | + BufferLogger, QuietFakeLogger) |
| 24 | from canonical.testing import LaunchpadZopelessLayer |
| 25 | from canonical.testing.layers import DatabaseLayer |
| 26 | from lp.buildmaster.interfaces.buildbase import BuildStatus |
| 27 | from lp.registry.interfaces.distribution import IDistributionSet |
| 28 | from lp.registry.interfaces.person import IPersonSet |
| 29 | +from lp.registry.interfaces.pocket import PackagePublishingPocket |
| 30 | from lp.services.job.interfaces.job import JobStatus |
| 31 | from lp.soyuz.interfaces.archive import ArchivePurpose, IArchiveSet |
| 32 | -from lp.soyuz.interfaces.archivearch import IArchiveArchSet |
| 33 | from lp.soyuz.interfaces.binarypackagebuild import IBinaryPackageBuildSet |
| 34 | from lp.soyuz.interfaces.packagecopyrequest import ( |
| 35 | IPackageCopyRequestSet, PackageCopyStatus) |
| 36 | from lp.soyuz.interfaces.publishing import PackagePublishingStatus |
| 37 | +from lp.soyuz.model.processor import ProcessorFamilySet |
| 38 | from lp.soyuz.scripts.ftpmaster import PackageLocationError, SoyuzScriptError |
| 39 | from lp.soyuz.scripts.populate_archive import ArchivePopulator |
| 40 | from lp.soyuz.tests.test_publishing import SoyuzTestPublisher |
| 41 | @@ -39,6 +41,16 @@ |
| 42 | return pub.sourcepackagerelease.sourcepackagename |
| 43 | |
| 44 | |
| 45 | +class PackageInfo: |
| 46 | + |
| 47 | + def __init__(self, name, version, |
| 48 | + status=PackagePublishingStatus.PUBLISHED, component="main"): |
| 49 | + self.name = name |
| 50 | + self.version = version |
| 51 | + self.status = status |
| 52 | + self.component = component |
| 53 | + |
| 54 | + |
| 55 | class TestPopulateArchiveScript(TestCaseWithFactory): |
| 56 | """Test the copy-package.py script.""" |
| 57 | |
| 58 | @@ -77,6 +89,10 @@ |
| 59 | |
| 60 | Use the hoary-RELEASE suite along with the main component. |
| 61 | """ |
| 62 | + # XXX: JamesWestby 2010-06-21 bug=596984: it is not clear |
| 63 | + # what this test is testing that is not covered in more |
| 64 | + # specific tests. It should be removed if there is nothing |
| 65 | + # else as it is fragile due to use of sampledata. |
| 66 | DatabaseLayer.force_dirty_database() |
| 67 | # Make sure a copy archive with the desired name does |
| 68 | # not exist yet. |
| 69 | @@ -118,14 +134,6 @@ |
| 70 | distro, ArchivePurpose.COPY, archive_name) |
| 71 | self.assertTrue(copy_archive is not None) |
| 72 | |
| 73 | - # Ascertain that the new copy archive was created with the 'enabled' |
| 74 | - # flag turned off. |
| 75 | - self.assertFalse(copy_archive.enabled) |
| 76 | - |
| 77 | - # Also, make sure that the builds for the new copy archive will be |
| 78 | - # carried out on non-virtual builders. |
| 79 | - self.assertTrue(copy_archive.require_virtualized) |
| 80 | - |
| 81 | # Make sure the right source packages were cloned. |
| 82 | self._verifyClonedSourcePackages(copy_archive, hoary) |
| 83 | |
| 84 | @@ -142,6 +150,405 @@ |
| 85 | |
| 86 | self.assertEqual(build_spns, self.expected_build_spns) |
| 87 | |
| 88 | + def createSourceDistroSeries(self): |
| 89 | + """Create a DistroSeries suitable for copying. |
| 90 | + |
| 91 | + Creates a distroseries with a DistroArchSeries and nominatedarchindep, |
| 92 | + which makes it suitable for copying because it will create some builds. |
| 93 | + """ |
| 94 | + distro_name = "foobuntu" |
| 95 | + distro = self.factory.makeDistribution(name=distro_name) |
| 96 | + distroseries_name = "maudlin" |
| 97 | + distroseries = self.factory.makeDistroSeries( |
| 98 | + distribution=distro, name=distroseries_name) |
| 99 | + das = self.factory.makeDistroArchSeries( |
| 100 | + distroseries=distroseries, architecturetag="i386", |
| 101 | + processorfamily=ProcessorFamilySet().getByName("x86"), |
| 102 | + supports_virtualized=True) |
| 103 | + distroseries.nominatedarchindep = das |
| 104 | + return distroseries |
| 105 | + |
| 106 | + def createTargetOwner(self): |
| 107 | + """Create a person suitable to own a copy archive.""" |
| 108 | + person_name = "copy-archive-owner" |
| 109 | + owner = self.factory.makePerson(name=person_name) |
| 110 | + return owner |
| 111 | + |
| 112 | + def getTargetArchiveName(self, distribution): |
| 113 | + """Get a suitable name for a copy archive. |
| 114 | + |
| 115 | + It also checks that the archive doesn't currently exist. |
| 116 | + """ |
| 117 | + archive_name = "msa%s" % int(time.time()) |
| 118 | + copy_archive = getUtility(IArchiveSet).getByDistroPurpose( |
| 119 | + distribution, ArchivePurpose.COPY, archive_name) |
| 120 | + # This is a sanity check: a copy archive with this name should not |
| 121 | + # exist yet. |
| 122 | + self.assertIs(None, copy_archive) |
| 123 | + return archive_name |
| 124 | + |
| 125 | + def createSourcePublication(self, info, distroseries): |
| 126 | + """Create a SourcePackagePublishingHistory based on a PackageInfo.""" |
| 127 | + self.factory.makeSourcePackagePublishingHistory( |
| 128 | + sourcepackagename=self.factory.getOrMakeSourcePackageName( |
| 129 | + name=info.name), |
| 130 | + distroseries=distroseries, component=self.factory.makeComponent( |
| 131 | + info.component), |
| 132 | + version=info.version, architecturehintlist='any', |
| 133 | + archive=distroseries.distribution.main_archive, |
| 134 | + status=info.status, pocket=PackagePublishingPocket.RELEASE) |
| 135 | + |
| 136 | + def createSourcePublications(self, package_infos, distroseries): |
| 137 | + """Create a source publication for each item in package_infos.""" |
| 138 | + for package_info in package_infos: |
| 139 | + self.createSourcePublication(package_info, distroseries) |
| 140 | + |
| 141 | + def getScript(self, test_args=None): |
| 142 | + """Return an ArchivePopulator instance.""" |
| 143 | + if test_args is None: |
| 144 | + test_args = [] |
| 145 | + script = ArchivePopulator("test copy archives", test_args=test_args) |
| 146 | + script.logger = QuietFakeLogger() |
| 147 | + script.txn = self.layer.txn |
| 148 | + return script |
| 149 | + |
| 150 | + def copyArchive(self, distroseries, archive_name, owner, |
| 151 | + architectures=None, component="main", from_user=None, |
| 152 | + from_archive=None): |
| 153 | + """Run the copy-archive script.""" |
| 154 | + extra_args = [ |
| 155 | + '--from-distribution', distroseries.distribution.name, |
| 156 | + '--from-suite', distroseries.name, |
| 157 | + '--to-distribution', distroseries.distribution.name, |
| 158 | + '--to-suite', distroseries.name, |
| 159 | + '--to-archive', archive_name, |
| 160 | + '--to-user', owner.name, |
| 161 | + '--reason', |
| 162 | + '"copy archive from %s"' % datetime.ctime(datetime.utcnow()), |
| 163 | + '--component', component, |
| 164 | + ] |
| 165 | + |
| 166 | + if from_user is not None: |
| 167 | + extra_args.extend(["--from-user", from_user]) |
| 168 | + |
| 169 | + if from_archive is not None: |
| 170 | + extra_args.extend(["--from-archive", from_archive]) |
| 171 | + |
| 172 | + if architectures is None: |
| 173 | + architectures = ["386"] |
| 174 | + |
| 175 | + for architecture in architectures: |
| 176 | + extra_args.extend(['-a', architecture]) |
| 177 | + |
| 178 | + script = self.getScript(test_args=extra_args) |
| 179 | + script.mainTask() |
| 180 | + |
| 181 | + # Make sure the copy archive with the desired name was |
| 182 | + # created |
| 183 | + copy_archive = getUtility(IArchiveSet).getByDistroPurpose( |
| 184 | + distroseries.distribution, ArchivePurpose.COPY, archive_name) |
| 185 | + self.assertTrue(copy_archive is not None) |
| 186 | + |
| 187 | + # Ascertain that the new copy archive was created with the 'enabled' |
| 188 | + # flag turned off. |
| 189 | + self.assertFalse(copy_archive.enabled) |
| 190 | + |
| 191 | + # Also, make sure that the builds for the new copy archive will be |
| 192 | + # carried out on non-virtual builders. |
| 193 | + self.assertTrue(copy_archive.require_virtualized) |
| 194 | + |
| 195 | + return copy_archive |
| 196 | + |
| 197 | + def checkCopiedSources(self, archive, distroseries, expected): |
| 198 | + """Check the sources published in an archive against an expected set. |
| 199 | + |
| 200 | + Given an archive and a target distroseries the sources published in |
| 201 | + that distroseries are checked against a set of PackageInfo to |
| 202 | + ensure that the correct package names and versions are published. |
| 203 | + """ |
| 204 | + expected_set = set([(info.name, info.version) for info in expected]) |
| 205 | + sources = archive.getPublishedSources( |
| 206 | + distroseries=distroseries, status=self.pending_statuses) |
| 207 | + actual_set = set() |
| 208 | + for source in sources: |
| 209 | + source = removeSecurityProxy(source) |
| 210 | + actual_set.add( |
| 211 | + (source.source_package_name, source.source_package_version)) |
| 212 | + self.assertEqual(expected_set, actual_set) |
| 213 | + |
| 214 | + def createSourceDistribution(self, package_infos): |
| 215 | + """Create a distribution to be the source of a copy archive.""" |
| 216 | + distroseries = self.createSourceDistroSeries() |
| 217 | + self.createSourcePublications(package_infos, distroseries) |
| 218 | + return distroseries |
| 219 | + |
| 220 | + def makeCopyArchive(self, package_infos, component="main"): |
| 221 | + """Make a copy archive based on a new distribution.""" |
| 222 | + owner = self.createTargetOwner() |
| 223 | + distroseries = self.createSourceDistribution(package_infos) |
| 224 | + archive_name = self.getTargetArchiveName(distroseries.distribution) |
| 225 | + copy_archive = self.copyArchive( |
| 226 | + distroseries, archive_name, owner, component=component) |
| 227 | + return (copy_archive, distroseries) |
| 228 | + |
| 229 | + def checkBuilds(self, archive, package_infos): |
| 230 | + """Check the build records pending in an archive. |
| 231 | + |
| 232 | + Given a set of PackageInfo objects check that each has a build |
| 233 | + created for it. |
| 234 | + """ |
| 235 | + expected_builds = list( |
| 236 | + [(info.name, info.version) for info in package_infos]) |
| 237 | + builds = list( |
| 238 | + getUtility(IBinaryPackageBuildSet).getBuildsForArchive( |
| 239 | + archive, status=BuildStatus.NEEDSBUILD)) |
| 240 | + actual_builds = list() |
| 241 | + for build in builds: |
| 242 | + naked_build = removeSecurityProxy(build) |
| 243 | + spr = naked_build.source_package_release |
| 244 | + actual_builds.append((spr.name, spr.version)) |
| 245 | + self.assertEqual(sorted(expected_builds), sorted(actual_builds)) |
| 246 | + |
| 247 | + def testCopyArchiveRunScript(self): |
| 248 | + """Check that we can exec the script to copy an archive.""" |
| 249 | + package_info = PackageInfo( |
| 250 | + "bzr", "2.1", status=PackagePublishingStatus.PUBLISHED) |
| 251 | + owner = self.createTargetOwner() |
| 252 | + distroseries = self.createSourceDistribution([package_info]) |
| 253 | + archive_name = self.getTargetArchiveName(distroseries.distribution) |
| 254 | + # We must commit as we are going to exec a script that will run |
| 255 | + # in a different transaction and must be able to see the |
| 256 | + # objects we just created. |
| 257 | + self.layer.commit() |
| 258 | + |
| 259 | + extra_args = [ |
| 260 | + '--from-distribution', distroseries.distribution.name, |
| 261 | + '--from-suite', distroseries.name, |
| 262 | + '--to-distribution', distroseries.distribution.name, |
| 263 | + '--to-suite', distroseries.name, |
| 264 | + '--to-archive', archive_name, |
| 265 | + '--to-user', owner.name, |
| 266 | + '--reason', |
| 267 | + '"copy archive from %s"' % datetime.ctime(datetime.utcnow()), |
| 268 | + '--component', "main", |
| 269 | + '-a', '386', |
| 270 | + ] |
| 271 | + (exitcode, out, err) = self.runWrapperScript(extra_args) |
| 272 | + # Check for zero exit code. |
| 273 | + self.assertEqual( |
| 274 | + exitcode, 0, "\n=> %s\n=> %s\n=> %s\n" % (exitcode, out, err)) |
| 275 | + # Make sure the copy archive with the desired name was |
| 276 | + # created |
| 277 | + copy_archive = getUtility(IArchiveSet).getByDistroPurpose( |
| 278 | + distroseries.distribution, ArchivePurpose.COPY, archive_name) |
| 279 | + self.assertTrue(copy_archive is not None) |
| 280 | + |
| 281 | + # Ascertain that the new copy archive was created with the 'enabled' |
| 282 | + # flag turned off. |
| 283 | + self.assertFalse(copy_archive.enabled) |
| 284 | + |
| 285 | + # Also, make sure that the builds for the new copy archive will be |
| 286 | + # carried out on non-virtual builders. |
| 287 | + self.assertTrue(copy_archive.require_virtualized) |
| 288 | + self.checkCopiedSources( |
| 289 | + copy_archive, distroseries, [package_info]) |
| 290 | + |
| 291 | + def testCopyArchiveCreateCopiesPublished(self): |
| 292 | + """Test that PUBLISHED sources are copied.""" |
| 293 | + package_info = PackageInfo( |
| 294 | + "bzr", "2.1", status=PackagePublishingStatus.PUBLISHED) |
| 295 | + copy_archive, distroseries = self.makeCopyArchive([package_info]) |
| 296 | + self.checkCopiedSources( |
| 297 | + copy_archive, distroseries, [package_info]) |
| 298 | + |
| 299 | + def testCopyArchiveCreateCopiesPending(self): |
| 300 | + """Test that PENDING sources are copied.""" |
| 301 | + package_info = PackageInfo( |
| 302 | + "bzr", "2.1", status=PackagePublishingStatus.PENDING) |
| 303 | + copy_archive, distroseries = self.makeCopyArchive([package_info]) |
| 304 | + self.checkCopiedSources( |
| 305 | + copy_archive, distroseries, [package_info]) |
| 306 | + |
| 307 | + def testCopyArchiveCreateDoesntCopySuperseded(self): |
| 308 | + """Test that SUPERSEDED sources are not copied.""" |
| 309 | + package_info = PackageInfo( |
| 310 | + "bzr", "2.1", status=PackagePublishingStatus.SUPERSEDED) |
| 311 | + copy_archive, distroseries = self.makeCopyArchive([package_info]) |
| 312 | + self.checkCopiedSources( |
| 313 | + copy_archive, distroseries, []) |
| 314 | + |
| 315 | + def testCopyArchiveCreateDoesntCopyDeleted(self): |
| 316 | + """Test that DELETED sources are not copied.""" |
| 317 | + package_info = PackageInfo( |
| 318 | + "bzr", "2.1", status=PackagePublishingStatus.DELETED) |
| 319 | + copy_archive, distroseries = self.makeCopyArchive([package_info]) |
| 320 | + self.checkCopiedSources( |
| 321 | + copy_archive, distroseries, []) |
| 322 | + |
| 323 | + def testCopyArchiveCreateDoesntCopyObsolete(self): |
| 324 | + """Test that OBSOLETE sources are not copied.""" |
| 325 | + package_info = PackageInfo( |
| 326 | + "bzr", "2.1", status=PackagePublishingStatus.OBSOLETE) |
| 327 | + copy_archive, distroseries = self.makeCopyArchive([package_info]) |
| 328 | + self.checkCopiedSources( |
| 329 | + copy_archive, distroseries, []) |
| 330 | + |
| 331 | + def testCopyArchiveCreatesBuilds(self): |
| 332 | + """Test that a copy archive creates builds for the copied packages.""" |
| 333 | + package_info = PackageInfo( |
| 334 | + "bzr", "2.1", status=PackagePublishingStatus.PUBLISHED) |
| 335 | + copy_archive, distroseries = self.makeCopyArchive([package_info]) |
| 336 | + self.checkBuilds(copy_archive, [package_info]) |
| 337 | + |
| 338 | + def testCopyArchiveArchTagNotAvailableInSource(self): |
| 339 | + """Test creating a copy archive for an arch not in the source. |
| 340 | + |
| 341 | + If we request a copy to an architecture that doesn't have |
| 342 | + a DistroArchSeries in the source then we won't get any builds |
| 343 | + created in the copy archive. |
| 344 | + """ |
| 345 | + family = self.factory.makeProcessorFamily(name="armel") |
| 346 | + self.factory.makeProcessor(family=family, name="armel") |
| 347 | + package_info = PackageInfo( |
| 348 | + "bzr", "2.1", status=PackagePublishingStatus.PUBLISHED) |
| 349 | + owner = self.createTargetOwner() |
| 350 | + # Creates an archive with just x86 |
| 351 | + distroseries = self.createSourceDistribution([package_info]) |
| 352 | + archive_name = self.getTargetArchiveName(distroseries.distribution) |
| 353 | + # Different architecture, so there won't be any builds |
| 354 | + copy_archive = self.copyArchive( |
| 355 | + distroseries, archive_name, owner, architectures=["armel"]) |
| 356 | + self.checkBuilds(copy_archive, []) |
| 357 | + |
| 358 | + # Also, make sure the package copy request status was updated. |
| 359 | + [pcr] = getUtility( |
| 360 | + IPackageCopyRequestSet).getByTargetArchive(copy_archive) |
| 361 | + self.assertTrue(pcr.status == PackageCopyStatus.COMPLETE) |
| 362 | + |
| 363 | + # This date is set when the copy request makes the transition to |
| 364 | + # the "in progress" state. |
| 365 | + self.assertTrue(pcr.date_started is not None) |
| 366 | + # This date is set when the copy request makes the transition to |
| 367 | + # the "completed" state. |
| 368 | + self.assertTrue(pcr.date_completed is not None) |
| 369 | + self.assertTrue(pcr.date_started <= pcr.date_completed) |
| 370 | + |
| 371 | + def testMultipleArchTagsWithSubsetInSource(self): |
| 372 | + """Try copy archive population with multiple architecture tags. |
| 373 | + |
| 374 | + The user may specify a number of given architecture tags on the |
| 375 | + command line. |
| 376 | + The script should create build records only for the specified |
| 377 | + architecture tags that are supported by the destination distro series. |
| 378 | + |
| 379 | + In this (test) case the script should create the build records for the |
| 380 | + '386' architecture. |
| 381 | + """ |
| 382 | + family = self.factory.makeProcessorFamily(name="armel") |
| 383 | + self.factory.makeProcessor(family=family, name="armel") |
| 384 | + package_info = PackageInfo( |
| 385 | + "bzr", "2.1", status=PackagePublishingStatus.PUBLISHED) |
| 386 | + owner = self.createTargetOwner() |
| 387 | + # Creates an archive with just x86 |
| 388 | + distroseries = self.createSourceDistribution([package_info]) |
| 389 | + archive_name = self.getTargetArchiveName(distroseries.distribution) |
| 390 | + # There is only a DAS for i386, so armel won't produce any |
| 391 | + # builds |
| 392 | + copy_archive = self.copyArchive( |
| 393 | + distroseries, archive_name, owner, |
| 394 | + architectures=["386", "armel"]) |
| 395 | + self.checkBuilds(copy_archive, [package_info]) |
| 396 | + |
| 397 | + def testCopyArchiveCreatesSubsetOfBuilds(self): |
| 398 | + """Create a copy archive with a subset of the architectures. |
| 399 | + |
| 400 | + We copy from an archive with multiple architecture DistroArchSeries, |
| 401 | + but request only one of those architectures in the target, |
| 402 | + so we only get builds for that one architecture. |
| 403 | + """ |
| 404 | + package_info = PackageInfo( |
| 405 | + "bzr", "2.1", status=PackagePublishingStatus.PUBLISHED) |
| 406 | + owner = self.createTargetOwner() |
| 407 | + distroseries = self.createSourceDistribution([package_info]) |
| 408 | + self.factory.makeDistroArchSeries( |
| 409 | + distroseries=distroseries, architecturetag="amd64", |
| 410 | + processorfamily=ProcessorFamilySet().getByName("amd64"), |
| 411 | + supports_virtualized=True) |
| 412 | + archive_name = self.getTargetArchiveName(distroseries.distribution) |
| 413 | + copy_archive = self.copyArchive( |
| 414 | + distroseries, archive_name, owner, |
| 415 | + architectures=["386"]) |
| 416 | + # We only get a single build, as we only requested 386, not |
| 417 | + # amd64 too |
| 418 | + self.checkBuilds(copy_archive, [package_info]) |
| 419 | + |
| 420 | + def testMultipleArchTags(self): |
| 421 | + """Test copying an archive with multiple architectures. |
| 422 | + |
| 423 | + We create a source with two architectures, and then request |
| 424 | + a copy of both, so we get a build for each of those architectures. |
| 425 | + """ |
| 426 | + package_info = PackageInfo( |
| 427 | + "bzr", "2.1", status=PackagePublishingStatus.PUBLISHED) |
| 428 | + owner = self.createTargetOwner() |
| 429 | + distroseries = self.createSourceDistribution([package_info]) |
| 430 | + self.factory.makeDistroArchSeries( |
| 431 | + distroseries=distroseries, architecturetag="amd64", |
| 432 | + processorfamily=ProcessorFamilySet().getByName("amd64"), |
| 433 | + supports_virtualized=True) |
| 434 | + archive_name = self.getTargetArchiveName(distroseries.distribution) |
| 435 | + copy_archive = self.copyArchive( |
| 436 | + distroseries, archive_name, owner, |
| 437 | + architectures=["386", "amd64"]) |
| 438 | + self.checkBuilds(copy_archive, [package_info, package_info]) |
| 439 | + |
| 440 | + def testCopyArchiveCopiesAllComponents(self): |
| 441 | + """Test that packages from all components are copied. |
| 442 | + |
| 443 | + When copying you specify a component, but that component doesn't |
| 444 | + limit the packages copied. We create a source in main and one in |
| 445 | + universe, and then copy with --component main, and expect to see |
| 446 | + both sources in the copy. |
| 447 | + """ |
| 448 | + package_infos = [ |
| 449 | + PackageInfo( |
| 450 | + "bzr", "2.1", status=PackagePublishingStatus.PUBLISHED, |
| 451 | + component="universe"), |
| 452 | + PackageInfo( |
| 453 | + "apt", "2.2", status=PackagePublishingStatus.PUBLISHED, |
| 454 | + component="main")] |
| 455 | + copy_archive, distroseries = self.makeCopyArchive(package_infos, |
| 456 | + component="main") |
| 457 | + self.checkBuilds(copy_archive, package_infos) |
| 458 | + |
| 459 | + def testCopyFromPPA(self): |
| 460 | + """Test we can create a copy archive with a PPA as the source.""" |
| 461 | + ppa_owner_name = "ppa-owner" |
| 462 | + ppa_name = "ppa" |
| 463 | + ppa_owner = self.factory.makePerson(name=ppa_owner_name) |
| 464 | + distroseries = self.createSourceDistroSeries() |
| 465 | + ppa = self.factory.makeArchive( |
| 466 | + name=ppa_name, purpose=ArchivePurpose.PPA, |
| 467 | + distribution=distroseries.distribution, owner=ppa_owner) |
| 468 | + package_info = PackageInfo( |
| 469 | + "bzr", "2.1", status=PackagePublishingStatus.PUBLISHED, |
| 470 | + component="universe") |
| 471 | + self.factory.makeSourcePackagePublishingHistory( |
| 472 | + sourcepackagename=self.factory.getOrMakeSourcePackageName( |
| 473 | + name=package_info.name), |
| 474 | + distroseries=distroseries, component=self.factory.makeComponent( |
| 475 | + package_info.component), |
| 476 | + version=package_info.version, archive=ppa, |
| 477 | + status=package_info.status, architecturehintlist='any', |
| 478 | + pocket=PackagePublishingPocket.RELEASE) |
| 479 | + owner = self.createTargetOwner() |
| 480 | + archive_name = self.getTargetArchiveName(distroseries.distribution) |
| 481 | + copy_archive = self.copyArchive( |
| 482 | + distroseries, archive_name, owner, from_user=ppa_owner_name, |
| 483 | + from_archive=ppa_name) |
| 484 | + self.checkCopiedSources( |
| 485 | + copy_archive, distroseries, [package_info]) |
| 486 | + |
| 487 | def runScript( |
| 488 | self, archive_name=None, suite='hoary', user='salgado', |
| 489 | exists_before=None, exists_after=None, exception_type=None, |
| 490 | @@ -261,7 +668,7 @@ |
| 491 | the script should fail with an appropriate error message. |
| 492 | """ |
| 493 | now = int(time.time()) |
| 494 | - # The colons in the name make it invalid. |
| 495 | + # The slashes in the name make it invalid. |
| 496 | invalid_name = "ra//%s" % now |
| 497 | |
| 498 | extra_args = ['-a', '386'] |
| 499 | @@ -305,77 +712,6 @@ |
| 500 | exception_type=SoyuzScriptError, |
| 501 | exception_text="Invalid user name: '%s'" % invalid_user) |
| 502 | |
| 503 | - def testArchWithoutBuilds(self): |
| 504 | - """Try copy archive population with no builds. |
| 505 | - |
| 506 | - The user may specify a number of given architecture tags on the |
| 507 | - command line. |
| 508 | - The script should create build records only for the specified |
| 509 | - architecture tags that are supported by the destination distro series. |
| 510 | - |
| 511 | - In this (test) case the specified architecture tag should have the |
| 512 | - effect that no build records are created. |
| 513 | - """ |
| 514 | - hoary = getUtility(IDistributionSet)['ubuntu']['hoary'] |
| 515 | - |
| 516 | - # Verify that we have the right source packages in the sample data. |
| 517 | - self._verifyPackagesInSampleData(hoary) |
| 518 | - |
| 519 | - # Restrict the builds to be created to the 'amd64' architecture |
| 520 | - # only. This should result in zero builds. |
| 521 | - extra_args = ['-a', 'amd64'] |
| 522 | - copy_archive = self.runScript( |
| 523 | - extra_args=extra_args, exists_after=True, reason="zero builds") |
| 524 | - |
| 525 | - # Make sure the right source packages were cloned. |
| 526 | - self._verifyClonedSourcePackages(copy_archive, hoary) |
| 527 | - |
| 528 | - # Now check that we have zero build records for the sources cloned. |
| 529 | - builds = list(getUtility(IBinaryPackageBuildSet).getBuildsForArchive( |
| 530 | - copy_archive, status=BuildStatus.NEEDSBUILD)) |
| 531 | - build_spns = [ |
| 532 | - get_spn(removeSecurityProxy(build)).name for build in builds] |
| 533 | - |
| 534 | - self.assertTrue(len(build_spns) == 0) |
| 535 | - |
| 536 | - # Also, make sure the package copy request status was updated. |
| 537 | - [pcr] = getUtility( |
| 538 | - IPackageCopyRequestSet).getByTargetArchive(copy_archive) |
| 539 | - self.assertTrue(pcr.status == PackageCopyStatus.COMPLETE) |
| 540 | - |
| 541 | - # This date is set when the copy request makes the transition to |
| 542 | - # the "in progress" state. |
| 543 | - self.assertTrue(pcr.date_started is not None) |
| 544 | - # This date is set when the copy request makes the transition to |
| 545 | - # the "completed" state. |
| 546 | - self.assertTrue(pcr.date_completed is not None) |
| 547 | - self.assertTrue(pcr.date_started <= pcr.date_completed) |
| 548 | - |
| 549 | - # Last but not least, check that the copy archive creation reason was |
| 550 | - # captured as well. |
| 551 | - self.assertTrue(pcr.reason == 'zero builds') |
| 552 | - |
| 553 | - def testCopyFromPPA(self): |
| 554 | - """Try copy archive population from a PPA. |
| 555 | - |
| 556 | - In this (test) case an archive is populated from a PPA. |
| 557 | - """ |
| 558 | - warty = getUtility(IDistributionSet)['ubuntu']['warty'] |
| 559 | - archive_set = getUtility(IArchiveSet) |
| 560 | - ppa = archive_set.getPPAByDistributionAndOwnerName( |
| 561 | - warty.distribution, 'cprov', 'ppa') |
| 562 | - |
| 563 | - # Verify that we have the right source packages in the sample data. |
| 564 | - packages = self._getPendingPackageNames(ppa, warty) |
| 565 | - |
| 566 | - # Take a snapshot of the PPA. |
| 567 | - extra_args = ['-a', 'amd64', '--from-user', 'cprov'] |
| 568 | - copy_archive = self.runScript( |
| 569 | - suite='warty', extra_args=extra_args, exists_after=True) |
| 570 | - |
| 571 | - copies = self._getPendingPackageNames(copy_archive, warty) |
| 572 | - self.assertEqual(packages, copies) |
| 573 | - |
| 574 | def testPackagesetDelta(self): |
| 575 | """Try to calculate the delta between two source package sets.""" |
| 576 | hoary = getUtility(IDistributionSet)['ubuntu']['hoary'] |
| 577 | @@ -402,7 +738,7 @@ |
| 578 | "INFO: * new-in-second-round (1.0)\n") |
| 579 | |
| 580 | extra_args = ['--package-set-delta'] |
| 581 | - copy_archive = self.runScript( |
| 582 | + self.runScript( |
| 583 | extra_args=extra_args, reason='', output_substr=expected_output, |
| 584 | copy_archive_name=first_stage.name) |
| 585 | |
| 586 | @@ -477,7 +813,7 @@ |
| 587 | This test should provoke a `SoyuzScriptError` exception. |
| 588 | """ |
| 589 | extra_args = ['-a', 'amd64', '--from-archive', '9th-level-cache'] |
| 590 | - copy_archive = self.runScript( |
| 591 | + self.runScript( |
| 592 | extra_args=extra_args, |
| 593 | exception_type=SoyuzScriptError, |
| 594 | exception_text="Origin archive does not exist: '9th-level-cache'") |
| 595 | @@ -488,7 +824,7 @@ |
| 596 | This test should provoke a `SoyuzScriptError` exception. |
| 597 | """ |
| 598 | extra_args = ['-a', 'amd64', '--from-user', 'king-kong'] |
| 599 | - copy_archive = self.runScript( |
| 600 | + self.runScript( |
| 601 | extra_args=extra_args, |
| 602 | exception_type=SoyuzScriptError, |
| 603 | exception_text="No PPA for user: 'king-kong'") |
| 604 | @@ -500,7 +836,7 @@ |
| 605 | """ |
| 606 | extra_args = [ |
| 607 | '-a', 'amd64', '--from-archive', '//'] |
| 608 | - copy_archive = self.runScript( |
| 609 | + self.runScript( |
| 610 | extra_args=extra_args, |
| 611 | exception_type=SoyuzScriptError, |
| 612 | exception_text="Invalid origin archive name: '//'") |
| 613 | @@ -511,7 +847,7 @@ |
| 614 | This test should provoke a `SoyuzScriptError` exception. |
| 615 | """ |
| 616 | extra_args = ['-a', 'wintel'] |
| 617 | - copy_archive = self.runScript( |
| 618 | + self.runScript( |
| 619 | extra_args=extra_args, |
| 620 | exception_type=SoyuzScriptError, |
| 621 | exception_text="Invalid architecture tag: 'wintel'") |
| 622 | @@ -531,7 +867,7 @@ |
| 623 | extra_args=extra_args, exists_before=False) |
| 624 | |
| 625 | extra_args = ['--merge-copy', '-a', '386', '-a', 'amd64'] |
| 626 | - copy_archive = self.runScript( |
| 627 | + self.runScript( |
| 628 | extra_args=extra_args, copy_archive_name=copy_archive.name, |
| 629 | exception_type=SoyuzScriptError, |
| 630 | exception_text=( |
| 631 | @@ -549,7 +885,7 @@ |
| 632 | needed. |
| 633 | """ |
| 634 | extra_args = ['-a', 'amd64'] |
| 635 | - copy_archive = self.runScript( |
| 636 | + self.runScript( |
| 637 | # Pass an empty reason parameter string to indicate that no |
| 638 | # '--reason' command line argument is to be provided. |
| 639 | extra_args=extra_args, reason='', |
| 640 | @@ -566,7 +902,7 @@ |
| 641 | *existing* archives. |
| 642 | """ |
| 643 | extra_args = ['--merge-copy', '-a', 'amd64'] |
| 644 | - copy_archive = self.runScript( |
| 645 | + self.runScript( |
| 646 | extra_args=extra_args, |
| 647 | exception_type=SoyuzScriptError, |
| 648 | exception_text=( |
| 649 | @@ -581,11 +917,11 @@ |
| 650 | with the same name and distribution. |
| 651 | """ |
| 652 | extra_args = ['-a', 'amd64'] |
| 653 | - copy_archive = self.runScript( |
| 654 | + self.runScript( |
| 655 | extra_args=extra_args, exists_after=True, |
| 656 | copy_archive_name='hello-1') |
| 657 | extra_args = ['-a', 'amd64'] |
| 658 | - copy_archive = self.runScript( |
| 659 | + self.runScript( |
| 660 | extra_args=extra_args, |
| 661 | copy_archive_name='hello-1', exception_type=SoyuzScriptError, |
| 662 | exception_text=( |
| 663 | @@ -596,60 +932,10 @@ |
| 664 | |
| 665 | This test should provoke a `SoyuzScriptError` exception. |
| 666 | """ |
| 667 | - copy_archive = self.runScript( |
| 668 | + self.runScript( |
| 669 | exception_type=SoyuzScriptError, |
| 670 | exception_text="error: architecture tags not specified.") |
| 671 | |
| 672 | - def testMultipleArchTags(self): |
| 673 | - """Try copy archive population with multiple architecture tags. |
| 674 | - |
| 675 | - The user may specify a number of given architecture tags on the |
| 676 | - command line. |
| 677 | - The script should create build records only for the specified |
| 678 | - architecture tags that are supported by the destination distro series. |
| 679 | - |
| 680 | - In this (test) case the script should create the build records for the |
| 681 | - '386' architecture. |
| 682 | - """ |
| 683 | - hoary = getUtility(IDistributionSet)['ubuntu']['hoary'] |
| 684 | - |
| 685 | - # Verify that we have the right source packages in the sample data. |
| 686 | - self._verifyPackagesInSampleData(hoary) |
| 687 | - |
| 688 | - # Please note: |
| 689 | - # * the 'amd64' DistroArchSeries has no resulting builds. |
| 690 | - # * the '-a' command line parameter is cumulative in nature |
| 691 | - # i.e. the 'amd64' architecture tag specified after the '386' |
| 692 | - # tag does not overwrite the latter but is added to it. |
| 693 | - extra_args = ['-a', '386', '-a', 'amd64'] |
| 694 | - copy_archive = self.runScript( |
| 695 | - extra_args=extra_args, exists_after=True) |
| 696 | - |
| 697 | - # Make sure the right source packages were cloned. |
| 698 | - self._verifyClonedSourcePackages(copy_archive, hoary) |
| 699 | - |
| 700 | - # Now check that we have the build records expected. |
| 701 | - builds = list(getUtility(IBinaryPackageBuildSet).getBuildsForArchive( |
| 702 | - copy_archive, status=BuildStatus.NEEDSBUILD)) |
| 703 | - build_spns = [ |
| 704 | - get_spn(removeSecurityProxy(build)).name for build in builds] |
| 705 | - self.assertEqual(build_spns, self.expected_build_spns) |
| 706 | - |
| 707 | - def get_family_names(result_set): |
| 708 | - """Extract processor family names from result set.""" |
| 709 | - family_names = [] |
| 710 | - for archivearch in rset: |
| 711 | - family_names.append( |
| 712 | - removeSecurityProxy(archivearch).processorfamily.name) |
| 713 | - |
| 714 | - family_names.sort() |
| 715 | - return family_names |
| 716 | - |
| 717 | - # Make sure that the processor family names specified for the copy |
| 718 | - # archive at hand were stored in the database. |
| 719 | - rset = getUtility(IArchiveArchSet).getByArchive(copy_archive) |
| 720 | - self.assertEqual(get_family_names(rset), [u'amd64', u'x86']) |
| 721 | - |
| 722 | def testBuildsPendingAndSuspended(self): |
| 723 | """All builds in the new copy archive are pending and suspended.""" |
| 724 | def build_in_wrong_state(build): |
| 725 | @@ -695,11 +981,11 @@ |
| 726 | # We will make a private PPA and then attempt to copy from it. |
| 727 | joe = self.factory.makePerson(name='joe') |
| 728 | ubuntu = getUtility(IDistributionSet).getByName('ubuntu') |
| 729 | - joes_private_ppa = self.factory.makeArchive( |
| 730 | + self.factory.makeArchive( |
| 731 | owner=joe, private=True, name="ppa", distribution=ubuntu) |
| 732 | |
| 733 | extra_args = ['--from-user', 'joe', '-a', 'amd64'] |
| 734 | - copy_archive = self.runScript( |
| 735 | + self.runScript( |
| 736 | extra_args=extra_args, exception_type=SoyuzScriptError, |
| 737 | exception_text=( |
| 738 | "Cannot copy from private archive ('joe/ppa')")) |
| 739 | @@ -719,7 +1005,7 @@ |
| 740 | enabled=False) |
| 741 | |
| 742 | extra_args = ['--from-user', 'cprov', '--merge-copy'] |
| 743 | - copy_archive = self.runScript( |
| 744 | + self.runScript( |
| 745 | copy_archive_name=disabled_archive.name, reason='', |
| 746 | extra_args=extra_args, exception_type=SoyuzScriptError, |
| 747 | exception_text='error: cannot copy to disabled archive') |
| 748 | @@ -758,11 +1044,11 @@ |
| 749 | ubuntu = getUtility(IDistributionSet).getByName('ubuntu') |
| 750 | hoary = ubuntu.getSeries('hoary') |
| 751 | test_publisher.addFakeChroots(hoary) |
| 752 | - unused = test_publisher.setUpDefaultDistroSeries(hoary) |
| 753 | - new_package = test_publisher.getPubSource( |
| 754 | + test_publisher.setUpDefaultDistroSeries(hoary) |
| 755 | + test_publisher.getPubSource( |
| 756 | sourcename="new-in-second-round", version="1.0", |
| 757 | distroseries=hoary, archive=ubuntu.main_archive) |
| 758 | - fresher_package = test_publisher.getPubSource( |
| 759 | + test_publisher.getPubSource( |
| 760 | sourcename="alsa-utils", version="2.0", distroseries=hoary, |
| 761 | archive=ubuntu.main_archive) |
| 762 | sources = ubuntu.main_archive.getPublishedSources( |
Hi James,
These changes look good to me; I only have a few comments.
review approve
On Mon, 2010-06-21 at 16:21 +0000, James Westby wrote:
[...]
>
> The copy-archive tests are hard to understand as they use
> the sampledata and test a lot of things in some methods.
> There is even a method to check that the sampledata doesn't
> change.
>
> Proposed fix
>
> Use the factory to create the data needed for a specific test,
> and then break the tests down in to unit tests that test one
> thing only.
>
> Pre-implementation notes
>
> None.
>
> Implementation details
>
> There is some duplication here, because I wasn't comfortable deleting
> some of the existing tests as I wasn't sure what I hadn't tested with
> new tests.
Maybe you can mark the old tests with XXXs for someone who knows more
about this check whether or not they can be removed?
>
> There are also two methods for running the script now, I left the original
> one as it handled checking error messages, and that's what most of the
> tests that use it now do.
>
> I also created a small helper class to save passing around lots of values
> between methods.
>
> Tests
>
> None as it is just test changes. You can run the tests in that file
> with
>
> ./bin/test -cvv -s lp.soyuz.
>
> Demo and Q/A
>
> N/A
>
> lint
>
> None.
>
> Thanks,
>
> James
>
> differences between files attachment (review-diff.txt)
> === modified file 'lib/lp/
> --- lib/lp/
> +++ lib/lp/
> @@ -386,7 +386,7 @@
> :param distroseries: the distro series for which to create builds.
> :param archive: the archive for which to create builds.
> :param proc_families: the list of processor families for
> - which to create builds (optional).
> + which to create builds.
> """
> # Avoid circular imports.
> from lp.soyuz.
>
> === modified file 'lib/lp/
> --- lib/lp/
> +++ lib/lp/
> @@ -39,6 +40,16 @@
> return pub.sourcepacka
>
>
> +class PackageInfo:
> +
> + def __init__(self, name, version,
> + status=
I think the style guide says that when you have a line break in the
middle of a list of arguments you should indent all arguments at the
same level, so
def foo(self, bar, baz,
> + self.name = name
> + self.version = version
> + self.status = status
> + self.component = component
> +
> +
> class TestPopulateArc
> """Test the copy-package.py script."""
>
> @@ -118,6 +129,114 @@
> distro, ArchivePurpose.
> self.assertTrue
>
> + # Make sure the right source packa...