Merge lp:~cjwatson/launchpad/more-pop-notifications into lp:launchpad

[Colin Watson]

Use pop_notifications in more places to avoid fragile patterns with stub.test_emails, and introduce an assertEmailQueueLength helper.

This is kind of yak-shaving. I've been making some progress on https://bugs.launchpad.net/launchpad/+bug/29744, but in order to turn off immediate mail delivery I need to make sure that doing so isn't going to hide test problems. A number of tests do something like "code_under_test(); self.assertEqual([], stub.test_emails);", and not all of them make sure to commit beforehand. With immediate mail delivery disabled, this could hide bugs because test_emails could be empty until the transaction is committed. The pop_notifications interface is in most situations better (it commits the transaction first by default, and it saves you having to clear test_emails yourself).

I've gone through the cases where test_emails was used directly and converted most of them to pop_notifications or something based on it. This branch contains only changes to files that contained unsafe uses of test_emails; the rest can come later.

Checking the length of the mail queue is a common pattern in tests, so I introduced an assertEmailQueueLength helper for this.

[William Grant (wgrant)]

I'd consider making assertEmailQueueLength include the actual set of emails in the mismatch, but it's no worse than it was before.

review: Approve (code)

=== modified file 'lib/lp/archivepublisher/tests/test_generate_ppa_htaccess.py'
--- lib/lp/archivepublisher/tests/test_generate_ppa_htaccess.py	2014-10-29 06:04:09 +0000
+++ lib/lp/archivepublisher/tests/test_generate_ppa_htaccess.py	2015-09-10 16:19:01 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test the generate_ppa_htaccess.py script. """
@@ -33,7 +33,6 @@
 from lp.registry.interfaces.teammembership import TeamMembershipStatus
 from lp.services.config import config
 from lp.services.log.logger import BufferLogger
-from lp.services.mail import stub
 from lp.services.osutils import (
     ensure_directory_exists,
     remove_if_exists,
@@ -281,9 +280,7 @@
             self.assertNotDeactivated(tokens[person])
 
         # Ensure that a cancellation email was sent.
-        num_emails = len(stub.test_emails)
-        self.assertEqual(
-            num_emails, 1, "Expected 1 email, got %s" % num_emails)
+        self.assertEmailQueueLength(1)
 
         # Promiscuous_person now leaves team1, but does not lose his
         # token because he's also in team2. No other tokens are
@@ -298,9 +295,7 @@
             self.assertNotDeactivated(tokens[person])
 
         # Ensure that a cancellation email was not sent.
-        num_emails = len(stub.test_emails)
-        self.assertEqual(
-            num_emails, 0, "Expected no emails, got %s" % num_emails)
+        self.assertEmailQueueLength(0)
 
         # Team 2 now leaves parent_team, and all its members lose their
         # tokens.
@@ -486,10 +481,6 @@
 
         script.sendCancellationEmail(tokens[0])
 
-        num_emails = len(stub.test_emails)
-        self.assertEqual(
-            num_emails, 1, "Expected 1 email, got %s" % num_emails)
-
         [email] = pop_notifications()
         self.assertEqual(
             email['Subject'],
@@ -537,9 +528,7 @@
 
         script.sendCancellationEmail(token)
 
-        num_emails = len(stub.test_emails)
-        self.assertEqual(
-            num_emails, 0, "Expected 0 emails, got %s" % num_emails)
+        self.assertEmailQueueLength(0)
 
     def test_getTimeToSyncFrom(self):
         # Sync from 1s before previous start to catch anything made during the

=== modified file 'lib/lp/archiveuploader/tests/test_ppauploadprocessor.py'
--- lib/lp/archiveuploader/tests/test_ppauploadprocessor.py	2015-08-26 13:41:21 +0000
+++ lib/lp/archiveuploader/tests/test_ppauploadprocessor.py	2015-09-10 16:19:01 +0000
@@ -9,7 +9,7 @@
 
 __metaclass__ = type
 
-from email import message_from_string
+from operator import itemgetter
 import os
 import shutil
 
@@ -27,7 +27,6 @@
 from lp.services.config import config
 from lp.services.database.constants import UTC_NOW
 from lp.services.librarian.interfaces import ILibraryFileAliasSet
-from lp.services.mail import stub
 from lp.soyuz.enums import (
     PackagePublishingStatus,
     PackageUploadStatus,
@@ -43,6 +42,7 @@
 from lp.soyuz.tests.fakepackager import FakePackager
 from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
 from lp.testing.dbuser import switch_dbuser
+from lp.testing.mail_helpers import pop_notifications
 
 
 class TestPPAUploadProcessorBase(TestUploadProcessorBase):
@@ -87,20 +87,19 @@
             "X-Launchpad-PPA" header; it defaults to "name16" and should be
             explicitly set to None for non-PPA or rejection notifications.
         """
-        for item in expected:
+        notifications = self.assertEmailQueueLength(
+            len(expected), sort_key=itemgetter('X-Envelope-To'))
+
+        for item, msg in zip(expected, notifications):
             recipient = item.get("recipient", self.name16_recipient)
             contents = item.get("contents", [])
             ppa_header = item.get("ppa_header", "name16")
 
-            from_addr, to_addrs, raw_msg = stub.test_emails.pop()
-            msg = message_from_string(raw_msg)
-
             # This is now a non-multipart message.
             self.assertFalse(msg.is_multipart())
             body = msg.get_payload(decode=True)
 
-            clean_recipients = [r.strip() for r in to_addrs]
-            self.assertContentEqual([recipient], clean_recipients)
+            self.assertEqual(recipient, msg['X-Envelope-To'])
 
             subject = "Subject: %s\n" % msg['Subject']
             body = subject + body
