[llvm] [lit] Clean up internal shell parse errors with ScriptFatal (PR #68496)
Joel E. Denny via llvm-commits
llvm-commits at lists.llvm.org
Tue Oct 10 13:43:20 PDT 2023
https://github.com/jdenny-ornl updated https://github.com/llvm/llvm-project/pull/68496
>From 909a48a4d1dd04b1841d5e6953797001a560c432 Mon Sep 17 00:00:00 2001
From: "Joel E. Denny" <jdenny.ornl at gmail.com>
Date: Sat, 7 Oct 2023 15:56:28 -0400
Subject: [PATCH 1/2] [lit] Clean up internal shell parse errors with
ScriptFatal
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.
---
libcxx/utils/libcxx/test/format.py | 11 ++++----
llvm/utils/lit/lit/TestRunner.py | 41 +++++++++++++++++++---------
llvm/utils/lit/tests/shtest-shell.py | 18 ++++++++----
3 files changed, 47 insertions(+), 23 deletions(-)
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)
>From 7954ad5d64c1b17fa0692345a2e5f6eaf06956ed Mon Sep 17 00:00:00 2001
From: "Joel E. Denny" <jdenny.ornl at gmail.com>
Date: Tue, 10 Oct 2023 13:22:10 -0700
Subject: [PATCH 2/2] Add type annotations on functions documented in this PR
---
llvm/utils/lit/lit/TestRunner.py | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/llvm/utils/lit/lit/TestRunner.py b/llvm/utils/lit/lit/TestRunner.py
index 590538f1e5e6039..6d8b2e6f2c4db4b 100644
--- a/llvm/utils/lit/lit/TestRunner.py
+++ b/llvm/utils/lit/lit/TestRunner.py
@@ -12,6 +12,7 @@
import shutil
import tempfile
import threading
+import typing
import io
@@ -1029,7 +1030,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
+) -> typing.Tuple[str, str, int, typing.Optional[str]]:
cmds = []
for i, ln in enumerate(commands):
# Within lit, we try to always add '%dbg(...)' to command lines in order
@@ -2142,10 +2145,11 @@ 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):
+def _runShTest(test, litConfig, useExternalSh, script, tmpBase) -> lit.Test.Result:
+ # Always returns the tuple (out, err, exitCode, timeoutInfo, status).
+ def runOnce(
+ execdir,
+ ) -> typing.Tuple[str, str, int, typing.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
More information about the llvm-commits
mailing list