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

[Jelmer Vernooij]

Add more tests for ChangesFile, DebBinaryUploadFile, DSCFile, CustomUploadFile in the archiveuploader. Make Changed-By and Date field mandatory in ChangesFile.

This branch adds a bunch of tests and testinfrastructure for ChangesFile, DSCFile and two of the classes in nascentuploadfile. I'm adding these tests in preparation of adding some more fields (bug 613468).

While writing the tests I noticed that we seem to unconditionally require the "Changed-By" and "Date" files in the Changes file but don't reject uploads with these fields, so I've added them to the list of mandatory fields (bug filed & linked).

[Graham Binns (gmb)]

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

review: Needs Fixing (code)

[Graham Binns (gmb)]

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.

review: Approve (code)

=== 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-19 15:51:44 +0000
@@ -41,7 +41,11 @@
 
     mandatory_fields = set([
         "source", "binary", "architecture", "version", "distribution",
-        "maintainer", "files", "changes"])
+        "maintainer", "files", "changes", "date",
+        # Changed-By is not technically mandatory according to
+        # Debian policy but Soyuz relies on it being set in
+        # various places.
+        "changed-by"])
 
     # Map urgencies to their dbschema values.
     # Debian policy only permits low, medium, high, emergency.
@@ -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 targeted at a section like
         '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 'urgency' not in self._dict:
+            # 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(
@@ -321,7 +329,7 @@
 
     @property
     def source(self):
-        """Return changesfiel claimed source name"""
+        """Return changesfile claimed source name."""
         return self._dict['source']
 
     @property

=== modified file 'lib/lp/archiveuploader/dscfile.py'
--- lib/lp/archiveuploader/dscfile.py	2010-08-03 08:49:19 +0000
+++ lib/lp/archiveuploader/dscfile.py	2010-08-19 15:51:44 +0000
@@ -740,6 +740,7 @@
     logger.debug("Copying copyright contents.")
     dsc_file.copyright = open(copyright_file).read().strip()
 
