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

[Varun Gandhi via Phabricator via llvm-commits]

varungandhi-apple marked 3 inline comments as done.
varungandhi-apple added a comment.

On second thought, I'm going to update the documentation alongside the behavior change, to make sure the two are in sync.
Patch 1: Change behavior of -v. (and default and -vv behavior). Update https://llvm.org/docs/CommandGuide/lit.html as necessary.
Patch 2: Add highlighting/colored output. Update https://llvm.org/docs/CommandGuide/lit.html as necessary.
Patch 3: Rename `echo_all_commands` to `runWithCommandOutput`.



================
Comment at: llvm/utils/lit/lit/TestRunner.py:1637-1639
+    output = createScriptOutput(litConfig, script,
+                                out if out else err,
+                                status, exitCode)
----------------
yln wrote:
> varungandhi-apple wrote:
> > yln wrote:
> > > varungandhi-apple wrote:
> > > > The `out if out else err` might seem a bit strange (this was tripping me up for a while). For reasons I haven't tried digging into (perhaps it has to do with internal vs external shell), looks like sometimes the command output that's applicable is coming from stdout and sometimes it is coming from stderr. You can look at the `shtest-run-at-line.py` test case to see this in action. So I ended up going with a best effort "try this or that". Maybe we can make this more precise.
> > > > 
> > > > [Now I'm wondering if this ^ should be a TODO in the code instead of a review comment...]
> > > The code below makes me think that we should try to find the RUN line (`locateLastFailingRunLine(cmdOutput)`) in both stdout and stderr.
> > We still need to break the tie somehow though; what happens if both the stdout and stderr have `RUN: at line` in them? Is your suggestion that we try finding in one, if that fails, then try finding it in the other?
> That sounds reasonable.  How do you propose to break the tie? I would probably go stderr -> stdout since errors are more likely in the stderr?!
Ok, that makes sense. I'll make it default stderr -> stdout.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1564
+               == OutputSettings.FullScriptWithHighlighting)
+        return highlightedOutput()
+
----------------
yln wrote:
> varungandhi-apple wrote:
> > yln wrote:
> > > 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.
> > I don't understand the question. Could you rephrase? If there's no failure, there's nothing to be highlighted. Similarly, if someone is using `-a`, we are not doing any highlighting (i.e. we get `FullScript`). We only highlight when we actually have a failure and we were asked to highlight the output.
> I think we should always highlight the failure if there is one (just as we do with command output).
Hmm. I think maybe we should have way to turn off the highlighting (both for the command output and the script), in case the heuristic returns the wrong result when someone is iterating on a bug. We can discuss this more in the subsequent revisions I send after breaking this one up.


================
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
----------------
yln wrote:
> varungandhi-apple wrote:
> > yln wrote:
> > > I think it should be possible to remove most of this code by setting the defaults/destination in `add_argument()`.
> > Do we expect the order of verbosity arguments to affect what the behavior is? The current code is saying that it should not affect the behavior. If you specify `-a` anywhere, you get all the output. We do have tests doing `-a -v` which seem to be expecting all the output, so that would need to be changed as well.
> I think we should try to preserve existing behavior, if the new/other behavior isn't a clear improvement.  Here, I don't think the order should matter.
> 
> I just meant that this block of code could probably be simpler if we use the appropriate arpgarse features.
> I think we should try to preserve existing behavior, if the new/other behavior isn't a clear improvement. Here, I don't think the order should matter.
>
> I just meant that this block of code could probably be simpler if we use the appropriate arpgarse features.

The problem with using the argparse features is that the mutation effect (for setting the value for the destination) will take place when the argument is parsed (which I'm guessing will happen left-to-right). Hence the final value will be different if the arguments appear in a different order.


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

https://reviews.llvm.org/D80694