Merge lp:~james-w/launchpad/copy-archive-test-refactor into lp:launchpad

[James Westby]

Move some copy archive tests to use the factory, and add some more specific tests.

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.scripts.tests -m test_populatearchive

Demo and Q/A

N/A

lint

None.

Thanks,

James

[Guilherme Salgado (salgado)]

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.scripts.tests -m test_populatearchive
>
> Demo and Q/A
>
> N/A
>
> lint
>
> None.
>
> Thanks,
>
> James
>
> differences between files attachment (review-diff.txt)
> === modified file 'lib/lp/soyuz/scripts/populate_archive.py'
> --- lib/lp/soyuz/scripts/populate_archive.py 2009-06-25 04:06:00 +0000
> +++ lib/lp/soyuz/scripts/populate_archive.py 2010-06-21 16:21:31 +0000
> @@ -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.interfaces.publishing import active_publishing_status
>
> === modified file 'lib/lp/soyuz/scripts/tests/test_populatearchive.py'
> --- lib/lp/soyuz/scripts/tests/test_populatearchive.py 2010-05-20 15:27:12 +0000
> +++ lib/lp/soyuz/scripts/tests/test_populatearchive.py 2010-06-21 16:21:31 +0000
> @@ -39,6 +40,16 @@
> return pub.sourcepackagerelease.sourcepackagename
>
>
> +class PackageInfo:
> +
> + def __init__(self, name, version,
> + status=PackagePublishingStatus.PUBLISHED, component="main"):

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...):

> + self.name = name
> + self.version = version
> + self.status = status
> + self.component = component
> +
> +
> class TestPopulateArchiveScript(TestCaseWithFactory):
> """Test the copy-package.py script."""
>
> @@ -118,6 +129,114 @@
> distro, ArchivePurpose.COPY, archive_name)
> self.assertTrue(copy_archive is not None)
>
> + # Make sure the right source packa...

review: Approve

[James Westby (james-w)]

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=PackagePublishingStatus.PUBLISHED, component="main"):
>
> 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 createSourceDistribution(self, package_infos):
> > + """Create a distribution to be the source of a copy archive."""
> > + distroseries = self.createSourceDistroSeries()
> > + self.createSourcePublications(package_infos, distroseries)
> > + 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_package_release is not public, though? Do
> you know?

I don't know.

> Does this one deserve a comment as well?

Yes, will add it.

Thanks,

James

[James Westby (james-w)]

All comments addressed.

Julian, would you take a look to check you are happy with the proposed
changes?

Thanks,

James

[Julian Edwards (julian-edwards)]

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

[James Westby (james-w)]

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

[Julian Edwards (julian-edwards)]

Loads better, thanks James.

review: Approve

=== modified file 'lib/lp/soyuz/scripts/populate_archive.py'
--- lib/lp/soyuz/scripts/populate_archive.py	2009-06-25 04:06:00 +0000
+++ lib/lp/soyuz/scripts/populate_archive.py	2010-06-22 11:23:29 +0000
@@ -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.interfaces.publishing import active_publishing_status

=== modified file 'lib/lp/soyuz/scripts/tests/test_populatearchive.py'
--- lib/lp/soyuz/scripts/tests/test_populatearchive.py	2010-05-20 15:27:12 +0000
+++ lib/lp/soyuz/scripts/tests/test_populatearchive.py	2010-06-22 11:23:29 +0000
@@ -14,19 +14,21 @@
 from zope.security.proxy import removeSecurityProxy
 
 from canonical.config import config
-from canonical.launchpad.scripts import BufferLogger
+from canonical.launchpad.scripts import (
+    BufferLogger, QuietFakeLogger)
 from canonical.testing import LaunchpadZopelessLayer
 from canonical.testing.layers import DatabaseLayer
 from lp.buildmaster.interfaces.buildbase import BuildStatus
 from lp.registry.interfaces.distribution import IDistributionSet
 from lp.registry.interfaces.person import IPersonSet
+from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.services.job.interfaces.job import JobStatus
 from lp.soyuz.interfaces.archive import ArchivePurpose, IArchiveSet
-from lp.soyuz.interfaces.archivearch import IArchiveArchSet
 from lp.soyuz.interfaces.binarypackagebuild import IBinaryPackageBuildSet
 from lp.soyuz.interfaces.packagecopyrequest import (
     IPackageCopyRequestSet, PackageCopyStatus)
 from lp.soyuz.interfaces.publishing import PackagePublishingStatus
