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

Varun Gandhi via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu May 28 17:38:28 PDT 2020


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

> 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?

In my experience, if there are similar lines later on, sometimes it is good to spot them (especially in CI). Not something you want often though, but I think it's good to have.

> 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.

I have only tried it locally. I was assuming that there is some CI which is running, based on seeing the "Harbormaster failed remote builds in B58171 <https://reviews.llvm.org/B58171>: Diff 266757!" message earlier. However, that seems to have failed with a 503 error which I don't know what to do with. Is there no running CI?

> 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.

I can break it up; although at this stage, it's unclear to me how many patches you'd like me to break this into, given the other comments.

>> Please also update these docs: https://llvm.org/docs/CommandGuide/lit.html
>  > I am okay with doing this in a separate patch.
> 
>> Please split up this patch to change the behavior of -v first and add the "highlighting" feature in a follow-up

Patch 1: Change behavior of `-v`. (and default and -vv behavior)
Patch 2: Add highlighting/colored output.
Patch 3: Updating https://llvm.org/docs/CommandGuide/lit.html
Patch 4: Rename `echo_all_commands` to `runWithCommandOutput`.

Can you clarify how many different patches you'd like to see and what the contents should be? Is it what I stated above?

Also, when you say "patch", do you mean different change requests (with their own reviews), or different commits but in this current change request (all in the same review)?

> 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.

Ok, that means I'll need to change all of it... I saw a bunch of recent code using camelCase, so I used camelCase. :(



================
Comment at: llvm/utils/lit/lit/OutputSettings.py:7
+    def __repr__(self):
+        return '%s(%r)' % (self.__class__.__name__, self.name)
+
----------------
yln wrote:
> The name of the two enums seems to be never used (only instances are compared).
I used it for print debugging; looks like I should've defined `__str__` instead for that.


================
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:
> > 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?


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1480
 
+def locateLastFailingRunLine(outputStr):
+    """
----------------
yln wrote:
> Phew, this function is pretty complicated.  Please make sure the major cases are covered by tests.
Do you mean adding unit tests for testing this function by itself (I don't recall seeing any unit tests in the code)? Or do you mean integration 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.
+
----------------
yln wrote:
> Can we add a test + comment for the failing case so the behavior is documented.
I will add the test. Could you elaborate on the comment part; I don't follow. What additional information should that comment describe?


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1564
+               == OutputSettings.FullScriptWithHighlighting)
+        return highlightedOutput()
+
----------------
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.


================
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:
> 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.


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

https://reviews.llvm.org/D80694





More information about the llvm-commits mailing list