This is a change that I've been craving. So much kudos for implementing it. I heartily approve of most of this branch. Of course as a reviewer, I have a reputation to uphold and thus: === modified file 'lib/lp/archiveuploader/nascentupload.py' --- lib/lp/archiveuploader/nascentupload.py 2011-08-23 14:35:43 +0000 +++ lib/lp/archiveuploader/nascentupload.py 2011-09-29 05:22:06 +0000 @@ -114,23 +110,14 @@ 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 + May raise UploadError due to unrecoverable problems building the ChangesFile object. If we're crossing the boundary of "fatal" here, consider replacing or removing the word "unrecoverable" as well. It's not so much the error that is unrecoverable, as an extent of code. But what extent exactly? Like "Fatal," "unrecoverable" sort of suggests that that extent is the entire script run. === modified file 'lib/lp/archiveuploader/tests/test_uploadprocessor.py' --- lib/lp/archiveuploader/tests/test_uploadprocessor.py 2011-08-26 06:14:54 +0000 +++ lib/lp/archiveuploader/tests/test_uploadprocessor.py 2011-09-29 05:22:06 +0000 @@ -1361,28 +1363,27 @@ self.options.builds = False processor = self.getUploadProcessor(self.layer.txn) - upload_dir = self.queueUpload("foocomm_1.0-1_proposed") - bogus_changesfile_data = ''' - Ubuntu is a community developed, Linux-based operating system that is - perfect for laptops, desktops and servers. It contains all the - applications you need - a web browser, presentation, document and - spreadsheet software, instant messaging and much more. - ''' - file_handle = open( - '%s/%s' % (upload_dir, 'bogus.changes'), 'w') - file_handle.write(bogus_changesfile_data) - file_handle.close() + self.queueUpload("foocomm_1.0-1_proposed") + + # Any code that causes an OOPS is a bug that must be fixed, so + # let's monkeypatch something in that we know will OOPS. I find this explanation unnecessarily confusing. To a hurried reader (and we all are from time to time) it seems to say "let's create bugs that must be fixed," which of course is fine from a job-security standpoint but... What I think you mean is: "This test verifies that oopses get handled properly, which is very important because oopses must be caught and fixed. The following simulates an error that will be treated as an oops. We verify that the oops gets handled properly later." If so, the first of those sentences probably belongs at the top of the test (if roughly the same thing isn't being said there already), and the last belongs at the point of verification. For some reason the term "let's" seems to be a very reliable indicator of this kind of muddled expression in tests, so treat it as inherently suspicious. + class SomeException(Exception): + pass + + def from_changesfile_path(cls, changesfile_path, policy, logger): + raise SomeException("I am an explanation.") + self.useFixture( + MonkeyPatch( + 'lp.archiveuploader.nascentupload.NascentUpload.' + 'from_changesfile_path', + classmethod(from_changesfile_path))) Have you tried monkey-patching a FakeMethod instead? Instead of defining a function with a name that is immaterial yet only makes sense in the context you're patching (rather than the one you define it in), you could make it a simple anonymous object: self.useFixture( MonkeyPatch( 'lp.archiveuploader.nascentupload.NascentUpload.' 'from_changes_file_path', FakeMethod(failure=SomeException("I am an explanation.")))) (I don't think you need the classmethod() wrapper either if you do it this way). @@ -1919,6 +1920,21 @@ self.PGPSignatureNotPreserved(archive=self.breezy.main_archive) self.switchToUploader() + def test_unsigned_upload_is_silently_rejected(self): + """Unsigned uploads are silently rejected without an OOPS.""" What exactly does "silently" mean here? I can see how it makes sense in the test name, which you need to keep brief. But you just repeat it in the docstring, and I can't tell whether "without an OOPS" is a separate piece of information or as a more specific re-phrasing of "silently." Can you think of a more precise and non-redundant way to say "silently" in the docstring? It's not _entirely_ silently obviously, since you log the error and the test even checks for that non-silence. Does "silently" refer to the absence of notification to the user? Or to the fact that processUpload returns normally? Something else again? All of the above? === modified file 'lib/lp/archiveuploader/uploadprocessor.py' --- lib/lp/archiveuploader/uploadprocessor.py 2011-05-04 06:37:50 +0000 +++ lib/lp/archiveuploader/uploadprocessor.py 2011-09-29 05:22:06 +0000 @@ -370,8 +370,20 @@ # 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(self.upload_path, changes_file) - upload = NascentUpload.from_changesfile_path( - changesfile_path, policy, self.processor.log) + try: + upload = NascentUpload.from_changesfile_path( + changesfile_path, policy, self.processor.log) + except UploadError, e: Future python tip: the more modern syntax for catching named exceptions is... except UploadError as e: (A small step towards python 3, and aesthetically one of the nicer changes IMHO). + # We failed to parse the changes file, so we have no key or + # Changed-By to notify of the rejection. Just log it and + # move on. + # XXX wgrant 2011-09-29 bug=499438: With some refactoring we + # could do better here: if we have a signature then we have + # somebody to email, even if the rest of the file is + # corrupt. + logger.debug("Failed to parse changes file: %s" % str(e)) + logger.debug("Nobody to notify.") + return UploadStatusEnum.REJECTED Well done, well explained. Just one question here: wouldn't logger.info make more sense than logger.debug? After all this situation is still a bit of a problem. In particular, I'm assuming that sometimes we'll want to search the logs for this message to find out what happened to an upload. If so, I imagine we would want this logged by default. Jeroen