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

Mon Aug 31 11:55:43 PDT 2020

yln added inline comments.

================
Comment at: llvm/docs/CommandGuide/lit.rst:102
+
+ Alias for ``-v``/``--verbose`` (for backwards compatibility).
 .. option:: -a, --show-all
----------------
blank line

================
Comment at: llvm/utils/lit/lit/OutputSettings.py:34
+ONLY_FAILING_COMMAND = CommandOutputStyle("OnlyFailing")
+UP_TO_AND_INCLUDING_FAILING_COMMAND = CommandOutputStyle("UpToAndIncluding")
+
----------------
varungandhi-apple wrote:
> yln wrote:
> > 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?  
> Not sure what you mean by two cases, I think there are three cases:
> 
> 1. Quiet -> Corresponds on `NO_COMMAND`
> 2. Default (no quiet, no verbose) -> Corresponds to `ONLY_FAILING`.
> 3. Verbose -> Corresponds to `UP_TO_AND_INCLUDING_FAILING`.
> 
> Since there is a 1-1 correspondence, we _could_ compare quiet vs (!quiet && !verbose) vs verbose in `make_command_output`. I chose not to do that because I figured this makes the intent clearer by distinguishing between user-specified options and derived options.
> 
> Does that make sense? Would you still prefer that I get rid of this?
Okay, I agree now that having an enum-like thing is really nice because we can give it a good explanatory name.

Can we roll the following options into one enum-like `OutputStyle.XXX` corresponding to our 3 cases: "quiet, default/only_failing, verbose/up_to_first_failing"; since the user's can't configure them individually.

I imagine the following things
`opts.show_output_on_failure`, `opts.script_output_style`, `opts.command_output_style`, and `opts.quiet` being replaced with `opts.output_style`

================
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.
----------------
varungandhi-apple wrote:
> yln wrote:
> > Why use a complicated parser-like return value? Our only caller below could just receive the potentially found RUN line.
> Sorry, it's not super clear because in this revision, there is only one caller, which could do with just using the index. However, the highlighting patch (which comes right after this logically) ends up making use of the substring. The line of reasoning is:
> 
> 1. A substring by itself is not good enough (and hence `make_command_output` in this patch makes use of the first index return value), because we want to distinguish the case `line_start == 0` from the case `line_start > 0` by printing `Command Output (stderr)` instead of `Command Output (stderr, truncated)` when `line_start == 0`.
> 2. An index by itself could be "good enough", but we try to be smarter in the highlighting patch (https://reviews.llvm.org/D82811) by not highlighting the shell-specific prefix, and starting highlighting from the "RUN", which `make_script_output` makes use of.
> 
> Does that make sense? I decided against splitting changes to this function into two revisions and have the full functionality in the first iteration because I felt like that would create more churn with the tests and the implementation, but I see how it can be confusing as to why two related-but-slightly-different-values are being returned simultaneously when only one is really being used in this patch.
Yes, please implement it in the "simplest way possible" for the current functionality without anticipating future requirements.  We can always make it more complex if required for a future feature, but's it's harder to "see" that there is opportunity for simplification once the code is there.

Also, another reviewer might suggest a solution you haven't thought about.

For example, could highlighting be implemented via (without worrying about indexes)?
```
s = get_important_bits(full_output)
s_highlighted = '<highlight>{}</highlight>'.format(s)
highlighted_output = full_output.replace(s, s_highlighted)
```

================
Comment at: llvm/utils/lit/lit/TestRunner.py:1593
+    assert(lit_config.script_output_style == OutputSettings.FULL_SCRIPT)
+    return default_output()
+
----------------
varungandhi-apple wrote:
> yln wrote:
> > 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 ...
> > ```
> It mimics the structure of the `make_command_output` function right below. Right now, this function is very simple, so it probably seems overkill. However, `make_script_output` becomes much more complicated in the highlighting patch after this: https://reviews.llvm.org/D82811, so I figured it would be helpful to have the two functions `make_script_output` and `make_command_output` follow a similar pattern.
I like symmetry in code, but please simplify this as much as possible.

================
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)

----------------
yln wrote:
> `opts.echo_all_commands = opts.command_output_style in {OutputSettings.UP_TO_AND_INCLUDING_FAILING_COMMAND, ...}`
Or even simpler (the 1 out of 3 cases that doesn't set it to true)?

`opts.echo_all_commands = opts.output_style != OutputStyle.QUIET`

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82791