Code review comment for lp:~mbp/bzr/test-errors

What was the failure from PQM for this?

I'm a little scared by some of these changes, though I think overall it's moving in the right direction.

+ self.repeat_failures_at_end = False

My understanding of the proposed flag was it would be either during *or* at the end, never both.

+class BaseTextTestResult(ExtendedTestResult):

Why a new class rather than just putting the method you want to share on ExtendedTestResult?

+ u'ERROR: %s\n' % self._test_description(test))

Using u"" ascii literals is well intentioned but isn't really useful. This is something of a common misconception. All it amounts to is asking for an error from this interpolation rather than elsewhere in the stack. Having the file object reject non-ascii bytestrings is a better choice.

     def _finishLogFile(self):
...
+ log_contents = self.get_log()

Moving this logic scares me. Robert did a lot of complicated coding to make sure the log was available from test initialisation rather than only at the end, which is partly why this code is still strange. If we really don't want the log until _finishLogFile is called, most of the code can be removed and replaced with a single addDetails call here.

+ if type(codec) is tuple:
+ # Python 2.4
+ encode = codec[0]

This code being copied out to a helper method can lose the compat stuff now.

+ # This isn't an expected failure in bzr, because there's nothing
+ # reasonable we can do about old buggy subunits.

This was an expected failure because we want to depend on a minimum subunit version to avoid failures, and remove the need for the hack in the test, see bug 531667.