Improve lit.py's usability by highlighting failing lines.

[varungandhi-apple]

Summary

• Before:
  • default (no flags): no script, no command output
  • -v: full script, no command output
  • -vv: full script, shows command output up to and including failing line
• After:
  • 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)

Background discussion: http://lists.llvm.org/pipermail/llvm-dev/2019-September/135098.html
This patch doesn't follow the exact suggestion in that thread, but I think it's pretty close and arguably better.

I do have a couple of questions:

1. 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).
2. Should TerminalController be split out into a different file?

I don't feel strongly about the colors/styling. Happy to change it if there are common color schemes where it would become unreadable.

[varungandhi-apple]

Consider the test file:

# RUN: true
# RUN: false
# RUN: true

After this patch, the default output becomes:

******************** TEST 'lit :: dummy.py' FAILED ********************

Command Output (stdout, truncated):
--
$ ":" "RUN: at line 2"
$ "false"
note: command had no output on stdout or stderr
error: command failed with exit status: 1

--
(NOTE: The failure may depend on preceding RUN lines.
       Use --verbose to see preceding RUN lines and outputs.)

If you use -v/--verbose, it becomes:

[varungandhi-apple]

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

[varungandhi-apple]

Fixed some variable names, added some doc comments, cleaned up some logic, added some more CHECK lines to tests.

[JDevlieghere]

I know this patch didn't introduce this, but I'm curious what the meaning/purpose is of the ":" in the -vv output?

Command Output (stdout, truncated):
--
$ ":" "RUN: at line 2"
  ^^^

[varungandhi-apple]

@JDevlieghere, that is part of the code that is writing out the command line.

# TestRunner.py
def executeScriptInternal(test, litConfig, tmpBase, commands, cwd):
    # ... skipped
    for i,result in enumerate(results):
        # Write the command line run.
        out += '$ %s\n' % (' '.join('"%s"' % s
                                    for s in result.command.args),)

The commit which added it has the following message:

commit 591838d1d33bc0dc5d2ffc66dd22176f280cc625
Author: Daniel Dunbar <[e-mail]>
Date:   Thu Jun 2 23:49:42 2016 +0000

    [lit] Improve readability of failing scripts.

     - This only applies to scripts executed by the _internal_ shell script
       interpreter.

     - This patch reworks the log to look more like a shell transcript, and be less
       verbose (but in the interest of calling attention to the important parts).

    Here is an example of the new format, for commands with/without failures and
    with/without output:

    $ true
    $ echo hi
    hi

    $ false
    note: command had no output on stdout or stderr
    error: command failed with exit status 1

While the commit message makes it seem like the double-quotes
should not be present, the commit does add tests with double-quotes,
so I'm guessing it is intentional and not accidental. I'm guessing the
answer to your question is probably the "but in the interest of calling
attention to the important parts", which involves specifying each arg.

llvm/utils/lit/lit/TestRunner.py

1647–1650

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

[varungandhi-apple]

Hmm, Phabricator attached my inline comment to my other larger comment, that's confusing.
I'm guessing I was supposed to make an out-of-line comment paired with the inline one,
that's what the "not submitted" was about.

[yln]

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

llvm/utils/lit/lit/LitConfig.py

30

snake_case

llvm/utils/lit/lit/OutputSettings.py

3

full unused.

7

The name of the two enums seems to be never used (only instances are compared).

11

I don't understand why we need a separate FullScriptWithHighlighting.

llvm/utils/lit/lit/TestRunner.py

1480

Phew, this function is pretty complicated. Please make sure the major cases are covered by tests.

1496

Can we add a test + comment for the failing case so the behavior is documented.

1528

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?

1564

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.

llvm/utils/lit/lit/cl_arguments.py

57

I think we can add the "aliases" to the above call to add_argument() now.

171

I think it should be possible to remove most of this code by setting the defaults/destination in add_argument().

[yln]

llvm/utils/lit/lit/TestRunner.py

1647–1650

The code below makes me think that we should try to find the RUN line (locateLastFailingRunLine(cmdOutput)) in both stdout and stderr.

[varungandhi-apple]

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

llvm/utils/lit/lit/OutputSettings.py

7

I used it for print debugging; looks like I should've defined __str__ instead for that.

llvm/utils/lit/lit/TestRunner.py

1480

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?

1496

I will add the test. Could you elaborate on the comment part; I don't follow. What additional information should that comment describe?

1564

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.

1647–1650

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?

llvm/utils/lit/lit/cl_arguments.py

171

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.

[yln]

Is there no running CI?