+from lp.soyuz.model.processor import ProcessorFamilySet
 from lp.soyuz.scripts.ftpmaster import PackageLocationError, SoyuzScriptError
 from lp.soyuz.scripts.populate_archive import ArchivePopulator
 from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
@@ -39,6 +41,16 @@
     return pub.sourcepackagerelease.sourcepackagename
 
 
+class PackageInfo:
+
+    def __init__(self, name, version,
+                 status=PackagePublishingStatus.PUBLISHED, component="main"):
+        self.name = name
+        self.version = version
+        self.status = status
+        self.component = component
+
+
 class TestPopulateArchiveScript(TestCaseWithFactory):
     """Test the copy-package.py script."""
 
@@ -77,6 +89,10 @@
 
         Use the hoary-RELEASE suite along with the main component.
         """
+        # XXX: JamesWestby 2010-06-21 bug=596984: it is not clear
+        # what this test is testing that is not covered in more
+        # specific tests. It should be removed if there is nothing
+        # else as it is fragile due to use of sampledata.
         DatabaseLayer.force_dirty_database()
         # Make sure a copy archive with the desired name does
         # not exist yet.
@@ -118,14 +134,6 @@
             distro, ArchivePurpose.COPY, archive_name)
         self.assertTrue(copy_archive is not None)
 
-        # Ascertain that the new copy archive was created with the 'enabled'
-        # flag turned off.
-        self.assertFalse(copy_archive.enabled)
-
-        # Also, make sure that the builds for the new copy archive will be
-        # carried out on non-virtual builders.
-        self.assertTrue(copy_archive.require_virtualized)
-
         # Make sure the right source packages were cloned.
         self._verifyClonedSourcePackages(copy_archive, hoary)
 
@@ -142,6 +150,405 @@
 
         self.assertEqual(build_spns, self.expected_build_spns)
 
+    def createSourceDistroSeries(self):
+        """Create a DistroSeries suitable for copying.
+
+        Creates a distroseries with a DistroArchSeries and nominatedarchindep,
+        which makes it suitable for copying because it will create some builds.
+        """
+        distro_name = "foobuntu"
+        distro = self.factory.makeDistribution(name=distro_name)
+        distroseries_name = "maudlin"
+        distroseries = self.factory.makeDistroSeries(
+            distribution=distro, name=distroseries_name)
+        das = self.factory.makeDistroArchSeries(
+            distroseries=distroseries, architecturetag="i386",
+            processorfamily=ProcessorFamilySet().getByName("x86"),
+            supports_virtualized=True)
+        distroseries.nominatedarchindep = das
+        return distroseries
+
+    def createTargetOwner(self):
+        """Create a person suitable to own a copy archive."""
+        person_name = "copy-archive-owner"
+        owner = self.factory.makePerson(name=person_name)
+        return owner
+
+    def getTargetArchiveName(self, distribution):
+        """Get a suitable name for a copy archive.
+
+        It also checks that the archive doesn't currently exist.
+        """
+        archive_name = "msa%s" % int(time.time())
+        copy_archive = getUtility(IArchiveSet).getByDistroPurpose(
+            distribution, ArchivePurpose.COPY, archive_name)
+        # This is a sanity check: a copy archive with this name should not
+        # exist yet.
+        self.assertIs(None, copy_archive)
+        return archive_name
+
+    def createSourcePublication(self, info, distroseries):
+        """Create a SourcePackagePublishingHistory based on a PackageInfo."""
+        self.factory.makeSourcePackagePublishingHistory(
+            sourcepackagename=self.factory.getOrMakeSourcePackageName(
+                name=info.name),
+            distroseries=distroseries, component=self.factory.makeComponent(
+                info.component),
+            version=info.version, architecturehintlist='any',
+            archive=distroseries.distribution.main_archive,
+            status=info.status, pocket=PackagePublishingPocket.RELEASE)
+
+    def createSourcePublications(self, package_infos, distroseries):
+        """Create a source publication for each item in package_infos."""
+        for package_info in package_infos:
+            self.createSourcePublication(package_info, distroseries)
+
+    def getScript(self, test_args=None):
+        """Return an ArchivePopulator instance."""
+        if test_args is None:
+           test_args = []
+        script = ArchivePopulator("test copy archives", test_args=test_args)
+        script.logger = QuietFakeLogger()
+        script.txn = self.layer.txn
+        return script
+
+    def copyArchive(self, distroseries, archive_name, owner,
+        architectures=None, component="main", from_user=None,
+        from_archive=None):
+        """Run the copy-archive script."""
+        extra_args = [
+            '--from-distribution', distroseries.distribution.name,
+            '--from-suite', distroseries.name,
+            '--to-distribution', distroseries.distribution.name,
+            '--to-suite', distroseries.name,
+            '--to-archive', archive_name,
+            '--to-user', owner.name,
+            '--reason',
+            '"copy archive from %s"' % datetime.ctime(datetime.utcnow()),
+            '--component', component,
+            ]
+
+        if from_user is not None:
+            extra_args.extend(["--from-user", from_user])
+
+        if from_archive is not None:
+            extra_args.extend(["--from-archive", from_archive])
+
+        if architectures is None:
+            architectures = ["386"]
+
+        for architecture in architectures:
+            extra_args.extend(['-a', architecture])
+
+        script = self.getScript(test_args=extra_args)
+        script.mainTask()
+
+        # Make sure the copy archive with the desired name was
+        # created
+        copy_archive = getUtility(IArchiveSet).getByDistroPurpose(
+            distroseries.distribution, ArchivePurpose.COPY, archive_name)
+        self.assertTrue(copy_archive is not None)
+
+        # Ascertain that the new copy archive was created with the 'enabled'
+        # flag turned off.
+        self.assertFalse(copy_archive.enabled)
+
+        # Also, make sure that the builds for the new copy archive will be
+        # carried out on non-virtual builders.
+        self.assertTrue(copy_archive.require_virtualized)
+
+        return copy_archive
+
+    def checkCopiedSources(self, archive, distroseries, expected):
+        """Check the sources published in an archive against an expected set.
+
+        Given an archive and a target distroseries the sources published in
+        that distroseries are checked against a set of PackageInfo to
+        ensure that the correct package names and versions are published.
+        """
+        expected_set = set([(info.name, info.version) for info in expected])
+        sources = archive.getPublishedSources(
+            distroseries=distroseries, status=self.pending_statuses)
+        actual_set = set()
+        for source in sources:
+            source = removeSecurityProxy(source)
+            actual_set.add(
+                (source.source_package_name, source.source_package_version))
+        self.assertEqual(expected_set, actual_set)
+
+    def createSourceDistribution(self, package_infos):
+        """Create a distribution to be the source of a copy archive."""
+        distroseries = self.createSourceDistroSeries()
+        self.createSourcePublications(package_infos, distroseries)
+        return distroseries
+
+    def makeCopyArchive(self, package_infos, component="main"):
+        """Make a copy archive based on a new distribution."""
+        owner = self.createTargetOwner()
+        distroseries = self.createSourceDistribution(package_infos)
+        archive_name = self.getTargetArchiveName(distroseries.distribution)
+        copy_archive = self.copyArchive(
+            distroseries, archive_name, owner, component=component)
+        return (copy_archive, distroseries)
+
+    def checkBuilds(self, archive, package_infos):
+        """Check the build records pending in an archive.
+
+        Given a set of PackageInfo objects check that each has a build
+        created for it.
+        """
+        expected_builds = list(
+            [(info.name, info.version) for info in package_infos])
+        builds = list(
+            getUtility(IBinaryPackageBuildSet).getBuildsForArchive(
+            archive, status=BuildStatus.NEEDSBUILD))
+        actual_builds = list()
+        for build in builds:
+            naked_build = removeSecurityProxy(build)
+            spr = naked_build.source_package_release
+            actual_builds.append((spr.name, spr.version))
+        self.assertEqual(sorted(expected_builds), sorted(actual_builds))
+
+    def testCopyArchiveRunScript(self):
+        """Check that we can exec the script to copy an archive."""
+        package_info = PackageInfo(
+            "bzr", "2.1", status=PackagePublishingStatus.PUBLISHED)
+        owner = self.createTargetOwner()
+        distroseries = self.createSourceDistribution([package_info])
+        archive_name = self.getTargetArchiveName(distroseries.distribution)
+        # We must commit as we are going to exec a script that will run
+        # in a different transaction and must be able to see the
+        # objects we just created.
+        self.layer.commit()
+
+        extra_args = [
+            '--from-distribution', distroseries.distribution.name,
+            '--from-suite', distroseries.name,
+            '--to-distribution', distroseries.distribution.name,
+            '--to-suite', distroseries.name,
+            '--to-archive', archive_name,
+            '--to-user', owner.name,
+            '--reason',
+            '"copy archive from %s"' % datetime.ctime(datetime.utcnow()),
+            '--component', "main",
+            '-a', '386',
+            ]
+        (exitcode, out, err) = self.runWrapperScript(extra_args)
+        # Check for zero exit code.
+        self.assertEqual(
+            exitcode, 0, "\n=> %s\n=> %s\n=> %s\n" % (exitcode, out, err))
+        # Make sure the copy archive with the desired name was
+        # created
+        copy_archive = getUtility(IArchiveSet).getByDistroPurpose(
+            distroseries.distribution, ArchivePurpose.COPY, archive_name)
+        self.assertTrue(copy_archive is not None)
+
+        # Ascertain that the new copy archive was created with the 'enabled'
+        # flag turned off.
+        self.assertFalse(copy_archive.enabled)
+
+        # Also, make sure that the builds for the new copy archive will be
+        # carried out on non-virtual builders.
+        self.assertTrue(copy_archive.require_virtualized)
+        self.checkCopiedSources(
+            copy_archive, distroseries, [package_info])
+
+    def testCopyArchiveCreateCopiesPublished(self):
+        """Test that PUBLISHED sources are copied."""
+        package_info = PackageInfo(
+            "bzr", "2.1", status=PackagePublishingStatus.PUBLISHED)
+        copy_archive, distroseries = self.makeCopyArchive([package_info])
+        self.checkCopiedSources(
+            copy_archive, distroseries, [package_info])
+
+    def testCopyArchiveCreateCopiesPending(self):
+        """Test that PENDING sources are copied."""
+        package_info = PackageInfo(
+            "bzr", "2.1", status=PackagePublishingStatus.PENDING)
+        copy_archive, distroseries = self.makeCopyArchive([package_info])
+        self.checkCopiedSources(
+            copy_archive, distroseries, [package_info])
+
+    def testCopyArchiveCreateDoesntCopySuperseded(self):
+        """Test that SUPERSEDED sources are not copied."""
+        package_info = PackageInfo(
+            "bzr", "2.1", status=PackagePublishingStatus.SUPERSEDED)
+        copy_archive, distroseries = self.makeCopyArchive([package_info])
+        self.checkCopiedSources(
+            copy_archive, distroseries, [])
+
+    def testCopyArchiveCreateDoesntCopyDeleted(self):
+        """Test that DELETED sources are not copied."""
+        package_info = PackageInfo(
+            "bzr", "2.1", status=PackagePublishingStatus.DELETED)
+        copy_archive, distroseries = self.makeCopyArchive([package_info])
+        self.checkCopiedSources(
+            copy_archive, distroseries, [])
+
+    def testCopyArchiveCreateDoesntCopyObsolete(self):
+        """Test that OBSOLETE sources are not copied."""
+        package_info = PackageInfo(
+            "bzr", "2.1", status=PackagePublishingStatus.OBSOLETE)
+        copy_archive, distroseries = self.makeCopyArchive([package_info])
+        self.checkCopiedSources(
+            copy_archive, distroseries, [])
+
+    def testCopyArchiveCreatesBuilds(self):
+        """Test that a copy archive creates builds for the copied packages."""
+        package_info = PackageInfo(
+            "bzr", "2.1", status=PackagePublishingStatus.PUBLISHED)
+        copy_archive, distroseries = self.makeCopyArchive([package_info])
+        self.checkBuilds(copy_archive, [package_info])
+
+    def testCopyArchiveArchTagNotAvailableInSource(self):
+        """Test creating a copy archive for an arch not in the source.
+
+        If we request a copy to an architecture that doesn't have
+        a DistroArchSeries in the source then we won't get any builds
+        created in the copy archive.
+        """
+        family = self.factory.makeProcessorFamily(name="armel")
+        self.factory.makeProcessor(family=family, name="armel")
+        package_info = PackageInfo(
+            "bzr", "2.1", status=PackagePublishingStatus.PUBLISHED)
+        owner = self.createTargetOwner()
+        # Creates an archive with just x86
+        distroseries = self.createSourceDistribution([package_info])
+        archive_name = self.getTargetArchiveName(distroseries.distribution)
+        # Different architecture, so there won't be any builds
+        copy_archive = self.copyArchive(
+            distroseries, archive_name, owner, architectures=["armel"])
+        self.checkBuilds(copy_archive, [])
+
+        # Also, make sure the package copy request status was updated.
+        [pcr] = getUtility(
+            IPackageCopyRequestSet).getByTargetArchive(copy_archive)
+        self.assertTrue(pcr.status == PackageCopyStatus.COMPLETE)
+
+        # This date is set when the copy request makes the transition to
+        # the "in progress" state.
+        self.assertTrue(pcr.date_started is not None)
+        # This date is set when the copy request makes the transition to
+        # the "completed" state.
+        self.assertTrue(pcr.date_completed is not None)
+        self.assertTrue(pcr.date_started <= pcr.date_completed)
+
+    def testMultipleArchTagsWithSubsetInSource(self):
+        """Try copy archive population with multiple architecture tags.
+
+        The user may specify a number of given architecture tags on the
+        command line.
+        The script should create build records only for the specified
+        architecture tags that are supported by the destination distro series.
+
+        In this (test) case the script should create the build records for the
+        '386' architecture.
+        """
+        family = self.factory.makeProcessorFamily(name="armel")
+        self.factory.makeProcessor(family=family, name="armel")
+        package_info = PackageInfo(
+            "bzr", "2.1", status=PackagePublishingStatus.PUBLISHED)
+        owner = self.createTargetOwner()
+        # Creates an archive with just x86
+        distroseries = self.createSourceDistribution([package_info])
+        archive_name = self.getTargetArchiveName(distroseries.distribution)
+        # There is only a DAS for i386, so armel won't produce any
+        # builds
+        copy_archive = self.copyArchive(
+            distroseries, archive_name, owner,
+            architectures=["386", "armel"])
+        self.checkBuilds(copy_archive, [package_info])
+
+    def testCopyArchiveCreatesSubsetOfBuilds(self):
+        """Create a copy archive with a subset of the architectures.
+
+        We copy from an archive with multiple architecture DistroArchSeries,
+        but request only one of those architectures in the target,
+        so we only get builds for that one architecture.
+        """
+        package_info = PackageInfo(
+            "bzr", "2.1", status=PackagePublishingStatus.PUBLISHED)
+        owner = self.createTargetOwner()
+        distroseries = self.createSourceDistribution([package_info])
+        self.factory.makeDistroArchSeries(
+            distroseries=distroseries, architecturetag="amd64",
+            processorfamily=ProcessorFamilySet().getByName("amd64"),
+            supports_virtualized=True)
+        archive_name = self.getTargetArchiveName(distroseries.distribution)
+        copy_archive = self.copyArchive(
+            distroseries, archive_name, owner,
+            architectures=["386"])
+        # We only get a single build, as we only requested 386, not
+        # amd64 too
+        self.checkBuilds(copy_archive, [package_info])
+
+    def testMultipleArchTags(self):
+        """Test copying an archive with multiple architectures.
+
+        We create a source with two architectures, and then request
+        a copy of both, so we get a build for each of those architectures.
+        """
+        package_info = PackageInfo(
+            "bzr", "2.1", status=PackagePublishingStatus.PUBLISHED)
+        owner = self.createTargetOwner()
+        distroseries = self.createSourceDistribution([package_info])
+        self.factory.makeDistroArchSeries(
+            distroseries=distroseries, architecturetag="amd64",
+            processorfamily=ProcessorFamilySet().getByName("amd64"),
+            supports_virtualized=True)
+        archive_name = self.getTargetArchiveName(distroseries.distribution)
+        copy_archive = self.copyArchive(
+            distroseries, archive_name, owner,
+            architectures=["386", "amd64"])
+        self.checkBuilds(copy_archive, [package_info, package_info])
+
+    def testCopyArchiveCopiesAllComponents(self):
+        """Test that packages from all components are copied.
+
+        When copying you specify a component, but that component doesn't
+        limit the packages copied. We create a source in main and one in
+        universe, and then copy with --component main, and expect to see
+        both sources in the copy.
+        """
+        package_infos = [
+            PackageInfo(
+                "bzr", "2.1", status=PackagePublishingStatus.PUBLISHED,
+                component="universe"),
+            PackageInfo(
+                "apt", "2.2", status=PackagePublishingStatus.PUBLISHED,
+                component="main")]
+        copy_archive, distroseries = self.makeCopyArchive(package_infos,
+            component="main")
+        self.checkBuilds(copy_archive, package_infos)
+
+    def testCopyFromPPA(self):
+        """Test we can create a copy archive with a PPA as the source."""
+        ppa_owner_name = "ppa-owner"
+        ppa_name = "ppa"
+        ppa_owner = self.factory.makePerson(name=ppa_owner_name)
+        distroseries = self.createSourceDistroSeries()
+        ppa = self.factory.makeArchive(
+            name=ppa_name, purpose=ArchivePurpose.PPA,
+            distribution=distroseries.distribution, owner=ppa_owner)
+        package_info = PackageInfo(
+                "bzr", "2.1", status=PackagePublishingStatus.PUBLISHED,
+                component="universe")
+        self.factory.makeSourcePackagePublishingHistory(
+            sourcepackagename=self.factory.getOrMakeSourcePackageName(
+                name=package_info.name),
+            distroseries=distroseries, component=self.factory.makeComponent(
+                package_info.component),
+            version=package_info.version, archive=ppa,
+            status=package_info.status, architecturehintlist='any',
+            pocket=PackagePublishingPocket.RELEASE)
+        owner = self.createTargetOwner()
+        archive_name = self.getTargetArchiveName(distroseries.distribution)
+        copy_archive = self.copyArchive(
+            distroseries, archive_name, owner, from_user=ppa_owner_name,
+            from_archive=ppa_name)
+        self.checkCopiedSources(
+            copy_archive, distroseries, [package_info])
+
     def runScript(
         self, archive_name=None, suite='hoary', user='salgado',
         exists_before=None, exists_after=None, exception_type=None,
@@ -261,7 +668,7 @@
         the script should fail with an appropriate error message.
         """
         now = int(time.time())
