Merge lp:~jml/launchpad/one-email-on-crash into lp:launchpad

[Jonathan Lange]

Only send one email when "ec2 test" fails catastrophically.

ec2 test has a fair bit of stuff in it to make sure that unexpected failures
don't screw things up too badly, and that appropriate folk get told about it.

Unfortunately, in the code currently in Launchpad, there's perhaps a bit too
much redundancy and safety. This code changes things so that you get exactly
one email if ec2 test fails surprisingly.

The reason I wanted to make this change is to:
 a) clarify, test and document the way we send email on failure.
 b) create separate interfaces for sending email on catastrophe vs normal
    test results
 c) reduce the amount of email I receive ever so slightly

Landing this branch will make it possible, finally, to free the normal
test email from the shackles of displaying the actual output of the test
runner.

In terms of code changes:
 * Refactor EC2Runner & LaunchpadTester to take an SMTPConnection object
   and use that for sending mail, thus allowing for easier testing of
   what mail gets sent.

 * Change WebTestLogger.error_in_testrunner to send the email on catastrophe,
   rather than got_result.

 * Change LaunchpadTester.test not to re-raise exceptions it receives from
   running the tests. Note: I'm not sure what the reason was for doing so
   in the first place.

Thanks,
jml

[Michael Nelson (michael.nelson)]

It would have been nice if the email handling could simply be a logging
configuration using:

http://docs.python.org/library/logging.html#smtphandler

but afaics, that doesn't handle attachments for emails.

Anyway, r=me, with some typos fixed below.

> === modified file 'lib/devscripts/ec2test/remote.py'
> --- lib/devscripts/ec2test/remote.py 2010-08-24 17:35:55 +0000
> +++ lib/devscripts/ec2test/remote.py 2010-08-26 13:19:57 +0000
> @@ -510,6 +519,13 @@
> summary.write('\n\nERROR IN TESTRUNNER\n\n')
> traceback.print_exception(exc_type, exc_value, exc_tb, file=summary)
> summary.flush()
> + if self._request.wants_email:
> + self._write_to_filename(
> + self._summary_filename,
> + '\n(See the attached file for the complete log)\n')
> + summary = self.get_summary_contents()
> + full_log_gz = gzip_data(self.get_full_log_contents())
> + self._request.send_report_email(False, summary, full_log_gz)

Is it worth reordering the args to send_report_email, so you could instead
write:
               self._request.send_report_email(
                   summary, full_log_gz, successful=False)

Probably not. I just had to look back to see what was False, but it's obvious
in retrospect :)

