Code review comment for lp:~jml/zope.testing/subunit-output-formatter

> Broad issues:
...
> As subunit has a progress abstraction, the 'cannot support progress' statement
> confused me. Perhaps say 'cannot support zopes concept of progress because
> xxx'.
>

Changed to:
    # subunit output is designed for computers, so displaying a progress bar
    # isn't helpful.

Which isn't _strictly_ true, since displaying a progress bar might help the humans running the process. The real reason is it's too hard.

> I see you've worked around the bug in subunit where there isn't a tag method
> on the test result; perhaps you could submit a patch for that ? the contract
> is(I think) clear, just 'not done'.
>

...

> 194 + Since subunit is a stream protocol format, it has no summary.
> perhaps 'no need for a summary - when the stream is displayed a summary can be
> created then.
...

Now says:
        Since subunit is a stream protocol format, it has no need for a
        summary. When the stream is finished other tools can generate a
        summary if so desired.

> 235 + def _exc_info_to_details(self, exc_info):
> 236 + """Translate 'exc_info' into a details dictionary usable with
> subunit.
> 237 + """
> 238 + import subunit
> 239 + content_type = subunit.content_type.ContentType(
> 240 + 'text', 'x-traceback', dict(language='python', charset='utf8'))
> 241 + formatter = OutputFormatter(None)
> 242 + traceback = formatter.format_traceback(exc_info)
> 243 + return {
> 244 + 'traceback': subunit.content.Content(
> 245 + content_type, lambda: [traceback.encode('utf8')])}
>
> This might be better as
> import testtools.content
> test = unittest.TestCase()
> content = TracebackContent(exc_info, test)
> return {'traceback': content}
>
> unless the formatter.format_traceback(exc_info) is doing something nonobvious
> (and if it is, perhaps you should mention that. If its doing something
> nonobvious, then I suggest subclassing testtools.content.Content similarly to
> testtools.content.TracebackContent.
>

It's the non-obvious thing. Added this comment:
        # In an ideal world, we'd use the pre-bundled 'TracebackContent' class
        # from testtools. However, 'OutputFormatter' contains special logic to
        # handle errors from doctests, so we have to use that and manually
        # create an object equivalent to an instance of 'TracebackContent'.

> Also, you might want a global import rather than a scope local.
>

Yeah, good point.

I've changed it to be a global import. Note that it's still a soft dependency.

>
> 270 + # XXX: Since the subunit stream is designed for machine reading, we
> 271 + # should really dump the binary profiler stats here. Sadly, the
> 272 + # "content" API doesn't support this yet. Instead, we dump the
> 273 + # stringified version of the stats dict, which is functionally the
> 274 + # same thing. -- jml, 2010-02-14.
> 275 + plain_text = subunit.content_type.ContentType(
> 276 + 'text', 'plain', {'charset': 'utf8'})
> 277 + details = {
> 278 + 'profiler-stats': subunit.content.Content(
> 279 + plain_text, lambda: [unicode(stats.stats).encode('utf8')])}
>
> meta: where some dependency is insufficient, it might be nice to file a bug
> saying 'please provide X', and then reference the bug in this patch. That way,
> when your later self returns, they have something to prompt the memory.

Good point.

> That said, I'm not sure what subunit is missing here:
> (see application/octet-stream in http://www.rfc-editor.org/rfc/rfc2046.txt for
> details)
> cprofile_type = testtools.content_type.ContentType('application', 'octet-
> stream', {'type':'cProfile'})
> content = testtools.content.Content(cprofile_type, lambda: [bpickle(stats)])
> return {'profiler-stats': content}
>

It's not missing anything. I've changed this code.

> You can also make the content types attributes on self to avoid calculating
> them every time; they are 'Value Objects'.
>

Changed.

> 287 +
> 288 + Simply not supported by the subunit formatter. Fancy summary output
> 289 + doesn't make sense.
>
> I agree, but perhaps the 'summaries are not needed' is a little clearer.
>

I gave it a spin and didn't like it. Leaving the text as-is for now.

>
> I think its great you've written docs/tests for this. I worry that they will
> be fragile because they encode the subunit stream representation, but thats
> not what you wrote: you wrote some object calls against subunit. I suggest you
> use the guts from things like subunit-ls, subunit2pyunit, subunit-stats to
> make your tests be done at the same level as the code wrote: to the object
> API. This will prevent you having to e.g. include multipart MIME or http
> chunking in the tests. Specifically I suspect you are really writing smoke
> tests, and just a stats output will do great for your needs [most of the
> time].
>

I suspect you are right. However, at this point I can't be bothered making that change. :)

> This is looking really comprehensive, can't wait to see it land.

Thanks for the review!

jml