-        # The colons in the name make it invalid.
+        # The slashes in the name make it invalid.
         invalid_name = "ra//%s" % now
 
         extra_args = ['-a', '386']
@@ -305,77 +712,6 @@
             exception_type=SoyuzScriptError,
             exception_text="Invalid user name: '%s'" % invalid_user)
 
-    def testArchWithoutBuilds(self):
-        """Try copy archive population with no builds.
-
-        The user may specify a number of given architecture tags on the
-        command line.
-        The script should create build records only for the specified
-        architecture tags that are supported by the destination distro series.
-
-        In this (test) case the specified architecture tag should have the
-        effect that no build records are created.
-        """
-        hoary = getUtility(IDistributionSet)['ubuntu']['hoary']
-
-        # Verify that we have the right source packages in the sample data.
-        self._verifyPackagesInSampleData(hoary)
-
-        # Restrict the builds to be created to the 'amd64' architecture
-        # only. This should result in zero builds.
-        extra_args = ['-a', 'amd64']
-        copy_archive = self.runScript(
-            extra_args=extra_args, exists_after=True, reason="zero builds")
-
-        # Make sure the right source packages were cloned.
-        self._verifyClonedSourcePackages(copy_archive, hoary)
-
-        # Now check that we have zero build records for the sources cloned.
-        builds = list(getUtility(IBinaryPackageBuildSet).getBuildsForArchive(
-            copy_archive, status=BuildStatus.NEEDSBUILD))
-        build_spns = [
-            get_spn(removeSecurityProxy(build)).name for build in builds]
-
-        self.assertTrue(len(build_spns) == 0)
-
-        # Also, make sure the package copy request status was updated.
-        [pcr] = getUtility(
-            IPackageCopyRequestSet).getByTargetArchive(copy_archive)
-        self.assertTrue(pcr.status == PackageCopyStatus.COMPLETE)
-
-        # This date is set when the copy request makes the transition to
-        # the "in progress" state.
-        self.assertTrue(pcr.date_started is not None)
-        # This date is set when the copy request makes the transition to
-        # the "completed" state.
-        self.assertTrue(pcr.date_completed is not None)
-        self.assertTrue(pcr.date_started <= pcr.date_completed)
-
-        # Last but not least, check that the copy archive creation reason was
-        # captured as well.
-        self.assertTrue(pcr.reason == 'zero builds')
-
-    def testCopyFromPPA(self):
-        """Try copy archive population from a PPA.
-
-        In this (test) case an archive is populated from a PPA.
-        """
-        warty = getUtility(IDistributionSet)['ubuntu']['warty']
-        archive_set = getUtility(IArchiveSet)
-        ppa = archive_set.getPPAByDistributionAndOwnerName(
-            warty.distribution, 'cprov', 'ppa')
-
-        # Verify that we have the right source packages in the sample data.
-        packages = self._getPendingPackageNames(ppa, warty)
-
-        # Take a snapshot of the PPA.
-        extra_args = ['-a', 'amd64', '--from-user', 'cprov']
-        copy_archive = self.runScript(
-            suite='warty', extra_args=extra_args, exists_after=True)
-
-        copies = self._getPendingPackageNames(copy_archive, warty)
-        self.assertEqual(packages, copies)
-
     def testPackagesetDelta(self):
         """Try to calculate the delta between two source package sets."""
         hoary = getUtility(IDistributionSet)['ubuntu']['hoary']