@@ -112,10 +111,6 @@
                 self.assertIn('X-Launchpad-PPA', msg.keys())
                 self.assertEqual(msg['X-Launchpad-PPA'], ppa_header)
 
-        self.assertEqual(
-            [], stub.test_emails,
-            "%d emails left over" % len(stub.test_emails))
-
     def checkFilesRestrictedInLibrarian(self, queue_item, condition):
         """Check the libraryfilealias restricted flag.
 
@@ -300,8 +295,8 @@
         self.assertEqual(
             self.uploadprocessor.last_processed_upload.queue_root.status,
             PackageUploadStatus.DONE)
-        # Consume the test email so the assertion futher down does not fail.
-        _from_addr, _to_addrs, _raw_msg = stub.test_emails.pop()
+        # Consume the test email so the assertion further down does not fail.
+        pop_notifications()
 
         # The SourcePackageRelease still has a component of universe:
         pub_foo = self.name16.archive.getPublishedSources(name=u"bar").one()
@@ -320,8 +315,7 @@
             self.build_uploadprocessor, upload_dir, build=build)
 
         # No mails are sent for successful binary uploads.
-        self.assertEqual(len(stub.test_emails), 0,
-                         "Unexpected email generated on binary upload.")
+        self.assertEmailQueueLength(0)
 
         # Publish the binary.
         [queue_item] = self.breezy.getPackageUploads(
@@ -477,7 +471,7 @@
             name12.preferredemail.email)
         self.assertEmails([
             {"ppa_header": "cprov", "recipient": expected_recipient}
-            for expected_recipient in reversed(sorted(expected_recipients))])
+            for expected_recipient in sorted(expected_recipients)])
 
     def testPPADistroSeriesOverrides(self):
         """It's possible to override target distroseries of PPA uploads.
@@ -850,9 +844,7 @@
         # Please note: this upload goes to the Ubuntu main archive.
         upload_dir = self.queueUpload("bar_1.0-10")
         self.processUpload(self.uploadprocessor, upload_dir)
-        # Discard the announcement email and check the acceptance message
-        # content.
-        stub.test_emails.pop()
+        pop_notifications()
 
         self.assertEqual(
             self.uploadprocessor.last_processed_upload.queue_root.status,
@@ -991,9 +983,9 @@
             u'Files specified in DSC are broken or missing, skipping package '
             u'unpack verification.')
 
-        # Also, the email generated should be sane.
-        from_addr, to_addrs, raw_msg = stub.test_emails.pop()
-        msg = message_from_string(raw_msg)
+        # Also, the email generated should be sane.  Any of the multiple
+        # notifications will do.
+        msg = pop_notifications()[-1]
         body = msg.get_payload(decode=True)
 
         self.assertTrue(
@@ -1015,8 +1007,7 @@
         self.processUpload(self.uploadprocessor, upload_dir)
 
         # The email generated should be sane.
-        from_addr, to_addrs, raw_msg = stub.test_emails.pop()
-        msg = message_from_string(raw_msg)
+        [msg] = pop_notifications()
         body = msg.get_payload(decode=True)
 
         self.assertTrue(
@@ -1148,7 +1139,7 @@
         queue_item.setAccepted()
         queue_item.realiseUpload()
         transaction.commit()
-        stub.test_emails.pop()
+        pop_notifications()
 
         # Now upload a 3.0 (quilt) source with missing orig*.tar.* to a
         # PPA. All of the missing files will be retrieved from the

=== modified file 'lib/lp/archiveuploader/tests/test_uploadprocessor.py'
--- lib/lp/archiveuploader/tests/test_uploadprocessor.py	2015-08-25 14:05:24 +0000
+++ lib/lp/archiveuploader/tests/test_uploadprocessor.py	2015-09-10 16:19:01 +0000
@@ -9,7 +9,6 @@
     "TestUploadProcessorBase",
     ]
 
-from email import message_from_string
 import os
 import shutil
 from StringIO import StringIO
@@ -65,7 +64,6 @@
     BufferLogger,
     DevNullLogger,
     )
-from lp.services.mail import stub
 from lp.soyuz.enums import (
     ArchivePermissionType,
     ArchivePurpose,
@@ -357,12 +355,16 @@
             over after checking the ones in expected.
         """
 
-        for item in expected:
+        if allow_leftover:
+            notifications = pop_notifications()
+            self.assertTrue(len(notifications) >= len(expected))
+        else:
+            notifications = self.assertEmailQueueLength(len(expected))
+
+        for item, msg in zip(expected, notifications):
             recipient = item.get("recipient", self.name16_recipient)
             contents = item.get("contents", [])
 
-            from_addr, to_addrs, raw_msg = stub.test_emails.pop()
-            msg = message_from_string(raw_msg)
             # This is now a MIMEMultipart message.
             body = msg.get_payload(0)
             body = body.get_payload(decode=True)
@@ -370,8 +372,7 @@
             # Only check the recipient if the caller didn't explicitly pass
             # "recipient": None.
             if recipient is not None:
-                clean_recipients = [r.strip() for r in to_addrs]
-                self.assertContentEqual([recipient], clean_recipients)
+                self.assertEqual(recipient, msg['X-Envelope-To'])
 
             subject = "Subject: %s\n" % msg['Subject']
             body = subject + body
@@ -381,11 +382,6 @@
                     content in body,
                     "Expect: '%s'\nGot:\n%s" % (content, body))
 
-        if not allow_leftover:
-            self.assertEqual(
-                [], stub.test_emails,
-                "%d emails left over" % len(stub.test_emails))
-
     def PGPSignatureNotPreserved(self, archive=None):
         """PGP signatures should be removed from .changes files.
 
@@ -423,13 +419,11 @@
 
     def _checkPartnerUploadEmailSuccess(self):
         """Ensure partner uploads generate the right email."""
-        from_addr, to_addrs, raw_msg = stub.test_emails.pop()
-        foo_bar = "[e-mail]"
-        self.assertEqual([e.strip() for e in to_addrs], [foo_bar])
+        [msg] = pop_notifications()
+        self.assertEqual("[e-mail]", msg["X-Envelope-To"])
         self.assertTrue(
-            "rejected" not in raw_msg,
-            "Expected acceptance email not rejection. Actually Got:\n%s"
-                % raw_msg)
+            "rejected" not in str(msg),
+            "Expected acceptance email not rejection. Actually Got:\n%s" % msg)
 
     def testInstantiate(self):
         """UploadProcessor should instantiate"""
