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, changes):
> +        tempdir = self.makeTemporaryDirectory()
> +        path = os.path.join(tempdir, filename)
> +        changes_fd = open(path, "w")
> +        try:
> +            changes.dump(changes_fd)
> +        finally:
> +            changes_fd.close()
> +        return ChangesFile(path, self.policy, self.logger)
> +
> +    def getBaseChanges(self):
> +        contents = Changes()
> +        contents["Source"] = "mypkg"
> +        contents["Binary"] = "binary"
> +        contents["Date"] = "Fri, 25 Jun 2010 11:20:22 -0600"
> +        contents["Architecture"] = "i386"
> +        contents["Version"] = "0.1"
> +        contents["Distribution"] = "nifty"
> +        contents["Maintainer"] = "Somebody"
> +        contents["Changes"] = "Something changed"
> +        contents["Description"] = "\n An awesome package."
> +        contents["Changed-By"] = "Somebody <email address hidden>"
> +        contents["Files"] = [{
> +            "md5sum": "d2bd347b3fed184fe28e112695be491c",
> +            "size": "1791",
> +            "section": "python",
> +            "priority": "optional",
> +            "name": "dulwich_0.4.1-1.dsc"}]
> +        return contents
> +
> +    def test_checkFileName(self):

This test needs a comment listing the expected behaviour it tests (e.g.
"# If the frobnob is poked it goes 'bloop'" at the start.

> +        contents = self.getBaseChanges()
> +        changes = self.createChangesFile("mypkg_0.1_i386.changes", contents)
> +        self.assertEquals([], list(changes.checkFileName()))
> +        changes = self.createChangesFile("mypkg_0.1.changes", contents)
> +        errors = list(changes.checkFileName())
> +        self.assertIsInstance(errors[0], UploadError)
> +        self.assertEquals(1, len(errors))
> +
> +    def test_filename(self):

Here too.

> +        changes = self.createChangesFile("mypkg_0.1_i386.changes",
> +            self.getBaseChanges())
> +        self.assertEquals("mypkg_0.1_i386.changes", changes.filename)
> +
> +    def test_suite_name(self):

And here.

> +        changes = self.createChangesFile("mypkg_0.1_i386.changes",
> +            self.getBaseChanges())

This should be formatted as 

        changes = self.createChangesFile(
            "mypkg_0.1_i386.changes",
            self.getBaseChanges())

> +        self.assertEquals("nifty", changes.suite_name)
> +
> +    def test_version(self):

Comment needed.

> +        changes = self.createChangesFile("mypkg_0.1_i386.changes",
> +            self.getBaseChanges())

Formatting as above.

> +        self.assertEquals("0.1", changes.version)
> +
> +    def test_architectures(self):

Comment needed.

> +        changes = self.createChangesFile("mypkg_0.1_i386.changes",
> +            self.getBaseChanges())

Formatting.

> +        self.assertEquals("i386", changes.architecture_line)
> +        self.assertEquals(set(["i386"]), changes.architectures)
> +
> +    def test_source(self):

Comment needed.

> +        changes = self.createChangesFile("mypkg_0.1_i386.changes",
> +            self.getBaseChanges())

Formatting.

> +        self.assertEquals("mypkg", changes.source)
> +
> +    def test_processAddresses(self):

Comment needed.

> +        contents = self.getBaseChanges()
> +        changes = self.createChangesFile("mypkg_0.1_i386.changes",
> +            contents)

Formatting.

> +        self.assertEquals(None, changes.changed_by)
> +        errors = list(changes.processAddresses())
> +        self.assertEquals(0, len(errors), "Errors: %r" % errors)
> +        self.assertEquals("Somebody <email address hidden>",
> +            changes.changed_by['rfc822'])

Formatting.

> +
> +    def test_simulated_changelog(self):

Comment.

> +        contents = self.getBaseChanges()
> +        changes = self.createChangesFile("mypkg_0.1_i386.changes",
> +            contents)

Formatting.

> +        self.assertEquals([], list(changes.processAddresses()))
> +        self.assertEquals("Something changed\n"
> +            " -- Somebody <email address hidden>   Fri, 25 Jun 2010 11:20:22 -0600",
> +            changes.simulated_changelog)

Formatting.

> +
> +    def test_requires_changed_by(self):

Comment.

> +        contents = self.getBaseChanges()
> +        del contents["Changed-By"]
> +        self.assertRaises(UploadError,
> +            self.createChangesFile, "mypkg_0.1_i386.changes", contents)

Formatting.

> 
> === added file 'lib/lp/archiveuploader/tests/test_nascentuploadfile.py'
> --- lib/lp/archiveuploader/tests/test_nascentuploadfile.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/archiveuploader/tests/test_nascentuploadfile.py	2010-08-17 13:21:05 +0000
> @@ -0,0 +1,232 @@
> +# Copyright 2010 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Test NascentUploadFile functionality."""
> +
> +__metaclass__ = type
> +
> +from debian.deb822 import Changes, Dsc
> +import hashlib
> +import os
> +
> +from canonical.launchpad.scripts.logger import BufferLogger
> +from lp.archiveuploader.changesfile import ChangesFile
> +from lp.archiveuploader.dscfile import DSCFile
> +from lp.archiveuploader.nascentuploadfile import (CustomUploadFile,
> +    DebBinaryUploadFile)

One line per import, first import on a new line, thus:

from lp.archiveuploader.nascentuploadfile import (
    CustomUploadFile,
    DebBinaryUploadFile,
    )

> +from lp.archiveuploader.uploadpolicy import AbsolutelyAnythingGoesUploadPolicy
> +from lp.soyuz.interfaces.queue import PackageUploadCustomFormat
> +from lp.testing import TestCaseWithFactory
> +from canonical.testing import LaunchpadZopelessLayer

This should be up with the other from canonical import.

> +
> +
> +class NascentUploadFileTestCase(TestCaseWithFactory):
> +    """Base class for all tests of classes deriving from NascentUploadFile."""
> +
> +    def setUp(self):
> +        super(NascentUploadFileTestCase, self).setUp()
> +        self.logger = BufferLogger()
> +        self.policy = AbsolutelyAnythingGoesUploadPolicy()
> +        self.distro = self.factory.makeDistribution()
> +        self.policy.archive = self.factory.makeArchive(
> +            distribution=self.distro)
> +
> +    def writeUploadFile(self, filename, contents):
> +        """Write a temporary file but with a specific filename.
> +
> +        :param filename: Filename to use
> +        :param contents: Contents of the file
> +        :return: Tuple with path, digest and size
> +        """
> +        path = os.path.join(self.makeTemporaryDirectory(), filename)
> +        f = open(path, 'w')
> +        try:
> +            f.write(contents)
> +        finally:
> +            f.close()
> +        return (path, hashlib.sha1(contents), len(contents))
> +
> +
> +class CustomUploadFileTests(NascentUploadFileTestCase):
> +    """Tests for CustomUploadFile."""
> +
> +    layer = LaunchpadZopelessLayer
> +
> +    def createCustomUploadFile(self, filename, contents,
> +        component_and_section, priority_name):

Arguments on a new line should line up with the first argument in the
definition.

> +        """Simple wrapper to create a CustomUploadFile."""
> +        (path, digest, size) = self.writeUploadFile(filename, contents)
> +        uploadfile = CustomUploadFile(
> +            path, digest, size, component_and_section, priority_name,
> +            self.policy, self.logger)
> +        return uploadfile
> +
> +    def test_custom_type(self):

Comment needed.

> +        uploadfile = self.createCustomUploadFile(
> +            "bla.txt", "data", "main/raw-installer", "extra")
> +        self.assertEquals(
> +            PackageUploadCustomFormat.DEBIAN_INSTALLER,
> +            uploadfile.custom_type)
> +
> +    def test_storeInDatabase(self):

Comment needed.

> +        uploadfile = self.createCustomUploadFile(
> +            "bla.txt", "data", "main/raw-installer", "extra")
> +        self.assertEquals("application/octet-stream", uploadfile.content_type)
> +        libraryfile = uploadfile.storeInDatabase()
> +        self.assertEquals("bla.txt", libraryfile.filename)
> +        self.assertEquals("application/octet-stream", libraryfile.mimetype)
> +
> +
> +class PackageUploadFileTestCase(NascentUploadFileTestCase):
> +    """Base class for all tests of classes deriving from PackageUploadFile."""
> +
> +    def setUp(self):
> +        super(PackageUploadFileTestCase, self).setUp()
> +        self.policy.distroseries = self.factory.makeDistroSeries(
> +            distribution=self.distro)
> +
> +    def getBaseChanges(self):
> +        contents = Changes()
> +        contents["Source"] = "mypkg"
> +        contents["Binary"] = "binary"
> +        contents["Architecture"] = "i386"
> +        contents["Version"] = "0.1"
> +        contents["Distribution"] = "nifty"
> +        contents["Description"] = "\n Foo"
> +        contents["Maintainer"] = "Somebody"
> +        contents["Changes"] = "Something changed"
> +        contents["Date"] = "Fri, 25 Jun 2010 11:20:22 -0600"
> +        contents["Urgency"] = "low"
> +        contents["Changed-By"] = "Seombody Else <email address hidden>"
> +        contents["Files"] = [{
> +            "md5sum": "d2bd347b3fed184fe28e112695be491c",
> +            "size": "1791",
> +            "section": "python",
> +            "priority": "optional",
> +            "name": "dulwich_0.4.1-1.dsc"}]
> +        return contents
> +
> +    def createChangesFile(self, filename, changes):
> +        tempdir = self.makeTemporaryDirectory()
> +        path = os.path.join(tempdir, filename)
> +        changes_fd = open(path, "w")
> +        try:
> +            changes.dump(changes_fd)
> +        finally:
> +            changes_fd.close()
> +        return ChangesFile(path, self.policy, self.logger)
> +
> +
> +class DSCFileTests(PackageUploadFileTestCase):
> +    """Tests for DSCFile."""
> +
> +    layer = LaunchpadZopelessLayer
> +
> +    def getBaseDsc(self):
> +        dsc = Dsc()
> +        dsc["Architecture"] = "all"
> +        dsc["Version"] = "0.42"
> +        dsc["Source"] = "dulwich"
> +        dsc["Binary"] = "python-dulwich"
> +        dsc["Standards-Version"] = "0.2.2"
> +        dsc["Maintainer"] = "Jelmer Vernooij <email address hidden>"
> +        dsc["Files"] = [{
> +            "md5sum": "5e8ba79b4074e2f305ddeaf2543afe83",
> +            "size": "182280",
> +            "name": "dulwich_0.42.tar.gz"}]
> +        return dsc
> +
> +    def createDSCFile(self, filename, dsc, component_and_section,
> +            priority_name, package, version, changes):

Argument formatting as above, please.

> +        (path, digest, size) = self.writeUploadFile(filename, dsc.dump())
> +        if changes:
> +            self.assertEquals([], list(changes.processAddresses()))
> +        return DSCFile(
> +            path, digest, size, component_and_section, priority_name, package,
> +            version, changes, self.policy, self.logger)
> +
> +    def test_filetype(self):

Needs a Comment.

> +        dsc = self.getBaseDsc()
> +        uploadfile = self.createDSCFile("foo.dsc", dsc,
> +            "main/net", "extra", "dulwich", "0.42", None)

Formatting as above.

> +        self.assertEquals("text/x-debian-source-package",
> +            uploadfile.content_type)

Here, too.

> +
> +    def test_storeInDatabase(self):

Comment needed.

> +        dsc = self.getBaseDsc()
> +        dsc["Build-Depends"] = "dpkg, bzr"
> +        changes = self.getBaseChanges()
> +        uploadfile = self.createDSCFile(
> +            "foo.dsc", dsc, "main/net", "extra", "dulwich", "0.42",
> +            self.createChangesFile("foo.changes", changes))
> +        (uploadfile.changelog_path, changelog_digest, changelog_size) = \
> +            self.writeUploadFile("changelog", "DUMMY")

We don't use \ to separate lines. Instead, this should use parens:

        (uploadfile.changelog_path, changelog_digest, changelog_size) = (
            self.writeUploadFile("changelog", "DUMMY"))

> +        uploadfile.files = []
> +        release = uploadfile.storeInDatabase(None)
> +        self.assertEquals("0.42", release.version)
> +        self.assertEquals("dpkg, bzr", release.builddepends)
> +
> +
> +class DebBinaryUploadFileTests(PackageUploadFileTestCase):
> +    """Tests for DebBinaryUploadFile."""
> +
> +    layer = LaunchpadZopelessLayer
> +
> +    def getBaseControl(self):
> +        return {
> +            "Package": "python-dulwich",
> +            "Source": "dulwich",
> +            "Version": "0.42",
> +            "Architecture": "i386",
> +            "Maintainer": "Jelmer Vernooij <email address hidden>",
> +            "Installed-Size": "524",
> +            "Depends": "python (<< 2.7), python (>= 2.5)",
> +            "Provides": "python2.5-dulwich, python2.6-dulwich",
> +            "Section": "python",
> +            "Priority": "optional",
> +            "Homepage": "http://samba.org/~jelmer/dulwich",
> +            "Description": "Pure-python Git library\n"
> +                "Dulwich is a Python implementation of the file formats and "
> +                "protocols"}

The closing brace should be on its own line, aligned with the items in
the dict, and the last item should have a trailling comma, thus:

            "Description": "Pure-python Git library\n"
                "Dulwich is a Python implementation of the file formats and "
                "protocols",
            }

