From 909a48a4d1dd04b1841d5e6953797001a560c432 Mon Sep 17 00:00:00 2001 From: "Joel E. Denny" Date: Sat, 7 Oct 2023 15:56:28 -0400 Subject: [PATCH 1/3] [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 c7c0bad681ddc..4106cd83db34b 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 a6314e35c1a04..590538f1e5e60 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 a043582d6ae2b..8685119488062 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" Date: Tue, 10 Oct 2023 13:22:10 -0700 Subject: [PATCH 2/3] 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 590538f1e5e60..6d8b2e6f2c4db 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 From e8a93f5bc7d5639bf521971127405ad25d08dcbd Mon Sep 17 00:00:00 2001 From: "Joel E. Denny" Date: Wed, 11 Oct 2023 16:12:07 -0700 Subject: [PATCH 3/3] from typing import Optional, Tuple --- llvm/utils/lit/lit/TestRunner.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/llvm/utils/lit/lit/TestRunner.py b/llvm/utils/lit/lit/TestRunner.py index 6d8b2e6f2c4db..788152846efe2 100644 --- a/llvm/utils/lit/lit/TestRunner.py +++ b/llvm/utils/lit/lit/TestRunner.py @@ -13,6 +13,7 @@ import tempfile import threading import typing +from typing import Optional, Tuple import io @@ -1032,7 +1033,7 @@ def formatOutput(title, data, limit=None): # from the script, and there is no execution trace. def executeScriptInternal( test, litConfig, tmpBase, commands, cwd, debug=True -) -> typing.Tuple[str, str, int, typing.Optional[str]]: +) -> 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 @@ -2149,7 +2150,7 @@ def _runShTest(test, litConfig, useExternalSh, script, tmpBase) -> lit.Test.Resu # Always returns the tuple (out, err, exitCode, timeoutInfo, status). def runOnce( execdir, - ) -> typing.Tuple[str, str, int, typing.Optional[str], Test.ResultCode]: + ) -> 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