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

jdenny-ornl · 2023-10-07T20:03:33Z

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.

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, 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.

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.

llvmbot · 2023-10-07T20:05:15Z

@llvm/pr-subscribers-testing-tools

@llvm/pr-subscribers-libcxx

Changes

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, 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 --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)

arichardson · 2023-10-09T15:49:54Z

llvm/utils/lit/lit/TestRunner.py

@@ -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


How about using a type annotation for the function signature instead of this comment? Then it can be checked by type checker tools as well as IDEs.

Thanks for the suggestion. Are you thinking of something like the following?

def executeScriptInternal( test, litConfig, tmpBase, commands, cwd, debug=True ) -> typing.Tuple[str, str, int, str | None]:

I tried running mypy to check this, and it complained:

llvm/utils/lit/lit/TestRunner.py:1035: error: X | Y syntax for unions requires Python 3.10 [syntax]

However, LLVM requires only python 3.6 currently.

Also, is there a way to provide names/descriptions for the tuple members? Is there a way to specify exceptions? I couldn't find a syntax for either.

Assuming the above limitations, it looks like the comment would be useful anyway. I'm not experienced with type annotations in python. Please let me know if I'm doing something wrong.

That's how you do it in Python 3.6:

def executeScriptInternal( test, litConfig, tmpBase, commands, cwd, debug=True ) -> typing.Optional[typing.Tuple[str, str, int, str]]:

Also, is there a way to provide names/descriptions for the tuple members?

https://docs.python.org/3/library/typing.html#typing.NamedTuple
Dataclasses might be relevant here as well.

Is there a way to specify exceptions?

It was an explicit design choice to not include exceptions in type annotations: https://stackoverflow.com/a/44282299/4182606

I'm not experienced with type annotations in python. Please let me know if I'm doing something wrong.

I have experience with type annotations in Python, specifically in 3.5 and 3.6 versions, so I'll be happy to review them.

Also, is there a way to provide names/descriptions for the tuple members?

https://docs.python.org/3/library/typing.html#typing.NamedTuple Dataclasses might be relevant here as well.

If I understand that correctly, I'd be changing the return type rather than just documenting/annotating it. Maybe the type annotation plus a comment naming the fields is enough?

I have experience with type annotations in Python, specifically in 3.5 and 3.6 versions, so I'll be happy to review them.

Thanks for the suggestions. I pushed a commit. Let me know if you think something more would be worthwhile.

If I understand that correctly, I'd be changing the return type rather than just documenting/annotating it. Maybe the type annotation plus a comment naming the fields is enough?

If you intend to add names to typing.Tuple[str, str, int, typing.Optional[str]], I'm not sure it's possible in a way that won't scare type checkers away, but I'm sure it's not conventional for Python. If you want to name tuple members, you should create a type with namedtuple(), and return it.
https://github.com/llvm/llvm-project/pull/68496/files#diff-06d85faa2a7a6970de33ccb78e3e004456eb51ab7303330ec52a5e35db92d415R53 would definitely benefit from named arguments.

RoboTux

LGTM, happy to approve once the typing annotation has been added and approved by people knowledgeable on it.

ldionne

LGTM for libc++ changes, thanks!

Endilll

Even though I'd prefer to see more type annotations (e.g. on function parameters), I'm leaning towards them being out of scope of this PR. There is nothing wrong with type annotations that are added.

llvm/utils/lit/lit/TestRunner.py

arichardson

LGTM (especially with the from typing import Optional, Tuple suggestion applied).

Local branch amd-gfx 7c4daea Merged main:ba8565fbcb97 into amd-gfx:ee21db2b70bf Remote branch main 080fb3e [lit] Clean up internal shell parse errors with ScriptFatal (llvm#68496)

jdenny-ornl requested review from mstorsjo, RoboTux, MaskRay, yln, ldionne, banach-space, AaronBallman, Endilll, pogo59 and jh7370 October 7, 2023 20:03

jdenny-ornl requested a review from a team as a code owner October 7, 2023 20:03

llvmbot added libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. llvm-lit testing-tools labels Oct 7, 2023

arichardson reviewed Oct 9, 2023

View reviewed changes

RoboTux reviewed Oct 10, 2023

View reviewed changes

ldionne approved these changes Oct 10, 2023

View reviewed changes

Add type annotations on functions documented in this PR

7954ad5

Endilll approved these changes Oct 11, 2023

View reviewed changes

llvm/utils/lit/lit/TestRunner.py Outdated Show resolved Hide resolved

arichardson approved these changes Oct 11, 2023

View reviewed changes

from typing import Optional, Tuple

e8a93f5

jdenny-ornl merged commit 080fb3e into llvm:main Oct 20, 2023

spupyrev mentioned this pull request Oct 23, 2023

[BOLT] Rename cds to cdsort #69966

Merged

banach-space mentioned this pull request Oct 24, 2023

[mlir][vector] Add scalable vectors to tests for vector.contract #70039

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

jdenny-ornl commented Oct 7, 2023

Uh oh!

llvmbot commented Oct 7, 2023 •

edited

Loading

Uh oh!

arichardson Oct 9, 2023

Uh oh!

jdenny-ornl Oct 10, 2023

Uh oh!

Endilll Oct 10, 2023

Uh oh!

jdenny-ornl Oct 10, 2023

Uh oh!

Endilll Oct 11, 2023

Uh oh!

RoboTux left a comment

Uh oh!

ldionne left a comment

Uh oh!

Endilll left a comment

Uh oh!

Uh oh!

arichardson left a comment

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

jdenny-ornl commented Oct 7, 2023

Uh oh!

llvmbot commented Oct 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arichardson Oct 9, 2023

Choose a reason for hiding this comment

Uh oh!

jdenny-ornl Oct 10, 2023

Choose a reason for hiding this comment

Uh oh!

Endilll Oct 10, 2023

Choose a reason for hiding this comment

Uh oh!

jdenny-ornl Oct 10, 2023

Choose a reason for hiding this comment

Uh oh!

Endilll Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

RoboTux left a comment

Choose a reason for hiding this comment

Uh oh!

ldionne left a comment

Choose a reason for hiding this comment

Uh oh!

Endilll left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arichardson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

llvmbot commented Oct 7, 2023 •

edited

Loading