Merge lp:~wgrant/launchpad/test-ddeb-matching into lp:launchpad/db-devel

[William Grant]

Add unit tests for NascentUpload's DDEB matching code.

This branch factors out and adds unit tests for the DDEB-matching part of NascentUpload.

To make the tests less foul, I've altered NascentUpload to allow passing in an existing ChangesFile-like object, rather than requiring a path to a real file. This should eventually let us shrink existing tests throughout archiveuploader, and even remove test package data.

I've left the existing basic end-to-end test at the end of nascentupload-ddebs.txt, to ensure that detected errors do in fact cause rejection and not terrible explosions.

[Māris Fogels (mars)]

Hi William,

This is a meaty change, lots of new code. Very nice. I especially like the refactoring of NascentUpload and _matchDDEBs(): both excellent changes.

Here is the list of notes I took while reviewing the code:

• Do ChangesFile objects have a .path attribute that you can use instead of string-sniffing in the constructor? If so, then perhaps you can push the fiddly conditional construction down into ChangesFile itself and at the same time eliminate the .changesfile_path attribute from the NascentUpload object. A NascentUpload.from_path() factory method would be even better, eliminating the conditional construction entirely.

• Switching from .reject() to 'yield UploadError()' is a pretty big API change, however I do not see any code in the diff (besides the tests) that is impacted by it. Why not?

• It would be good to have a docstring for _matchDDEBs() that tells you that it is a generator method.

• I think the test names could be improved a bit: the comment should be used as test method name, because the comments tell you what the desired behaviour is. They tell you what the test is actually testing.

• You might be able to rewrite all those "self.assertEquals([], list(self.upload._matchDDEBs()))" statements using the nifty testtools.TestCase.assertThat() method: self.assertThat(self.upload, matchesDDEBs([]))

With the above points taken into consideration, r=mars

review: Approve (code)

[William Grant (wgrant)]

> Hi William,
>
> This is a meaty change, lots of new code. Very nice. I especially like the
> refactoring of NascentUpload and _matchDDEBs(): both excellent changes.
>
> Here is the list of notes I took while reviewing the code:
>
> • Do ChangesFile objects have a .path attribute that you can use instead of
> string-sniffing in the constructor? If so, then perhaps you can push the
> fiddly conditional construction down into ChangesFile itself and at the same
> time eliminate the .changesfile_path attribute from the NascentUpload object.
> A NascentUpload.from_path() factory method would be even better, eliminating
> the conditional construction entirely.

Both fixed. All path-based NascentUpload() callsites updated to use NascentUpload.from_changesfile_path() instead.

> • Switching from .reject() to 'yield UploadError()' is a pretty big API
> change, however I do not see any code in the diff (besides the tests) that is
> impacted by it. Why not?

It's a pattern used throughout archiveuploader -- we call
run_and_collect_errors on an error generator, and it rejects with all the
yielded errors.

> • It would be good to have a docstring for _matchDDEBs() that tells you that
> it is a generator method.

Missed that. Fixed.

> • I think the test names could be improved a bit: the comment should be used
> as test method name, because the comments tell you what the desired behaviour
> is. They tell you what the test is actually testing.

Fair point. Fixed.

> • You might be able to rewrite all those "self.assertEquals([],
> list(self.upload._matchDDEBs()))" statements using the nifty
> testtools.TestCase.assertThat() method: self.assertThat(self.upload,
> matchesDDEBs([]))

I opted to instead create a tiny domain-specific assertion method.

> With the above points taken into consideration, r=mars

Thanks.

[Māris Fogels (mars)]

These changes still look good to me. I'll run it through ec2 land.

Maris

=== modified file 'lib/canonical/launchpad/testing/fakepackager.py'
--- lib/canonical/launchpad/testing/fakepackager.py	2009-09-14 04:07:18 +0000
+++ lib/canonical/launchpad/testing/fakepackager.py	2010-08-04 22:12:18 +0000
@@ -278,7 +278,8 @@
         if suite is not None:
             policy.setDistroSeriesAndPocket(suite)
 
-        upload = NascentUpload(changesfile_path, policy, logger)
+        upload = NascentUpload.from_changesfile_path(
+            changesfile_path, policy, logger)
         upload.process()
 
         return upload

=== modified file 'lib/lp/archiveuploader/nascentupload.py'
--- lib/lp/archiveuploader/nascentupload.py	2010-08-03 08:49:19 +0000
+++ lib/lp/archiveuploader/nascentupload.py	2010-08-04 22:12:18 +0000
@@ -86,24 +86,35 @@
     # Defined if we successfully do_accept() and storeObjectsInDatabase()
     queue_root = None
 
-    def __init__(self, changesfile_path, policy, logger):
-        """Setup a ChangesFile based on given changesfile path.
+    def __init__(self, changesfile, policy, logger):
+        """Setup a NascentUpload for the given ChangesFile.
 
-        May raise FatalUploadError due to unrecoverable problems building
-        the ChangesFile object.
-        Also store given and initialized Upload Policy, as 'policy'
+        :param changesfile: the ChangesFile object to be uploaded.
+        :param policy: the upload policy to be used.
+        :param logger: the logger to be used.
         """
-        self.changesfile_path = changesfile_path
         self.policy = policy
         self.logger = logger
 
         self.rejections = []
         self.warnings = []
-
         self.librarian = getUtility(ILibraryFileAliasSet)
+
+        self.changes = changesfile
+
+    @classmethod
+    def from_changesfile_path(cls, changesfile_path, policy, logger):
+        """Create a NascentUpload from the given changesfile path.
+
+        May raise FatalUploadError due to unrecoverable problems building
+        the ChangesFile object.
+
+        :param changesfile_path: path to the changesfile to be uploaded.
+        :param policy: the upload policy to be used.
+        :param logger: the logger to be used.
+        """
         try:
-            self.changes = ChangesFile(
-                changesfile_path, self.policy, self.logger)
+            changesfile = ChangesFile(changesfile_path, policy, logger)
         except UploadError, e:
             # We can't run reject() because unfortunately we don't have
             # the address of the uploader to notify -- we broke in that
@@ -112,6 +123,7 @@
             # rejection to the archive admins. For now, this will end
             # up in the script log.
             raise FatalUploadError(str(e))
+        return cls(changesfile, policy, logger)
 
     def process(self):
         """Process this upload, checking it against policy, loading it into
