Code review comment for lp:~jml/testrepository/show-failures-incrementally-613152

On Sun, Sep 26, 2010 at 9:46 AM, Robert Collins
<email address hidden> wrote:
> Review: Approve
> +    def _make_result(self, repo, evaluator):
> +        if self.ui.options.list:
> +            return evaluator
>
> this two things seem disconnected; perhaps rather than evaluator you should say list_result or something. I think something originally together has been split out far enough that the parameter needs a better name.

Done.

>
>
> +Wildcard = _Wildcard()
>
> perhaps
> wildcard = Wildcard()
>
> would be nicer. Many things (like str, object, etc) are lowercase for instances in Python.
>

Instances of 'type', perhaps. I chose the case to reflect None, True
and False: other singleton constants in Python.

>
> +    def _output_run(self, run_id):
>
> def _output_summary
>
>  - I think.
>

Changed.

>
> +        return ''.join([
> +            self.sep1,
> +            '%s: %s\n' % (label, test.id()),
> +            self.sep2,
> +            error_text,
> +            ])
>
> Looks like a lurking UnicodeDecodeError to me; we either need to make this unicode always or manually encode error. One way to test that would be to throw a mixed encoding, non-ascii test outcome at it.

Well, it's not a *new* lurking UnicodeDecodeError. It's equivalent to
what was there earlier.

We are always going to be getting the error_text from the base
TestResult. In this case, we are relying on testtools to store
unicode. I've changed all of the literals to be unicode to at least
communicate this more clearly.

I was going to add a test (below), but it seems to be a case of
"Doctor, it hurts when I do this!".

    def test_format_error_mixed_encoding(self):
        result = self.make_result()
        error_text = 'foo' + u'проба'.encode('utf-16')
        error = result._format_error(u'label', self, error_text)
        expected = u'%s%s: %s\n%s%s' % (
            result.sep1, u'label', self.id(), result.sep2, error_text)
        self.assertEqual(error, expected)

>
> Would you be kind enough to do these tweaks? then its definitely gtg.

Thanks,
jml