Code review comment for lp:~jameinel/bzr/2.3-filter-tests

On 22/09/2010, John Arbash Meinel <email address hidden> wrote:
>>
>> + def _report_skip(self, result, e):
>>
>> Rather than adding this, put the call in _do_skip instead?
>
> IIRC, there was a reason to put it in _report_skip. I don't remember
> exactly, but I think it was that _report_skip gets called for all code
> paths, but _do_skip doesn't. (NotAvailable, vs Skip, vs XFAIL?)

I think this is true for fallbacks, but we shouldn't be using any? If
it does need doing this way explaining why in the docstring would be
good.

>> ExtendedTestResult.addSuccess appears to have a similar hack spelt in a
>> different way:
>>
>> test._log_contents = ''
>>
>> Remove that and add your new version instead?
>
> Given that that one doesn't actually seem to work, I guess that would be
> reasonable.

Removing the log from successes would be the biggest win as you've noted.

>> Would really like some kind of test.
>
> I think I can work something out in test_selftest, about what details
> are present in the final result. I'll give it a poke and see what I get.
>
> I'd certainly also like to suppress the log details for success cases.
>
> John
> =:->