@@ -148,6 +160,7 @@
             self._check_sourceful_consistency()
         if self.binaryful:
             self._check_binaryful_consistency()
+            self.run_and_collect_errors(self._matchDDEBs)
 
         self.run_and_collect_errors(self.changes.verify)
 
@@ -155,35 +168,6 @@
         for uploaded_file in self.changes.files:
             self.run_and_collect_errors(uploaded_file.verify)
 
-        unmatched_ddebs = {}
-        for uploaded_file in self.changes.files:
-            if isinstance(uploaded_file, DdebBinaryUploadFile):
-                ddeb_key = (uploaded_file.package, uploaded_file.version,
-                            uploaded_file.architecture)
-                if ddeb_key in unmatched_ddebs:
-                    self.reject("Duplicated debug packages: %s %s (%s)" %
-                        ddeb_key)
-                else:
-                    unmatched_ddebs[ddeb_key] = uploaded_file
-
-        for uploaded_file in self.changes.files:
-            # We need exactly a DEB, not a DDEB.
-            if (isinstance(uploaded_file, DebBinaryUploadFile) and
-                not isinstance(uploaded_file, DdebBinaryUploadFile)):
-                try:
-                    matching_ddeb = unmatched_ddebs.pop(
-                        (uploaded_file.package + '-dbgsym',
-                         uploaded_file.version,
-                         uploaded_file.architecture))
-                except KeyError:
-                    continue
-                uploaded_file.ddeb_file = matching_ddeb
-                matching_ddeb.deb_file = uploaded_file
-
-        if len(unmatched_ddebs) > 0:
-            self.reject("Orphaned debug packages: %s" % ', '.join('%s %s (%s)' % d
-                for d in unmatched_ddebs))
-
         if (len(self.changes.files) == 1 and
             isinstance(self.changes.files[0], CustomUploadFile)):
             self.logger.debug("Single Custom Upload detected.")
@@ -197,7 +181,7 @@
                     "Upload rejected because it contains binary packages.",
                     "Ensure you are using `debuild -S`, or an equivalent",
                     "command, to generate only the source package before",