@@ -563,13 +557,13 @@
         self.processUpload(uploadprocessor, upload_dir)
 
         # Check the mailer stub has a rejection email for Daniel
-        from_addr, to_addrs, raw_msg = stub.test_emails.pop()
+        [msg] = pop_notifications()
         # This is now a MIMEMultipart message.
-        msg = message_from_string(raw_msg)
         body = msg.get_payload(0)
         body = body.get_payload(decode=True)
 
-        self.assertEqual(["[e-mail]"], to_addrs)
+        self.assertEqual(
+            "[e-mail]", msg["X-Envelope-To"])
         self.assertTrue("Unhandled exception processing upload: Exception "
                         "raised by BrokenUploadPolicy for testing."
                         in body)
@@ -599,15 +593,14 @@
         self.processUpload(uploadprocessor, upload_dir)
 
         # Check it went ok to the NEW queue and all is going well so far.
-        foo_bar = "[e-mail]"
         daniel = "[e-mail]"
-        for expected_to_addr in foo_bar, daniel:
-            from_addr, to_addrs, raw_msg = stub.test_emails.pop()
-            to_addrs = [e.strip() for e in to_addrs]
-            self.assertContentEqual(to_addrs, [expected_to_addr])
+        foo_bar = "[e-mail]"
+        notifications = self.assertEmailQueueLength(2)
+        for expected_to_addr, msg in zip((daniel, foo_bar), notifications):
+            self.assertEqual(expected_to_addr, msg["X-Envelope-To"])
             self.assertTrue(
-                "NEW" in raw_msg, "Expected email containing 'NEW', got:\n%s"
-                % raw_msg)
+                "NEW" in str(msg),
+                "Expected email containing 'NEW', got:\n%s" % msg)
 
         # Accept and publish the upload.
         # This is required so that the next upload of a later version of
@@ -635,13 +628,12 @@
         self.processUpload(uploadprocessor, upload_dir)
 
         # Verify we get an email talking about awaiting approval.
