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

Broad issues:
content and content_type are testtools modules; don't import from subunit, it only has them for compatibility glue.

Perhaps tag tests in layer foo with zope:layer:foo, not zope:testing:foo. In fact it looks like they are, and its simply a docstring bug to claim otherwise.

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

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.

What is this?
+ def import_errors(self, import_errors):
221 + """Report test-module import errors (if any)."""
222 + # FIXME: Implement this.
... there is code here

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.

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

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.
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}

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

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 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].

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