> +
> +    def createDebBinaryUploadFile(self, filename, component_and_section,
> +        priority_name, package, version, changes):
> +        """Create a DebBinaryUploadFile."""
> +        (path, digest, size) = self.writeUploadFile(filename, "DUMMY DATA")
> +        return DebBinaryUploadFile(
> +            path, digest, size, component_and_section, priority_name, package,
> +            version, changes, self.policy, self.logger)
> +
> +    def test_unknown_priority(self):

Comment needed.

> +        uploadfile = self.createDebBinaryUploadFile(
> +            "foo_0.42_i386.deb", "main/net", "unknown", "mypkg", "0.42", None)
> +        self.assertEquals("extra", uploadfile.priority_name)
> +
> +    def test_parseControl(self):

Comment needed.

> +        uploadfile = self.createDebBinaryUploadFile(
> +            "foo_0.42_i386.deb", "main/python", "unknown", "mypkg", "0.42",
> +            None)
> +        control = self.getBaseControl()
> +        uploadfile.parseControl(control)
> +        self.assertEquals("python", uploadfile.section_name)
> +        self.assertEquals("dulwich", uploadfile.source_name)
> +        self.assertEquals("0.42", uploadfile.source_version)
> +        self.assertEquals("0.42", uploadfile.control_version)
> +
> +    def test_storeInDatabase(self):

Comment needed.

> +        uploadfile = self.createDebBinaryUploadFile(
> +            "foo_0.42_i386.deb", "main/python", "unknown", "mypkg", "0.42",
> +            None)
> +        control = self.getBaseControl()
> +        uploadfile.parseControl(control)
> +        build = self.factory.makeBinaryPackageBuild()
> +        bpr = uploadfile.storeInDatabase(build)
> +        self.assertEquals(u'python (<< 2.7), python (>= 2.5)', bpr.depends)
> +        self.assertEquals(
> +            u"Dulwich is a Python implementation of the file formats "
> +            u"and protocols", bpr.description)
> +        self.assertEquals(False, bpr.essential)
> +        self.assertEquals(524, bpr.installedsize)
> +        self.assertEquals(True, bpr.architecturespecific)
> +        self.assertEquals(None, bpr.recommends)
> +        self.assertEquals("0.42", bpr.version)
>