Code review comment for lp:~jml/launchpad/one-email-on-crash

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=["<email address hidden>"])
> + 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('<email address hidden>', message['To'])
> + self.assertIn('ZeroDivisionError', str(message))
> +
> + def test_email_with_launchpad_tester_failure(self):

           # LaunchpadTester sends email on failure.

> + email_log = []
> + to_emails = ['<email address hidden>']
> + 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

Nice!