-                    "re-uploading."
+                    "re-uploading.",
                     ]
                 if self.is_ppa:
                     messages.append(
@@ -241,7 +225,7 @@
     @property
     def filename(self):
         """Return the changesfile name."""
-        return os.path.basename(self.changesfile_path)
+        return os.path.basename(self.changesfile.filepath)
 
     @property
     def is_new(self):
@@ -254,7 +238,6 @@
     #
     # Overall consistency checks
     #
-
     def _check_overall_consistency(self):
         """Heuristics checks on upload contents and declared architecture.
 
@@ -329,7 +312,8 @@
         """Heuristic checks on a sourceful upload.
 
         Raises AssertionError when called for a non-sourceful upload.
-        Ensures a sourceful upload has exactly one DSC.
+        Ensures a sourceful upload has exactly one DSC. All further source
+        checks are performed later by the DSC.
         """
         assert self.sourceful, (
             "Source consistency check called for a non-source upload")
@@ -369,10 +353,49 @@
         if len(considered_archs) > max:
             self.reject("Upload has more architetures than it is supported.")
 
+    def _matchDDEBs(self):
+        """Check and link DEBs and DDEBs in the upload.
+
+        Matches each DDEB to its corresponding DEB, adding links in both
+        directions. Unmatched or duplicated DDEBs result in upload errors.
+
+        This method is an error generator, i.e, it returns an iterator over
+        all exceptions that are generated while processing all mentioned
+        files.
+        """
+        unmatched_ddebs = {}
+        for uploaded_file in self.changes.files:
+            if isinstance(uploaded_file, DdebBinaryUploadFile):
+                ddeb_key = (uploaded_file.package, uploaded_file.version,
+                            uploaded_file.architecture)
+                if ddeb_key in unmatched_ddebs:
+                    yield UploadError(
+                        "Duplicated debug packages: %s %s (%s)" % ddeb_key)
+                else:
+                    unmatched_ddebs[ddeb_key] = uploaded_file
+
+        for uploaded_file in self.changes.files:
+            # We need exactly a DEB, not a DDEB.
+            if (isinstance(uploaded_file, DebBinaryUploadFile) and
+                not isinstance(uploaded_file, DdebBinaryUploadFile)):
+                try:
+                    matching_ddeb = unmatched_ddebs.pop(
+                        (uploaded_file.package + '-dbgsym',
+                         uploaded_file.version,
+                         uploaded_file.architecture))
+                except KeyError:
+                    continue
+                uploaded_file.ddeb_file = matching_ddeb
+                matching_ddeb.deb_file = uploaded_file
+
+        if len(unmatched_ddebs) > 0:
+            yield UploadError(
+                "Orphaned debug packages: %s" % ', '.join(
+                    '%s %s (%s)' % d for d in unmatched_ddebs))
+
     #
     # Helpers for warnings and rejections
     #
-
     def run_and_check_error(self, callable):
         """Run the given callable and process errors and warnings.
 
@@ -471,7 +494,6 @@
     #
     # Signature and ACL stuff
     #
-
     def verify_acl(self):
         """Check the signer's upload rights.
 
@@ -501,7 +523,7 @@
             ISourcePackageNameSet).queryByName(self.changes.dsc.package)
 
         rejection_reason = archive.checkUpload(
-            uploader, self.policy.distroseries, source_name, 
+            uploader, self.policy.distroseries, source_name,
             self.changes.dsc.component, self.policy.pocket, not self.is_new)
 
         if rejection_reason is not None:
@@ -510,7 +532,6 @@
     #
     # Handling checking of versions and overrides
     #
-
     def getSourceAncestry(self, uploaded_file):
         """Return the last published source (ancestry) for a given file.
 
@@ -739,8 +760,8 @@
             return
 
         component_override_map = {
-            'contrib' : 'multiverse',
-            'non-free' : 'multiverse',
+            'contrib': 'multiverse',
+            'non-free': 'multiverse',
             }
 
         # Apply the component override and default to universe.
@@ -814,7 +835,6 @@
     #
     # Actually processing accepted or rejected uploads -- and mailing people
     #
-
     def do_accept(self, notify=True):
         """Accept the upload into the queue.
 
@@ -921,7 +941,6 @@
     #
     # Inserting stuff in the database
     #
-
     def storeObjectsInDatabase(self):
         """Insert this nascent upload into the database."""
 
@@ -1044,8 +1063,7 @@
             # See if there is an archive to override with.
             distribution = self.policy.distroseries.distribution
             archive = distribution.getArchiveByComponent(
-                PARTNER_COMPONENT_NAME
-                )
+                PARTNER_COMPONENT_NAME)
 
             # Check for data problems:
             if not archive:

=== modified file 'lib/lp/archiveuploader/tests/nascentupload-announcements.txt'
--- lib/lp/archiveuploader/tests/nascentupload-announcements.txt	2010-07-24 09:12:37 +0000
+++ lib/lp/archiveuploader/tests/nascentupload-announcements.txt	2010-08-04 22:12:18 +0000
@@ -79,7 +79,7 @@
   >>> sync_policy = getPolicy(
   ...     name='sync', distro='ubuntu', distroseries='hoary')
 
-  >>> bar_src = NascentUpload(
+  >>> bar_src = NascentUpload.from_changesfile_path(
   ...     datadir('suite/bar_1.0-1/bar_1.0-1_source.changes'),
   ...     sync_policy, mock_logger_quiet)
   >>> bar_src.process()
@@ -132,7 +132,7 @@
 
 Uploading the same package again will result in a rejection email:
 
-  >>> bar_src = NascentUpload(
+  >>> bar_src = NascentUpload.from_changesfile_path(
   ...     datadir('suite/bar_1.0-1/bar_1.0-1_source.changes'),
   ...     sync_policy, mock_logger_quiet)
   >>> bar_src.process()
@@ -270,7 +270,7 @@
   >>> ppa_policy.archive = name16_ppa
   >>> ppa_policy.setDistroSeriesAndPocket('hoary')
 
-  >>> ppa_bar_src = NascentUpload(
+  >>> ppa_bar_src = NascentUpload.from_changesfile_path(
   ...     datadir('suite/bar_1.0-1/bar_1.0-1_source.changes'),
   ...     ppa_policy, mock_logger_quiet)
   >>> ppa_bar_src.process()
@@ -308,7 +308,7 @@
   ...     name='sync', distro='ubuntu', distroseries='hoary')
   >>> modified_sync_policy.can_upload_binaries = True
 
-  >>> bar_src = NascentUpload(
+  >>> bar_src = NascentUpload.from_changesfile_path(
   ...     datadir('suite/bar_1.0-1_binary/bar_1.0-1_i386.changes'),
   ...     modified_sync_policy, mock_logger_quiet)
   >>> bar_src.process()
@@ -336,7 +336,7 @@
   >>> modified_sync_policy = getPolicy(
   ...     name='sync', distro='ubuntu', distroseries='hoary')
 
-  >>> lang_pack = NascentUpload(
+  >>> lang_pack = NascentUpload.from_changesfile_path(
   ...     datadir('language-packs/language-pack-pt_1.0-1_source.changes'),
   ...     modified_sync_policy, mock_logger_quiet)
   >>> lang_pack.process()
@@ -367,7 +367,7 @@
   >>> modified_sync_policy = getPolicy(
   ...     name='sync', distro='ubuntu', distroseries='hoary')
 
-  >>> lang_pack = NascentUpload(
+  >>> lang_pack = NascentUpload.from_changesfile_path(
   ...     datadir('language-packs/language-pack-pt_1.0-2_source.changes'),
   ...     modified_sync_policy, mock_logger_quiet)
   >>> lang_pack.process()
@@ -398,7 +398,7 @@
   ...     name='insecure', distro='ubuntu', distroseries=None)
   >>> insecure_policy.setDistroSeriesAndPocket('hoary-updates')
 
-  >>> lang_pack = NascentUpload(
+  >>> lang_pack = NascentUpload.from_changesfile_path(
   ...     datadir('language-packs/language-pack-pt_1.0-3_source.changes'),
   ...     insecure_policy, mock_logger_quiet)
   >>> lang_pack.process()
@@ -423,7 +423,7 @@
 An UNAPPROVED binary upload via insecure will send one email saying that
 the upload is waiting for approval:
 
-  >>> bar_src = NascentUpload(
+  >>> bar_src = NascentUpload.from_changesfile_path(
   ...     datadir('suite/bar_1.0-2/bar_1.0-2_source.changes'),
   ...     insecure_policy, mock_logger_quiet)
   >>> bar_src.process()
@@ -451,7 +451,7 @@
   >>> unapproved_backports_policy = getPolicy(
   ...     name='insecure', distro='ubuntu', distroseries=None)
   >>> unapproved_backports_policy.setDistroSeriesAndPocket('hoary-backports')
-  >>> bar_src = NascentUpload(
+  >>> bar_src = NascentUpload.from_changesfile_path(
   ...     datadir('suite/bar_1.0-3_valid/bar_1.0-3_source.changes'),
   ...     unapproved_backports_policy, mock_logger_quiet)
   >>> bar_src.process()
@@ -479,7 +479,7 @@
   ...     name='sync', distro='ubuntu', distroseries=None)
   >>> modified_sync_policy.setDistroSeriesAndPocket('hoary-backports')
 
-  >>> bar_src = NascentUpload(
+  >>> bar_src = NascentUpload.from_changesfile_path(
   ...     datadir('suite/bar_1.0-4/bar_1.0-4_source.changes'),
   ...     modified_sync_policy, mock_logger_quiet)
   >>> bar_src.process()
@@ -536,7 +536,7 @@
   ...     name='security', distro='ubuntu', distroseries=None)
   >>> security_policy.setDistroSeriesAndPocket('hoary-security')
 
-  >>> bar_src = NascentUpload(
+  >>> bar_src = NascentUpload.from_changesfile_path(
   ...     datadir('suite/bar_1.0-2/bar_1.0-2_source.changes'),
   ...     security_policy, mock_logger_quiet)
   >>> bar_src.process()
@@ -729,7 +729,7 @@
 
   >>> hoary.status = SeriesStatus.DEVELOPMENT
 
-  >>> bar_src = NascentUpload(
+  >>> bar_src = NascentUpload.from_changesfile_path(
   ...     datadir(
   ...          'suite/bar_1.0-5_debian_auto_sync/bar_1.0-5_source.changes'),
   ...     sync_policy, mock_logger_quiet)
@@ -749,7 +749,7 @@
 
 In contrast, manual sync uploads do generate the announcement:
 
-  >>> bar_src = NascentUpload(
+  >>> bar_src = NascentUpload.from_changesfile_path(
   ...     datadir(
   ...          'suite/bar_1.0-6/bar_1.0-6_source.changes'),
   ...     sync_policy, mock_logger_quiet)
@@ -789,7 +789,7 @@
   ...     name='security', distro='ubuntu', distroseries=None)
   >>> security_policy.setDistroSeriesAndPocket('hoary-security')
 
-  >>> bar_bin = NascentUpload(
+  >>> bar_bin = NascentUpload.from_changesfile_path(
   ...     datadir('suite/bar_1.0-2_binary/bar_1.0-2_i386.changes'),
   ...     security_policy, mock_logger_quiet)
   >>> bar_bin.process()
@@ -849,7 +849,7 @@
   >>> modified_sync_policy.can_upload_mixed = True
   >>> modified_sync_policy.can_upload_binaries = True
   >>> modified_sync_policy.can_upload_source = True
-  >>> foo_v1 = NascentUpload(
+  >>> foo_v1 = NascentUpload.from_changesfile_path(
   ...     datadir('suite/foo_1.0-1_mixed/foo_1.0-1_i386.changes'),
   ...     modified_sync_policy, mock_logger_quiet)
   >>> foo_v1.process()
@@ -867,7 +867,7 @@
   >>> security_policy = getPolicy(
   ...     name='security', distro='ubuntu', distroseries=None)
   >>> security_policy.setDistroSeriesAndPocket('hoary-security')
-  >>> foo_mixed = NascentUpload(
+  >>> foo_mixed = NascentUpload.from_changesfile_path(
   ...     datadir('suite/foo_1.0-2.1/foo_1.0-2.1_source.changes'),
   ...     security_policy, mock_logger_quiet)
   >>> foo_mixed.process()
@@ -905,7 +905,7 @@
   >>> sync_policy = getPolicy(
   ...     name='sync', distro='ubuntu', distroseries='hoary')
 
-  >>> bar_src = NascentUpload(
+  >>> bar_src = NascentUpload.from_changesfile_path(
   ...     datadir('suite/bar_1.0-1/bar_1.0-1_source.changes'),
   ...     sync_policy, mock_logger_quiet)
   >>> bar_src.process()
@@ -975,7 +975,7 @@
   >>> hoary.status = SeriesStatus.DEVELOPMENT
   >>> anything_policy = getPolicy(
   ...     name='anything', distro='ubuntu', distroseries='hoary')
-  >>> bar_upload = NascentUpload(
+  >>> bar_upload = NascentUpload.from_changesfile_path(
   ...     datadir(
   ...         'suite/bar_1.0-10_utf8_changesfile/bar_1.0-10_source.changes'),
   ...     anything_policy, mock_logger_quiet)
@@ -1106,7 +1106,8 @@
 
 And then try to upload using the changes file with the malformed name.
 
-  >>> bar_src = NascentUpload(copyp, sync_policy, mock_logger_quiet)
+  >>> bar_src = NascentUpload.from_changesfile_path(
+  ...     copyp, sync_policy, mock_logger_quiet)
   >>> bar_src.process()
   Traceback (most recent call last):
   ...

=== modified file 'lib/lp/archiveuploader/tests/nascentupload-packageset.txt'
--- lib/lp/archiveuploader/tests/nascentupload-packageset.txt	2010-05-11 14:09:44 +0000
+++ lib/lp/archiveuploader/tests/nascentupload-packageset.txt	2010-08-04 22:12:18 +0000
@@ -31,7 +31,7 @@
 This time the upload will fail because the ACLs don't let
 "name16", the key owner, upload a package.
 
-    >>> bar_failed = NascentUpload(
+    >>> bar_failed = NascentUpload.from_changesfile_path(
     ...     datadir('suite/bar_1.0-1/bar_1.0-1_source.changes'),
     ...     insecure_policy, mock_logger_quiet)
 
@@ -111,7 +111,7 @@
 
 Let's retry the upload.
 
-    >>> bar_failed = NascentUpload(
+    >>> bar_failed = NascentUpload.from_changesfile_path(
     ...     datadir('suite/bar_1.0-1/bar_1.0-1_source.changes'),
     ...     insecure_policy, mock_logger_quiet)
 
@@ -166,7 +166,7 @@
 
     >>> commit()
     >>> LaunchpadZopelessLayer.switchDbUser('uploader')
-    >>> bar2 = NascentUpload(
+    >>> bar2 = NascentUpload.from_changesfile_path(
     ...     datadir('suite/bar_1.0-1/bar_1.0-1_source.changes'),
     ...     insecure_policy, mock_logger_quiet)
     >>> bar2.process()

=== modified file 'lib/lp/archiveuploader/tests/nascentupload-security-uploads.txt'
--- lib/lp/archiveuploader/tests/nascentupload-security-uploads.txt	2010-07-24 09:12:37 +0000
+++ lib/lp/archiveuploader/tests/nascentupload-security-uploads.txt	2010-08-04 22:12:18 +0000
@@ -55,7 +55,7 @@
 The upload in question contains a source and its 2 builds for i386 and
 powerpc:
 
-  >>> foo_mixed_upload = NascentUpload(
+  >>> foo_mixed_upload = NascentUpload.from_changesfile_path(
   ...     datadir('suite/foo_1.0-1_multi_binary/foo_1.0-1_multi.changes'),
   ...     security_policy, mock_logger_quiet)
   >>> foo_mixed_upload.process()
@@ -121,7 +121,7 @@
 security upload, for example, detecting that the source and the
 binaries sent do not match.
 
-  >>> bar_mixed_upload = NascentUpload(
+  >>> bar_mixed_upload = NascentUpload.from_changesfile_path(
   ...     datadir('suite/foo_1.0-1_broken_binary/bar_1.0-1_multi.changes'),
   ...     security_policy, mock_logger_quiet)
   >>> bar_mixed_upload.process()

=== modified file 'lib/lp/archiveuploader/tests/nascentupload.txt'
--- lib/lp/archiveuploader/tests/nascentupload.txt	2010-07-24 09:12:37 +0000
+++ lib/lp/archiveuploader/tests/nascentupload.txt	2010-08-04 22:12:18 +0000
@@ -45,14 +45,15 @@
 doc/nascentuploadfile.txt) object based on that. If anything goes
 wrong during this process FatalUploadError is raised:
 
-  >>> NascentUpload(datadir("DOES-NOT-EXIST"), buildd_policy, mock_logger)
+  >>> NascentUpload.from_changesfile_path(
+  ...     datadir("DOES-NOT-EXIST"), buildd_policy, mock_logger)
   Traceback (most recent call last):
   ...
   FatalUploadError:...
 
 Otherwise a ChangesFile object is ready to use.
 
-  >>> quodlibet = NascentUpload(
+  >>> quodlibet = NascentUpload.from_changesfile_path(
   ...     datadir("quodlibet_0.13.1-1_i386.changes"),
   ...     anything_policy, mock_logger_quiet)
 
@@ -110,7 +111,7 @@
 is 'native' (only a TARBALL, no diff + orig) or 'has_orig' (uses ORIG
 + DIFF source form).
 
-  >>> ed_source_upload = NascentUpload(
+  >>> ed_source_upload = NascentUpload.from_changesfile_path(
   ...     datadir("ed_0.2-20_i386.changes.source-only-unsigned"),
   ...     sync_policy, mock_logger_quiet)
 
@@ -167,7 +168,7 @@
 
 Let's try a simple binary upload:
 
-  >>> ed_binary_upload = NascentUpload(
+  >>> ed_binary_upload = NascentUpload.from_changesfile_path(
   ...     datadir("ed_0.2-20_i386.changes.binary-only-unsigned"),
   ...     buildd_policy, mock_logger_quiet)
 
@@ -220,7 +221,7 @@
   >>> modified_buildd_policy.can_upload_source = True
   >>> modified_buildd_policy.can_upload_mixed = True
 
-  >>> ed_mismatched_upload = NascentUpload(
+  >>> ed_mismatched_upload = NascentUpload.from_changesfile_path(
   ...     datadir("ed_0.2-20_i386.changes.mismatched-arch-unsigned"),
   ...     modified_buildd_policy, mock_logger_quiet)
 
@@ -258,7 +259,7 @@
   >>> modified_insecure_policy.can_upload_binaries = True
   >>> modified_insecure_policy.can_upload_mixed = True
 
-  >>> ed_mixed_upload = NascentUpload(
+  >>> ed_mixed_upload = NascentUpload.from_changesfile_path(
   ...     datadir("ed_0.2-20_i386.changes"),
   ...     modified_insecure_policy, mock_logger_quiet)
 
@@ -317,7 +318,7 @@
 
 First up, construct an upload of just the ed source...
 
-  >>> ed_src = NascentUpload(
+  >>> ed_src = NascentUpload.from_changesfile_path(
   ...     datadir('split-upload-test/ed_0.2-20_source.changes'),
   ...     sync_policy, mock_logger_quiet)
   >>> ed_src.process()
@@ -407,7 +408,7 @@
 maintainer's side), however it cannot store a proper
 SourcePackageRelease.copyright content. See bug #134567.
 
-  >>> nocopyright_src = NascentUpload(
+  >>> nocopyright_src = NascentUpload.from_changesfile_path(
   ...     datadir('suite/nocopyright_1.0-1/nocopyright_1.0-1_source.changes'),
   ...     sync_policy, mock_logger_quiet)
   >>> nocopyright_src.process()
@@ -451,7 +452,7 @@
 published in the archive, which provides the same source package name
 and source package version for the distroseries in question.
 
-  >>> ed_src_dup = NascentUpload(
+  >>> ed_src_dup = NascentUpload.from_changesfile_path(
   ...     datadir('split-upload-test/ed_0.2-20_source.changes'),
   ...     sync_policy, mock_logger_quiet)
   >>> ed_src_dup.process()
@@ -519,7 +520,7 @@
   ...     name='sync', distro='ubuntu', distroseries='hoary')
   >>> modified_sync_policy.can_upload_binaries = True
 
-  >>> ed_bin = NascentUpload(
+  >>> ed_bin = NascentUpload.from_changesfile_path(
   ...      datadir('split-upload-test/ed_0.2-20_i386.changes'),
   ...      modified_sync_policy, mock_logger_quiet)
 
@@ -606,7 +607,7 @@
 
 Upload new source 'multibar', step 1:
 
-  >>> multibar_src_upload = NascentUpload(
+  >>> multibar_src_upload = NascentUpload.from_changesfile_path(
   ...     datadir('suite/multibar_1.0-1/multibar_1.0-1_source.changes'),
   ...     sync_policy, mock_logger_quiet)
   >>> multibar_src_upload.process()
@@ -658,7 +659,7 @@
   ...     name='buildd', distro='ubuntu', distroseries='hoary',
   ...     buildid=multibar_build.id)
 
-  >>> multibar_bin_upload = NascentUpload(
+  >>> multibar_bin_upload = NascentUpload.from_changesfile_path(
   ...     datadir('suite/multibar_1.0-1/multibar_1.0-1_i386.changes'),
   ...     buildd_policy, mock_logger_quiet)
   >>> multibar_bin_upload.process()
@@ -727,7 +728,7 @@
   >>> norelease_sync_policy = getPolicy(
   ...     name='sync', distro='ubuntu')
 
-  >>> ed_src = NascentUpload(
+  >>> ed_src = NascentUpload.from_changesfile_path(
   ...     datadir('updates-upload-test/ed_0.2-20_source.changes'),
   ...     norelease_sync_policy, mock_logger_quiet)
   >>> ed_src.process()
@@ -756,7 +757,7 @@
 Check the uploader behaviour against a missing orig.tar.gz file,
       bug # 30741.
 
-  >>> ed21_src = NascentUpload(
+  >>> ed21_src = NascentUpload.from_changesfile_path(
   ...     datadir('ed-0.2-21/ed_0.2-21_source.changes'),
   ...     sync_policy, mock_logger_quiet)
   >>> ed21_src.process()
@@ -774,7 +775,7 @@
 'Standards-Version' field in DSC. See bug #75874 for further
 information.
 
-  >>> inst_src = NascentUpload(
+  >>> inst_src = NascentUpload.from_changesfile_path(
   ...     datadir('test75874_0.1_source.changes'),
   ...     sync_policy, mock_logger_quiet)
   >>> inst_src.process()
@@ -822,7 +823,7 @@
 When using 'insecure' policy, NascentUpload instace stores the DSC
 sigining key reference as an IGPGKey:
 
-  >>> bar_ok = NascentUpload(
+  >>> bar_ok = NascentUpload.from_changesfile_path(
   ...     datadir('suite/bar_1.0-1/bar_1.0-1_source.changes'),
   ...     insecure_policy, mock_logger_quiet)
   >>> bar_ok.process()
@@ -871,7 +872,7 @@
 This time the upload will fail because the ACLs don't let
 "name16", the key owner, upload a package.
 
-  >>> bar_failed = NascentUpload(
+  >>> bar_failed = NascentUpload.from_changesfile_path(
   ...     datadir('suite/bar_1.0-1/bar_1.0-1_source.changes'),
   ...     insecure_policy, mock_logger_quiet)
 
@@ -917,7 +918,7 @@
 
 Now try the "bar" upload:
 
-    >>> bar2 = NascentUpload(
+    >>> bar2 = NascentUpload.from_changesfile_path(
     ...     datadir('suite/bar_1.0-1/bar_1.0-1_source.changes'),
     ...     insecure_policy, mock_logger_quiet)
     >>> bar2.process()
@@ -943,7 +944,7 @@
 Make this upload policy pertain to the copy archive.
 
     >>> copy_archive_policy.archive = copy_archive
-    >>> quodlibet = NascentUpload(
+    >>> quodlibet = NascentUpload.from_changesfile_path(
     ...     datadir("quodlibet_0.13.1-1_i386.changes"),
     ...     copy_archive_policy, mock_logger_quiet)
 

=== added file 'lib/lp/archiveuploader/tests/test_nascentupload.py'
--- lib/lp/archiveuploader/tests/test_nascentupload.py	1970-01-01 00:00:00 +0000
+++ lib/lp/archiveuploader/tests/test_nascentupload.py	2010-08-04 22:12:18 +0000
@@ -0,0 +1,85 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test NascentUpload functionality."""
+
+__metaclass__ = type
+
+from testtools import TestCase
+
+from canonical.testing import LaunchpadZopelessLayer
+from lp.archiveuploader.changesfile import determine_file_class_and_name
+from lp.archiveuploader.nascentupload import NascentUpload
+from lp.archiveuploader.tests import mock_logger
+
+
+class FakeChangesFile:
+
+    def __init__(self):
+        self.files = []
+
+
+class TestMatchDDEBs(TestCase):
+    """Tests that NascentUpload correctly links DEBs to their DDEBs.
+
+    Also verifies detection of DDEB-related error cases.
+    """
+
+    layer = LaunchpadZopelessLayer
+
+    def setUp(self):
+        super(TestMatchDDEBs, self).setUp()
+        self.changes = FakeChangesFile()
+        self.upload = NascentUpload(self.changes, None, mock_logger)
+
+    def addFile(self, filename):
+        """Add a file of the right type to the upload."""
+        package, cls = determine_file_class_and_name(filename)
+        file = cls(
+            filename, None, 100, 'devel', 'extra', package, '666',
+            self.changes, None, self.upload.logger)
+        self.changes.files.append(file)
+
+    def assertMatchDDEBErrors(self, error_list):
+        self.assertEquals(
+            error_list, [str(e) for e in self.upload._matchDDEBs()])
+
+    def testNoLinksWithNoBinaries(self):
+        # No links will be made if there are no binaries whatsoever.
+        self.addFile('something_1.0.diff.gz')
+        self.assertMatchDDEBErrors([])
+
+    def testNoLinksWithJustDEBs(self):
+        # No links will be made if there are no DDEBs.
+        self.addFile('blah_1.0_all.deb')
+        self.addFile('libblah_1.0_i386.deb')
+        self.assertMatchDDEBErrors([])
+        for file in self.changes.files:
+            self.assertIs(None, file.ddeb_file)
+
+    def testLinksMatchingDDEBs(self):
+        # DDEBs will be linked to their matching DEBs.
+        self.addFile('blah_1.0_all.deb')
+        self.addFile('libblah_1.0_i386.deb')
+        self.addFile('libblah-dbgsym_1.0_i386.ddeb')
+        self.assertMatchDDEBErrors([])
+        self.assertIs(None, self.changes.files[0].ddeb_file)
+        self.assertIs(self.changes.files[2], self.changes.files[1].ddeb_file)
+        self.assertIs(self.changes.files[1], self.changes.files[2].deb_file)
+        self.assertIs(None, self.changes.files[2].ddeb_file)
+
+    def testDuplicateDDEBsCauseErrors(self):
+        # An error will be raised if a DEB has more than one matching
+        # DDEB.
+        self.addFile('libblah_1.0_i386.deb')
+        self.addFile('libblah-dbgsym_1.0_i386.ddeb')
+        self.addFile('libblah-dbgsym_1.0_i386.ddeb')
+        self.assertMatchDDEBErrors(
+            ['Duplicated debug packages: libblah-dbgsym 666 (i386)'])
+
+    def testMismatchedDDEBsCauseErrors(self):
+        # An error will be raised if a DDEB has no matching DEB.
+        self.addFile('libblah_1.0_i386.deb')
+        self.addFile('libblah-dbgsym_1.0_amd64.ddeb')
+        self.assertMatchDDEBErrors(
+            ['Orphaned debug packages: libblah-dbgsym 666 (amd64)'])

=== modified file 'lib/lp/archiveuploader/tests/test_nascentupload_documentation.py'
--- lib/lp/archiveuploader/tests/test_nascentupload_documentation.py	2009-06-24 23:33:29 +0000
+++ lib/lp/archiveuploader/tests/test_nascentupload_documentation.py	2010-08-04 22:12:18 +0000
@@ -26,20 +26,25 @@
 def getUploadForSource(upload_path):
     """Return a NascentUpload object for a source."""
     policy = getPolicy(name='sync', distro='ubuntu', distroseries='hoary')
-    return NascentUpload(datadir(upload_path), policy, mock_logger_quiet)
+    return NascentUpload.from_changesfile_path(
+        datadir(upload_path), policy, mock_logger_quiet)
+
 
 def getPPAUploadForSource(upload_path, ppa):
     """Return a NascentUpload object for a PPA source."""
     policy = getPolicy(name='insecure', distro='ubuntu', distroseries='hoary')
     policy.archive = ppa
-    return NascentUpload(datadir(upload_path), policy, mock_logger_quiet)
+    return NascentUpload.from_changesfile_path(
+        datadir(upload_path), policy, mock_logger_quiet)
+
 
 def getUploadForBinary(upload_path):
     """Return a NascentUpload object for binaries."""
     policy = getPolicy(name='sync', distro='ubuntu', distroseries='hoary')
     policy.can_upload_binaries = True
     policy.can_upload_mixed = True
-    return NascentUpload(datadir(upload_path), policy, mock_logger_quiet)
+    return NascentUpload.from_changesfile_path(
+        datadir(upload_path), policy, mock_logger_quiet)
 
 
 def testGlobalsSetup(test):

=== modified file 'lib/lp/archiveuploader/uploadprocessor.py'
--- lib/lp/archiveuploader/uploadprocessor.py	2010-08-02 09:40:22 +0000
+++ lib/lp/archiveuploader/uploadprocessor.py	2010-08-04 22:12:18 +0000
@@ -335,7 +335,8 @@
         # The path we want for NascentUpload is the path to the folder
         # containing the changes file (and the other files referenced by it).
         changesfile_path = os.path.join(upload_path, changes_file)
-        upload = NascentUpload(changesfile_path, policy, self.log)
+        upload = NascentUpload.from_changesfile_path(
+            changesfile_path, policy, self.log)
 
         # Reject source upload to buildd upload paths.
         first_path = relative_path.split(os.path.sep)[0]

=== modified file 'lib/lp/soyuz/browser/tests/distroseriesqueue-views.txt'
--- lib/lp/soyuz/browser/tests/distroseriesqueue-views.txt	2009-06-12 16:36:02 +0000
+++ lib/lp/soyuz/browser/tests/distroseriesqueue-views.txt	2010-08-04 22:12:18 +0000
@@ -260,7 +260,7 @@
     ...    distroseries='hoary')
     >>> sync_policy.can_upload_binaries = True
     >>> sync_policy.can_upload_mixed = True
