[Util] Only run --update-tests functions on failing tests #155148
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The early exit we relied on to only invoke test updaters for failing tests requires that there was no output to stdout or stderr, and that timeouts weren't enabled. When these conditions weren't fulfilled, test updaters would be invoked even on passing or XFAILed tests.
|
@llvm/pr-subscribers-testing-tools Author: Henrik G. Olsson (hnrklssn) ChangesThe early exit we relied on to only invoke test updaters for failing tests requires that there was no output to stdout or stderr, and that timeouts weren't enabled. When these conditions weren't fulfilled, test updaters would be invoked even on passing or XFAILed tests. Full diff: https://github.com/llvm/llvm-project/pull/155148.diff 10 Files Affected:
diff --git a/llvm/utils/lit/lit/TestRunner.py b/llvm/utils/lit/lit/TestRunner.py
index f2c5c6d0dbe93..516c63ec9e78e 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
+import traceback
from typing import Optional, Tuple
import io
@@ -47,6 +48,16 @@ def __init__(self, message):
super().__init__(message)
+class TestUpdaterException(Exception):
+ """
+ There was an error not during test execution, but while invoking a function
+ in test_updaters on a failing RUN line.
+ """
+
+ def __init__(self, message):
+ super().__init__(message)
+
+
kIsWindows = platform.system() == "Windows"
# Don't use close_fds on Windows.
@@ -1192,13 +1203,22 @@ def executeScriptInternal(
str(result.timeoutReached),
)
- if litConfig.update_tests:
+ if (litConfig.update_tests
+ and result.exitCode != 0
+ and not timeoutInfo
+ # Don't run test updaters on XPASS failures -
+ # they are not expected to be able to fix them
+ and not test.isExpectedToFail()
+ ):
for test_updater in litConfig.test_updaters:
try:
update_output = test_updater(result, test)
except Exception as e:
- out += f"Exception occurred in test updater: {e}"
- continue
+ output = out
+ output += err
+ output += "Exception occurred in test updater:\n"
+ output += traceback.format_exc()
+ raise TestUpdaterException(output)
if update_output:
for line in update_output.splitlines():
out += f"# {line}\n"
diff --git a/llvm/utils/lit/lit/worker.py b/llvm/utils/lit/lit/worker.py
index 8e78bfd45d38b..0e36f3e9a2424 100644
--- a/llvm/utils/lit/lit/worker.py
+++ b/llvm/utils/lit/lit/worker.py
@@ -13,6 +13,7 @@
import lit.Test
import lit.util
+from lit.TestRunner import TestUpdaterException
_lit_config = None
@@ -75,6 +76,10 @@ def _execute_test_handle_errors(test, lit_config):
try:
result = test.config.test_format.execute(test, lit_config)
return _adapt_result(result)
+ except TestUpdaterException as e:
+ if lit_config.debug:
+ raise
+ return lit.Test.Result(lit.Test.UNRESOLVED, str(e))
except:
if lit_config.debug:
raise
diff --git a/llvm/utils/lit/tests/Inputs/pass-test-update/fail.test b/llvm/utils/lit/tests/Inputs/pass-test-update/fail.test
new file mode 100644
index 0000000000000..a410ef9293129
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/pass-test-update/fail.test
@@ -0,0 +1 @@
+# RUN: not echo "fail"
diff --git a/llvm/utils/lit/tests/Inputs/pass-test-update/lit.cfg b/llvm/utils/lit/tests/Inputs/pass-test-update/lit.cfg
new file mode 100644
index 0000000000000..e0dbc3220678c
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/pass-test-update/lit.cfg
@@ -0,0 +1,14 @@
+import lit.formats
+
+config.name = "pass-test-update"
+config.suffixes = [".test"]
+config.test_format = lit.formats.ShTest()
+config.test_source_root = None
+config.test_exec_root = None
+
+import sys
+import os
+sys.path.append(os.path.dirname(__file__))
+from should_not_run import should_not_run
+lit_config.test_updaters.append(should_not_run)
+
diff --git a/llvm/utils/lit/tests/Inputs/pass-test-update/pass-silent.test b/llvm/utils/lit/tests/Inputs/pass-test-update/pass-silent.test
new file mode 100644
index 0000000000000..07cafbc66a897
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/pass-test-update/pass-silent.test
@@ -0,0 +1,2 @@
+# RUN: echo ""
+
diff --git a/llvm/utils/lit/tests/Inputs/pass-test-update/pass.test b/llvm/utils/lit/tests/Inputs/pass-test-update/pass.test
new file mode 100644
index 0000000000000..3a12c5fee4bb0
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/pass-test-update/pass.test
@@ -0,0 +1 @@
+# RUN: echo "passed"
diff --git a/llvm/utils/lit/tests/Inputs/pass-test-update/should_not_run.py b/llvm/utils/lit/tests/Inputs/pass-test-update/should_not_run.py
new file mode 100644
index 0000000000000..adba049753594
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/pass-test-update/should_not_run.py
@@ -0,0 +1,3 @@
+def should_not_run(foo, bar):
+ raise Exception("this test updater should only run on failure")
+
diff --git a/llvm/utils/lit/tests/Inputs/pass-test-update/xfail.test b/llvm/utils/lit/tests/Inputs/pass-test-update/xfail.test
new file mode 100644
index 0000000000000..044b6a19b4a19
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/pass-test-update/xfail.test
@@ -0,0 +1,2 @@
+# XFAIL: *
+# RUN: not echo "failed"
diff --git a/llvm/utils/lit/tests/Inputs/pass-test-update/xpass.test b/llvm/utils/lit/tests/Inputs/pass-test-update/xpass.test
new file mode 100644
index 0000000000000..e0c1c39762ee9
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/pass-test-update/xpass.test
@@ -0,0 +1,2 @@
+# XFAIL: *
+# RUN: echo "accidentally passed"
diff --git a/llvm/utils/lit/tests/pass-test-update.py b/llvm/utils/lit/tests/pass-test-update.py
new file mode 100644
index 0000000000000..b3cb7057ef163
--- /dev/null
+++ b/llvm/utils/lit/tests/pass-test-update.py
@@ -0,0 +1,40 @@
+# RUN: %{lit} --update-tests --ignore-fail -v %S/Inputs/pass-test-update | FileCheck %s --implicit-check-not Exception
+
+# CHECK: UNRESOLVED: pass-test-update :: fail.test (1 of 5)
+# CHECK-NEXT: ******************** TEST 'pass-test-update :: fail.test' FAILED ********************
+# CHECK-NEXT: # {{R}}UN: at line 1
+# CHECK-NEXT: not echo "fail"
+# CHECK-NEXT: # executed command: not echo fail
+# CHECK-NEXT: # .---command stdout------------
+# CHECK-NEXT: # | fail
+# CHECK-NEXT: # `-----------------------------
+# CHECK-NEXT: # error: command failed with exit status: 1
+# CHECK-NEXT: Exception occurred in test updater:
+# CHECK-NEXT: Traceback (most recent call last):
+# CHECK-NEXT: File {{.*}}, line {{.*}}, in {{.*}}
+# CHECK-NEXT: update_output = test_updater(result, test)
+# CHECK-NEXT: File "{{.*}}/should_not_run.py", line {{.*}}, in should_not_run
+# CHECK-NEXT: raise Exception("this test updater should only run on failure")
+# CHECK-NEXT: Exception: this test updater should only run on failure
+# CHECK-EMPTY:
+# CHECK-NEXT: ********************
+# CHECK-NEXT: PASS: pass-test-update :: pass-silent.test (2 of 5)
+# CHECK-NEXT: PASS: pass-test-update :: pass.test (3 of 5)
+# CHECK-NEXT: {{X}}FAIL: pass-test-update :: xfail.test (4 of 5)
+# CHECK-NEXT: XPASS: pass-test-update :: xpass.test (5 of 5)
+# CHECK-NEXT: ******************** TEST 'pass-test-update :: xpass.test' FAILED ********************
+# CHECK-NEXT: Exit Code: 0
+# CHECK-EMPTY:
+# CHECK-NEXT: Command Output (stdout):
+# CHECK-NEXT: --
+# CHECK-NEXT: # {{R}}UN: at line 2
+# CHECK-NEXT: echo "accidentally passed"
+# CHECK-NEXT: # executed command: echo 'accidentally passed'
+# CHECK-NEXT: # .---command stdout------------
+# CHECK-NEXT: # | accidentally passed
+# CHECK-NEXT: # `-----------------------------
+# CHECK-EMPTY:
+# CHECK-NEXT: --
+# CHECK-EMPTY:
+# CHECK-NEXT: ********************
+
|
|
✅ With the latest revision this PR passed the Python code formatter. |
jroelofs
reviewed
Aug 24, 2025
jroelofs
approved these changes
Aug 25, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/175/builds/24070 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/137/builds/24222 Here is the relevant piece of the build log for the reference |
|
follow-up to address buildbot failures: #155303 |
dyung
added a commit
to dyung/llvm-project
that referenced
this pull request
Aug 26, 2025
Should fix Windows build bot failures such as https://lab.llvm.org/buildbot/#/builders/46/builds/22281.
dyung
added a commit
that referenced
this pull request
Aug 26, 2025
…155354) Should fix Windows build bot failures such as https://lab.llvm.org/buildbot/#/builders/46/builds/22281. The test (and the followup fix in #155303) did not properly account for Windows style path separators.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The early exit we relied on to only invoke test updaters for failing tests requires that there was no output to stdout or stderr, and that timeouts weren't enabled. When these conditions weren't fulfilled, test updaters would be invoked even on passing or XFAILed tests.