> === modified file 'lib/devscripts/ec2test/tests/test_remote.py'
> --- lib/devscripts/ec2test/tests/test_remote.py 2010-08-24 17:44:22 +0000
> +++ lib/devscripts/ec2test/tests/test_remote.py 2010-08-26 13:19:57 +0000
> @@ -60,7 +71,7 @@
>
> def make_request(self, branch_url=None, revno=None,
> trunk=None, sourcecode_path=None,
> - emails=None, pqm_message=None):
> + emails=None, pqm_message=None, emails_sent=None):
> """Make a request to test, specifying only things we care about.
>
> Note that the returned request object will not ever send email, but

The above comment needs updating with your change.

> @@ -94,6 +106,13 @@
> tree.commit(message=str(i))
> return 'sourcecode/'
>
> + def make_tester(self, logger=None, test_directory=None, test_options=()):
> + if not logger:
> + logger = self.make_logger()
> + if not test_directory:
> + test_directory = 'unspecified-test-directory'

why is test_directory optional? If set it like this, I would have assumed
that Popen() would fail when it tries to change into this directory?

I haven't tried it, but from the docs:

If cwd is not None, the child’s current directory will be changed to cwd
before it is executed.

[1] http://docs.python.org/library/subprocess.html#module-subprocess

We discussed this:

15:51 < noodles775> jml: why do you set test_directory when it's passed as None?
15:52 < noodles775> (that is, why do you set it to a non-existent directory)
15:52 < jml> noodles775, umm, not sure. maybe because stuff explodes.
15:52 * jml tries.
15:53 < noodles775> According to the docs, I'd assume it wouldn't explode when its None, but will in its current state?
15:53 < noodles775> But yes, science is the best answer :)
15:54 < salgado> thanks for the reviews, noodles775
1...

review: Approve (code)

[Jonathan Lange (jml)]

On Thu, Aug 26, 2010 at 3:13 PM, Michael Nelson
<email address hidden> wrote:
> Review: Approve code
> It would have been nice if the email handling could simply be a logging
> configuration using:
>
> http://docs.python.org/library/logging.html#smtphandler
>
> but afaics, that doesn't handle attachments for emails.
>
> Anyway, r=me, with some typos fixed below.
>
>> === modified file 'lib/devscripts/ec2test/remote.py'
>> --- lib/devscripts/ec2test/remote.py  2010-08-24 17:35:55 +0000
>> +++ lib/devscripts/ec2test/remote.py  2010-08-26 13:19:57 +0000
>> @@ -510,6 +519,13 @@
>>          summary.write('\n\nERROR IN TESTRUNNER\n\n')
>>          traceback.print_exception(exc_type, exc_value, exc_tb, file=summary)
>>          summary.flush()
>> +        if self._request.wants_email:
>> +            self._write_to_filename(
>> +                self._summary_filename,
>> +                '\n(See the attached file for the complete log)\n')
>> +            summary = self.get_summary_contents()
>> +            full_log_gz = gzip_data(self.get_full_log_contents())
>> +            self._request.send_report_email(False, summary, full_log_gz)
>
> Is it worth reordering the args to send_report_email, so you could instead
> write:
>               self._request.send_report_email(
>                   summary, full_log_gz, successful=False)
>
> Probably not. I just had to look back to see what was False, but it's obvious
> in retrospect :)
>

I won't do it in this branch, for the reasons you give here. I plan a
follow-up branch that will probably change the API & behaviour a bit,
since it would be nice if the subject for "failed tests" is noticeably
different to the subject for "something went horribly wrong".

>> === modified file 'lib/devscripts/ec2test/tests/test_remote.py'
>> --- lib/devscripts/ec2test/tests/test_remote.py       2010-08-24 17:44:22 +0000
>> +++ lib/devscripts/ec2test/tests/test_remote.py       2010-08-26 13:19:57 +0000
>> @@ -60,7 +71,7 @@
>>
>>      def make_request(self, branch_url=None, revno=None,
>>                       trunk=None, sourcecode_path=None,
>> -                     emails=None, pqm_message=None):
>> +                     emails=None, pqm_message=None, emails_sent=None):
>>          """Make a request to test, specifying only things we care about.
>>
>>          Note that the returned request object will not ever send email, but
>
> The above comment needs updating with your change.
>

Done.

>> @@ -94,6 +106,13 @@
>>                  tree.commit(message=str(i))
>>          return 'sourcecode/'
>>
>> +    def make_tester(self, logger=None, test_directory=None, test_options=()):
>> +        if not logger:
>> +            logger = self.make_logger()
>> +        if not test_directory:
>> +            test_directory = 'unspecified-test-directory'
>
> why is test_directory optional? If set it like this, I would have assumed
> that Popen() would fail when it tries to change into this directory?
>
> I haven't tried it, but from the docs:
>
> If cwd is not None, the child’s current directory will be changed to cwd
> before it is executed.
>
> [1] http://docs.python.org/library/subprocess.html#module-subprocess
>
> We discus...

=== modified file 'lib/devscripts/ec2test/remote.py'
--- lib/devscripts/ec2test/remote.py	2010-08-24 17:35:55 +0000
+++ lib/devscripts/ec2test/remote.py	2010-08-26 12:56:33 +0000
@@ -40,11 +40,12 @@
 
 import bzrlib.branch
 import bzrlib.config
-import bzrlib.email_message
 import bzrlib.errors
-import bzrlib.smtp_connection
 import bzrlib.workingtree
 
+from bzrlib.email_message import EmailMessage
+from bzrlib.smtp_connection import SMTPConnection
+
 import subunit
 
 
