On Sun, Aug 1, 2010 at 12:06 AM, James Westby <email address hidden> wrote:
> James Westby has proposed merging lp:~james-w/launchpad/no-more-sampledata-0 into lp:launchpad/devel.
>
> Requested reviews:
>  Launchpad code reviewers (launchpad-reviewers)
>
>
> Hi,
>
> This changes a bunch of soyuz tests to not use the sampledata.
>

Woot.

> It goes a bit further than that in that it restructures them to
> have less setup and to create the things they need using the
> factory rather than SoyuzTestPublisher.
>
> In addition I moved some other code that is too deeply buried
> right now in to lp.testing.sampledata, and extended the factory
> with some new methods to satisfy the needs of the changed code.
>
> Thanks,
>

Thank you.

The review below mostly picks up on some naming standard glitches and
points out some things that could be improved in the soyuz tests
(rather than in your changes). However, I've also got a couple of
questions about your changes and would very much appreciate hearing
back from you.

I also feel a little guilty about making so many petty comments when
you've done such a helpful thing. Please do not feel obliged to act on
all of my suggestions.

jml

> === modified file 'lib/lp/soyuz/tests/ppa.py'
> --- lib/lp/soyuz/tests/ppa.py   2010-03-11 16:48:37 +0000
> +++ lib/lp/soyuz/tests/ppa.py   2010-07-31 23:06:48 +0000
> @@ -25,6 +25,10 @@
>  from lp.soyuz.interfaces.binarypackagename import IBinaryPackageNameSet
>  from lp.soyuz.interfaces.publishing import (
>     PackagePublishingPriority, PackagePublishingStatus)
> +from lp.testing.sampledata import (
> +    HOARY_DISTROSERIES_NAME, I386_ARCHITECTURE_NAME, MAIN_COMPONENT_NAME,
> +    MOZILLA_FIREFOX_SOURCEPACKAGENAME, MOZILLA_FIREFOX_SOURCEPACKAGEVERSION,
> +    NAME16_PERSON_NAME, UBUNTU_TEAM_NAME)
>

I'm very glad you've started putting things in the sampledata module.

Would it be possible to change these names to clarify the intent
behind their use?

For example, NAME16_PERSON_NAME tells me nothing about the person
object, and when I see it in a test, I don't know why name16 was
chosen instead of name15. UBUNTU_TEAM_MEMBER_NAME might be better.
Likewise, it would be good to have intent-revealing names for the
FIREFOX constants and the HOARY constants.

> === modified file 'lib/lp/soyuz/tests/soyuz.py'
> --- lib/lp/soyuz/tests/soyuz.py 2010-02-08 11:40:06 +0000
> +++ lib/lp/soyuz/tests/soyuz.py 2010-07-31 23:06:48 +0000
> @@ -27,14 +27,19 @@
>  from lp.soyuz.model.publishing import (
>     SourcePackagePublishingHistory,
>     BinaryPackagePublishingHistory)
> +from lp.testing.sampledata import (
> +    CHROOT_LFA, CPROV_NAME, I386_ARCHITECTURE_NAME, LAUNCHPAD_DBUSER_NAME,
> +    UBUNTU_DISTRIBUTION_NAME, WARTY_DISTROSERIES_NAME,
> +    WARTY_UPDATES_SUITE_NAME)
>

Ditto CPROV, LAUNCHPAD_DBUSER_NAME and WARTY_*