There are bots (managed by different people) but usually lit is only tested as part of check-all. We want to avoid breaking too many people, especially because this is a feature interacting with the external shell I can see it causing problems on different platforms/environments.

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.

That sounds like a good plan.

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

Yes, different "revisions" with their own review each.

llvm/utils/lit/lit/TestRunner.py

1480

I just meant: we have 4 different returns and should make sure that all code paths are tested.

1496

I meant: add the test to "pin down" the behavior (even if it's not the optimal desired) and also put this explaining comment there.

1564

I think we should always highlight the failure if there is one (just as we do with command output).

1647–1650

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

llvm/utils/lit/lit/cl_arguments.py

171

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.

[varungandhi-apple]

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.

llvm/utils/lit/lit/TestRunner.py

1564

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.

1647–1650

Ok, that makes sense. I'll make it default stderr -> stdout.

llvm/utils/lit/lit/cl_arguments.py

171

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.

[varungandhi-apple]

I've split this patch up into four different patches. Here is the suggested review order:

1. D82808: Small, adds some documentation for -s.
2. D82791: Largish, adds the logic and test cases of finding the failing line.
3. D82811: Small, adds coloring on top of the previous one.
4. D82814: Small, renames echo_all_commands field.

I will close this revision.

llvm/utils/lit/lit/LitConfig.py

 from __future__ import absolute_import
 import inspect
 import os
 import platform
 import sys
 
 import lit.Test
 import lit.formats
 import lit.TestingConfig
+import lit.OutputSettings
 import lit.util
 
 # LitConfig must be a new style class for properties to work
 class LitConfig(object):
     """LitConfig - Configuration data for a 'lit' test runner instance, shared
     across all tests.
 
     The LitConfig object is also used to communicate with client configuration
     files, it is always passed in as the global variable 'lit' so that
     configuration files can access common functionality and internal components
     easily.
     """
 
     def __init__(self, progname, path, quiet,
                  useValgrind, valgrindLeakCheck, valgrindArgs,
                  noExecute, debug, isWindows,
                  params, config_prefix = None,
                  maxIndividualTestTime = 0,
                  parallelism_groups = {},
-                 echo_all_commands = False):
+                 runWithCommandOutput = False,
+                 scriptOutputStyle = lit.OutputSettings.NoScript,
+                 commandOutputStyle = lit.OutputSettings.OnlyFailingCommand):
         # The name of the test runner.
         self.progname = progname
         # The items to add to the PATH environment variable.
         self.path = [str(p) for p in path]
         self.quiet = bool(quiet)
         self.useValgrind = bool(useValgrind)
         self.valgrindLeakCheck = bool(valgrindLeakCheck)
         self.valgrindUserArgs = list(valgrindArgs)