-    >>> foo_upload = NascentUpload(
+    >>> foo_upload = NascentUpload.from_changesfile_path(
     ...    datadir('suite/foo_1.0-2_multi_binary/foo_1.0-2_i386.changes'),
     ...    sync_policy, mock_logger_quiet)
     >>> foo_upload.process()

=== modified file 'lib/lp/soyuz/doc/build-failedtoupload-workflow.txt'
--- lib/lp/soyuz/doc/build-failedtoupload-workflow.txt	2010-05-12 08:17:20 +0000
+++ lib/lp/soyuz/doc/build-failedtoupload-workflow.txt	2010-08-04 22:12:18 +0000
@@ -165,7 +165,7 @@
   ...     distroseries=failedtoupload_candidate.distro_series.name,
   ...     buildid=failedtoupload_candidate.id)
 
-  >>> cdrkit_bin_upload = NascentUpload(
+  >>> cdrkit_bin_upload = NascentUpload.from_changesfile_path(
   ...     datadir('suite/cdrkit_1.0/cdrkit_1.0_i386.changes'),
   ...     buildd_policy, mock_logger_quiet)
   >>> cdrkit_bin_upload.process()

=== modified file 'lib/lp/soyuz/doc/distroseriesqueue-ddtp-tarball.txt'
--- lib/lp/soyuz/doc/distroseriesqueue-ddtp-tarball.txt	2009-05-13 14:05:27 +0000
+++ lib/lp/soyuz/doc/distroseriesqueue-ddtp-tarball.txt	2010-08-04 22:12:18 +0000
@@ -47,7 +47,7 @@
   >>> sync_policy = getPolicy(
   ...     name='sync', distro='ubuntutest', distroseries=None)
 
-  >>> upload = NascentUpload(
+  >>> upload = NascentUpload.from_changesfile_path(
   ...     datadir('ddtp-tarball/translations-main_20060728.changes'),
   ...     sync_policy, mock_logger_quiet)
   >>> upload.process()
@@ -60,7 +60,7 @@
   >>> insecure_policy = getPolicy(
   ...     name='insecure', distro='ubuntutest', distroseries=None)
 
-  >>> upload = NascentUpload(
+  >>> upload = NascentUpload.from_changesfile_path(
   ...     datadir('ddtp-tarball/translations-main_20060728_all.changes'),
   ...     insecure_policy, mock_logger)
   DEBUG: Verifying signature on translations-main_20060728_all.changes
@@ -228,7 +228,7 @@
   >>> insecure_policy = getPolicy(
   ...     name='insecure', distro='ubuntutest', distroseries=None)
 
-  >>> upload = NascentUpload(
+  >>> upload = NascentUpload.from_changesfile_path(
   ...     datadir('ddtp-tarball/translations-main_20060817_all.changes'),
   ...     insecure_policy, mock_logger_quiet)
   >>> upload.process()

=== modified file 'lib/lp/soyuz/doc/distroseriesqueue-debian-installer.txt'
--- lib/lp/soyuz/doc/distroseriesqueue-debian-installer.txt	2009-05-13 14:05:27 +0000
+++ lib/lp/soyuz/doc/distroseriesqueue-debian-installer.txt	2010-08-04 22:12:18 +0000
@@ -57,7 +57,7 @@
 
   >>> anything_policy.distroseries.changeslist = '[e-mail]'
 
-  >>> upload = NascentUpload(
+  >>> upload = NascentUpload.from_changesfile_path(
   ...     datadir(
   ...       'debian-installer/debian-installer_20070214ubuntu1_i386.changes'),
   ...     anything_policy, mock_logger_quiet)

=== modified file 'lib/lp/soyuz/doc/distroseriesqueue-dist-upgrader.txt'
--- lib/lp/soyuz/doc/distroseriesqueue-dist-upgrader.txt	2010-08-03 22:03:56 +0000
+++ lib/lp/soyuz/doc/distroseriesqueue-dist-upgrader.txt	2010-08-04 22:12:18 +0000
@@ -23,7 +23,7 @@
   >>> sync_policy = getPolicy(
   ...     name='sync', distro='ubuntutest', distroseries=None)
 
-  >>> upload = NascentUpload(
+  >>> upload = NascentUpload.from_changesfile_path(
   ...     datadir('dist-upgrader/dist-upgrader_20060302.0120.changes'),
   ...     sync_policy, mock_logger)
   DEBUG: Changes file can be unsigned.
@@ -37,7 +37,7 @@
   >>> insecure_policy = getPolicy(
   ...     name='insecure', distro='ubuntutest', distroseries=None)
 
-  >>> upload = NascentUpload(
+  >>> upload = NascentUpload.from_changesfile_path(
   ...     datadir('dist-upgrader/dist-upgrader_20060302.0120_all.changes'),
   ...     insecure_policy, mock_logger)
   DEBUG: Verifying signature on dist-upgrader_20060302.0120_all.changes
@@ -214,7 +214,7 @@
   >>> insecure_policy = getPolicy(
   ...     name='insecure', distro='ubuntutest', distroseries=None)
 
-  >>> upload = NascentUpload(
+  >>> upload = NascentUpload.from_changesfile_path(
   ...     datadir('dist-upgrader/dist-upgrader_20070219.1234_all.changes'),
   ...     insecure_policy, mock_logger)
   DEBUG: Verifying signature on dist-upgrader_20070219.1234_all.changes
@@ -287,7 +287,7 @@
 
   >>> insecure_policy.archive = foobar_archive
 
-  >>> ppa_upload = NascentUpload(
+  >>> ppa_upload = NascentUpload.from_changesfile_path(
   ...     datadir('dist-upgrader/dist-upgrader_20060302.0120_all.changes'),
   ...     insecure_policy, mock_logger)
   DEBUG: Verifying signature on dist-upgrader_20060302.0120_all.changes

=== modified file 'lib/lp/soyuz/doc/distroseriesqueue-translations.txt'
--- lib/lp/soyuz/doc/distroseriesqueue-translations.txt	2010-05-14 04:47:38 +0000
+++ lib/lp/soyuz/doc/distroseriesqueue-translations.txt	2010-08-04 22:12:18 +0000
@@ -77,7 +77,7 @@
   ...     name='buildd', distro='ubuntu', distroseries='dapper',
   ...     buildid=build.id)
 
-  >>> pmount_upload = NascentUpload(
+  >>> pmount_upload = NascentUpload.from_changesfile_path(
   ...     datadir('pmount_0.9.7-2ubuntu2_amd64.changes'),
   ...     buildd_policy, mock_logger)
   DEBUG: Changes file can be unsigned.

=== modified file 'lib/lp/soyuz/doc/distroseriesqueue.txt'
--- lib/lp/soyuz/doc/distroseriesqueue.txt	2010-05-14 04:51:42 +0000
+++ lib/lp/soyuz/doc/distroseriesqueue.txt	2010-08-04 22:12:18 +0000
@@ -69,7 +69,7 @@
   >>> anything_policy.can_upload_binaries = True
   >>> anything_policy.can_upload_mixed = True
 
-  >>> ed_upload = NascentUpload(
+  >>> ed_upload = NascentUpload.from_changesfile_path(
   ...     datadir('ed_0.2-20_i386.changes'),
   ...     anything_policy, mock_logger_quiet)
 
@@ -263,7 +263,7 @@
   >>> insecure_policy = getPolicy(
   ...     name='insecure', distro='ubuntu', distroseries='hoary')
 
-  >>> bar_ok = NascentUpload(
+  >>> bar_ok = NascentUpload.from_changesfile_path(
   ...     datadir('suite/bar_1.0-1/bar_1.0-1_source.changes'),
   ...     insecure_policy, mock_logger_quiet)
   >>> bar_ok.process()

=== modified file 'lib/lp/soyuz/scripts/tests/test_queue.py'
--- lib/lp/soyuz/scripts/tests/test_queue.py	2010-02-09 00:17:40 +0000
+++ lib/lp/soyuz/scripts/tests/test_queue.py	2010-08-04 22:12:18 +0000
@@ -123,7 +123,7 @@
         LaunchpadZopelessLayer.switchDbUser("uploader")
         sync_policy = getPolicy(
             name='sync', distro='ubuntu', distroseries='breezy-autotest')
-        bar_src = NascentUpload(
+        bar_src = NascentUpload.from_changesfile_path(
             datadir(changesfile),
             sync_policy, mock_logger_quiet)
         bar_src.process()