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

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 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.

Changed according to our discussion.

>> +
>> +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.
>

I've defined an inline function and added a test comment. It now reads:

    def test_run(self):
        # EC2Runner.run() runs the given function, passing through whatever
        # arguments and keyword arguments it has been given.
        calls = []
        runner = self.make_ec2runner()
        def function(*args, **kwargs):
            calls.append(args, kwargs)
        runner.run("boring test method", function, "foo", "bar", baz="qux")
        self.assertEqual([(("foo", "bar"), {'baz': 'qux'})], calls)

Still a bit of a mouthful, but that's Python for you.

>> +    def test_email_on_failure_some_emails(self):
>> +        # If no emails are specified, then no email is sent on failure.
>
> Copy-n-paste error? ^^^
>

Fixed. Thanks.

>> +    def test_email_with_launchpad_tester_failure(self):
>
>           # LaunchpadTester sends email on failure.
>

Added.

Thanks for the review.
jml