@@ -134,19 +135,24 @@
     SHUTDOWN_DELAY = 60
 
     def __init__(self, daemonize, pid_filename, shutdown_when_done,
-                 emails=None):
+                 smtp_connection=None, emails=None):
         """Make an EC2Runner.
 
         :param daemonize: Whether or not we will daemonize.
         :param pid_filename: The filename to store the pid in.
         :param shutdown_when_done: Whether or not to shut down when the tests
             are done.
+        :param smtp_connection: The `SMTPConnection` to use to send email.
         :param emails: The email address(es) to send catastrophic failure
             messages to. If not provided, the error disappears into the ether.
         """
         self._should_daemonize = daemonize
         self._pid_filename = pid_filename
         self._shutdown_when_done = shutdown_when_done
+        if smtp_connection is None:
+            config = bzrlib.config.GlobalConfig()
+            smtp_connection = SMTPConnection(config)
+        self._smtp_connection = smtp_connection
         self._emails = emails
         self._daemonized = False
 
@@ -188,10 +194,12 @@
             config = bzrlib.config.GlobalConfig()
             # Handle exceptions thrown by the test() or daemonize() methods.
             if self._emails:
-                bzrlib.email_message.EmailMessage.send(
-                    config, config.username(),
-                    self._emails, '%s FAILED' % (name,),
-                    traceback.format_exc())
+                msg = EmailMessage(
+                    from_address=config.username(),
+                    to_address=self._emails,
+                    subject='%s FAILED' % (name,),
+                    body=traceback.format_exc())
+                self._smtp_connection.send_email(msg)
             raise
         finally:
             # When everything is over, if we've been ask to shut down, then
@@ -267,9 +275,7 @@
             exit_status = popen.wait()
         except:
             self._logger.error_in_testrunner(sys.exc_info())
-            exit_status = 1
-            raise
-        finally:
+        else:
             self._logger.got_result(not exit_status)
 
     def _gather_test_output(self, input_stream, logger):
@@ -287,7 +293,7 @@
     """A request to have a branch tested and maybe landed."""
 
     def __init__(self, branch_url, revno, local_branch_path, sourcecode_path,
-                 emails=None, pqm_message=None):
+                 emails=None, pqm_message=None, smtp_connection=None):
         """Construct a `Request`.
 
         :param branch_url: The public URL to the Launchpad branch we are
@@ -303,6 +309,7 @@
             provided, no emails are sent.
         :param pqm_message: The message to submit to PQM. If not provided, we
             don't submit to PQM.
+        :param smtp_connection: The `SMTPConnection` to use to send email.
         """
         self._branch_url = branch_url
         self._revno = revno
@@ -312,11 +319,13 @@
         self._pqm_message = pqm_message
         # Used for figuring out how to send emails.
         self._bzr_config = bzrlib.config.GlobalConfig()
+        if smtp_connection is None:
+            smtp_connection = SMTPConnection(self._bzr_config)
+        self._smtp_connection = smtp_connection
 
     def _send_email(self, message):
         """Actually send 'message'."""
-        conn = bzrlib.smtp_connection.SMTPConnection(self._bzr_config)
-        conn.send_email(message)
+        self._smtp_connection.send_email(message)
 
     def get_target_details(self):
         """Return (branch_url, revno) for trunk."""
@@ -510,6 +519,13 @@
         summary.write('\n\nERROR IN TESTRUNNER\n\n')
         traceback.print_exception(exc_type, exc_value, exc_tb, file=summary)
         summary.flush()
+        if self._request.wants_email:
+            self._write_to_filename(
+                self._summary_filename,
+                '\n(See the attached file for the complete log)\n')
+            summary = self.get_summary_contents()
+            full_log_gz = gzip_data(self.get_full_log_contents())
+            self._request.send_report_email(False, summary, full_log_gz)
 
     def get_index_contents(self):
         """Return the contents of the index.html page."""
@@ -805,16 +821,19 @@
 
     pid_filename = os.path.join(LAUNCHPAD_DIR, 'ec2test-remote.pid')
 
