[PATCH] D80694: Improve lit.py's usability by highlighting failing lines.

[Julian Lettner via Phabricator via llvm-commits]

yln added a comment.

I think this is a good improvement for the usability of lit.  Thanks for working on this!

> - default (no flags): no script, show only failing line in command output
> - -v: full script, highlight failing line in script, shows command output up to and including failing line, highlight failing output
> - -vv: alias to -v (preserve backwards compatibility)

Quick question: We show the command output up to and including the failing line.  Should we do the same for the script part, i.e., skip the un-executed part of the script?  Is it ever useful to know which script line/command would have been executed next?

Linux/Windows/environments without support for non-printable characters:
Did you try this out in an environment with a "dumb" console, e.g., build bot? Ideally, someone helps us test this on Linux and Windows before landing.

Please also update these docs: https://llvm.org/docs/CommandGuide/lit.html
I am okay with doing this in a separate patch.

> Is it ok to rename echo_all_commands to runWithCommandOutput? I think the latter represents the meaning better (since the behavior is no longer controlled by the --echo-all-commands flag), but that might break compatibility for out-of-tree users (should be a small fix though).

I am in favor of this.  Would it be possible to break the rename out into a separate patch to reduce the size of this one?  Also should make it easer for downstream users to adopt/or in case we have to revert the rename temporarily.

> Should TerminalController be split out into a different file?

I am okay with the current state.  Let's move it to its own file in case we need to extend/touch it.

In D80694#2060326 <https://reviews.llvm.org/D80694#2060326>, @varungandhi-apple wrote:

> Ah, I just realized there are some inconsistent variable names, some are snake_case, some are camelCase. Should I be consistently using camelCase? I did see a bunch of snake_case variables, that's probably what tripped me up...


Yes, the mixture/mess is unfortunate.  :(

I don't think we have a documented style guide (as we do for LLVM C++ code), but for new code I think we should follow official Python style: https://www.python.org/dev/peps/pep-0008/
This means snake_case for variable and function names.  I use a linter to try to avoid introducing additional style issues and cleanup issues in code I touch, but usually avoid unrelated cleanups that would make my patches larger.  And of course: use your own good judgement.



================
Comment at: llvm/utils/lit/lit/LitConfig.py:30
                  parallelism_groups = {},
-                 echo_all_commands = False):
+                 runWithCommandOutput = False,
+                 scriptOutputStyle = lit.OutputSettings.NoScript,
----------------
snake_case


================
Comment at: llvm/utils/lit/lit/OutputSettings.py:3
+class ScriptOutputStyle:
+    def __init__(self, name, full):
+        self.name = name
----------------
full unused.


================
Comment at: llvm/utils/lit/lit/OutputSettings.py:7
+    def __repr__(self):
+        return '%s(%r)' % (self.__class__.__name__, self.name)
+
----------------
The name of the two enums seems to be never used (only instances are compared).


================
Comment at: llvm/utils/lit/lit/OutputSettings.py:11
+FullScript = ScriptOutputStyle("Full", True)
+FullScriptWithHighlighting = ScriptOutputStyle("FullWithHighlighting", True)
+
----------------
I don't understand why we need a separate FullScriptWithHighlighting.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1480
 
+def locateLastFailingRunLine(outputStr):
+    """
----------------
Phew, this function is pretty complicated.  Please make sure the major cases are covered by tests.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1496
+    # However, this is not perfect either :(. For example, if an external shell
+    # prepends line numbers on 'set +x', then this will fail.
+
----------------
Can we add a test + comment for the failing case so the behavior is documented.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1528
+
+def highlightFailingText(text):
+    term = TerminalController()
----------------
Please split up this patch to change the behavior of `-v` first and add the "highlighting" feature in a follow-up.  Also: do we need to highlight the failing output considering that it will always be the last thing printed?


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1564
+               == OutputSettings.FullScriptWithHighlighting)
+        return highlightedOutput()
+
----------------
Why is there a separate case for highlighted script output?  For command output with highlight (if supported).  I think we should do the same for script output.


================
Comment at: llvm/utils/lit/lit/cl_arguments.py:57
             action="store_true")
     format_group.add_argument("-vv", "--echo-all-commands",
             dest="echoAllCommands",
----------------
I think we can add the "aliases" to the above call to `add_argument()` now.


================
Comment at: llvm/utils/lit/lit/cl_arguments.py:171
 
-    # Validate command line options
-    if opts.echoAllCommands:
-        opts.showOutput = True
+    # default output settings
+    opts.showOutputOnFailure = True
----------------
I think it should be possible to remove most of this code by setting the defaults/destination in `add_argument()`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80694/new/

https://reviews.llvm.org/D80694