> === modified file 'lib/lp/soyuz/tests/test_archive.py'
> --- lib/lp/soyuz/tests/test_archive.py  2010-07-21 07:45:50 +0000
> +++ lib/lp/soyuz/tests/test_archive.py  2010-07-31 23:06:48 +0000
...
> @@ -42,118 +41,110 @@
>
>  class TestGetPublicationsInArchive(TestCaseWithFactory):
>
> -    layer = LaunchpadZopelessLayer
> -
> -    def setUp(self):
> -        """Use `SoyuzTestPublisher` to publish some sources in archives."""
> -        super(TestGetPublicationsInArchive, self).setUp()
> -
> -        self.distribution = getUtility(IDistributionSet)['ubuntutest']
> -
> -        # Create two PPAs for gedit.
> -        self.archives = {}
> -        self.archives['ubuntu-main'] = self.distribution.main_archive
> -        self.archives['gedit-nightly'] = self.factory.makeArchive(
> -            name="gedit-nightly", distribution=self.distribution)
> -        self.archives['gedit-beta'] = self.factory.makeArchive(
> -            name="gedit-beta", distribution=self.distribution)
> -
> -        self.publisher = SoyuzTestPublisher()
> -        self.publisher.prepareBreezyAutotest()
> -
> -        # Publish gedit in all three archives, but with different
> -        # upload dates.
> -        self.gedit_nightly_src_hist = self.publisher.getPubSource(
> -            sourcename="gedit", archive=self.archives['gedit-nightly'],
> -            date_uploaded=datetime(2010, 12, 1, tzinfo=pytz.UTC),
> -            status=PackagePublishingStatus.PUBLISHED)
> -        self.gedit_beta_src_hist = self.publisher.getPubSource(
> -            sourcename="gedit", archive=self.archives['gedit-beta'],
> -            date_uploaded=datetime(2010, 11, 30, tzinfo=pytz.UTC),
> -            status=PackagePublishingStatus.PUBLISHED)
> -        self.gedit_main_src_hist = self.publisher.getPubSource(
> -            sourcename="gedit", archive=self.archives['ubuntu-main'],
> -            date_uploaded=datetime(2010, 12, 30, tzinfo=pytz.UTC),
> -            status=PackagePublishingStatus.PUBLISHED)
> -
> -        # Save the archive utility for easy access, as well as the gedit
> -        # source package name.
> -        self.archive_set = getUtility(IArchiveSet)
> -        spr = self.gedit_main_src_hist.sourcepackagerelease
> -        self.gedit_name = spr.sourcepackagename
> +    layer = DatabaseFunctionalLayer
> +
> +    def makeThreeArchivesOneDistribution(self):
> +        distribution = self.factory.makeDistribution()
> +        archives = []
> +        for i in range(3):
> +            archives.append(
> +                self.factory.makeArchive(distribution=distribution))
> +        return archives
> +

Why did you encode the number of archives in the function name? Why
not a parameter?

> +    def makeThreeArchivesWithPublications(self):
> +        archives = self.makeThreeArchivesOneDistribution()
> +        sourcepackagename = self.factory.makeSourcePackageName()
> +        for archive in archives:
> +            self.factory.makeSourcePackagePublishingHistory(
> +                sourcepackagename=sourcepackagename, archive=archive,
> +                status=PackagePublishingStatus.PUBLISHED,
> +                )
> +        return archives, sourcepackagename
> +

Likewise here.

> +    def getPublications(self, sourcepackagename, archives, distribution):
> +        return getUtility(IArchiveSet).getPublicationsInArchives(
> +            sourcepackagename, archives, distribution=distribution)
>
>     def testReturnsAllPublishedPublications(self):

If you are feeling super awesome (but only then), could you please
update all of these test methods to be test_returnsAll... style rather
than testReturnsAll style?

>         # Returns all currently published publications for archives
> -        results = self.archive_set.getPublicationsInArchives(
> -            self.gedit_name, self.archives.values(),
> -            distribution=self.distribution)
> +        archives, sourcepackagename = self.makeThreeArchivesWithPublications()
> +        results = self.getPublications(
> +            sourcepackagename, archives, archives[0].distribution)
>         num_results = results.count()
> -        self.assertEquals(3, num_results, "Expected 3 publications but "
> -                                          "got %s" % num_results)
> +        self.assertEquals(
> +            3, num_results,
> +            "Expected 3 publications but got %s" % num_results)
>

I know it's in the original, but I don't think that error message in
the assertEquals() call actually helps. I recommend dropping it.

>     def testEmptyListOfArchives(self):
>         # Passing an empty list of archives will result in an empty
>         # resultset.
> -        results = self.archive_set.getPublicationsInArchives(
> -            self.gedit_name, [], distribution=self.distribution)
> +        archives, sourcepackagename = self.makeThreeArchivesWithPublications()
> +        results = self.getPublications(
> +            sourcepackagename, [], archives[0].distribution)
>         self.assertEquals(0, results.count())
>

self.assertEquals([], list(results)) gives a more helpful error
message on failure.

Don't feel obliged to change this.

