Unverified Commit 080fb3e5 authored by Joel E. Denny's avatar Joel E. Denny Committed by GitHub
Browse files

[lit] Clean up internal shell parse errors with ScriptFatal (#68496) 

Without this patch, the functions `executeScriptInternal` and thus
`runOnce` in `llvm/utils/lit/lit/TestRunner.py` return either a tuple like
`(out, err, exitCode, timeoutInfo)` or a `lit.Test.Result` object. They
return the latter only when there's a lit internal shell parse error in
a RUN line. In my opinion, a more straight-forward way to handle
exceptional cases like that is to use python exceptions.

For that purpose, this patch introduces `ScriptFatal`. Thus, this patch
changes `executeScriptInternal` to always either return the tuple or
raise the `ScriptFatal` exception. It updates `runOnce` and
`libcxx/utils/libcxx/test/format.py` to catch the exception rather than
check for the special return type.

This patch also changes `runOnce` to convert the exception to a
`Test.UNRESOLVED` result instead of `TEST.FAIL`. The former is the
proper result for such a malformed test, for which a rerun (given an
`ALLOW_RETRIES:`) serves no purpose. There are at least two benefits
from this change. First, `_runShTest` no longer has to specially and
cryptically handle this case to avoid unnecessary reruns. Second, an
`XFAIL:` directive no longer hides such a failure [as we saw
previously](https://reviews.llvm.org/D154987#4501125).

To facilitate the `_runShTest` change, this patch inserts the internal
shell parse error diagnostic into the format of the test's normal debug
output rather than suppressing the latter entirely. That change is also
important for [D154987](https://reviews.llvm.org/D154987), which
proposes to reuse `ScriptFatal` for python compile errors in PYTHON
lines or in `config.prologue`. In that case, the diagnostic might follow
debugging output from the test's previous RUN or PYTHON lines, so
suppressing the normal debug output would lose information.
parent 6795bfce

libcxx/utils/libcxx/test/format.py

+6 −5
Original line number Diff line number Diff line
@@ -45,11 +45,12 @@ def _executeScriptInternal(test, litConfig, commands): 


    _, tmpBase = _getTempPaths(test)

    execDir = os.path.dirname(test.getExecPath())

    try:

        res = lit.TestRunner.executeScriptInternal(

            test, litConfig, tmpBase, parsedCommands, execDir, debug=False

        )

    if isinstance(res, lit.Test.Result):  # Handle failure to parse the Lit test

        res = ("", res.output, 127, None)

    except lit.TestRunner.ScriptFatal as e:

        res = ("", str(e), 127, None)

    (out, err, exitCode, timeoutInfo) = res



    return (out, err, exitCode, timeoutInfo, parsedCommands)

llvm/utils/lit/lit/TestRunner.py

+36 −16
Original line number Diff line number Diff line
@@ -12,6 +12,8 @@ import shlex 
import shutil

import tempfile

import threading

import typing

from typing import Optional, Tuple



import io
@@ -34,6 +36,17 @@ class InternalShellError(Exception): 
        self.message = message





class ScriptFatal(Exception):

    """

    A script had a fatal error such that there's no point in retrying.  The

    message has not been emitted on stdout or stderr but is instead included in

    this exception.

    """



    def __init__(self, message):

        super().__init__(message)





kIsWindows = platform.system() == "Windows"



# Don't use close_fds on Windows.
@@ -1009,7 +1022,8 @@ def formatOutput(title, data, limit=None): 
    return out





# Normally returns out, err, exitCode, timeoutInfo.

# Always either returns the tuple (out, err, exitCode, timeoutInfo) or raises a

# ScriptFatal exception.

#

# If debug is True (the normal lit behavior), err is empty, and out contains an

# execution trace, including stdout and stderr shown per command executed.
@@ -1017,7 +1031,9 @@ def formatOutput(title, data, limit=None): 
# If debug is False (set by some custom lit test formats that call this

# function), out contains only stdout from the script, err contains only stderr

# from the script, and there is no execution trace.

def executeScriptInternal(test, litConfig, tmpBase, commands, cwd, debug=True):

def executeScriptInternal(

    test, litConfig, tmpBase, commands, cwd, debug=True

) -> Tuple[str, str, int, Optional[str]]:

    cmds = []

    for i, ln in enumerate(commands):

        # Within lit, we try to always add '%dbg(...)' to command lines in order
@@ -1043,9 +1059,9 @@ def executeScriptInternal(test, litConfig, tmpBase, commands, cwd, debug=True): 
                ShUtil.ShParser(ln, litConfig.isWindows, test.config.pipefail).parse()

            )

        except:

            return lit.Test.Result(

                Test.FAIL, f"shell parser error on {dbg}: {command.lstrip()}\n"

            )

            raise ScriptFatal(

                f"shell parser error on {dbg}: {command.lstrip()}\n"

            ) from None



    cmd = cmds[0]

    for c in cmds[1:]:
@@ -2130,8 +2146,11 @@ def parseIntegratedTestScript(test, additional_parsers=[], require_script=True): 
    return script





def _runShTest(test, litConfig, useExternalSh, script, tmpBase):

    def runOnce(execdir):

def _runShTest(test, litConfig, useExternalSh, script, tmpBase) -> lit.Test.Result:

    # Always returns the tuple (out, err, exitCode, timeoutInfo, status).

    def runOnce(

        execdir,

    ) -> Tuple[str, str, int, Optional[str], Test.ResultCode]:

        # script is modified below (for litConfig.per_test_coverage, and for

        # %dbg expansions).  runOnce can be called multiple times, but applying

        # the modifications multiple times can corrupt script, so always modify
@@ -2158,12 +2177,16 @@ def _runShTest(test, litConfig, useExternalSh, script, tmpBase): 
                    command = buildPdbgCommand(dbg, command)

                scriptCopy[i] = command



        try:

            if useExternalSh:

                res = executeScript(test, litConfig, tmpBase, scriptCopy, execdir)

            else:

            res = executeScriptInternal(test, litConfig, tmpBase, scriptCopy, execdir)

        if isinstance(res, lit.Test.Result):

            return res

                res = executeScriptInternal(

                    test, litConfig, tmpBase, scriptCopy, execdir

                )

        except ScriptFatal as e:

            out = f"# " + "\n# ".join(str(e).splitlines()) + "\n"

            return out, "", 1, None, Test.UNRESOLVED



        out, err, exitCode, timeoutInfo = res

        if exitCode == 0:
@@ -2183,9 +2206,6 @@ def _runShTest(test, litConfig, useExternalSh, script, tmpBase): 
    attempts = test.allowed_retries + 1

    for i in range(attempts):

        res = runOnce(execdir)

        if isinstance(res, lit.Test.Result):

            return res



        out, err, exitCode, timeoutInfo, status = res

        if status != Test.FAIL:

            break

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

+13 −5
Original line number Diff line number Diff line
@@ -561,10 +561,17 @@ 


# FIXME: The output here sucks.

#

# CHECK: FAIL: shtest-shell :: error-1.txt

# CHECK: *** TEST 'shtest-shell :: error-1.txt' FAILED ***

# CHECK: shell parser error on RUN: at line 3: echo "missing quote

# CHECK: ***

#       CHECK: UNRESOLVED: shtest-shell :: error-1.txt

#  CHECK-NEXT: *** TEST 'shtest-shell :: error-1.txt' FAILED ***

#  CHECK-NEXT: Exit Code: 1

# CHECK-EMPTY:

#  CHECK-NEXT: Command Output (stdout):

#  CHECK-NEXT: --

#  CHECK-NEXT: # shell parser error on RUN: at line 3: echo "missing quote

# CHECK-EMPTY:

#  CHECK-NEXT: --

# CHECK-EMPTY:

#  CHECK-NEXT: ***



# CHECK: FAIL: shtest-shell :: error-2.txt

# CHECK: *** TEST 'shtest-shell :: error-2.txt' FAILED ***
@@ -643,4 +650,5 @@ 
#      CHECK: ***



# CHECK: PASS: shtest-shell :: valid-shell.txt

# CHECK: Failed Tests (39)
 
# CHECK: Unresolved Tests (1)

# CHECK: Failed Tests (38)