-        for expected_to_addr in foo_bar, daniel:
-            from_addr, to_addrs, raw_msg = stub.test_emails.pop()
-            to_addrs = [e.strip() for e in to_addrs]
-            self.assertContentEqual(to_addrs, [expected_to_addr])
-            self.assertTrue("Waiting for approval" in raw_msg,
-                            "Expected an 'upload awaits approval' email.\n"
-                            "Got:\n%s" % raw_msg)
+        notifications = self.assertEmailQueueLength(2)
+        for expected_to_addr, msg in zip((daniel, foo_bar), notifications):
+            self.assertEqual(expected_to_addr, msg["X-Envelope-To"])
+            self.assertTrue(
+                "Waiting for approval" in str(msg),
+                "Expected an 'upload awaits approval' email.\nGot:\n%s" % msg)
 
         # And verify that the queue item is in the unapproved state.
         queue_items = self.breezy.getPackageUploads(
@@ -918,10 +910,10 @@
         self.processUpload(uploadprocessor, upload_dir)
 
         # Check that it was rejected appropriately.
-        from_addr, to_addrs, raw_msg = stub.test_emails.pop()
-        self.assertTrue(
-            "Partner archive for distro '%s' not found" % self.ubuntu.name
-                in raw_msg)
+        [msg] = pop_notifications()
+        self.assertIn(
+            "Partner archive for distro '%s' not found" % self.ubuntu.name,
+            str(msg))
 
     def testMixedPartnerUploadFails(self):
         """Uploads with partner and non-partner files are rejected.
@@ -937,13 +929,12 @@
         self.processUpload(uploadprocessor, upload_dir)
 
         # Check that it was rejected.
-        from_addr, to_addrs, raw_msg = stub.test_emails.pop()
-        foo_bar = "[e-mail]"
-        self.assertEqual([e.strip() for e in to_addrs], [foo_bar])
+        [msg] = pop_notifications()
+        self.assertEqual("[e-mail]", msg["X-Envelope-To"])
         self.assertTrue(
-            "Cannot mix partner files with non-partner." in raw_msg,
+            "Cannot mix partner files with non-partner." in str(msg),
             "Expected email containing 'Cannot mix partner files with "
-            "non-partner.', got:\n%s" % raw_msg)
+            "non-partner.', got:\n%s" % msg)
 
     def testPartnerReusingOrigFromPartner(self):
         """Partner uploads reuse 'orig.tar.gz' from the partner archive."""
@@ -977,8 +968,7 @@
         self.processUpload(uploadprocessor, upload_dir)
         # Discard the announcement email and check the acceptance message
         # content.
-        from_addr, to_addrs, raw_msg = stub.test_emails.pop()
-        msg = message_from_string(raw_msg)
+        msg = pop_notifications()[-1]
         # This is now a MIMEMultipart message.
         body = msg.get_payload(0)
         body = body.get_payload(decode=True)
@@ -1095,11 +1085,10 @@
         self.processUpload(uploadprocessor, upload_dir)
 
         # Check it went ok to the NEW queue and all is going well so far.
-        from_addr, to_addrs, raw_msg = stub.test_emails.pop()
+        [msg] = pop_notifications()
         self.assertTrue(
-            "NEW" in raw_msg,
-            "Expected email containing 'NEW', got:\n%s"
-            % raw_msg)
+            "NEW" in str(msg),
+            "Expected email containing 'NEW', got:\n%s" % msg)
 
         # Accept and publish the upload.
         partner_archive = getUtility(IArchiveSet).getByDistroPurpose(
@@ -1198,10 +1187,10 @@
         # Check it is rejected.
         expect_msg = ("Partner uploads must be for the RELEASE or "
                       "PROPOSED pocket.")
-        from_addr, to_addrs, raw_msg = stub.test_emails.pop()
+        [msg] = pop_notifications()
         self.assertTrue(
-            expect_msg in raw_msg,
-            "Expected email with %s, got:\n%s" % (expect_msg, raw_msg))
+            expect_msg in str(msg),
+            "Expected email with %s, got:\n%s" % (expect_msg, msg))
 
         # And an oops should be filed for the error.
         error_report = self.oopses[new_index]
@@ -1428,10 +1417,9 @@
         upload_dir = self.queueUpload("bar_1.0-1_lzma")
         self.processUpload(uploadprocessor, upload_dir)
         # Make sure it went ok:
-        from_addr, to_addrs, raw_msg = stub.test_emails.pop()
-        self.assertTrue(
-            "rejected" not in raw_msg,
-            "Failed to upload bar source:\n%s" % raw_msg)
+        [msg] = pop_notifications()
+        self.assertFalse(
+            "rejected" in str(msg), "Failed to upload bar source:\n%s" % msg)
         self.publishPackage("bar", "1.0-1")
         # Clear out emails generated during upload.
         pop_notifications()
@@ -1441,11 +1429,7 @@
         self.processUpload(uploadprocessor, upload_dir)
 
         # Successful binary uploads won't generate any email.
-        if len(stub.test_emails) != 0:
-            from_addr, to_addrs, raw_msg = stub.test_emails.pop()
-        self.assertEqual(
-            len(stub.test_emails), 0,
-            "Expected no emails!  Actually got:\n%s" % raw_msg)
+        self.assertEmailQueueLength(0)
 
         # Check in the queue to see if it really made it:
         queue_items = self.breezy.getPackageUploads(
@@ -1472,10 +1456,9 @@
         upload_dir = self.queueUpload("bar_1.0-1_xz")
         self.processUpload(uploadprocessor, upload_dir)
         # Make sure it went ok:
-        from_addr, to_addrs, raw_msg = stub.test_emails.pop()
-        self.assertTrue(
-            "rejected" not in raw_msg,
-            "Failed to upload bar source:\n%s" % raw_msg)
+        [msg] = pop_notifications()
+        self.assertFalse(
+            "rejected" in str(msg), "Failed to upload bar source:\n%s" % msg)
         self.publishPackage("bar", "1.0-1")
         # Clear out emails generated during upload.
         pop_notifications()
@@ -1485,11 +1468,7 @@
         self.processUpload(uploadprocessor, upload_dir)
 
         # Successful binary uploads won't generate any email.
-        if len(stub.test_emails) != 0:
-            from_addr, to_addrs, raw_msg = stub.test_emails.pop()
-        self.assertEqual(
-            len(stub.test_emails), 0,
-            "Expected no emails!  Actually got:\n%s" % raw_msg)
+        self.assertEmailQueueLength(0)
 
         # Check in the queue to see if it really made it:
         queue_items = self.breezy.getPackageUploads(
@@ -1683,8 +1662,8 @@
         self.processUpload(uploadprocessor, upload_dir)
 
         # Check that it failed (permissions were granted for wrong series).
-        from_addr, to_addrs, raw_msg = stub.test_emails.pop()
-        msg = message_from_string(raw_msg)
+        # Any of the multiple notifications will do.
+        msg = pop_notifications()[-1]
         self.assertEqual(
             msg['Subject'], '[ubuntu] bar_1.0-2_source.changes (Rejected)')
 
@@ -1747,16 +1726,16 @@
         expected = []
         expected.append({
             "contents": base_contents + [
-                "You are receiving this email because you made this upload."],
-            "recipient": "[e-mail]",
-            })
-        expected.append({
-            "contents": base_contents + [
                 "You are receiving this email because you are the most "
                     "recent person",
                 "listed in this package's changelog."],
             "recipient": "[e-mail]",
             })
+        expected.append({
+            "contents": base_contents + [
+                "You are receiving this email because you made this upload."],
+            "recipient": "[e-mail]",
+            })
         self.assertEmails(expected)
 
     def test30QuiltUploadToUnsupportingSeriesIsRejected(self):
@@ -1774,11 +1753,11 @@
         upload_dir = self.queueUpload("bar_1.0-1_3.0-quilt")
         self.processUpload(uploadprocessor, upload_dir)
         # Make sure it was rejected.
-        from_addr, to_addrs, raw_msg = stub.test_emails.pop()
+        [msg] = pop_notifications()
         self.assertTrue(
             "bar_1.0-1.dsc: format '3.0 (quilt)' is not permitted in "
-            "breezy." in raw_msg,
-            "Source was not rejected properly:\n%s" % raw_msg)
+            "breezy." in str(msg),
+            "Source was not rejected properly:\n%s" % msg)
 
     def test30QuiltUpload(self):
         """Ensure that 3.0 (quilt) uploads work properly. """
@@ -1792,10 +1771,9 @@
         upload_dir = self.queueUpload("bar_1.0-1_3.0-quilt")
         self.processUpload(uploadprocessor, upload_dir)
         # Make sure it went ok:
-        from_addr, to_addrs, raw_msg = stub.test_emails.pop()
-        self.assertTrue(
-            "rejected" not in raw_msg,
-            "Failed to upload bar source:\n%s" % raw_msg)
+        [msg] = pop_notifications()
+        self.assertFalse(
+            "rejected" in str(msg), "Failed to upload bar source:\n%s" % msg)
         spph = self.publishPackage("bar", "1.0-1")
 
         self.assertEquals(
@@ -1827,10 +1805,9 @@
         upload_dir = self.queueUpload("bar_1.0-1_3.0-quilt")
         self.processUpload(uploadprocessor, upload_dir)
         # Make sure it went ok:
-        from_addr, to_addrs, raw_msg = stub.test_emails.pop()
-        self.assertTrue(
-            "rejected" not in raw_msg,
-            "Failed to upload bar source:\n%s" % raw_msg)
+        [msg] = pop_notifications()
+        self.assertFalse(
+            "rejected" in str(msg), "Failed to upload bar source:\n%s" % msg)
         self.publishPackage("bar", "1.0-1")
 
         # Upload another source sharing the same (component) orig.
@@ -1865,10 +1842,9 @@
         upload_dir = self.queueUpload("bar_1.0_3.0-native")
         self.processUpload(uploadprocessor, upload_dir)
         # Make sure it went ok:
-        from_addr, to_addrs, raw_msg = stub.test_emails.pop()
-        self.assertTrue(
-            "rejected" not in raw_msg,
-            "Failed to upload bar source:\n%s" % raw_msg)
+        [msg] = pop_notifications()
+        self.assertFalse(
+            "rejected" in str(msg), "Failed to upload bar source:\n%s" % msg)
         spph = self.publishPackage("bar", "1.0")
 
         self.assertEquals(
@@ -1890,11 +1866,11 @@
         upload_dir = self.queueUpload("bar_1.0-1_1.0-bzip2")
         self.processUpload(uploadprocessor, upload_dir)
         # Make sure it was rejected.
-        from_addr, to_addrs, raw_msg = stub.test_emails.pop()
+        [msg] = pop_notifications()
         self.assertTrue(
             "bar_1.0-1.dsc: is format 1.0 but uses bzip2 compression."
-            in raw_msg,
-            "Source was not rejected properly:\n%s" % raw_msg)
+            in str(msg),
+            "Source was not rejected properly:\n%s" % msg)
 
     def testUploadToWrongPocketIsRejected(self):
         # Uploads to the wrong pocket are rejected.
@@ -1922,16 +1898,16 @@
         expected = []
         expected.append({
             "contents": base_contents + [
-                "You are receiving this email because you made this upload."],
-            "recipient": "[e-mail]",
-            })
-        expected.append({
-            "contents": base_contents + [
                 "You are receiving this email because you are the most "
                     "recent person",
                 "listed in this package's changelog."],
             "recipient": "[e-mail]",
             })
+        expected.append({
+            "contents": base_contents + [
+                "You are receiving this email because you made this upload."],
+            "recipient": "[e-mail]",
+            })
         self.assertEmails(expected)
 
     def testPGPSignatureNotPreserved(self):
@@ -1969,7 +1945,7 @@
         self.assertEqual(UploadStatusEnum.REJECTED, result)
         self.assertLogContains(
             "INFO Failed to parse changes file")
-        self.assertEqual(len(stub.test_emails), 0)
+        self.assertEmailQueueLength(0)
         self.assertEqual([], self.oopses)
 
     def test_ddeb_upload_overrides(self):
@@ -2137,13 +2113,13 @@
                 queue_entry=leaf_name)
         self.options.context = 'buildd'
         self.options.builds = True
-        last_stub_mail_count = len(stub.test_emails)
+        pop_notifications()
         BuildUploadHandler(self.uploadprocessor, self.incoming_folder,
             leaf_name).process()
         self.layer.txn.commit()
         # No emails are sent on success
-        self.assertEquals(len(stub.test_emails), last_stub_mail_count)
-        self.assertEquals(BuildStatus.FULLYBUILT, build.status)
+        self.assertEmailQueueLength(0)
+        self.assertEqual(BuildStatus.FULLYBUILT, build.status)
         # Upon full build the upload log is unset.
         self.assertIs(None, build.upload_log)
 
@@ -2192,7 +2168,7 @@
         See bug 778437.
         """
         self.doSuccessRecipeBuild()
-        self.assertEquals(len(pop_notifications()), 0, pop_notifications)
+        self.assertEmailQueueLength(0)
 
     def doFailureRecipeBuild(self):
         # A source package recipe build will fail if no files are present.
@@ -2284,7 +2260,7 @@
             status=PackageUploadStatus.ACCEPTED,
             version=u"1.0-1", name=u"bar")
         queue_item.setDone()
