[llvm] 080fb3e - [lit] Clean up internal shell parse errors with ScriptFatal (#68496)
via llvm-commits
llvm-commits at lists.llvm.org
Fri Oct 20 08:53:21 PDT 2023
Author: Joel E. Denny
Date: 2023-10-20T11:53:18-04:00
New Revision: 080fb3e5b73bcfd90f11335ff3d175cf3199f814
URL: https://github.com/llvm/llvm-project/commit/080fb3e5b73bcfd90f11335ff3d175cf3199f814
DIFF: https://github.com/llvm/llvm-project/commit/080fb3e5b73bcfd90f11335ff3d175cf3199f814.diff
LOG: [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.
Added:
Modified:
libcxx/utils/libcxx/test/format.py
llvm/utils/lit/lit/TestRunner.py
llvm/utils/lit/tests/shtest-shell.py
Removed:
################################################################################
diff --git a/libcxx/utils/libcxx/test/format.py b/libcxx/utils/libcxx/test/format.py
index c7c0bad681ddc8b..4106cd83db34b7d 100644
--- a/libcxx/utils/libcxx/test/format.py
+++ b/libcxx/utils/libcxx/test/format.py
@@ -45,11 +45,12 @@ def _executeScriptInternal(test, litConfig, commands):
_, tmpBase = _getTempPaths(test)
execDir = os.path.dirname(test.getExecPath())
- 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)
+ try:
+ res = lit.TestRunner.executeScriptInternal(
+ test, litConfig, tmpBase, parsedCommands, execDir, debug=False
+ )
+ except lit.TestRunner.ScriptFatal as e:
+ res = ("", str(e), 127, None)
(out, err, exitCode, timeoutInfo) = res
return (out, err, exitCode, timeoutInfo, parsedCommands)
diff --git a/llvm/utils/lit/lit/TestRunner.py b/llvm/utils/lit/lit/TestRunner.py
index a6314e35c1a045c..788152846efe2fa 100644
--- a/llvm/utils/lit/lit/TestRunner.py
+++ b/llvm/utils/lit/lit/TestRunner.py
@@ -12,6 +12,8 @@
import shutil
import tempfile
import threading
+import typing
+from typing import Optional, Tuple
import io
@@ -34,6 +36,17 @@ def __init__(self, command, message):
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 runOnce(execdir):
command = buildPdbgCommand(dbg, command)
scriptCopy[i] = command
- 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
+ try:
+ if useExternalSh:
+ res = executeScript(test, litConfig, tmpBase, scriptCopy, execdir)
+ else:
+ 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 runOnce(execdir):
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
diff --git a/llvm/utils/lit/tests/shtest-shell.py b/llvm/utils/lit/tests/shtest-shell.py
index a043582d6ae2b9a..86851194880620b 100644
--- a/llvm/utils/lit/tests/shtest-shell.py
+++ b/llvm/utils/lit/tests/shtest-shell.py
@@ -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)
More information about the llvm-commits
mailing list