launchpad-reviewers team mailing list archive

[Jonathan Lange <jml@xxxxxxxxxxxxx>]

On Mon, Aug 9, 2010 at 11:39 PM, James Westby <jw+debian@xxxxxxxxxxxxxxx> wrote:
> James Westby has proposed merging lp:~james-w/launchpad/move-soyuz-test-publisher into lp:launchpad/devel with lp:~james-w/launchpad/more-matchers as a prerequisite.
>
> Requested reviews:
>  Launchpad code reviewers (launchpad-reviewers)
>
>
> Hi,
>
> This branch moves SoyuzTestPublisher and the associated
> TestNativePublishingBase to lp.soyuz.testing.publisher,
> and also changes the API somewhat.
>

Thanks!

FWIW, code reviews are much simpler if simple code moves are separated
from API changes. However, I do not wish to look a gift horse in the
mouth, so...

> The main thrust of this change is to get tests away from
> sampledata, but there are far to many using these classes
> to do it in one go. Therefore I created a new class that
> can be transitioned to gradually.
>

Seems sensible.

> In addition to this I changed the API somewhat to be what
> I consider to be cleaner. This way mainly a taste thing,
> but some of the changes will also stop tests being too
> tightly bound to the implementation of these classes, giving
> us more freedom to change things in future.

That's ok, I have excellent taste.

> Some of the changes
> were a little more than that though, such as removing lots
> of commits from the tests using TestNativePublishingBase.
> If there are lots of tests that need that many commits then
> we can add it back as an opt-in thing, we should punish
> every test with it.
>

+1

> In addition I also documented the new interface with docstrings,
> and a module docstring to help with porting.
>

Sweet!

Only typos noticed below, along with a couple of small nitpicks. Feel
free to land without getting another review.

...
> === added file 'lib/lp/soyuz/testing/__init__.py'
> === renamed file 'lib/lp/soyuz/testing/__init__.py' => 'lib/lp/soyuz/testing/publisher.py'
> --- lib/lp/soyuz/testing/__init__.py    2010-08-09 22:38:48 +0000
> +++ lib/lp/soyuz/testing/publisher.py   2010-08-09 22:38:53 +0000
> @@ -0,0 +1,1059 @@
> +# Copyright 2010 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Testing helpers for publishing.
> +
> +Porting from TestNativePublishingBase to PublishingTestCase:
> +
...
> +
> +Porting from `lp.soyuz.tests.test_publishing.SoyuzTestPublisher` to
> +`lp.soyuz.testing.publisher.SoyuzTestPublisher`:
> +
...
> +"""
> +
> +__metaclass__ = type
> +
> +__all__ = [
> +    'makeDistributionLucilleconfig',
> +    'makeDistroSeriesLucilleconfig',
> +    'PublishingTestCase',
> +    'SoyuzTestPublisher',
> +]
> +
...
> +
> +class SoyuzTestPublisher:
> +    """Helper class able to publish source and binary packages.
> +
...
> +
> +    Each method has a number of paramters that can be used to control

"parameters"

...
> +    def addFile(self, filename, filecontent=None, restricted=False,
> +                expires=None):
> +        """Add a file to the Librarian.
> +
> +        :param filename: the filename to add.
> +        :param filecontent: the content of the file, or None to generate
> +            some content.
> +        :param restricted: whether the file should be on the restricted
> +            librarian.
> +        :param exipres: a `datetime.datetime` at which the file should

"expires"

...
> +    def makeSourcePackageRelease(self, distroseries=None, sourcename=None,
> +                                 archive=None, version=None, urgency=None,
> +                                 component_name=None, section_name=None,
> +                                 maintainer=None, creator=None,
> +                                 builddepends=None, builddependsindep=None,
> +                                 build_conflicts=None,
> +                                 build_conflicts_indep=None,
> +                                 architecturehintlist=None,
> +                                 dsc_signing_key=None,
> +                                 dsc_maintainer_rfc822=None,
> +                                 dsc_standards_version=None,
> +                                 dsc_format=None, dsc_binaries=None,
> +                                 date_uploaded=None, changelog_entry=None):
> +        """Make a `SourcePackageRelease`.
> +
> +        :param distroseries: the distroseries that the package should
> +            be targered to, or None to use the default.

"targeted"

...
> +    def createSource(self, sourcename, version, archive=None,
> +                     distroseries=None, new_version=None,
> +                     component_name=None):
> +        """Create source with meaningful '.changes' file.
> +
...
> +        :rtype: `SourcePackagePublishingHistory` or None
> +        """
> +        top = 'lib/lp/archiveuploader/tests/data/suite'

Yikes.

A nicer way to do this is to import lp.archiveuploader.tests and use
its __file__ attribute to get the directory location.

> +class PublishingTestCase(TestCaseWithFactory):
> +    """A test case that has access to a `SoyuzTestPublisher`.
> +
> +    In addition:
> +
> +        * the default distribution and distroseries of
> +          the publisher will have lucille configurations, so that they
> +          can be used with the publisher.

I once spent many hours trying to find out what "lucille" was, only to
find it's actually the old name for Launchpad, which was then only
what we now call Soyuz. What do you mean by lucille here? Is there
some way we could put your answer into the documentation?

Other than that, all good.

jml
-- 
https://code.launchpad.net/~james-w/launchpad/move-soyuz-test-publisher/+merge/32157
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~james-w/launchpad/move-soyuz-test-publisher into lp:launchpad/devel.