-        stub.test_emails = []
+        pop_notifications()
 
         build.buildqueue_record.markAsBuilding(self.factory.makeBuilder())
         build.updateStatus(status)
@@ -2300,7 +2276,7 @@
             "bar_1.0-1_binary", queue_entry=leaf_name)
         self.options.context = 'buildd'
         self.options.builds = True
-        self.assertEqual([], stub.test_emails)
+        self.assertEmailQueueLength(0)
         BuildUploadHandler(self.uploadprocessor, self.incoming_folder,
             leaf_name).process()
         self.layer.txn.commit()

=== modified file 'lib/lp/bugs/doc/externalbugtracker-debbugs.txt'
--- lib/lp/bugs/doc/externalbugtracker-debbugs.txt	2015-03-13 19:05:50 +0000
+++ lib/lp/bugs/doc/externalbugtracker-debbugs.txt	2015-09-10 16:19:01 +0000
@@ -596,15 +596,15 @@
     ...     '<[e-mail]>')
     '<[e-mail]>'
 
-We can look in the test_emails list for the mail that would have been
-sent.
-
-    >>> from lp.services.mail import stub
-    >>> recipent, to_addrs, comment_email = stub.test_emails.pop()
-    >>> print to_addrs
-    ['[e-mail]']
-
-    >>> print comment_email
+We can look for the mail that would have been sent.
+
+    >>> from lp.testing.mail_helpers import pop_notifications
+    >>> [msg] = pop_notifications()
+    >>> print msg['X-Envelope-To']
+    [e-mail]
+
+    >>> print str(msg)
+    From ...
     Content-Type...
     Message-Id: <[e-mail]>
     To: [e-mail]

