[llvm] [lit] Clean up internal shell parse errors with ScriptFatal (PR #68496)
via llvm-commits
llvm-commits at lists.llvm.org
Sat Oct 7 13:05:21 PDT 2023
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-libcxx
<details>
<summary>Changes</summary>
Without this patch, the functions `executeScriptInternal` and thus `runOnce` in `llvm/utils/lit/lit/TestRunner.py` return either the tuple `(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.
---
Full diff: https://github.com/llvm/llvm-project/pull/68496.diff
3 Files Affected:
- (modified) libcxx/utils/libcxx/test/format.py (+6-5)
- (modified) llvm/utils/lit/lit/TestRunner.py (+28-13)
- (modified) llvm/utils/lit/tests/shtest-shell.py (+13-5)
``````````diff
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..590538f1e5e6039 100644
--- a/llvm/utils/lit/lit/TestRunner.py
+++ b/llvm/utils/lit/lit/TestRunner.py
@@ -34,6 +34,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 +1020,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.
@@ -1043,9 +1055,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,7 +2142,9 @@ def parseIntegratedTestScript(test, additional_parsers=[], require_script=True):
return script
+# Always returns a lit.Test.Result.
def _runShTest(test, litConfig, useExternalSh, script, tmpBase):
+ # Always returns the tuple (out, err, exitCode, timeoutInfo).
def runOnce(execdir):
# script is modified below (for litConfig.per_test_coverage, and for
# %dbg expansions). runOnce can be called multiple times, but applying
@@ -2158,12 +2172,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 +2201,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)
``````````
</details>
https://github.com/llvm/llvm-project/pull/68496
More information about the llvm-commits
mailing list