>     def testReturnsOnlyPublicationsForGivenArchives(self):
>         # Returns only publications for the specified archives
> -        results = self.archive_set.getPublicationsInArchives(
> -            self.gedit_name, [self.archives['gedit-beta']],
> -            distribution=self.distribution)
> +        archives, sourcepackagename = self.makeThreeArchivesWithPublications()
> +        results = self.getPublications(
> +            sourcepackagename, [archives[0]], archives[0].distribution)
>         num_results = results.count()
> -        self.assertEquals(1, num_results, "Expected 1 publication but "
> -                                          "got %s" % num_results)
> -        self.assertEquals(self.archives['gedit-beta'],
> -                          results[0].archive,
> -                          "Expected publication from %s but was instead "
> -                          "from %s." % (
> -                              self.archives['gedit-beta'].displayname,
> -                              results[0].archive.displayname))
> +        self.assertEquals(
> +            1, num_results,
> +            "Expected 1 publication but got %s" % num_results)
> +        self.assertEquals(
> +            archives[0], results[0].archive,
> +            "Expected publication from %s but was instead from %s." % (
> +                archives[0].displayname, results[0].archive.displayname))
>

My comment about the other error message applies here. I think it can
be dropped. (If archives have unhelpful __repr__ implementations, we
should fix that rather than working around it all the time in tests).

>     def testReturnsOnlyPublishedPublications(self):
>         # Publications that are not published will not be returned.
> -        secure_src_hist = self.gedit_beta_src_hist
> -        secure_src_hist.status = PackagePublishingStatus.PENDING
> -
> -        results = self.archive_set.getPublicationsInArchives(
> -            self.gedit_name, [self.archives['gedit-beta']],
> -            distribution=self.distribution)
> +        archive = self.factory.makeArchive()
> +        sourcepackagename = self.factory.makeSourcePackageName()
> +        self.factory.makeSourcePackagePublishingHistory(
> +            archive=archive, sourcepackagename=sourcepackagename,
> +            status=PackagePublishingStatus.PENDING)
> +        results = self.getPublications(
> +            sourcepackagename, [archive], archive.distribution)
>         num_results = results.count()
>         self.assertEquals(0, num_results, "Expected 0 publication but "
>                                           "got %s" % num_results)
>

Again, I prefer self.assertEquals([], list(results)) style assertions,
but don't feel obliged to change it.

>     def testPubsForSpecificDistro(self):
>         # Results can be filtered for specific distributions.
> -
> -        # Add a publication in the ubuntu distribution
> -        ubuntu = getUtility(IDistributionSet)['ubuntu']
> -        warty = ubuntu['warty']
> -        gedit_main_src_hist = self.publisher.getPubSource(
> -            sourcename="gedit",
> -            archive=self.archives['ubuntu-main'],
> -            distroseries=warty,
> -            date_uploaded=datetime(2010, 12, 30, tzinfo=pytz.UTC),
> -            status=PackagePublishingStatus.PUBLISHED)
> -
> -        # Only the 3 results for ubuntutest are returned when requested:
> -        results = self.archive_set.getPublicationsInArchives(
> -            self.gedit_name, self.archives.values(),
> -            distribution=self.distribution)
> -        num_results = results.count()
> -        self.assertEquals(3, num_results, "Expected 3 publications but "
> -                                          "got %s" % num_results)
> -
> -        # Similarly, requesting the ubuntu publications only returns the
> -        # one we created:
> -        results = self.archive_set.getPublicationsInArchives(
> -            self.gedit_name, self.archives.values(),
> -            distribution=ubuntu)
> -        num_results = results.count()
> -        self.assertEquals(1, num_results, "Expected 1 publication but "
> -                                          "got %s" % num_results)
> +        sourcepackagename = self.factory.makeSourcePackageName()
> +
> +        distribution = self.factory.makeDistribution()
> +        distroseries = self.factory.makeDistroSeries(
> +            distribution=distribution)
> +        archive = self.factory.makeArchive(distribution=distribution)
> +        self.factory.makeSourcePackagePublishingHistory(
> +            archive=archive, sourcepackagename=sourcepackagename,
> +            distroseries=distroseries,
> +            status=PackagePublishingStatus.PUBLISHED)
> +
> +        other_distribution = self.factory.makeDistribution()
> +        other_distroseries = self.factory.makeDistroSeries(
> +            distribution=other_distribution)
> +        other_archive = self.factory.makeArchive(
> +            distribution=other_distribution)
> +        self.factory.makeSourcePackagePublishingHistory(
> +            archive=other_archive, sourcepackagename=sourcepackagename,
> +            distroseries=other_distroseries,
> +            status=PackagePublishingStatus.PUBLISHED)
> +
> +        # We don't get the results for other_distribution
> +        results = self.getPublications(
> +            sourcepackagename, [archive, other_archive],
> +            distribution=distribution)
> +        num_results = results.count()
> +        self.assertEquals(
> +            1, num_results,
> +            "Expected 1 publication but got %s" % num_results)
> +        self.assertEquals(
> +            archive, results[0].archive,
> +            "Expected publication from %s but was instead from %s." % (
> +                archive.displayname, results[0].archive.displayname))
>

