Merge lp:~jml/zope.testing/subunit-output-formatter into lp:~vcs-imports/zope.testing/trunk

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

Oh, I forgot to mention; for profiling, using bzrlib.lsprof.profile you can get kcachegrind output out really easily; I love it much more than the damaged cProfile output, and it is actually plain text anyway :).

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

OK, I think this is ready for landing into trunk. Would appreciate a review and guidance through whatever hoops I must jump through to get this landed.

On Thu, 2010-03-11 at 20:31 +0000, Jonathan Lange wrote:
> > 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.

This still leaves me confused: subunit can fairly easily do a progress
bar, in-process or out-of-process. I was guessing that zopes concept of
progress was strange or something.

> > 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. :)

It's your dime ;)

Let me know of anything I can do to help.

-Rob