Code review comment for lp:~jml/launchpad/subunit-by-default

On Tue, Feb 2, 2010 at 2:01 PM, Gavin Panella
<email address hidden> wrote:
> Review: Approve
> Looks good. I have a couple of questions/suggestions. I'm going to
> borrow SummaryResult if that's okay, for my very-long-in-the-tooth-
> but-still-going parallelization branch.
>

It's fine. It'll be in trunk, of course, by the time you get there :)

>
>> === modified file 'lib/devscripts/ec2test/ec2test-remote.py'
>> --- lib/devscripts/ec2test/ec2test-remote.py  2009-10-09 15:04:24 +0000
>> +++ lib/devscripts/ec2test/ec2test-remote.py  2010-02-02 13:45:04 +0000
>> @@ -106,80 +129,82 @@
>>          call = self.build_test_command()
>>
>>          try:
>> +            popen = subprocess.Popen(
>> +                call, bufsize=-1,
>> +                stdout=subprocess.PIPE, stderr=subprocess.STDOUT,
>> +                cwd=self.test_dir)
>> +
>> +            self._gather_test_output(popen.stdout, summary_file, out_file)
>> +
>> +            # Grab the testrunner exit status
>> +            result = popen.wait()
>> +
>> +            if self.pqm_message is not None:
>> +                subject = self.pqm_message.get('Subject')
>> +                if result:
>> +                    # failure
>> +                    summary_file.write(
>> +                        '\n\n**NOT** submitted to PQM:\n%s\n' % (subject,))
>> +                else:
>> +                    # success
>> +                    conn = bzrlib.smtp_connection.SMTPConnection(config)
>> +                    conn.send_email(self.pqm_message)
>> +                    summary_file.write(
>> +                        '\n\nSUBMITTED TO PQM:\n%s\n' % (subject,))
>> +        except:
>> +            summary_file.write('\n\nERROR IN TESTRUNNER\n\n')
>> +            traceback.print_exc(file=summary_file)
>> +            result = 1
>> +            raise
>> +        finally:
>> +            # It probably isn't safe to close the log files ourselves,
>> +            # since someone else might try to write to them later.
>>              try:
>> -                try:
>> -                    popen = subprocess.Popen(
>> -                        call, bufsize=-1,
>> -                        stdout=subprocess.PIPE, stderr=subprocess.STDOUT,
>> -                        cwd=self.test_dir)
>> -
>> -                    self._gather_test_output(popen, summary_file, out_file)
>> -
>> -                    # Grab the testrunner exit status
>> -                    result = popen.wait()
>> -
>> -                    if self.pqm_message is not None:
>> -                        subject = self.pqm_message.get('Subject')
>> -                        if result:
>> -                            # failure
>> -                            summary_file.write(
>> -                                '\n\n**NOT** submitted to PQM:\n%s\n' %
>> -                                (subject,))
>> -                        else:
>> -                            # success
>> -                            conn = bzrlib.smtp_connection.SMTPConnection(
>> -                                config)
>> -                            conn.send_email(self.pqm_message)
>> -                            summary_file.write('\n\nSUBMITTED TO PQM:\n%s\n' %
>> -                                               (subject,))
>> -                except:
>> -                    summary_file.write('\n\nERROR IN TESTRUNNER\n\n')
>> -                    traceback.print_exc(file=summary_file)
>> -                    result = 1
>> -                    raise
>> -            finally:
>> -                # It probably isn't safe to close the log files ourselves,
>> -                # since someone else might try to write to them later.
>>                  summary_file.close()
>>                  if self.email is not None:
>>                      subject = 'Test results: %s' % (
>>                          result and 'FAILURE' or 'SUCCESS')
>>                      summary_file = open(self.logger.summary_filename, 'r')
>> +                    out_file = open(self.logger.out_filename, 'r')
>>                      bzrlib.email_message.EmailMessage.send(
>>                          config, self.email[0], self.email,
>> -                        subject, summary_file.read())
>> +                        subject, summary_file.read(),
>> +                        attachment=out_file.read(),
>> +                        attachment_filename='%s.log' % self.get_nick(),
>> +                        attachment_mime_subtype='subunit')
>
> Do you know if this displays in most browsers and/or email clients?
> Could it be worth sticking to text/plain for usabiliity? Or maybe
> there are greater benefits from correctness.
>

It works in GMail, and I reckon will work in most clients.

>>                      summary_file.close()
>> -        finally:
>> -            # we do this at the end because this is a trigger to ec2test.py
>> -            # back at home that it is OK to kill the process and take control
>> -            # itself, if it wants to.
>> -            out_file.close()
>> -            self.logger.close_logs()
>> -
>> -    def _gather_test_output(self, test_process, summary_file, out_file):
>> +            finally:
>> +                # we do this at the end because this is a trigger to
>> +                # ec2test.py back at home that it is OK to kill the process
>> +                # and take control itself, if it wants to.
>> +                out_file.close()
>> +                self.logger.close_logs()
>> +
>> +    def get_nick(self):
>> +        """Return the nick name of the branch that we are testing."""
>> +        return self.public_branch.strip('/').split('/')[-1]
>> +
>> +    def _gather_test_output(self, input_stream, summary_file, out_file):
>>          """Write the testrunner output to the logs."""
>> +        # XXX: JonathanLange 2010-01-23: This is evil. Hack the sys.path so
>> +        # that the 'lib' directory is included so that we can import subunit,
>> +        # then unhack it.
>> +        libs = os.path.join(os.path.dirname(__file__), 'test', 'lib')
>> +        sys.path.append(libs)
>> +        import subunit
>> +        sys.path.pop()
>>          # Only write to stdout if we are running as the foreground process.
>>          echo_to_stdout = not self.daemonized
>> -
>> -        last_line = ''
>> -        while 1:
>> -            data = test_process.stdout.read(256)
>> -            if data:
>> -                out_file.write(data)
>> -                out_file.flush()
>> -                if echo_to_stdout:
>> -                    sys.stdout.write(data)
>> -                    sys.stdout.flush()
>> -                lines = data.split('\n')
>> -                lines[0] = last_line + lines[0]
>> -                last_line = lines.pop()
>> -                for line in lines:
>> -                    if not self.ignore_line(line):
>> -                        summary_file.write(line + '\n')
>> -                summary_file.flush()
>> -            else:
>> -                summary_file.write(last_line)
>> -                break
>> +        result = SummaryResult(summary_file)
>> +        subunit_server = subunit.TestProtocolServer(result, summary_file)
>> +        for line in input_stream:
>> +            subunit_server.lineReceived(line)
>> +            out_file.write(line)
>> +            out_file.flush()
>> +            if echo_to_stdout:
>> +                sys.stdout.write(line)
>> +                sys.stdout.flush()
>> +            summary_file.flush()
>
> I wonder if the summary_file should be flushed by
> SummaryResult.printError() instead? It's not obvious why it's
> happening here.
>

Yeah, good call. Done.

> An alternative for all the logs is to open them line buffered,
> assuming that works as advertised.
>

How do you do that?

jml