@@ -402,7 +738,7 @@
             "INFO: * new-in-second-round (1.0)\n")
 
         extra_args = ['--package-set-delta']
-        copy_archive = self.runScript(
+        self.runScript(
             extra_args=extra_args, reason='', output_substr=expected_output,
             copy_archive_name=first_stage.name)
 
@@ -477,7 +813,7 @@
         This test should provoke a `SoyuzScriptError` exception.
         """
         extra_args = ['-a', 'amd64', '--from-archive', '9th-level-cache']
-        copy_archive = self.runScript(
+        self.runScript(
             extra_args=extra_args,
             exception_type=SoyuzScriptError,
             exception_text="Origin archive does not exist: '9th-level-cache'")
@@ -488,7 +824,7 @@
         This test should provoke a `SoyuzScriptError` exception.
         """
         extra_args = ['-a', 'amd64', '--from-user', 'king-kong']
-        copy_archive = self.runScript(
+        self.runScript(
             extra_args=extra_args,
             exception_type=SoyuzScriptError,
             exception_text="No PPA for user: 'king-kong'")
@@ -500,7 +836,7 @@
         """
         extra_args = [
             '-a', 'amd64', '--from-archive', '//']
-        copy_archive = self.runScript(
+        self.runScript(
             extra_args=extra_args,
             exception_type=SoyuzScriptError,
             exception_text="Invalid origin archive name: '//'")
@@ -511,7 +847,7 @@
         This test should provoke a `SoyuzScriptError` exception.
         """
         extra_args = ['-a', 'wintel']
-        copy_archive = self.runScript(
+        self.runScript(
             extra_args=extra_args,
             exception_type=SoyuzScriptError,
             exception_text="Invalid architecture tag: 'wintel'")
@@ -531,7 +867,7 @@
             extra_args=extra_args, exists_before=False)
 
         extra_args = ['--merge-copy', '-a', '386', '-a', 'amd64']
-        copy_archive = self.runScript(
+        self.runScript(
             extra_args=extra_args, copy_archive_name=copy_archive.name,
             exception_type=SoyuzScriptError,
             exception_text=(
@@ -549,7 +885,7 @@
         needed.
         """
         extra_args = ['-a', 'amd64']
-        copy_archive = self.runScript(
+        self.runScript(
             # Pass an empty reason parameter string to indicate that no
             # '--reason' command line argument is to be provided.
             extra_args=extra_args, reason='',
@@ -566,7 +902,7 @@
         *existing* archives.
         """
         extra_args = ['--merge-copy', '-a', 'amd64']
-        copy_archive = self.runScript(
+        self.runScript(
             extra_args=extra_args,
             exception_type=SoyuzScriptError,
             exception_text=(
@@ -581,11 +917,11 @@
         with the same name and distribution.
         """
         extra_args = ['-a', 'amd64']
-        copy_archive = self.runScript(
+        self.runScript(
             extra_args=extra_args, exists_after=True,
             copy_archive_name='hello-1')
         extra_args = ['-a', 'amd64']
-        copy_archive = self.runScript(
+        self.runScript(
             extra_args=extra_args,
             copy_archive_name='hello-1', exception_type=SoyuzScriptError,
             exception_text=(
@@ -596,60 +932,10 @@
 
         This test should provoke a `SoyuzScriptError` exception.
         """
-        copy_archive = self.runScript(
+        self.runScript(
             exception_type=SoyuzScriptError,
             exception_text="error: architecture tags not specified.")
 
-    def testMultipleArchTags(self):
-        """Try copy archive population with multiple architecture tags.
-
-        The user may specify a number of given architecture tags on the
-        command line.
-        The script should create build records only for the specified
-        architecture tags that are supported by the destination distro series.
-
-        In this (test) case the script should create the build records for the
-        '386' architecture.
-        """
-        hoary = getUtility(IDistributionSet)['ubuntu']['hoary']
-
-        # Verify that we have the right source packages in the sample data.
-        self._verifyPackagesInSampleData(hoary)
-
-        # Please note:
-        #   * the 'amd64' DistroArchSeries has no resulting builds.
-        #   * the '-a' command line parameter is cumulative in nature
-        #     i.e. the 'amd64' architecture tag specified after the '386'
-        #     tag does not overwrite the latter but is added to it.
-        extra_args = ['-a', '386', '-a', 'amd64']
-        copy_archive = self.runScript(
-            extra_args=extra_args, exists_after=True)
-
-        # Make sure the right source packages were cloned.
-        self._verifyClonedSourcePackages(copy_archive, hoary)
-
-        # Now check that we have the build records expected.
-        builds = list(getUtility(IBinaryPackageBuildSet).getBuildsForArchive(
-            copy_archive, status=BuildStatus.NEEDSBUILD))
-        build_spns = [
-            get_spn(removeSecurityProxy(build)).name for build in builds]
-        self.assertEqual(build_spns, self.expected_build_spns)
-
-        def get_family_names(result_set):
-            """Extract processor family names from result set."""
-            family_names = []
-            for archivearch in rset:
-                family_names.append(
-                    removeSecurityProxy(archivearch).processorfamily.name)
-
-            family_names.sort()
-            return family_names
-
-        # Make sure that the processor family names specified for the copy
-        # archive at hand were stored in the database.
-        rset = getUtility(IArchiveArchSet).getByArchive(copy_archive)
-        self.assertEqual(get_family_names(rset), [u'amd64', u'x86'])
-
     def testBuildsPendingAndSuspended(self):
         """All builds in the new copy archive are pending and suspended."""
         def build_in_wrong_state(build):
@@ -695,11 +981,11 @@
         # We will make a private PPA and then attempt to copy from it.
         joe = self.factory.makePerson(name='joe')
         ubuntu = getUtility(IDistributionSet).getByName('ubuntu')
-        joes_private_ppa = self.factory.makeArchive(
+        self.factory.makeArchive(
             owner=joe, private=True, name="ppa", distribution=ubuntu)
 
         extra_args = ['--from-user', 'joe', '-a', 'amd64']
-        copy_archive = self.runScript(
+        self.runScript(
             extra_args=extra_args, exception_type=SoyuzScriptError,
             exception_text=(
                 "Cannot copy from private archive ('joe/ppa')"))
@@ -719,7 +1005,7 @@
             enabled=False)
 
         extra_args = ['--from-user', 'cprov', '--merge-copy']
-        copy_archive = self.runScript(
+        self.runScript(
             copy_archive_name=disabled_archive.name, reason='',
             extra_args=extra_args, exception_type=SoyuzScriptError,
             exception_text='error: cannot copy to disabled archive')
@@ -758,11 +1044,11 @@
         ubuntu = getUtility(IDistributionSet).getByName('ubuntu')
         hoary = ubuntu.getSeries('hoary')
         test_publisher.addFakeChroots(hoary)
-        unused = test_publisher.setUpDefaultDistroSeries(hoary)
-        new_package = test_publisher.getPubSource(
+        test_publisher.setUpDefaultDistroSeries(hoary)
+        test_publisher.getPubSource(
             sourcename="new-in-second-round", version="1.0",
             distroseries=hoary, archive=ubuntu.main_archive)
-        fresher_package = test_publisher.getPubSource(
+        test_publisher.getPubSource(
             sourcename="alsa-utils", version="2.0", distroseries=hoary,
             archive=ubuntu.main_archive)
         sources = ubuntu.main_archive.getPublishedSources(