[PATCH] D82791: [lit] Improve lit's output with default settings and --verbose.

[Julian Lettner via Phabricator via cfe-commits]

yln added a comment.

The changes here are for text printed for failing tests, right?
And `showAllOutput` is orthogonal and just shows everything even for non-failing tests?



================
Comment at: llvm/utils/lit/lit/OutputSettings.py:34
+ONLY_FAILING_COMMAND = CommandOutputStyle("OnlyFailing")
+UP_TO_AND_INCLUDING_FAILING_COMMAND = CommandOutputStyle("UpToAndIncluding")
+
----------------
I really like the "communicating intent" part of this infrastructure.  However, this is a lot of code and `if`s considering we really only have to distinguish two cases.  From your commit message:

- default (no flags): no script, show only failing line in command output
- `-v`: full script, adds 'set +x', shows command output

Am I missing something or could everything be keyed off a single verbose (reading from the code below `showAllOutput` implies verbose) flag.  Do we anticipate other combinations being useful? (In general I would argue for striving for the simplest implementation that gives us what we currently want and not try to anticipate future extensions.)

Have you considered (or started out with) just using a single verbose flag to base your decisions in the implementation functions?  


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1499
 
+def locate_last_run_line(output_str):
+    """
----------------
I feel like this function is the most complex part of this patch.  I don't fully follow it, but you experimented and believe this is the best approach and wrote a whole test suite so I am happy with it.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1503-1508
+    Returns a pair of:
+    - The index in ``output_str`` pointing to immediately after the preceding
+      newline, i.e. the start of the RUN line, before any shell-specific
+      prefix.
+    - The matched substring itself, including the number at the end,
+      starting with 'RUN', skipping the shell-specific prefix.
----------------
Why use a complicated parser-like return value? Our only caller below could just receive the potentially found RUN line.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1593
+    assert(lit_config.script_output_style == OutputSettings.FULL_SCRIPT)
+    return default_output()
+
----------------
Why are we using two local functions here?

The whole thing could be (already assuming just one verbose flag):

```
def make_script_output(lit_config, script_lines, exit_code):
    if not lit_config.verbose:
        return ""
    return ...
```


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1614-1617
+    line_start, run_line_str = locate_last_run_line(cmd_output)
+    # maybe there was an error, or maybe we are not truncating anything
+    if run_line_str is None or line_start == 0:
+        return default_output()
----------------
This block should be pushed into the `lit_config.command_output_style == OutputSettings.ONLY_FAILING_COMMAND` case, otherwise we are always searching for the last line, even if we don't really use the result of the computation.

Also: if we fail to find the RUN line, we might print the note to use `--verbose` even if the user already specified it.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1624
+           == OutputSettings.UP_TO_AND_INCLUDING_FAILING_COMMAND)
+    return make_output(cmd_output, is_truncated=False)
 
----------------
Please try to simplify this a bit, maybe the following?

```
def make_command_output():
    if not lit_config.quiet:
        return ""
    
    verbose = lit_config.verbose
    output = "header {} ...".format(", truncated" if verbose else "")
    if verbose:
        output += cmd_output
    else:
        run_line = locate_last_run_line() # then deal with "not found" case
        output += ...
        output += "Note:  try to use --verbose"

     return output
```


================
Comment at: llvm/utils/lit/lit/cl_arguments.py:56
             action="store_true")
+    # TODO(python3): Use aliases kwarg for add_argument above.
     format_group.add_argument("-vv", "--echo-all-commands",
----------------
I think aliases kwarg is something else (seems to be related to subparsers).  You could just add `"-vv", "--echo-all-commands"` after `"--verbose"` to allow for additional names for the flag, but I think I prefer to have it separate (makes it easer to remove it if we ever decide to drop the alias in the future).

So long comment short: just drop this comment please.


================
Comment at: llvm/utils/lit/lit/cl_arguments.py:178
+    opts.command_output_style = OutputSettings.ONLY_FAILING_COMMAND
+    opts.echo_all_commands = True
+
----------------
Unconditionally overwritten below


================
Comment at: llvm/utils/lit/lit/cl_arguments.py:195
+        cmd_output_style == OutputSettings.UP_TO_AND_INCLUDING_FAILING_COMMAND
+        or cmd_output_style == OutputSettings.ONLY_FAILING_COMMAND)
 
----------------
`opts.echo_all_commands = opts.command_output_style in {OutputSettings.UP_TO_AND_INCLUDING_FAILING_COMMAND, ...}`


================
Comment at: llvm/utils/lit/tests/shtest-run-at-line.py:70
+
+################################################################################
+# Checking lines for verbose output
----------------
I really like the extension to this test.  Thanks!


================
Comment at: llvm/utils/lit/tests/unittest-failing-locator.py:122
+
+unittest.main()
----------------
Thanks for being diligent and adding these tests.  This will also make it easy to add additional tests for corner cases if we need them in the future!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82791