def __init__(self, progname, path, quiet,
                 self.valgrindArgs.append('--leak-check=full')
             else:
                 # The default is 'summary'.
                 self.valgrindArgs.append('--leak-check=no')
             self.valgrindArgs.extend(self.valgrindUserArgs)
 
         self.maxIndividualTestTime = maxIndividualTestTime
         self.parallelism_groups = parallelism_groups
-        self.echo_all_commands = echo_all_commands
+        self.runWithCommandOutput = runWithCommandOutput
+        self.scriptOutputStyle = scriptOutputStyle
+        self.commandOutputStyle = commandOutputStyle
 
     @property
     def maxIndividualTestTime(self):
         """
             Interface for getting maximum time to spend executing
             a single test
         """
         return self._maxIndividualTestTime

llvm/utils/lit/lit/OutputSettings.py

+# TODO(python3): Could be an enum
+class ScriptOutputStyle:
+    def __init__(self, name, full):
+        self.name = name
+
+    def __repr__(self):
+        return '%s(%r)' % (self.__class__.__name__, self.name)
+
+NoScript = ScriptOutputStyle("None", False)
+FullScript = ScriptOutputStyle("Full", True)
+FullScriptWithHighlighting = ScriptOutputStyle("FullWithHighlighting", True)
+
+# Consider a script
+#
+# ```
+# RUN: true
+# RUN: false
+# RUN: true
+# ```
+#
+# FullScript will show the whole thing, without highlighting.
+# FullScriptWithHighlighting will show something like
+#
+# ```
+# Script:
+# --
+# : 'RUN: at line 1';   true
+# : 'RUN: at line 2';   false
+# **: 'RUN: at line 3';   true**
+# --
+# ```
+# (The ** indicates colored output)
+
+# TODO(python3): Could be enum
+class CommandOutputStyle:
+    def __init__(self, name):
+        self.name = name
+
+    def __repr__(self):
+        return '%s(%r)' % (self.__class__.__name__, self.name)
+
+NoCommand = CommandOutputStyle("None")
+OnlyFailingCommand = CommandOutputStyle("OnlyFailing")
+UpToAndIncludingFailingCommand = CommandOutputStyle("UpToAndIncluding")
+
+# Consider a script
+#
+# ```
+# RUN: true
+# RUN: false
+# RUN: true
+# ```
+#
+# OnlyFailingCommand will show something like
+#
+# ```
+# Command output (stderr, truncated)
+# --
+# + : 'RUN: at line 2'
+# + false
+# --
+# NOTE: The failure might depend on preceding RUN lines.
+#       Use --verbose to see preceding RUN lines and outputs.
+# ```
+#
+# UpToAndIncludingFailingCommand will show something like
+#
+# ```
+# Command Output (stderr):
+# --
+# + : 'RUN: at line 1'
+# + true
+# **+ : 'RUN: at line 2'
+# + false**
+# --
+# ```
+# (The ** indicates colored output)

llvm/utils/lit/lit/TestRunner.py

 import threading
 
 import io
 try:
     from StringIO import StringIO
 except ImportError:
     from io import StringIO
 
+from lit.ProgressBar import TerminalController
+import lit.OutputSettings as OutputSettings
 from lit.ShCommands import GlobItem, Command
 import lit.ShUtil as ShUtil
 import lit.Test as Test
 import lit.util
 from lit.util import to_bytes, to_string, to_unicode
 from lit.BooleanExpression import BooleanExpression
 
 class InternalShellError(Exception):
def executeScript(test, litConfig, tmpBase, commands, cwd):
     if litConfig.isWindows and not isWin32CMDEXE:
         mode += 'b'  # Avoid CRLFs when writing bash scripts.
     elif sys.version_info > (3,0):
         open_kwargs['encoding'] = 'utf-8'
     f = open(script, mode, **open_kwargs)
     if isWin32CMDEXE:
         for i, ln in enumerate(commands):
             commands[i] = re.sub(kPdbgRegex, "echo '\\1' > nul && ", ln)
-        if litConfig.echo_all_commands:
+        if litConfig.runWithCommandOutput:
             f.write('@echo on\n')
         else:
             f.write('@echo off\n')
         f.write('\n@if %ERRORLEVEL% NEQ 0 EXIT\n'.join(commands))
     else:
         for i, ln in enumerate(commands):
             commands[i] = re.sub(kPdbgRegex, ": '\\1'; ", ln)
         if test.config.pipefail:
             f.write(b'set -o pipefail;' if mode == 'wb' else 'set -o pipefail;')
-        if litConfig.echo_all_commands:
+        if litConfig.runWithCommandOutput:
             f.write(b'set -x;' if mode == 'wb' else 'set -x;')
         if sys.version_info > (3,0) and mode == 'wb':
             f.write(bytes('{ ' + '; } &&\n{ '.join(commands) + '; }', 'utf-8'))
         else:
             f.write('{ ' + '; } &&\n{ '.join(commands) + '; }')
     f.write(b'\n' if mode == 'wb' else '\n')
     f.close()
 
def parseIntegratedTestScript(test, additional_parsers=[],
     if not test.isWithinFeatureLimits():
         msg = ', '.join(test.config.limit_to_features)
         return lit.Test.Result(Test.UNSUPPORTED,
                                "Test does not require any of the features "
                                "specified in limit_to_features: %s" % msg)
 
     return script
 
+def locateLastFailingRunLine(outputStr):
+    """
+    Tries to locate the last run line in 'outputStr' and returns a pair of:
+    - The index immediately after the preceding newline
+      (i.e. the start of the RUN line).
+    - The matched string itself, including the number at the end,
+      starting with 'RUN' (shell-specific prefix is skipped).
+
+    Returns (-1, None) on failure.
+    """
+    # The first attempt is more complicated than simply finding the last
+    # 'RUN: at line'; the latter is more likely to lead to mistakes
+    # when the command output itself has a 'RUN: at line' due to something
+    # else (possibly because we are testing lit itself).
+    #
+    # However, this is not perfect either :(. For example, if an external shell
+    # prepends line numbers on 'set +x', then this will fail.
+
+    error = (-1, None)
+    runStr = "RUN: at line"
+    firstRunStrStart = outputStr.find(runStr)
+    if firstRunStrStart == -1:
+        return error
+    # Use the first RUN line to create a regex for searching.
+    nlIdx = outputStr[:firstRunStrStart].rfind('\n')
+    firstRunLineStart = 0 if nlIdx == -1 else nlIdx + 1
+    firstRunStrEnd = firstRunStrStart + len(runStr)
+    runLineRegex = ('(' + re.escape(outputStr[firstRunLineStart:firstRunStrEnd])
+                    + ' [0-9]+)')
+
+    def reversedMatches(regex, s):
+        return reversed(list(re.finditer(regex, s)))
+
+    for runLine in reversedMatches(runLineRegex, outputStr):
+        # trim the shell-specific prefix
+        delta = firstRunStrStart - firstRunLineStart
+        return (runLine.start(), runLine.group(0)[delta:])
+
+    # The complicated method didn't work, try something simple instead.
+    simpleRunLineRegex = '(' + re.escape(runStr)+ ' [0-9]+)'
+    for runLine in reversedMatches(simpleRunLineRegex, outputStr):
+        nlIdx = outputStr[:runLine.start()].rfind('\n')
+        runLineStart = 0 if nlIdx == -1 else nlIdx
+        return (runLineStart, runLine.group(0))
+
+    # weird, we should've at least gotten the first run line...
+    return error
+
+def highlightFailingText(text):
+    term = TerminalController()
+    return term.render('${BOLD}${RED}%s${NORMAL}' % text)
+
+def createScriptOutput(litConfig, scriptLines, cmdOutput, status, exitCode):
+    def makeOutput(outputLines):
+        return ("""Script:\n--\n%s\n--\nExit Code: %d\n"""
+                % ('\n'.join(outputLines), exitCode))
+
+    def defaultOutput():
+        return makeOutput(scriptLines)
+
+    def splitOnSubstr(strList, substr):
+        for i, s in enumerate(strList):
+            if substr in s:
+                return (strList[:i], s, strList[i+1:])
+        return (None, None, None)
+
+    def highlightedOutput():
+        _, lastRunLine = locateLastFailingRunLine(cmdOutput)
+        if lastRunLine is None:
+            return defaultOutput()
+        (pre, failingLine, post) = splitOnSubstr(scriptLines, lastRunLine)
+        if failingLine is None:
+            return defaultOutput()
+        return makeOutput(pre + [highlightFailingText(failingLine)] + post)
+
+    if litConfig.scriptOutputStyle == OutputSettings.NoScript:
+        return ""
+    elif (litConfig.scriptOutputStyle == OutputSettings.FullScript
+          or not status.isFailure
+          or (not cmdOutput)):
+        return defaultOutput()
+    else:
+        assert(litConfig.scriptOutputStyle
+               == OutputSettings.FullScriptWithHighlighting)
+        return highlightedOutput()
+
+def createCommandOutput(litConfig, cmdOutput, streamName, testStatus):
+    def makeOutput(outputStr, isTruncated=False):
+        if isTruncated:
+            formatStr = """Command Output (%s, truncated):\n--\n%s\n--\n"""
+            formatStr += (
+                "(NOTE: The failure may depend on preceding RUN lines.\n"
+                "       Use --verbose to see preceding RUN lines and outputs.)\n")
+        else:
+            formatStr = """Command Output (%s):\n--\n%s\n--\n"""
+        return formatStr % (streamName, outputStr)
+
+    def defaultOutput():
+        return makeOutput(cmdOutput)
+
+    def failingCommandOnlyOutput():
+        lineStart, runLineStr = locateLastFailingRunLine(cmdOutput)
+        # maybe there was an error, or maybe we are not truncating anything
+        if runLineStr is None or lineStart == 0:
+            return defaultOutput()
+        return makeOutput(cmdOutput[lineStart:], isTruncated=True)
+
+    def allCommandsHighlightingFailingOutput():
+        lineStart, runLineStr = locateLastFailingRunLine(cmdOutput)
+        # maybe there was an error, or maybe we are not truncating anything
+        if runLineStr is None or lineStart == 0:
+            return defaultOutput()
+        return makeOutput(cmdOutput[:lineStart]
+                          + highlightFailingText(cmdOutput[lineStart:]),
+                          isTruncated=False)
+
+    if litConfig.commandOutputStyle == OutputSettings.NoCommand:
+        return ""
+    elif not testStatus.isFailure:
+        return defaultOutput()
+    elif litConfig.commandOutputStyle == OutputSettings.OnlyFailingCommand:
+        return failingCommandOnlyOutput()
+    else:
+        assert(litConfig.commandOutputStyle
+               == OutputSettings.UpToAndIncludingFailingCommand)
+        return allCommandsHighlightingFailingOutput()
 
 def _runShTest(test, litConfig, useExternalSh, script, tmpBase):
     def runOnce(execdir):
         if useExternalSh:
             res = executeScript(test, litConfig, tmpBase, script, execdir)
         else:
             res = executeScriptInternal(test, litConfig, tmpBase, script, execdir)
         if isinstance(res, lit.Test.Result):
for i in range(attempts):
             break
 
     # If we had to run the test more than once, count it as a flaky pass. These
     # will be printed separately in the test summary.
     if i > 0 and status == Test.PASS:
         status = Test.FLAKYPASS
 
     # Form the output log.
-    output = """Script:\n--\n%s\n--\nExit Code: %d\n""" % (
-        '\n'.join(script), exitCode)
+    output = createScriptOutput(litConfig, script,
+                                out if out else err,
+                                status, exitCode)
 
     if timeoutInfo is not None:
         output += """Timeout: %s\n""" % (timeoutInfo,)
     output += "\n"
 
     # Append the outputs, if present.
     if out:
-        output += """Command Output (stdout):\n--\n%s\n--\n""" % (out,)
+        output += createCommandOutput(litConfig, out, "stdout", status)
     if err:
-        output += """Command Output (stderr):\n--\n%s\n--\n""" % (err,)
+        output += createCommandOutput(litConfig, err, "stderr", status)
 
     return lit.Test.Result(status, output)
 
 
 def executeShTest(test, litConfig, useExternalSh,
                   extra_substitutions=[],
                   preamble_commands=[]):
     if test.config.unsupported:

llvm/utils/lit/lit/cl_arguments.py

 import argparse
 import os
 import shlex
 import sys
 
+import lit.OutputSettings as OutputSettings
 import lit.reports
 import lit.util
 
 
 def parse_args():
     parser = argparse.ArgumentParser(prog='lit')
     parser.add_argument('test_paths',
             nargs='+',
def parse_args():
 
     format_group = parser.add_argument_group("Output Format")
     # FIXME: I find these names very confusing, although I like the
     # functionality.
     format_group.add_argument("-q", "--quiet",
             help="Suppress no error output",
             action="store_true")
     format_group.add_argument("-s", "--succinct",
-            help="Reduce amount of output",
+            help="Reduce amount of output."
+                 " Additionally, show a progress bar,"
+                 " unless --no-progress-bar is specified.",
             action="store_true")
     format_group.add_argument("-v", "--verbose",
-            dest="showOutput",
-            help="Show test output for failures",
+            help="Show test output for failures."
+                 " The last command under 'Output' is the failing one."
+                 " All substituted commands are shown under 'Script',"
+                 " including any commands after the failing command."
+                 " The failing lines are highlighted based on heuristics.",
             action="store_true")
     format_group.add_argument("-vv", "--echo-all-commands",
             dest="echoAllCommands",
             action="store_true",
-            help="Echo all commands as they are executed to stdout. In case of "
-                 "failure, last command shown will be the failing one.")
+            help="alias for -v (for backwards compatibility)")
     format_group.add_argument("-a", "--show-all",
             dest="showAllOutput",
             help="Display all commandlines and output",
             action="store_true")
     format_group.add_argument("-o", "--output",
             type=lit.reports.JsonReport,
             help="Write test results to the provided path",
             metavar="PATH")
debug_group.add_argument("--show-tests",
             help="Show all discovered tests and exit",
             action="store_true")
 
     # LIT is special: environment variables override command line arguments.
     env_args = shlex.split(os.environ.get("LIT_OPTS", ""))
     args = sys.argv[1:] + env_args
     opts = parser.parse_args(args)
 
-    # Validate command line options
-    if opts.echoAllCommands:
-        opts.showOutput = True
+    # default output settings
+    opts.showOutputOnFailure = True
+    opts.scriptOutputStyle = OutputSettings.NoScript
+    opts.commandOutputStyle = OutputSettings.OnlyFailingCommand
+    opts.runWithCommandOutput = True
+
+    if opts.quiet:
+        opts.showOutputOnFailure = False
+        opts.commandOutputStyle = OutputSettings.NoCommand
+    elif opts.verbose or opts.echoAllCommands:
+        opts.scriptOutputStyle = OutputSettings.FullScriptWithHighlighting
+        opts.commandOutputStyle = OutputSettings.UpToAndIncludingFailingCommand
+
+    # preserve old behavior by (re)setting this last
+    if opts.showAllOutput:
+        opts.scriptOutputStyle = OutputSettings.FullScript
+        opts.commandOutputStyle = OutputSettings.UpToAndIncludingFailingCommand
+
+    # If we call out to an external shell, we ask for all output,
+    # regardless of whether we're display all commands that were run,
+    # or only the last command (that might've failed).
+    opts.runWithCommandOutput = (
+        opts.commandOutputStyle == OutputSettings.UpToAndIncludingFailingCommand
+        or opts.commandOutputStyle == OutputSettings.OnlyFailingCommand)
 
     # TODO(python3): Could be enum
     if opts.shuffle:
         opts.order = 'random'
     elif opts.incremental:
         opts.order = 'failing-first'
     else:
         opts.order = 'default'

llvm/utils/lit/lit/display.py

class Display(object):
 
     def print_result(self, test):
         # Show the test result line.
         test_name = test.getFullName()
         print('%s: %s (%d of %d)' % (test.result.code.name, test_name,
                                      self.completed, self.tests))
 
         # Show the test failure output, if requested.
-        if (test.isFailure() and self.opts.showOutput) or \
+        if (test.isFailure() and self.opts.showOutputOnFailure) or \
            self.opts.showAllOutput:
             if test.isFailure():
                 print("%s TEST '%s' FAILED %s" % ('*'*20, test.getFullName(),
                                                   '*'*20))
             out = test.result.output
             # Encode/decode so that, when using Python 3.6.5 in Windows 10,
             # print(out) doesn't raise UnicodeEncodeError if out contains
             # special characters.  However, Python 2 might try to decode

llvm/utils/lit/lit/main.py

lit_config = lit.LitConfig.LitConfig(
         useValgrind=opts.useValgrind,
         valgrindLeakCheck=opts.valgrindLeakCheck,
         valgrindArgs=opts.valgrindArgs,
         noExecute=opts.noExecute,
         debug=opts.debug,
         isWindows=is_windows,
         params=params,
         config_prefix=opts.configPrefix,
-        echo_all_commands=opts.echoAllCommands)
+        runWithCommandOutput=opts.runWithCommandOutput,
+        scriptOutputStyle=opts.scriptOutputStyle,
+        commandOutputStyle=opts.commandOutputStyle)
 
     discovered_tests = lit.discovery.find_tests_for_inputs(lit_config, opts.test_paths)
     if not discovered_tests:
         sys.stderr.write('error: did not discover any tests for provided path(s)\n')
         sys.exit(2)
 
     if opts.show_suites or opts.show_tests:
         print_discovered(discovered_tests, opts.show_suites, opts.show_tests)

llvm/utils/lit/tests/shtest-format.py

 # CHECK: FAIL: shtest-format :: external_shell/fail.txt
 # CHECK-NEXT: *** TEST 'shtest-format :: external_shell/fail.txt' FAILED ***
 # CHECK: Command Output (stdout):
 # CHECK-NEXT: --
 # CHECK-NEXT: line 1: failed test output on stdout
 # CHECK-NEXT: line 2: failed test output on stdout
 # CHECK: Command Output (stderr):
 # CHECK-NEXT: --
-# CHECK-NEXT: cat{{(\.exe)?}}: {{cannot open does-not-exist|does-not-exist: No such file or directory}}
+# CHECK-NEXT: 'RUN: at line 3'
+#
+# Skipping some lines...
+#
+# CHECK:      'RUN: at line 5'
+# CHECK:      cat{{(\.exe)?}}: {{cannot open does-not-exist|does-not-exist: No such file or directory}}
 # CHECK: --
 
 # CHECK: FAIL: shtest-format :: external_shell/fail_with_bad_encoding.txt
 # CHECK-NEXT: *** TEST 'shtest-format :: external_shell/fail_with_bad_encoding.txt' FAILED ***
 # CHECK: Command Output (stdout):
 # CHECK-NEXT: --
 # CHECK-NEXT: a line with bad encoding:
 # CHECK: --

llvm/utils/lit/tests/shtest-run-at-line.py

-# Check that -vv makes the line number of the failing RUN command clear.
-# (-v is actually sufficient in the case of the internal shell.)
+# Check that the default terse output shows only the failing
+# line in the output, and doesn't show the script.
 #
-# RUN: not %{lit} -j 1 -vv %{inputs}/shtest-run-at-line > %t.out
-# RUN: FileCheck --input-file %t.out %s
+# RUN: not %{lit} -j 1 %{inputs}/shtest-run-at-line > %t-terse.out
+# RUN: FileCheck --input-file %t-terse.out %s --check-prefix=TERSE
+
+# Check that --verbose outputs both Script and Command Output.
+#
+# RUN: not %{lit} -j 1 --verbose %{inputs}/shtest-run-at-line > %t-verbose.out
+# RUN: FileCheck --input-file %t-verbose.out %s --check-prefix=VERBOSE
 #
 # END.
 
-
-# CHECK: Testing: 4 tests
-
-
 # In the case of the external shell, we check for only RUN lines in stderr in
 # case some shell implementations format "set -x" output differently.
 
-# CHECK-LABEL: FAIL: shtest-run-at-line :: external-shell/basic.txt
+################################################################################
+# Checking lines for terse output
+
+# TERSE-LABEL: FAIL: shtest-run-at-line :: external-shell/basic.txt
+
+# TERSE-NOT:      Script:
 
-# CHECK:      Script:
-# CHECK:      RUN: at line 4{{.*}}  true
-# CHECK-NEXT: RUN: at line 5{{.*}}  false
-# CHECK-NEXT: RUN: at line 6{{.*}}  true
-
-# CHECK:     RUN: at line 4
-# CHECK:     RUN: at line 5
-# CHECK-NOT: RUN
-
-# CHECK-LABEL: FAIL: shtest-run-at-line :: external-shell/line-continuation.txt
-
-# CHECK:      Script:
-# CHECK:      RUN: at line 4{{.*}}  echo 'foo bar'  | FileCheck
-# CHECK-NEXT: RUN: at line 6{{.*}}  echo 'foo baz'  | FileCheck
-# CHECK-NEXT: RUN: at line 9{{.*}}  echo 'foo bar'  | FileCheck
-
-# CHECK:     RUN: at line 4
-# CHECK:     RUN: at line 6
-# CHECK-NOT: RUN
-
-
-# CHECK-LABEL: FAIL: shtest-run-at-line :: internal-shell/basic.txt
-
-# CHECK:      Script:
-# CHECK:      : 'RUN: at line 1';  true
-# CHECK-NEXT: : 'RUN: at line 2';  false
-# CHECK-NEXT: : 'RUN: at line 3';  true
-
-# CHECK:      Command Output (stdout)
-# CHECK:      $ ":" "RUN: at line 1"
-# CHECK-NEXT: $ "true"
-# CHECK-NEXT: $ ":" "RUN: at line 2"
-# CHECK-NEXT: $ "false"
-# CHECK-NOT:  RUN
-
-# CHECK-LABEL: FAIL: shtest-run-at-line :: internal-shell/line-continuation.txt
-
-# CHECK:      Script:
-# CHECK:      : 'RUN: at line 1';  : first line continued to second line
-# CHECK-NEXT: : 'RUN: at line 3';  echo 'foo bar'  | FileCheck
-# CHECK-NEXT: : 'RUN: at line 5';  echo  'foo baz'  | FileCheck
-# CHECK-NEXT: : 'RUN: at line 8';  echo 'foo bar'  | FileCheck
-
-# CHECK:      Command Output (stdout)
-# CHECK:      $ ":" "RUN: at line 1"
-# CHECK-NEXT: $ ":" "first" "line" "continued" "to" "second" "line"
-# CHECK-NEXT: $ ":" "RUN: at line 3"
-# CHECK-NEXT: $ "echo" "foo bar"
-# CHECK-NEXT: $ "FileCheck" "{{.*}}"
-# CHECK-NEXT: $ ":" "RUN: at line 5"
-# CHECK-NEXT: $ "echo" "foo baz"
-# CHECK-NEXT: $ "FileCheck" "{{.*}}"
-# CHECK-NOT:  RUN
+# TERSE:      Command Output (stderr, truncated)
+# TERSE:      --
+# TERSE-NOT:  RUN: at line 4
+# TERSE-NOT:  true
+# TERSE:      RUN: at line 5
+# TERSE:      false
+# TERSE-NOT:  RUN
+# TERSE:      NOTE: The failure may depend on preceding RUN lines.
+
+# TERSE-LABEL: FAIL: shtest-run-at-line :: external-shell/line-continuation.txt
+
+# TERSE-NOT:  Script:
+
+# TERSE:      Command Output (stderr, truncated)
+# TERSE-NOT:  RUN: at line 4
+# TERSE:      RUN: at line 6
+# TERSE:      echo 'foo baz'
+# TERSE:      FileCheck
+# TERSE-NOT:  RUN
+# TERSE:      NOTE: The failure may depend on preceding RUN lines.
+
+# TERSE-LABEL: FAIL: shtest-run-at-line :: internal-shell/basic.txt
+
+# TERSE-NOT:      Script:
+
+# TERSE:      Command Output (stdout, truncated)
+# TERSE-NOT:  $ ":" "RUN: at line 1"
+# TERSE:      $ ":" "RUN: at line 2"
+# TERSE-NEXT: $ "false"
+# TERSE-NOT:  RUN
+# TERSE:      NOTE: The failure may depend on preceding RUN lines.
+
+# TERSE-LABEL: FAIL: shtest-run-at-line :: internal-shell/line-continuation.txt
+
+# TERSE-NOT:  Script:
+
+# TERSE:      Command Output (stdout, truncated)
+# TERSE-NOT:  $ ":" "RUN: at line 1"
+# TERSE-NOT:  $ ":" "RUN: at line 3"
+# TERSE:      $ ":" "RUN: at line 5"
+# TERSE-NEXT: $ "echo" "foo baz"
+# TERSE-NEXT: $ "FileCheck" "{{.*}}"
+# TERSE-NOT:  RUN
+# TERSE:      (NOTE: The failure may depend on preceding RUN lines.
+# TERSE-NEXT:        Use --verbose to see preceding RUN lines and outputs.)
+
+################################################################################
+# Checking lines for verbose output
+
+# VERBOSE: Testing: 4 tests
+
+# VERBOSE-LABEL: FAIL: shtest-run-at-line :: external-shell/basic.txt
+
+# VERBOSE:      Script:
+# VERBOSE:      RUN: at line 4{{.*}}  true
+# VERBOSE-NEXT: RUN: at line 5{{.*}}  false
+# VERBOSE-NEXT: RUN: at line 6{{.*}}  true
+
+# VERBOSE:     RUN: at line 4
+# VERBOSE:     RUN: at line 5
+# VERBOSE-NOT: RUN: at line 6
+# VERBOSE-NOT: (NOTE: The failure may depend on preceding RUN lines.
+
+# VERBOSE-LABEL: FAIL: shtest-run-at-line :: external-shell/line-continuation.txt
+
+# VERBOSE:      Script:
+# VERBOSE:      RUN: at line 4{{.*}}  echo 'foo bar'  | FileCheck
+# VERBOSE-NEXT: RUN: at line 6{{.*}}  echo 'foo baz'  | FileCheck
+# VERBOSE-NEXT: RUN: at line 9{{.*}}  echo 'foo bar'  | FileCheck
+
+# VERBOSE:     RUN: at line 4
+# VERBOSE:     RUN: at line 6
+# VERBOSE-NOT: RUN
+# VERBOSE-NOT: (NOTE: The failure may depend on preceding RUN lines.
+
+# VERBOSE-LABEL: FAIL: shtest-run-at-line :: internal-shell/basic.txt
+
+# VERBOSE:      Script:
+# VERBOSE:      : 'RUN: at line 1';  true
+# VERBOSE-NEXT: : 'RUN: at line 2';  false
+# VERBOSE-NEXT: : 'RUN: at line 3';  true
+
+# VERBOSE:      Command Output (stdout)
+# VERBOSE:      $ ":" "RUN: at line 1"
+# VERBOSE-NEXT: $ "true"
+# VERBOSE-NEXT: $ ":" "RUN: at line 2"
+# VERBOSE-NEXT: $ "false"
+# VERBOSE-NOT:  RUN
+# VERBOSE-NOT: (NOTE: The failure may depend on preceding RUN lines.
+
+# VERBOSE-LABEL: FAIL: shtest-run-at-line :: internal-shell/line-continuation.txt
+
+# VERBOSE:      Script:
+# VERBOSE:      : 'RUN: at line 1';  : first line continued to second line
+# VERBOSE-NEXT: : 'RUN: at line 3';  echo 'foo bar'  | FileCheck
+# VERBOSE-NEXT: : 'RUN: at line 5';  echo  'foo baz'  | FileCheck
+# VERBOSE-NEXT: : 'RUN: at line 8';  echo 'foo bar'  | FileCheck
+
+# VERBOSE:      Command Output (stdout)
+# VERBOSE:      $ ":" "RUN: at line 1"
+# VERBOSE-NEXT: $ ":" "first" "line" "continued" "to" "second" "line"
+# VERBOSE-NEXT: $ ":" "RUN: at line 3"
+# VERBOSE-NEXT: $ "echo" "foo bar"
+# VERBOSE-NEXT: $ "FileCheck" "{{.*}}"
+# VERBOSE-NEXT: $ ":" "RUN: at line 5"
+# VERBOSE-NEXT: $ "echo" "foo baz"
+# VERBOSE-NEXT: $ "FileCheck" "{{.*}}"
+# VERBOSE-NOT:  RUN
+# VERBOSE-NOT: (NOTE: The failure may depend on preceding RUN lines.