Hmm. These assertions look very much like the ones previous. Is there
a domain-specific assertion method or Matcher to be had here?

>
>  class TestArchiveRepositorySize(TestCaseWithFactory):
> @@ -167,60 +158,59 @@
>         self.ppa = self.factory.makeArchive(
>             name="testing", distribution=self.publisher.ubuntutest)
>
> +    def publish_ddeb_in_archive(self, archive):

This is not our method naming style, sadly.

Please change to publishDdebInArchive (or publishDDEBInArchive, I
guess). Also, please add a docstring.

> +        binarypackagerelease = self.factory.makeBinaryPackageRelease(
> +            binpackageformat=BinaryPackageFormat.DDEB)
> +        self.factory.makeBinaryPackagePublishingHistory(
> +            archive=archive, binarypackagerelease=binarypackagerelease,
> +            status=PackagePublishingStatus.PUBLISHED)
> +        self.factory.makeBinaryPackageFile(
> +            binarypackagerelease=binarypackagerelease,
> +            filetype=BinaryPackageFileType.DDEB)
> +
>     def test_binaries_size_does_not_include_ddebs_for_ppas(self):
>         # DDEBs are not computed in the PPA binaries size because
>         # they are not being published. See bug #399444.
> -        self.assertEquals(0, self.ppa.binaries_size)
> -        self.publisher.getPubBinaries(
> -            filecontent='X', format=BinaryPackageFormat.DDEB,
> -            archive=self.ppa)
> -        self.assertEquals(0, self.ppa.binaries_size)
> +        ppa = self.factory.makeArchive(purpose=ArchivePurpose.PPA)
> +        self.assertEquals(0, ppa.binaries_size)
> +        self.publish_ddeb_in_archive(ppa)
> +        self.assertEquals(0, ppa.binaries_size)
>

Technically speaking, this is actually two tests. The first test shows
that binaries_size is zero for newly-created archives, the second
shows that DDEBs are not included in binaries_size. The giveaway is
the manipulation of the SUT between assertions.

As an optional cleanup, please split this test into two.

>     def test_binaries_size_includes_ddebs_for_other_archives(self):
>         # DDEBs size are computed for all archive purposes, except PPAs.
> -        previous_size = self.publisher.ubuntutest.main_archive.binaries_size
> -        self.publisher.getPubBinaries(
> -            filecontent='X', format=BinaryPackageFormat.DDEB)
> -        self.assertEquals(
> -            previous_size + 1,
> -            self.publisher.ubuntutest.main_archive.binaries_size)
> +        archive = self.factory.makeArchive(purpose=ArchivePurpose.PRIMARY)
> +        self.assertEquals(0, archive.binaries_size)
> +        self.publish_ddeb_in_archive(archive)
> +        self.assertNotEquals(0, archive.binaries_size)
>

Likewise, although perhaps we don't need a second test showing that
binaries_size is zero for newly created non-PPA archives.

>     def test_sources_size_on_empty_archive(self):
>         # Zero is returned for an archive without sources.
> +        archive = self.factory.makeArchive()
>         self.assertEquals(
> -            0, self.ppa.sources_size,
> +            0, archive.sources_size,
>             'Zero should be returned for an archive without sources.')
>

I wonder who that error message would help.

> +    def publish_source_file(self, archive, libraryfile):

Our naming standard sucks: please change to publishSourceFile, library_file.


