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 15:55 < noodles775> jml: ah, you just moved that code... I'd not realised. But still worth checking. 15:55 < noodles775> salgado: np. 15:56 < jml> noodles775, yeah, deleting that bit works. 15:57 < noodles775> jml: OK, the other option would be to make it a required arg (which I guess was the original intent?) 16:02 < jml> noodles775, well, it's a required arg of the constructor, but most tests just don't care about it. 16:02 < noodles775> k 16:02 < jml> noodles775, so I'd rather leave it optional in the make_foo() method 16:02 < noodles775> Great. > @@ -611,6 +630,93 @@ > "\n\nERROR IN TESTRUNNER\n\n%s" % (stack,), [snip] > + > +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) Yikes, erm, I'm impressed, but think that needs a comment (or replacing it with an inline function. It makes sense after spending 20seconds parsing it. > + > + 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. Copy-n-paste error? ^^^ > + log = [] > + runner = self.make_ec2runner( > + email_log=log, emails=["