+    smtp_connection = SMTPConnection(bzrlib.config.GlobalConfig())
+
     request = Request(
         options.public_branch, options.public_branch_revno, TEST_DIR,
-        SOURCECODE_DIR, options.email, pqm_message)
+        SOURCECODE_DIR, options.email, pqm_message, smtp_connection)
     # Only write to stdout if we are running as the foreground process.
     echo_to_stdout = not options.daemon
     logger = WebTestLogger.make_in_directory(
         '/var/www', request, echo_to_stdout)
 
     runner = EC2Runner(
-        options.daemon, pid_filename, options.shutdown, options.email)
+        options.daemon, pid_filename, options.shutdown,
+        smtp_connection, options.email)
 
     tester = LaunchpadTester(logger, TEST_DIR, test_options=args[1:])
     runner.run("Test runner", tester.test)

=== modified file 'lib/devscripts/ec2test/tests/test_remote.py'
--- lib/devscripts/ec2test/tests/test_remote.py	2010-08-24 17:44:22 +0000
+++ lib/devscripts/ec2test/tests/test_remote.py	2010-08-26 12:56:33 +0000
@@ -26,6 +26,7 @@
 from testtools.content_type import ContentType
 
 from devscripts.ec2test.remote import (
+    EC2Runner,
     FlagFallStream,
     gunzip_data,
     gzip_data,
@@ -38,6 +39,16 @@
     )
 
 
+class LoggingSMTPConnection(object):
+    """An SMTPConnection double that logs sent email."""
+
+    def __init__(self, log):
+        self._log = log
+
+    def send_email(self, message):
+        self._log.append(message)
+
+
 class RequestHelpers:
 
     def patch(self, obj, name, value):