> +        sourcepackagerelease = self.factory.makeSourcePackageRelease()
> +        self.factory.makeSourcePackagePublishingHistory(
> +            archive=archive, sourcepackagerelease=sourcepackagerelease,
> +            status=PackagePublishingStatus.PUBLISHED)
> +        self.factory.makeSourcePackageReleaseFile(
> +            sourcepackagerelease=sourcepackagerelease,
> +            libraryfile=libraryfile)
> +
>     def test_sources_size_does_not_count_duplicated_files(self):
>         # If there are multiple copies of the same file name/size
>         # only one will be counted.
> -        pub_1 = self.publisher.getPubSource(
> -            filecontent='22', version='0.5.11~ppa1', archive=self.ppa)
> -
> -        pub_2 = self.publisher.getPubSource(
> -            filecontent='333', version='0.5.11~ppa2', archive=self.ppa)
> -
> -        self.assertEquals(5, self.ppa.sources_size)
> -
> -        shared_tarball = self.publisher.addMockFile(
> -            filename='foo_0.5.11.tar.gz', filecontent='1')
> -
> -        # After adding a the shared tarball to the ppa1 version,
> -        # the sources_size updates to reflect the change.
> -        pub_1.sourcepackagerelease.addFile(shared_tarball)
> -        self.assertEquals(
> -            6, self.ppa.sources_size,
> -            'The sources_size should update after a file is added.')
> -
> -        # But after adding a copy of the shared tarball to the ppa2 version,
> -        # the sources_size is unchanged.
> -        shared_tarball_copy = self.publisher.addMockFile(
> -            filename='foo_0.5.11.tar.gz', filecontent='1')
> -
> -        pub_2.sourcepackagerelease.addFile(shared_tarball_copy)
> -        self.assertEquals(
> -            6, self.ppa.sources_size,
> -            'The sources_size should change after adding a duplicate file.')
> +        archive = self.factory.makeArchive()
> +        libraryfile = self.factory.makeLibraryFileAlias()

Likewise library_file.

> +        self.publish_source_file(archive, libraryfile)
> +        self.assertEquals(
> +            libraryfile.content.filesize, archive.sources_size)
> +
> +        self.publish_source_file(archive, libraryfile)
> +        self.assertEquals(
> +            libraryfile.content.filesize, archive.sources_size)
>
>
>  class TestSeriesWithSources(TestCaseWithFactory):
>
> === modified file 'lib/lp/testing/factory.py'
> --- lib/lp/testing/factory.py   2010-07-30 20:33:47 +0000
> +++ lib/lp/testing/factory.py   2010-07-31 23:06:48 +0000
> @@ -128,7 +128,7 @@
>  from lp.registry.interfaces.projectgroup import IProjectGroupSet
>  from lp.registry.interfaces.series import SeriesStatus
>  from lp.registry.interfaces.sourcepackage import (
> -    ISourcePackage, SourcePackageUrgency)
> +    ISourcePackage, SourcePackageFileType, SourcePackageUrgency)
>  from lp.registry.interfaces.sourcepackagename import (
>     ISourcePackageNameSet)
>  from lp.registry.interfaces.ssh import ISSHKeySet
> @@ -146,18 +146,20 @@
>  from lp.soyuz.interfaces.archive import (
>     default_name_by_purpose, IArchiveSet, ArchivePurpose)
>  from lp.soyuz.interfaces.binarypackagebuild import IBinaryPackageBuildSet
> -from lp.soyuz.interfaces.binarypackagerelease import BinaryPackageFormat
> +from lp.soyuz.interfaces.binarypackagerelease import (
> +    BinaryPackageFileType, BinaryPackageFormat)
>  from lp.soyuz.interfaces.component import IComponentSet
>  from lp.soyuz.interfaces.packageset import IPackagesetSet
>  from lp.soyuz.interfaces.processor import IProcessorFamilySet
>  from lp.soyuz.interfaces.publishing import (
> -    PackagePublishingPriority, PackagePublishingStatus)
> +    IPublishingSet, PackagePublishingPriority, PackagePublishingStatus)
>  from lp.soyuz.interfaces.section import ISectionSet
>  from lp.soyuz.model.binarypackagename import BinaryPackageName
>  from lp.soyuz.model.binarypackagerelease import BinaryPackageRelease
> +from lp.soyuz.model.files import BinaryPackageFile, SourcePackageReleaseFile
>  from lp.soyuz.model.processor import ProcessorFamilySet
>  from lp.soyuz.model.publishing import (
> -    BinaryPackagePublishingHistory, SourcePackagePublishingHistory)
> +    SourcePackagePublishingHistory)
>  from lp.testing import (
>     ANONYMOUS,
>     login,
> @@ -2338,6 +2340,19 @@
>             dateuploaded=date_uploaded,
>             source_package_recipe_build=source_package_recipe_build)
>
> +    def makeSourcePackageReleaseFile(self, sourcepackagerelease=None,
> +                                     libraryfile=None, filetype=None):
> +        if sourcepackagerelease is None:
> +            sourcepackagerelease = self.makeSourcePackageRelease()
> +        if libraryfile is None:
> +            libraryfile = self.makeLibraryFileAlias()
> +        if filetype is None:
> +            filetype = SourcePackageFileType.DSC
> +        return ProxyFactory(
> +            SourcePackageReleaseFile(
> +                sourcepackagerelease=sourcepackagerelease,
> +                libraryfile=libraryfile, filetype=filetype))
> +

