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. > === 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. > 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. An alternative for all the logs is to open them line buffered, assuming that works as advertised.