=== modified file 'lib/lp/bugs/mail/tests/test_handler.py'
--- lib/lp/bugs/mail/tests/test_handler.py	2012-09-17 16:13:40 +0000
+++ lib/lp/bugs/mail/tests/test_handler.py	2015-09-10 16:19:01 +0000
@@ -1,11 +1,10 @@
-# Copyright 2010-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test MaloneHandler."""
 
 __metaclass__ = type
 
-import email
 import time
 
 import transaction
@@ -32,7 +31,6 @@
 from lp.registry.enums import BugSharingPolicy
 from lp.services.config import config
 from lp.services.identity.interfaces.emailaddress import EmailAddressStatus
-from lp.services.mail import stub
 from lp.services.webapp.authorization import LaunchpadSecurityPolicy
 from lp.testing import (
     celebrity_logged_in,
@@ -124,12 +122,11 @@
         response = handler.extractAndAuthenticateCommands(
             message, '[e-mail]')
         mail_handled, add_comment_to_bug, commands = response
-        self.assertEquals(mail_handled, True)
-        emails = self.getSentMail()
-        self.assertEquals(1, len(emails))
-        self.assertEquals(['[e-mail]'], emails[0][1])
-        self.assertTrue(
-            'Subject: Launchpad Bug Tracker Email Interface' in emails[0][2])
+        self.assertTrue(mail_handled)
+        emails = self.assertEmailQueueLength(1)
+        self.assertEqual('[e-mail]', emails[0]['X-Envelope-To'])
+        self.assertEqual(
+            'Launchpad Bug Tracker Email Interface Help', emails[0]['Subject'])
 
     def test_mailToHelpFromUnknownUser(self):
         """Mail from people of no account to help@ is simply dropped.
@@ -140,8 +137,8 @@
         mail_handled, add_comment_to_bug, commands = \
             handler.extractAndAuthenticateCommands(message,
                 '[e-mail]')
-        self.assertEquals(mail_handled, True)
-        self.assertEquals(self.getSentMail(), [])
+        self.assertTrue(mail_handled)
+        self.assertEmailQueueLength(0)
 
     def test_mailToHelp(self):
         """Mail to help@ generates a help command."""
@@ -152,19 +149,11 @@
             mail_handled, add_comment_to_bug, commands = \
                 handler.extractAndAuthenticateCommands(message,
                     '[e-mail]')
-        self.assertEquals(mail_handled, True)
-        emails = self.getSentMail()
-        self.assertEquals(1, len(emails))
-        self.assertEquals([message['From']], emails[0][1])
-        self.assertTrue(
-            'Subject: Launchpad Bug Tracker Email Interface' in emails[0][2])
-
-    def getSentMail(self):
-        # Sending mail is (unfortunately) a side effect of parsing the
-        # commands, and unfortunately you must commit the transaction to get
-        # them sent.
-        transaction.commit()
-        return stub.test_emails[:]
+        self.assertEqual(mail_handled, True)
+        emails = self.assertEmailQueueLength(1)
+        self.assertEqual(message['From'], emails[0]['X-Envelope-To'])
+        self.assertEqual(
+            'Launchpad Bug Tracker Email Interface Help', emails[0]['Subject'])
 
     def getFailureForMessage(self, to_address, from_address=None, body=None):
         mail = self.factory.makeSignedMessage(
@@ -683,22 +672,6 @@
         self.timestamp = timestamp
 
 
-def get_last_email():
-    from_addr, to_addrs, raw_message = stub.test_emails[-1]
-    sent_msg = email.message_from_string(raw_message)
-    error_mail, original_mail = sent_msg.get_payload()
-    # clear the emails so we don't accidentally get one from a previous test
-    return dict(
-        subject=sent_msg['Subject'],
-        body=error_mail.get_payload(decode=True))
-
-
-BAD_SIGNATURE_TIMESTAMP_MESSAGE = (
-    'The message you sent included commands to modify the bug '
-    'report, but the\nsignature was (apparently) generated too far '
-    'in the past or future.')
-
-
 class TestSignatureTimestampValidation(TestCaseWithFactory):
     """GPG signature timestamps are checked for emails containing commands."""
 
@@ -715,10 +688,9 @@
         handler = MaloneHandler()
         with person_logged_in(self.factory.makePerson()):
             handler.process(msg, msg['To'])
-        transaction.commit()
         # Since there were no commands in the poorly-timestamped message, no
         # error emails were generated.
-        self.assertEqual(stub.test_emails, [])
+        self.assertEmailQueueLength(0)
 
     def test_bad_timestamp_but_no_commands(self):
         # If an email message's GPG signature's timestamp is too far in the
@@ -732,10 +704,9 @@
         msg.signature = FakeSignature(timestamp=now + one_week)
         handler = MaloneHandler()
         # Clear old emails before potentially generating more.
-        del stub.test_emails[:]
+        pop_notifications()
         with person_logged_in(self.factory.makePerson()):
             handler.process(msg, msg['To'])
-        transaction.commit()
         # Since there were no commands in the poorly-timestamped message, no
         # error emails were generated.
-        self.assertEqual(stub.test_emails, [])
+        self.assertEmailQueueLength(0)

=== modified file 'lib/lp/services/mail/tests/incomingmail.txt'
--- lib/lp/services/mail/tests/incomingmail.txt	2015-07-08 16:05:11 +0000
+++ lib/lp/services/mail/tests/incomingmail.txt	2015-09-10 16:19:01 +0000
@@ -121,8 +121,9 @@
 wasn't a handler registered for that domain, an OOPS was recorded with
 a link to the original message.
 
-    >>> from lp.services.mail import stub
-    >>> print stub.test_emails[-1][2]
+    >>> from lp.testing.mail_helpers import pop_notifications
+    >>> print str(pop_notifications()[-1])
+    From ...
     Content-Type: multipart/mixed...
     ...
     To: Sample Person <[e-mail]>
@@ -140,8 +141,6 @@
     Subject: Signed Email
     ...
 
-    >>> stub.test_emails = []
-
 ---------------------------------------------
 Mail from Persons not registered in Launchpad
 ---------------------------------------------
@@ -156,7 +155,7 @@
     >>> msgid not in foo_handler.handledMails
     True
 
-    >>> stub.test_emails = []
+    >>> _ = pop_notifications()
 
 However, bar_handler specifies that it can handle such emails:
 
@@ -171,7 +170,7 @@
     >>> msgid in bar_handler.handledMails
     True
 
-    >>> stub.test_emails = []
+    >>> _ = pop_notifications()
 
 ---------------------------------------------------------
 Mail from Persons with with an inactive Launchpad account
@@ -260,7 +259,8 @@
 An exception is raised, an OOPS is recorded, and an email is sent back
 to the user, citing the OOPS ID, with the original message attached.
 
-    >>> print stub.test_emails[-1][2]
+    >>> print str(pop_notifications()[-1])
+    From ...
     Content-Type: multipart/mixed...
     ...
     To: Foo Bar <[e-mail]>
@@ -279,8 +279,6 @@
     Subject: doesn't matter
     ...
 
-    >>> stub.test_emails = []
-
 Unauthorized exceptions, which are ignored for the purpose of OOPS
 reporting in the web interface, are not ignored in the email interface.
 
@@ -305,7 +303,8 @@
     ...
     Unauthorized
 
-    >>> print stub.test_emails[-1][2]
+    >>> print str(pop_notifications()[-1])
+    From ...
     Content-Type: multipart/mixed...
     ...
     To: Foo Bar <[e-mail]>
@@ -324,8 +323,6 @@
     Subject: doesn't matter
     ...
 
-    >>> stub.test_emails = []
-
 -------------
 DB exceptions
 -------------
@@ -382,6 +379,7 @@
 There is only one mail left in the mail box - the one sent back to
 the user reporting the error:
 
+    >>> from lp.services.mail import stub
     >>> len(stub.test_emails)
     1
 
@@ -412,7 +410,7 @@
     2
 
     >>> LibrarianLayer.reveal()
-    >>> stub.test_emails = []
+    >>> _ = pop_notifications()
 
 ----------------
 Handling bounces
@@ -425,7 +423,6 @@
 
 Emails with an empty Return-Path header should be dropped:
 
-    >>> stub.test_emails = []
     >>> msg = read_test_message('signed_detached.txt')
     >>> msg.replace_header('To', '[e-mail]')
     >>> msg['Return-Path'] = '<>'
@@ -437,8 +434,8 @@
 Since this happens way too often, as we seem to get more spam than
 legitimate email, an email is not sent about it to the errors-list.
 
-    >>> len(stub.test_emails)
-    0
+    >>> pop_notifications()
+    []
 
 If the content type is multipart/report, it's most likely a DSN
 (RFC 3464), so those get dropped as well. Normally a DSN should have an
@@ -455,8 +452,8 @@
     >>> msgid in foo_handler.handledMails
     False
 
-    >>> len(stub.test_emails)
-    0
+    >>> pop_notifications()
+    []
 
 Email with the Precedence header are probably from an auto-responder or
 another robot. We also drop those.
@@ -470,8 +467,8 @@
     >>> msgid in foo_handler.handledMails
     False
 
-    >>> len(stub.test_emails)
-    0
+    >>> pop_notifications()
+    []
 
 
 .. Doctest cleanup

=== modified file 'lib/lp/soyuz/tests/test_packageupload.py'
--- lib/lp/soyuz/tests/test_packageupload.py	2015-09-04 12:19:07 +0000
+++ lib/lp/soyuz/tests/test_packageupload.py	2015-09-10 16:19:01 +0000
@@ -29,7 +29,6 @@
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.job.runner import JobRunner
 from lp.services.librarian.browser import ProxiedLibraryFileAlias
-from lp.services.mail import stub
 from lp.soyuz.adapters.overrides import SourceOverride
 from lp.soyuz.enums import (
     PackagePublishingStatus,
@@ -192,10 +191,11 @@
         with dbuser(config.IPackageUploadNotificationJobSource.dbuser):
             JobRunner([job]).runAll()
 
-    def assertEmail(self, expected_to_addrs):
-        """Pop an email from the stub queue and check its recipients."""
-        _, to_addrs, _ = stub.test_emails.pop()
-        self.assertEqual(expected_to_addrs, to_addrs)
+    def assertEmails(self, expected_to_addrs):
+        """Pop emails from the stub queue and check their recipients."""
+        notifications = self.assertEmailQueueLength(len(expected_to_addrs))
+        for expected_to_addr, msg in zip(expected_to_addrs, notifications):
+            self.assertEqual(expected_to_addr, msg["X-Envelope-To"])
 
     def test_acceptFromQueue_source_sends_email(self):
         # Accepting a source package sends emails to the announcement list
@@ -204,10 +204,11 @@
         upload, uploader = self.makeSourcePackageUpload()
         upload.acceptFromQueue()
         self.runPackageUploadNotificationJob()
-        self.assertEqual(2, len(stub.test_emails))
         # Emails sent are the uploader's notification and the announcement:
-        self.assertEmail([uploader.preferredemail.email])
-        self.assertEmail(["[e-mail]"])
+        self.assertEmails([
+            uploader.preferredemail.email,
+            "[e-mail]",
+            ])
 
     def test_acceptFromQueue_source_backports_sends_no_announcement(self):
         # Accepting a source package into BACKPORTS does not send an
@@ -219,10 +220,9 @@
             pocket=PackagePublishingPocket.BACKPORTS)
         upload.acceptFromQueue()
         self.runPackageUploadNotificationJob()
-        self.assertEqual(1, len(stub.test_emails))
         # Only one email is sent, to the person in the changed-by field.  No
         # announcement email is sent.
-        self.assertEmail([uploader.preferredemail.email])
+        self.assertEmails([uploader.preferredemail.email])
 
     def test_acceptFromQueue_source_translations_sends_no_email(self):
         # Accepting source packages in the "translations" section (i.e.
@@ -235,7 +235,7 @@
         upload.acceptFromQueue()
         self.runPackageUploadNotificationJob()
         self.assertEqual("DONE", upload.status.name)
-        self.assertEqual(0, len(stub.test_emails))
+        self.assertEmailQueueLength(0)
 
     def test_acceptFromQueue_source_creates_builds(self):
         # Accepting a source package creates build records.
@@ -281,7 +281,7 @@
         upload, _ = self.makeBuildPackageUpload()
         upload.acceptFromQueue()
         self.runPackageUploadNotificationJob()
-        self.assertEqual(0, len(stub.test_emails))
+        self.assertEmailQueueLength(0)
 
     def test_acceptFromQueue_handles_duplicates(self):
         # Duplicate queue entries are handled sensibly.
@@ -329,8 +329,7 @@
         upload, uploader = self.makeSourcePackageUpload()
         upload.rejectFromQueue(self.factory.makePerson())
         self.runPackageUploadNotificationJob()
-        self.assertEqual(1, len(stub.test_emails))
-        self.assertEmail([uploader.preferredemail.email])
+        self.assertEmails([uploader.preferredemail.email])
 
     def test_rejectFromQueue_binary_sends_email(self):
         # Rejecting a binary package sends an email to the uploader.
@@ -338,8 +337,7 @@
         upload, uploader = self.makeBuildPackageUpload()
         upload.rejectFromQueue(self.factory.makePerson())
         self.runPackageUploadNotificationJob()
-        self.assertEqual(1, len(stub.test_emails))
-        self.assertEmail([uploader.preferredemail.email])
+        self.assertEmails([uploader.preferredemail.email])
 
     def test_rejectFromQueue_source_translations_sends_no_email(self):
         # Rejecting a language pack sends no email.
@@ -350,7 +348,7 @@
             section_name="translations")
         upload.rejectFromQueue(self.factory.makePerson())
         self.runPackageUploadNotificationJob()
-        self.assertEqual(0, len(stub.test_emails))
+        self.assertEmailQueueLength(0)
 
     def test_rejectFromQueue_source_with_reason(self):
         # Rejecting a source package with a reason includes it in the email to
@@ -360,10 +358,10 @@
         person = self.factory.makePerson()
         upload.rejectFromQueue(user=person, comment='Because.')
         self.runPackageUploadNotificationJob()
-        self.assertEqual(1, len(stub.test_emails))
+        [msg] = self.assertEmailQueueLength(1)
         self.assertIn(
             'Rejected:\nRejected by %s: Because.' % person.displayname,
-            stub.test_emails[0][-1])
+            str(msg))
 
 
 class TestPackageUploadSecurity(TestCaseWithFactory):

=== modified file 'lib/lp/testing/__init__.py'
--- lib/lp/testing/__init__.py	2015-08-03 12:59:18 +0000
+++ lib/lp/testing/__init__.py	2015-09-10 16:19:01 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 from __future__ import absolute_import
@@ -184,6 +184,7 @@
 from lp.testing.dbuser import switch_dbuser
 from lp.testing.fixture import CaptureOops
 from lp.testing.karma import KarmaRecorder
+from lp.testing.mail_helpers import pop_notifications
 
 # The following names have been imported for the purpose of being
 # exported. They are referred to here to silence lint warnings.
@@ -744,6 +745,19 @@
                     sorted(
                         expected_permissions[permission] - attribute_names)))
 
+    def assertEmailQueueLength(self, length, sort_key=None):
+        """Pop the email queue, assert its length, and return it.
+
+        This commits the transaction as part of pop_notifications.
+        """
+        notifications = pop_notifications(sort_key=sort_key)
+        self.assertEqual(
+            length, len(notifications),
+            "Expected %d emails, got %d:\n\n%s" % (
+                length, len(notifications),
+                "\n\n".join(str(n) for n in notifications)))
+        return notifications
+
 
 class TestCaseWithFactory(TestCase):