Merge lp:~jelmer/launchpad/nascentuploadfile-tests into lp:launchpad

Hi Jelmer,

Thanks for adding tests; always a good thing.

This is a good branch, but it suffers from a lot of issues w.r.t our
coding standards. All of the tests should have a leading comment
explaining the behaviour that they test. Also, the formatting of
callsites is inconsistent and should be changed to match our standards.

> === modified file 'lib/lp/archiveuploader/changesfile.py'
> --- lib/lp/archiveuploader/changesfile.py 2010-08-01 06:59:04 +0000
> +++ lib/lp/archiveuploader/changesfile.py 2010-08-17 13:21:05 +0000
> @@ -148,7 +152,7 @@
> def isCustom(self, component_and_section):
> """Check if given 'component_and_section' matches a custom upload.
>
> - We recognize an upload as custom if it is taget to a section like
> + We recognize an upload as custom if it is targetted at a section like

Targeted has only two 't's.

> 'raw-<something>'.
> Further checks will be performed in CustomUploadFile class.
> """
> @@ -215,6 +219,10 @@
> if len(self.files) == 0:
> yield UploadError("No files found in the changes")
>
> + if not 'urgency' in self._dict:

This should be expressed as:

    if 'urgency' not in self._dict:

The current condition confused me.

> + # Urgency is recommended but not mandatory. Default to 'low'
> + self._dict['urgency'] = "low"
> +
> raw_urgency = self._dict['urgency'].lower()
> if raw_urgency not in self.urgency_map:
> yield UploadWarning(
>
> === modified file 'lib/lp/archiveuploader/tests/test_changesfile.py'
> --- lib/lp/archiveuploader/tests/test_changesfile.py 2010-08-01 06:59:04 +0000
> +++ lib/lp/archiveuploader/tests/test_changesfile.py 2010-08-17 13:21:05 +0000
> @@ -5,14 +5,20 @@
>
> __metaclass__ = type
>
> -from testtools import TestCase
> +from debian.deb822 import Changes
> +import os
> +
> +from canonical.launchpad.scripts.logger import BufferLogger
> +from canonical.testing import LaunchpadZopelessLayer
>
> from lp.archiveuploader.changesfile import (CannotDetermineFileTypeError,
> - determine_file_class_and_name)
> + ChangesFile, determine_file_class_and_name)

Since you're here, you can tidy this up into the new one-line-per-import
format.

> from lp.archiveuploader.dscfile import DSCFile
> from lp.archiveuploader.nascentuploadfile import (
> DebBinaryUploadFile, DdebBinaryUploadFile, SourceUploadFile,
> - UdebBinaryUploadFile)
> + UdebBinaryUploadFile, UploadError)

Same here.

> +from lp.archiveuploader.uploadpolicy import AbsolutelyAnythingGoesUploadPolicy
> +from lp.testing import TestCase
>
>
> class TestDetermineFileClassAndName(TestCase):
> @@ -52,3 +58,104 @@
> CannotDetermineFileTypeError,
> determine_file_class_and_name,
> 'foo')
> +
> +
> +class ChangesFileTests(TestCase):
> + """Tests for ChangesFile."""
> +
> + layer = LaunchpadZopelessLayer
> +
> + def setUp(self):
> + super(ChangesFileTests, self).setUp()
> + self.logger = BufferLogger()
> + self.policy = AbsolutelyAnythingGoesUploadPolicy()
> +
> + def createChangesFile(self, filename, cha...

Hi Jelmer,

Thanks for those changes. Everything looks good, but you need to change the comments you've added to the start of the tests from the "Test that.." form to a straight statement of the expected behaviour.

So, instead of " # Test that unknown priorities automatically get changed to 'extra'," the comment should read " # Unknown priorities automatically get changed to 'extra'." This is a carry-over from doctest land, but being able to glance at a comment and know that foo should do bar rather than having to parse out the 'test that...' is nice. We know it's testing something because the method name starts with test_.

Other than that, this branch is good to land, so I'll mark it approved. No need to ping me about any subsequent changes unless they're not in-scope for this review.