+
 def findChangelog(dsc_file, source_dir, logger):
     """Find and move any debian/changelog.
 

=== modified file 'lib/lp/archiveuploader/nascentuploadfile.py'
--- lib/lp/archiveuploader/nascentuploadfile.py	2010-07-21 11:13:19 +0000
+++ lib/lp/archiveuploader/nascentuploadfile.py	2010-08-19 15:51:44 +0000
@@ -321,7 +321,6 @@
                 "%s: Unknown component %r" % (
                 self.filename, self.component_name))
 
-
     @property
     def component(self):
         """Return an IComponent for self.component.name."""
@@ -492,12 +491,15 @@
                 yield UploadError(
                     "%s: control file lacks mandatory field %r"
                      % (self.filename, mandatory_field))
+        control = {}
+        for key in control_lines.keys():
+            control[key] = control_lines.Find(key)
+        self.parseControl(control)
 
+    def parseControl(self, control):
         # XXX kiko 2007-02-15: We never use the Maintainer information in
         # the control file for anything. Should we? --
-        self.control = {}
-        for key in control_lines.keys():
-            self.control[key] = control_lines.Find(key)
+        self.control = control
 
         control_source = self.control.get("Source", None)
         if control_source is not None:

=== 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-19 15:51:44 +0000
@@ -5,14 +5,27 @@
 
 __metaclass__ = type
 
-from testtools import TestCase
-
-from lp.archiveuploader.changesfile import (CannotDetermineFileTypeError,
-    determine_file_class_and_name)
+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,
+    ChangesFile,
+    determine_file_class_and_name,
+    )
 from lp.archiveuploader.dscfile import DSCFile
 from lp.archiveuploader.nascentuploadfile import (
-    DebBinaryUploadFile, DdebBinaryUploadFile, SourceUploadFile,
-    UdebBinaryUploadFile)
+    DebBinaryUploadFile,
+    DdebBinaryUploadFile,
+    SourceUploadFile,
+    UdebBinaryUploadFile,
+    UploadError,
+    )
+from lp.archiveuploader.tests import AbsolutelyAnythingGoesUploadPolicy
+from lp.testing import TestCase
 
 
 class TestDetermineFileClassAndName(TestCase):
@@ -52,3 +65,118 @@
             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 <[e-mail]>"
+        contents["Files"] = [{
+            "md5sum": "d2bd347b3fed184fe28e112695be491c",
+            "size": "1791",
+            "section": "python",
+            "priority": "optional",
+            "name": "dulwich_0.4.1-1.dsc"}]
+        return contents
+
+    def test_checkFileName(self):
+        # checkFileName() yields an UploadError if the filename is invalid.
+        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):
+        # Filename gets set to the basename of the changes file on disk.
+        changes = self.createChangesFile(
+            "mypkg_0.1_i386.changes", self.getBaseChanges())
+        self.assertEquals("mypkg_0.1_i386.changes", changes.filename)
+
+    def test_suite_name(self):
+        # The suite name gets extracted from the changes file.
+        changes = self.createChangesFile(
+            "mypkg_0.1_i386.changes", self.getBaseChanges())
+        self.assertEquals("nifty", changes.suite_name)
+
+    def test_version(self):
+        # The version gets extracted from the changes file.
+        changes = self.createChangesFile(
+            "mypkg_0.1_i386.changes", self.getBaseChanges())
+        self.assertEquals("0.1", changes.version)
+
+    def test_architectures(self):
+        # The architectures get extracted from the changes file
+        # and parsed correctly.
+        changes = self.createChangesFile(
+            "mypkg_0.1_i386.changes", self.getBaseChanges())
+        self.assertEquals("i386", changes.architecture_line)
+        self.assertEquals(set(["i386"]), changes.architectures)
+
+    def test_source(self):
+        # The source package name gets extracted from the changes file.
+        changes = self.createChangesFile(
+            "mypkg_0.1_i386.changes", self.getBaseChanges())
+        self.assertEquals("mypkg", changes.source)
+
+    def test_processAddresses(self):
+        # processAddresses parses the changes file and sets the
+        # changed_by field.
+        contents = self.getBaseChanges()
+        changes = self.createChangesFile(
+            "mypkg_0.1_i386.changes", contents)
+        self.assertEquals(None, changes.changed_by)
+        errors = list(changes.processAddresses())
+        self.assertEquals(0, len(errors), "Errors: %r" % errors)
+        self.assertEquals(
+            "Somebody <[e-mail]>", changes.changed_by['rfc822'])
+
+    def test_simulated_changelog(self):
+        # The simulated_changelog property returns a changelog entry based on
+        # the control fields.
+        contents = self.getBaseChanges()
+        changes = self.createChangesFile(
+            "mypkg_0.1_i386.changes", contents)
+        self.assertEquals([], list(changes.processAddresses()))
+        self.assertEquals(
+            "Something changed\n"
+            " -- Somebody <[e-mail]>   Fri, 25 Jun 2010 11:20:22 -0600",
+            changes.simulated_changelog)
+
+    def test_requires_changed_by(self):
+        # A changes file is rejected if it does not have a Changed-By field.
+        contents = self.getBaseChanges()
+        del contents["Changed-By"]
+        self.assertRaises(
+            UploadError,
+            self.createChangesFile, "mypkg_0.1_i386.changes", contents)

=== 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-19 15:51:44 +0000
@@ -0,0 +1,243 @@
+# 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 canonical.testing import LaunchpadZopelessLayer
+
+from lp.archiveuploader.changesfile import ChangesFile
+from lp.archiveuploader.dscfile import DSCFile
+from lp.archiveuploader.nascentuploadfile import (
+    CustomUploadFile,
+    DebBinaryUploadFile,
+    )
+from lp.archiveuploader.tests import AbsolutelyAnythingGoesUploadPolicy
+from lp.soyuz.interfaces.queue import PackageUploadCustomFormat
+from lp.testing import TestCaseWithFactory
+
+
+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):
+        """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):
+        # The mime type gets set according to PackageUploadCustomFormat.
+        uploadfile = self.createCustomUploadFile(
+            "bla.txt", "data", "main/raw-installer", "extra")
+        self.assertEquals(
+            PackageUploadCustomFormat.DEBIAN_INSTALLER,
+            uploadfile.custom_type)
+
+    def test_storeInDatabase(self):
+        # storeInDatabase creates a library file.
+        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 <[e-mail]>"
+        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 <[e-mail]>"
+        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):
+        (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):
+        # The filetype attribute is set based on the file extension.
+        dsc = self.getBaseDsc()
+        uploadfile = self.createDSCFile(
+            "foo.dsc", dsc, "main/net", "extra", "dulwich", "0.42", None)
+        self.assertEquals(
+            "text/x-debian-source-package", uploadfile.content_type)
+
+    def test_storeInDatabase(self):
+        # storeInDatabase creates a SourcePackageRelease.
+        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"))
+        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 <[e-mail]>",
+            "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"
+            }
+
+    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):
+        # Unknown priorities automatically get changed to 'extra'.
+        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):
+        # parseControl sets various fields on DebBinaryUploadFile.
+        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):
+        # storeInDatabase creates a BinaryPackageRelease.
+        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(u"", bpr.recommends)
+        self.assertEquals("0.42", bpr.version)