Code review comment for lp://staging/~coreygoldberg/autopilot/texttest-run

Revision history for this message
Corey Goldberg (coreygoldberg) wrote :

from irc with thomi:

<thomi> you should flush stdout afdter startTest, since we want to see that output in the case where a test takes a long time to run
<cgoldberg> right
<thomi> also, I think the skip line should have the optional message in '( )'
 so like:
 test.id ... SKIP (Not runnable on the device)
 for example
<cgoldberg> yup.. agree
<thomi> also, I think the status messages can be a bit more verbose.
 particularly, please change:
 XFAIL => EXPECTED FAIL
 and
 NOTOK => UNEXPECTED SUCCESS
<cgoldberg> right. makes sense. I copied the unittest runner's output, but there's no reason not to be a little more explicit
<thomi> also, you should generalise the code (diff lines 82-92) to go in a separate function, something like _wrap_result_with_output_decorator
 which accepts a result object, and wraps it in either a LoggingResultDecorator or your new decorator
 that way, it's easier to test, and we keep the complexity of the construct_XXX functions down
<thomi> cgoldberg: in your tests, I think you can make your 'assertOutput' lines more readable, by doing:
<cgoldberg> thomi, ok.. i can wrap it in a function
<thomi> self.assertOutput('{id} ... OK\n', 'pass')
 one line is better than 3, since we read left->right
 other than those minor quibbles above ^^, this looks great, but you're missing some tests still
<cgoldberg> thomi, hah.. will do
<thomi> I'd like to see an integration test that shows that when you specify the various result formats without the verbose flag, we get the correct result object & wrapper
 similarly for when you do specify the verbose flag
<cgoldberg> gotcha.. yea i can add that too
<thomi> Finally, I think you should add 6 functional tests to the functional test suite
 1 test fro each format, * 1 with verbose, and one without verbose
 actually, the verbose case is almost certainly covered already (but please do check)
 so maybe it's only 3 new functional tests :)

« Back to merge proposal