@@ -60,7 +71,7 @@
 
     def make_request(self, branch_url=None, revno=None,
                      trunk=None, sourcecode_path=None,
-                     emails=None, pqm_message=None):
+                     emails=None, pqm_message=None, emails_sent=None):
         """Make a request to test, specifying only things we care about.
 
         Note that the returned request object will not ever send email, but
@@ -73,11 +84,12 @@
                 [('a', 'http://example.com/bzr/a', 2),
                  ('b', 'http://example.com/bzr/b', 3),
                  ('c', 'http://example.com/bzr/c', 5)])
+        if emails_sent is None:
+            emails_sent = []
+        smtp_connection = LoggingSMTPConnection(emails_sent)
         request = Request(
             branch_url, revno, trunk.basedir, sourcecode_path, emails,
-            pqm_message)
-        request.emails_sent = []
-        request._send_email = request.emails_sent.append
+            pqm_message, smtp_connection)
         return request
 
     def make_sourcecode(self, branches):
@@ -94,6 +106,13 @@
                 tree.commit(message=str(i))
         return 'sourcecode/'
 
+    def make_tester(self, logger=None, test_directory=None, test_options=()):
+        if not logger:
+            logger = self.make_logger()
+        if not test_directory:
+            test_directory = 'unspecified-test-directory'
+        return LaunchpadTester(logger, test_directory, test_options)
+
     def make_logger(self, request=None, echo_to_stdout=False):
         if request is None:
             request = self.make_request()
@@ -199,13 +218,6 @@
 
 class TestLaunchpadTester(TestCaseWithTransport, RequestHelpers):
 
-    def make_tester(self, logger=None, test_directory=None, test_options=()):
-        if not logger:
-            logger = self.make_logger()
-        if not test_directory:
-            test_directory = 'unspecified-test-directory'
-        return LaunchpadTester(logger, test_directory, test_options)
-
     def test_build_test_command_no_options(self):
         # The LaunchpadTester runs "make check" if given no options.
         tester = self.make_tester()
@@ -243,14 +255,15 @@
         # got_result with the result.  This test is more of a smoke test to
         # make sure that everything integrates well.
         message = {'Subject': "One Crowded Hour"}
-        request = self.make_request(pqm_message=message)
+        log = []
+        request = self.make_request(pqm_message=message, emails_sent=log)
         logger = self.make_logger(request=request)
         tester = self.make_tester(logger=logger)
         output = "test output\n"
         tester._spawn_test_process = lambda: FakePopen(output, 0)
         tester.test()
         # Message being sent implies got_result thought it got a success.
-        self.assertEqual([message], request.emails_sent)
+        self.assertEqual([message], log)
 
     def test_error_in_testrunner(self):
         # Any exception is raised within LaunchpadTester.test() is an error in
@@ -260,28 +273,30 @@
         #      failure.
         #   3. Re-raise the error. In the script, this triggers an email.
         message = {'Subject': "One Crowded Hour"}
-        request = self.make_request(pqm_message=message)
+        log = []
+        request = self.make_request(pqm_message=message, emails_sent=log)
         logger = self.make_logger(request=request)
         tester = self.make_tester(logger=logger)
         # Break the test runner deliberately. In production, this is more
         # likely to be a system error than a programming error.
         tester._spawn_test_process = lambda: 1/0
-        self.assertRaises(ZeroDivisionError, tester.test)
+        tester.test()
         # Message not being sent implies got_result thought it got a failure.
-        self.assertEqual([], request.emails_sent)
+        self.assertEqual([], log)
         self.assertIn("ERROR IN TESTRUNNER", logger.get_summary_contents())
         self.assertIn("ZeroDivisionError", logger.get_summary_contents())
 
     def test_nonzero_exit_code(self):
         message = {'Subject': "One Crowded Hour"}
-        request = self.make_request(pqm_message=message)
+        log = []
+        request = self.make_request(pqm_message=message, emails_sent=log)
         logger = self.make_logger(request=request)
         tester = self.make_tester(logger=logger)
         output = "test output\n"
         tester._spawn_test_process = lambda: FakePopen(output, 10)
         tester.test()
         # Message not being sent implies got_result thought it got a failure.
-        self.assertEqual([], request.emails_sent)
+        self.assertEqual([], log)
 
 
 class TestPidfileHelpers(TestCase):
@@ -449,9 +464,10 @@
 
     def test_submit_to_pqm_no_message_doesnt_send(self):
         # If there's no PQM message, then 'submit_to_pqm' returns None.
-        req = self.make_request(pqm_message=None)
+        log = []
+        req = self.make_request(pqm_message=None, emails_sent=log)
         req.submit_to_pqm(successful=True)
-        self.assertEqual([], req.emails_sent)
+        self.assertEqual([], log)
 
     def test_submit_to_pqm_unsuccessful(self):
         # submit_to_pqm returns the subject of the PQM mail even if it's
@@ -464,17 +480,19 @@
     def test_submit_to_pqm_unsuccessful_no_email(self):
         # submit_to_pqm doesn't send any email if the run was unsuccessful.
         message = {'Subject:': 'My PQM message'}
-        req = self.make_request(pqm_message=message)
+        log = []
+        req = self.make_request(pqm_message=message, emails_sent=log)
         req.submit_to_pqm(successful=False)
-        self.assertEqual([], req.emails_sent)
+        self.assertEqual([], log)
 
     def test_submit_to_pqm_successful(self):
         # submit_to_pqm returns the subject of the PQM mail.
         message = {'Subject:': 'My PQM message'}
-        req = self.make_request(pqm_message=message)
+        log = []
+        req = self.make_request(pqm_message=message, emails_sent=log)
         subject = req.submit_to_pqm(successful=True)
         self.assertIs(message.get('Subject'), subject)
-        self.assertEqual([message], req.emails_sent)
+        self.assertEqual([message], log)
 
     def test_report_email_subject_success(self):
         req = self.make_request(emails=['[e-mail]'])
@@ -523,10 +541,11 @@
             "gobbledygook", attachment.get_payload().decode('base64'))
 
     def test_send_report_email_sends_email(self):
-        req = self.make_request(emails=['[e-mail]'])
+        log = []
+        req = self.make_request(emails=['[e-mail]'], emails_sent=log)
         expected = req._build_report_email(False, "foo", "gobbledygook")
         req.send_report_email(False, "foo", "gobbledygook")
-        [observed] = req.emails_sent
+        [observed] = log
         # The standard library sucks. None of the MIME objects have __eq__
         # implementations.
         for expected_part, observed_part in izip(
@@ -597,7 +616,7 @@
         self.assertEqual('foo\n', logger.get_full_log_contents())
         self.assertEqual('foo\n', logger.get_summary_contents())
 
-    def test_error_in_testrunner(self):
+    def test_error_in_testrunner_logs_to_summary(self):
         # error_in_testrunner logs the traceback to the summary log in a very
         # prominent way.
         try:
@@ -611,6 +630,93 @@
             "\n\nERROR IN TESTRUNNER\n\n%s" % (stack,),
             logger.get_summary_contents())
 
+    def test_error_in_testrunner_sends_email(self):
+        # If email addresses are configurd, error_in_testrunner emails them
+        # with the failure and the full log.
+        try:
+            1/0
+        except ZeroDivisionError:
+            exc_info = sys.exc_info()
+        log = []
+        request = self.make_request(
+            emails=['[e-mail]'], emails_sent=log)
+        logger = self.make_logger(request=request)
+        logger.error_in_testrunner(exc_info)
+        [email] = log
+        self.assertEqual(
+            'Test results: %s: FAILURE' % request.get_merge_description(),
+            email['Subject'])
+        [body, attachment] = email.get_payload()
+        self.assertIsInstance(body, MIMEText)
+        self.assertEqual('inline', body['Content-Disposition'])
+        self.assertEqual('text/plain; charset="utf-8"', body['Content-Type'])
+        self.assertEqual(
+            logger.get_summary_contents(), body.get_payload(decode=True))
+        self.assertIsInstance(attachment, MIMEApplication)
+        self.assertEqual('application/x-gzip', attachment['Content-Type'])
+        self.assertEqual(
+            'attachment;',
+            attachment['Content-Disposition'][:len('attachment;')])
+        self.assertEqual(
+            logger.get_full_log_contents(),
+            gunzip_data(attachment.get_payload().decode('base64')))
+
+
+class TestEC2Runner(TestCaseWithTransport, RequestHelpers):
+
+    def make_ec2runner(self, emails=None, email_log=None):
+        if email_log is None:
+            email_log = []
+        smtp_connection = LoggingSMTPConnection(email_log)
+        return EC2Runner(
+            False, "who-cares.pid", False, smtp_connection, emails=emails)
+
+    def test_run(self):
+        calls = []
+        runner = self.make_ec2runner()
+        runner.run(
+            "boring test method",
+            lambda *a, **kw: calls.append((a, kw)), "foo", "bar", baz="qux")
+        self.assertEqual([(("foo", "bar"), {'baz': 'qux'})], calls)
+
+    def test_email_on_failure_no_emails(self):
+        # If no emails are specified, then no email is sent on failure.
+        log = []
+        runner = self.make_ec2runner(email_log=log)
+        self.assertRaises(
+            ZeroDivisionError, runner.run, "failing method", lambda: 1/0)
+        self.assertEqual([], log)
+
+    def test_email_on_failure_some_emails(self):
+        # If no emails are specified, then no email is sent on failure.
+        log = []
+        runner = self.make_ec2runner(
+            email_log=log, emails=["[e-mail]"])
+        self.assertRaises(
+            ZeroDivisionError, runner.run, "failing method", lambda: 1/0)
+        # XXX: Expect this to fail. Fix the test to be more correct.
+        [message] = log
+        self.assertEqual('failing method FAILED', message['Subject'])
+        self.assertEqual('[e-mail]', message['To'])
+        self.assertIn('ZeroDivisionError', str(message))
+
+    def test_email_with_launchpad_tester_failure(self):
+        email_log = []
+        to_emails = ['[e-mail]']
+        request = self.make_request(emails=to_emails, emails_sent=email_log)
+        logger = self.make_logger(request=request)
+        tester = self.make_tester(logger=logger)
+        # Deliberately break 'tester'.  A likely failure in production is not
+        # being able to spawn the child process.
+        tester._spawn_test_process = lambda: 1/0
+        runner = self.make_ec2runner(emails=to_emails, email_log=email_log)
+        runner.run("launchpad tester", tester.test)
+        # The primary thing we care about is that email *was* sent.
+        self.assertNotEqual([], email_log)
+        [tester_msg] = email_log
+        self.assertEqual('[e-mail]', tester_msg['To'])
+        self.assertIn('ZeroDivisionError', str(tester_msg))
+
 
 class TestDaemonizationInteraction(TestCaseWithTransport, RequestHelpers):
 
@@ -662,59 +768,69 @@
         return email.get_payload()[0].get_payload()
 
     def test_success_no_emails(self):
-        request = self.make_request(emails=[])
+        log = []
+        request = self.make_request(emails=[], emails_sent=log)
         logger = self.make_logger(request=request)
         logger.got_result(True)
-        self.assertEqual([], request.emails_sent)
+        self.assertEqual([], log)
 
     def test_failure_no_emails(self):
-        request = self.make_request(emails=[])
+        log = []
+        request = self.make_request(emails=[], emails_sent=log)
         logger = self.make_logger(request=request)
         logger.got_result(False)
-        self.assertEqual([], request.emails_sent)
+        self.assertEqual([], log)
 
     def test_submits_to_pqm_on_success(self):
+        log = []
         message = {'Subject': 'foo'}
-        request = self.make_request(emails=[], pqm_message=message)
+        request = self.make_request(
+            emails=[], pqm_message=message, emails_sent=log)
         logger = self.make_logger(request=request)
         logger.got_result(True)
-        self.assertEqual([message], request.emails_sent)
+        self.assertEqual([message], log)
 
     def test_records_pqm_submission_in_email(self):
+        log = []
         message = {'Subject': 'foo'}
         request = self.make_request(
-            emails=['[e-mail]'], pqm_message=message)
+            emails=['[e-mail]'], pqm_message=message, emails_sent=log)
         logger = self.make_logger(request=request)
         logger.got_result(True)
-        [pqm_message, user_message] = request.emails_sent
+        [pqm_message, user_message] = log
         self.assertEqual(message, pqm_message)
         self.assertIn(
             'SUBMITTED TO PQM:\n%s' % (message['Subject'],),
             self.get_body_text(user_message))
 
     def test_doesnt_submit_to_pqm_no_failure(self):
+        log = []
         message = {'Subject': 'foo'}
-        request = self.make_request(emails=[], pqm_message=message)
+        request = self.make_request(
+            emails=[], pqm_message=message, emails_sent=log)
         logger = self.make_logger(request=request)
         logger.got_result(False)
-        self.assertEqual([], request.emails_sent)
+        self.assertEqual([], log)
 
     def test_records_non_pqm_submission_in_email(self):
+        log = []
         message = {'Subject': 'foo'}
         request = self.make_request(
-            emails=['[e-mail]'], pqm_message=message)
+            emails=['[e-mail]'], pqm_message=message, emails_sent=log)
         logger = self.make_logger(request=request)
         logger.got_result(False)
-        [user_message] = request.emails_sent
+        [user_message] = log
         self.assertIn(
             '**NOT** submitted to PQM:\n%s' % (message['Subject'],),
             self.get_body_text(user_message))
 
     def test_email_refers_to_attached_log(self):
-        request = self.make_request(emails=['[e-mail]'])
+        log = []
+        request = self.make_request(
+            emails=['[e-mail]'], emails_sent=log)
         logger = self.make_logger(request=request)
         logger.got_result(False)
-        [user_message] = request.emails_sent
+        [user_message] = log
         self.assertIn(
             '(See the attached file for the complete log)\n',
             self.get_body_text(user_message))
@@ -726,23 +842,27 @@
         # now and can vary independently from the contents of the sent
         # email. We probably just want the contents of the email to be a list
         # of failing tests.
-        request = self.make_request(emails=['[e-mail]'])
+        log = []
+        request = self.make_request(
+            emails=['[e-mail]'], emails_sent=log)
         logger = self.make_logger(request=request)
         logger.get_summary_stream().write('bar\nbaz\nqux\n')
         logger.got_result(False)
-        [user_message] = request.emails_sent
+        [user_message] = log
         self.assertEqual(
             'bar\nbaz\nqux\n\n(See the attached file for the complete log)\n',
             self.get_body_text(user_message))
 
     def test_gzip_of_full_log_attached(self):
         # The full log is attached to the email.
-        request = self.make_request(emails=['[e-mail]'])
+        log = []
+        request = self.make_request(
+            emails=['[e-mail]'], emails_sent=log)
         logger = self.make_logger(request=request)
         logger.got_line("output from test process\n")
         logger.got_line("more output\n")
         logger.got_result(False)
-        [user_message] = request.emails_sent
+        [user_message] = log
         [body, attachment] = user_message.get_payload()
         self.assertEqual('application/x-gzip', attachment['Content-Type'])
         self.assertEqual(