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

Hi,

A few things:

40 - '-v', '--verbose', required=False, default=False, action='count',
41 - help="Show autopilot log messages. Set twice to also log data "
42 - "useful for debugging autopilot itself.")
43 + "-v", "--verbosity", action='count', default=0, required=False,
44 + help="Increase verbosity of test details and 'text' mode output. "
45 + "Set twice to also log data useful for debugging autopilot "
46 + "itself.")

This is incorrect, since this is for the vis tool. The original message was correct, I don't think you need to change it at all.

54 - '-v', '--verbose', required=False, default=False, action='count',
55 - help="Show autopilot log messages. Set twice to also log data useful "
56 - "for debugging autopilot itself.")
57 + "-v", "--verbosity", action='count', default=0, required=False,
58 + help="Increase verbosity of test details and 'text' mode output. "
59 + "Set twice to also log data useful for debugging autopilot "
60 + "itself.")

Same here - this is for the launch command.

A small thing, but this:

85 -# Copyright (C) 2012-2013 Canonical
86 +# Copyright (C) 2012,2013,2014 Canonical

Should be "2012-2014", not "2012,2013,2014", as per legal advice.

102 from autopilot import get_version_string, parse_arguments
103 -import autopilot.globals
104 from autopilot._debug import get_all_debug_profiles
105 -from autopilot.testresult import get_output_formats
106 -from autopilot.utilities import DebugLogFilter, LogFormatter
107 from autopilot.application._launcher import (
108 _get_app_env_from_string_hint,
109 get_application_launcher_wrapper,
110 launch_process,
111 )
112 +import autopilot.globals
113 +from autopilot.testresult import get_output_formats
114 +from autopilot.utilities import DebugLogFilter, LogFormatter

This isn't the standard we've set in the AP codebase. We mix 'import FOO' and 'from FOO import BAR' statements in one block, and alphabeticise the module names.

565 + def test_failfast_text_mode(self):

Please move the test_failfast test to a new class, and use scenarios to make sure that *all* test result formats support failfast, not just text and status.

Here's the big change I think we need to make:

Currently, the logger is set up to log to the same stream as the result object. This works for the text format, but not the others. Instead, we should make it so the test log is added to the test result as a detail, for every format. This will help us with the subunit format, for example. TBH, I thought this already happend, but I can't see the code now. Feel free to point it out to me :)

Cheers