Can you please add tests for this to lp/testing/tests/test_factory.py?

>     def makeBinaryPackageBuild(self, source_package_release=None,
>             distroarchseries=None, archive=None, builder=None):
>         """Create a BinaryPackageBuild.
> @@ -2399,6 +2414,7 @@
>                                            dsc_standards_version='3.6.2',
>                                            dsc_format='1.0',
>                                            dsc_binaries='foo-bin',
> +                                           sourcepackagerelease=None,
>                                            ):
>         """Make a `SourcePackagePublishingHistory`."""
>         if distroseries is None:
> @@ -2419,40 +2435,38 @@
>         if status is None:
>             status = PackagePublishingStatus.PENDING
>
> -        spr = self.makeSourcePackageRelease(
> -            archive=archive,
> -            sourcepackagename=sourcepackagename,
> -            distroseries=distroseries,
> -            maintainer=maintainer,
> -            creator=creator, component=component,
> -            section_name=section_name,
> -            urgency=urgency,
> -            version=version,
> -            builddepends=builddepends,
> -            builddependsindep=builddependsindep,
> -            build_conflicts=build_conflicts,
> -            build_conflicts_indep=build_conflicts_indep,
> -            architecturehintlist=architecturehintlist,
> -            dsc_standards_version=dsc_standards_version,
> -            dsc_format=dsc_format,
> -            dsc_binaries=dsc_binaries,
> -            date_uploaded=date_uploaded)
> -
> -        sspph = SourcePackagePublishingHistory(
> -            distroseries=distroseries,
> -            sourcepackagerelease=spr,
> -            component=spr.component,
> -            section=spr.section,
> -            status=status,
> -            datecreated=date_uploaded,
> -            dateremoved=dateremoved,
> -            scheduleddeletiondate=scheduleddeletiondate,
> -            pocket=pocket,
> -            archive=archive)
> -
> -        # SPPH and SSPPH IDs are the same, since they are SPPH is a SQLVIEW
> -        # of SSPPH and other useful attributes.
> -        return SourcePackagePublishingHistory.get(sspph.id)
> +        if sourcepackagerelease is None:
> +            sourcepackagerelease = self.makeSourcePackageRelease(
> +                archive=archive,
> +                sourcepackagename=sourcepackagename,
> +                distroseries=distroseries,
> +                maintainer=maintainer,
> +                creator=creator, component=component,
> +                section_name=section_name,
> +                urgency=urgency,
> +                version=version,
> +                builddepends=builddepends,
> +                builddependsindep=builddependsindep,
> +                build_conflicts=build_conflicts,
> +                build_conflicts_indep=build_conflicts_indep,
> +                architecturehintlist=architecturehintlist,
> +                dsc_standards_version=dsc_standards_version,
> +                dsc_format=dsc_format,
> +                dsc_binaries=dsc_binaries,
> +                date_uploaded=date_uploaded)
> +
> +        return ProxyFactory(
> +            SourcePackagePublishingHistory(
> +                distroseries=distroseries,
> +                sourcepackagerelease=sourcepackagerelease,
> +                component=sourcepackagerelease.component,
> +                section=sourcepackagerelease.section,
> +                status=status,
> +                datecreated=date_uploaded,
> +                dateremoved=dateremoved,
> +                scheduleddeletiondate=scheduleddeletiondate,
> +                pocket=pocket,
> +                archive=archive))
>

And tests for this change too. You don't have to retrospectively add
tests for every path in the method, just tests for the change you've
made in this branch.

>     def makeBinaryPackagePublishingHistory(self, binarypackagerelease=None,
>                                            distroarchseries=None,
> @@ -2485,28 +2499,40 @@
>         if priority is None:
>             priority = PackagePublishingPriority.OPTIONAL
>
> -        bpr = self.makeBinaryPackageRelease(
> -            component=component,
> -            section_name=section_name,
> -            priority=priority)
> +        if binarypackagerelease is None:
> +            binarypackagerelease = self.makeBinaryPackageRelease(
> +                component=component,
> +                section_name=section_name,
> +                priority=priority)
>
> -        return BinaryPackagePublishingHistory(
> -            distroarchseries=distroarchseries,
> -            binarypackagerelease=bpr,
> -            component=bpr.component,
> -            section=bpr.section,
> -            status=status,
> -            dateremoved=dateremoved,
> -            scheduleddeletiondate=scheduleddeletiondate,
> -            pocket=pocket,
> -            priority=priority,
> -            archive=archive)
> +        bpph = getUtility(IPublishingSet).newBinaryPublication(
> +            archive, binarypackagerelease, distroarchseries,
> +            binarypackagerelease.component, binarypackagerelease.section,
> +            priority, pocket)
> +        naked_bpph = removeSecurityProxy(bpph)
> +        naked_bpph.status = status
> +        naked_bpph.dateremoved = dateremoved
> +        naked_bpph.scheduleddeletiondate = scheduleddeletiondate
> +        naked_bpph.priority = priority
> +        return bpph
>

Likewise here.

>     def makeBinaryPackageName(self, name=None):
>         if name is None:
>             name = self.getUniqueString("binarypackage")
>         return BinaryPackageName(name=name)
>
> +    def makeBinaryPackageFile(self, binarypackagerelease=None,
> +                              libraryfile=None, filetype=None):
> +        if binarypackagerelease is None:
> +            binarypackagerelease = self.makeBinaryPackageRelease()
> +        if libraryfile is None:
> +            libraryfile = self.makeLibraryFileAlias()
> +        if filetype is None:
> +            filetype = BinaryPackageFileType.DEB
> +        return ProxyFactory(BinaryPackageFile(
> +            binarypackagerelease=binarypackagerelease,
> +            libraryfile=libraryfile, filetype=filetype))
> +

Could you please add tests for this?

>     def makeBinaryPackageRelease(self, binarypackagename=None,
>                                  version=None, build=None,
>                                  binpackageformat=None, component=None,
> @@ -2531,13 +2557,13 @@
>             summary = self.getUniqueString("summary")
>         if description is None:
>             description = self.getUniqueString("description")
> -        return BinaryPackageRelease(binarypackagename=binarypackagename,
> -                                    version=version, build=build,
> -                                    binpackageformat=binpackageformat,
> -                                    component=component, section=section,
> -                                    priority=priority, summary=summary,
> -                                    description=description,
> -                                    architecturespecific=architecturespecific)
> +        return ProxyFactory(
> +            BinaryPackageRelease(
> +                binarypackagename=binarypackagename, version=version,
> +                build=build, binpackageformat=binpackageformat,
> +                component=component, section=section, priority=priority,
> +                summary=summary, description=description,
> +                architecturespecific=architecturespecific))
>

I don't know what our testing strategy is for guaranteeing
ProxyFactory returns. It would be nice to have a test for this though.

>     def makeSection(self, name=None):
>         """Make a `Section`."""
>
> === modified file 'lib/lp/testing/sampledata.py'
> --- lib/lp/testing/sampledata.py        2010-07-17 15:52:19 +0000
> +++ lib/lp/testing/sampledata.py        2010-07-31 23:06:48 +0000
> @@ -9,8 +9,35 @@
>
>  __metaclass__ = type
>  __all__ = [
> +    'CHROOT_LFA',
> +    'CPROV_NAME',
> +    'HOARY_DISTROSERIES_NAME',
> +    'I386_ARCHITECTURE_NAME',
> +    'LAUNCHPAD_DBUSER_NAME',
> +    'MAIN_COMPONENT_NAME',
> +    'MOZILLA_FIREFOX_SOURCEPACKAGENAME',
> +    'MOZILLA_FIREFOX_SOURCEPACKAGEVERSION',
> +    'NAME16_PERSON_NAME',
>     'NO_PRIVILEGE_EMAIL',
> +    'UBUNTU_DISTRIBUTION_NAME',
> +    'UBUNTU_TEAM_NAME',
> +    'UBUNTUTEST_DISTRIBUTION_NAME',
> +    'WARTY_DISTROSERIES_NAME',
> +    'WARTY_UPDATES_SUITE_NAME',
>     ]
>
>
> +CHROOT_LFA = 1

I think this one needs a comment. What's an LFA?

jml