[Utils] Adds support for diff based tests to lit's --update-tests #154147
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
This reverts commit e495231 to reland the --update-tests feature, originally landed in llvm#108425.
This adds an updater to lit's --update-tests flag with support for `diff`. If a RUN line containing the `diff` command fails, this function will use heuristics to try to deduce which file is the "reference" file, and copy the contents of the other file to the reference. If it cannot deduce which file is the reference file, it does nothing. The heuristics are currently: - does one of the files end in .expected while the other does not? Then the .expected file is the reference. - does one of the file paths contain the substring ".tmp" while the other does not? Then the file not containing ".tmp" is the reference. This matches cases where one file path is constructed using the `%t` substitution.
llvmbot
added
clang
|
@llvm/pr-subscribers-testing-tools @llvm/pr-subscribers-clang Author: Henrik G. Olsson (hnrklssn) ChangesThis adds an updater to lit's --update-tests flag with support for The heuristics are currently:
Full diff: https://github.com/llvm/llvm-project/pull/154147.diff 20 Files Affected:
diff --git a/clang/test/lit.cfg.py b/clang/test/lit.cfg.py
index 1957bb1715eb6..12e4d0e454270 100644
--- a/clang/test/lit.cfg.py
+++ b/clang/test/lit.cfg.py
@@ -410,3 +410,13 @@ def calculate_arch_features(arch_string):
# possibly be present in system and user configuration files, so disable
# default configs for the test runs.
config.environment["CLANG_NO_DEFAULT_CONFIG"] = "1"
+
+if lit_config.update_tests:
+ import sys
+ import os
+
+ utilspath = os.path.join(config.llvm_src_root, "utils")
+ sys.path.append(utilspath)
+ from update_any_test_checks import utc_lit_plugin
+
+ lit_config.test_updaters.append(utc_lit_plugin)
diff --git a/llvm/docs/CommandGuide/lit.rst b/llvm/docs/CommandGuide/lit.rst
index eb90e950a3770..15c249d8e6d31 100644
--- a/llvm/docs/CommandGuide/lit.rst
+++ b/llvm/docs/CommandGuide/lit.rst
@@ -399,6 +399,11 @@ ADDITIONAL OPTIONS
Show all features used in the test suite (in ``XFAIL``, ``UNSUPPORTED`` and
``REQUIRES``) and exit.
+.. option:: --update-tests
+
+ Pass failing tests to functions in the ``lit_config.test_updaters`` list to
+ check whether any of them know how to update the test to make it pass.
+
EXIT STATUS
-----------
diff --git a/llvm/test/lit.cfg.py b/llvm/test/lit.cfg.py
index 8c2d1a454e8f9..bc240425d6d0e 100644
--- a/llvm/test/lit.cfg.py
+++ b/llvm/test/lit.cfg.py
@@ -715,3 +715,13 @@ def host_unwind_supports_jit():
if config.has_logf128:
config.available_features.add("has_logf128")
+
+if lit_config.update_tests:
+ import sys
+ import os
+
+ utilspath = os.path.join(config.llvm_src_root, "utils")
+ sys.path.append(utilspath)
+ from update_any_test_checks import utc_lit_plugin
+
+ lit_config.test_updaters.append(utc_lit_plugin)
diff --git a/llvm/utils/lit/lit/DiffUpdater.py b/llvm/utils/lit/lit/DiffUpdater.py
new file mode 100644
index 0000000000000..2e0a71e1938e3
--- /dev/null
+++ b/llvm/utils/lit/lit/DiffUpdater.py
@@ -0,0 +1,39 @@
+import shutil
+
+
+def get_source_and_target(a, b):
+ """
+ Try to figure out which file is the test output and which is the reference.
+ """
+ expected_suffix = ".expected"
+ if a.endswith(expected_suffix) and not b.endswith(expected_suffix):
+ return b, a
+ if b.endswith(expected_suffix) and not a.endswith(expected_suffix):
+ return a, b
+
+ tmp_substr = ".tmp"
+ if tmp_substr in a and not tmp_substr in b:
+ return a, b
+ if tmp_substr in b and not tmp_substr in a:
+ return b, a
+
+ return None
+
+
+def filter_flags(args):
+ return [arg for arg in args if not arg.startswith("-")]
+
+
+def diff_test_updater(result, test):
+ args = filter_flags(result.command.args)
+ if len(args) != 3:
+ return None
+ [cmd, a, b] = args
+ if cmd != "diff":
+ return None
+ res = get_source_and_target(a, b)
+ if not res:
+ return f"update-diff-test: could not deduce source and target from {a} and {b}"
+ source, target = res
+ shutil.copy(source, target)
+ return f"update-diff-test: copied {source} to {target}"
diff --git a/llvm/utils/lit/lit/LitConfig.py b/llvm/utils/lit/lit/LitConfig.py
index cb4aef6f72a87..8cef3c1fd8569 100644
--- a/llvm/utils/lit/lit/LitConfig.py
+++ b/llvm/utils/lit/lit/LitConfig.py
@@ -8,6 +8,7 @@
import lit.formats
import lit.TestingConfig
import lit.util
+from lit.DiffUpdater import diff_test_updater
# LitConfig must be a new style class for properties to work
class LitConfig(object):
@@ -39,6 +40,7 @@ def __init__(
parallelism_groups={},
per_test_coverage=False,
gtest_sharding=True,
+ update_tests=False,
):
# The name of the test runner.
self.progname = progname
@@ -91,6 +93,8 @@ def __init__(
self.parallelism_groups = parallelism_groups
self.per_test_coverage = per_test_coverage
self.gtest_sharding = bool(gtest_sharding)
+ self.update_tests = update_tests
+ self.test_updaters = [diff_test_updater]
@property
def maxIndividualTestTime(self):
diff --git a/llvm/utils/lit/lit/TestRunner.py b/llvm/utils/lit/lit/TestRunner.py
index e7cd70766a3dd..f2c5c6d0dbe93 100644
--- a/llvm/utils/lit/lit/TestRunner.py
+++ b/llvm/utils/lit/lit/TestRunner.py
@@ -1192,6 +1192,18 @@ def executeScriptInternal(
str(result.timeoutReached),
)
+ if litConfig.update_tests:
+ 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
+ if update_output:
+ for line in update_output.splitlines():
+ out += f"# {line}\n"
+ break
+
return out, err, exitCode, timeoutInfo
diff --git a/llvm/utils/lit/lit/cl_arguments.py b/llvm/utils/lit/lit/cl_arguments.py
index e88951520e660..8f9211ee3f538 100644
--- a/llvm/utils/lit/lit/cl_arguments.py
+++ b/llvm/utils/lit/lit/cl_arguments.py
@@ -230,6 +230,12 @@ def parse_args():
action="store_true",
help="Exit with status zero even if some tests fail",
)
+ execution_group.add_argument(
+ "--update-tests",
+ dest="update_tests",
+ action="store_true",
+ help="Try to update regression tests to reflect current behavior, if possible",
+ )
execution_test_time_group = execution_group.add_mutually_exclusive_group()
execution_test_time_group.add_argument(
"--skip-test-time-recording",
diff --git a/llvm/utils/lit/lit/llvm/config.py b/llvm/utils/lit/lit/llvm/config.py
index 649636d4bcf4c..44119ec8c0eca 100644
--- a/llvm/utils/lit/lit/llvm/config.py
+++ b/llvm/utils/lit/lit/llvm/config.py
@@ -64,12 +64,17 @@ def __init__(self, lit_config, config):
self.with_environment("_TAG_REDIR_ERR", "TXT")
self.with_environment("_CEE_RUNOPTS", "FILETAG(AUTOCVT,AUTOTAG) POSIX(ON)")
+ if lit_config.update_tests:
+ self.use_lit_shell = True
+
# Choose between lit's internal shell pipeline runner and a real shell.
# If LIT_USE_INTERNAL_SHELL is in the environment, we use that as an
# override.
lit_shell_env = os.environ.get("LIT_USE_INTERNAL_SHELL")
if lit_shell_env:
self.use_lit_shell = lit.util.pythonize_bool(lit_shell_env)
+ if not self.use_lit_shell and lit_config.update_tests:
+ print("note: --update-tests is not supported when using external shell")
if not self.use_lit_shell:
features.add("shell")
diff --git a/llvm/utils/lit/lit/main.py b/llvm/utils/lit/lit/main.py
index 9650a0e901173..5255e2c5e1b51 100755
--- a/llvm/utils/lit/lit/main.py
+++ b/llvm/utils/lit/lit/main.py
@@ -43,6 +43,7 @@ def main(builtin_params={}):
per_test_coverage=opts.per_test_coverage,
gtest_sharding=opts.gtest_sharding,
maxRetriesPerTest=opts.maxRetriesPerTest,
+ update_tests=opts.update_tests,
)
discovered_tests = lit.discovery.find_tests_for_inputs(
diff --git a/llvm/utils/lit/tests/Inputs/diff-test-update/.gitignore b/llvm/utils/lit/tests/Inputs/diff-test-update/.gitignore
new file mode 100644
index 0000000000000..2211df63dd283
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/diff-test-update/.gitignore
@@ -0,0 +1 @@
+*.txt
diff --git a/llvm/utils/lit/tests/Inputs/diff-test-update/1.in b/llvm/utils/lit/tests/Inputs/diff-test-update/1.in
new file mode 100644
index 0000000000000..b7d6715e2df11
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/diff-test-update/1.in
@@ -0,0 +1 @@
+FOO
diff --git a/llvm/utils/lit/tests/Inputs/diff-test-update/2.in b/llvm/utils/lit/tests/Inputs/diff-test-update/2.in
new file mode 100644
index 0000000000000..ba578e48b1836
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/diff-test-update/2.in
@@ -0,0 +1 @@
+BAR
diff --git a/llvm/utils/lit/tests/Inputs/diff-test-update/diff-bail.test b/llvm/utils/lit/tests/Inputs/diff-test-update/diff-bail.test
new file mode 100644
index 0000000000000..ded931384f192
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/diff-test-update/diff-bail.test
@@ -0,0 +1,3 @@
+# There is no indication here of which file is the reference file to update
+# RUN: diff %S/1.in %S/2.in
+
diff --git a/llvm/utils/lit/tests/Inputs/diff-test-update/diff-bail2.test b/llvm/utils/lit/tests/Inputs/diff-test-update/diff-bail2.test
new file mode 100644
index 0000000000000..26e12a3b2b289
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/diff-test-update/diff-bail2.test
@@ -0,0 +1,7 @@
+# RUN: mkdir %t
+# RUN: cp %S/1.in %t/1.txt
+# RUN: cp %S/2.in %t/2.txt
+
+# There is no indication here of which file is the reference file to update
+# RUN: diff %t/1.txt %t/2.txt
+
diff --git a/llvm/utils/lit/tests/Inputs/diff-test-update/diff-expected.test b/llvm/utils/lit/tests/Inputs/diff-test-update/diff-expected.test
new file mode 100644
index 0000000000000..a26c6d338f964
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/diff-test-update/diff-expected.test
@@ -0,0 +1,5 @@
+# RUN: mkdir %t
+# RUN: cp %S/1.in %t/my-file.expected
+# RUN: cp %S/2.in %t/my-file.txt
+# RUN: diff %t/my-file.expected %t/my-file.txt
+
diff --git a/llvm/utils/lit/tests/Inputs/diff-test-update/diff-tmp-dir.test b/llvm/utils/lit/tests/Inputs/diff-test-update/diff-tmp-dir.test
new file mode 100644
index 0000000000000..929c2c1c6c7d3
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/diff-test-update/diff-tmp-dir.test
@@ -0,0 +1,6 @@
+# RUN: mkdir %t
+# RUN: touch %S/empty.txt
+# RUN: cp %S/1.in %t/1.txt
+
+# RUN: diff %t/1.txt %S/empty.txt
+
diff --git a/llvm/utils/lit/tests/Inputs/diff-test-update/diff-tmp.test b/llvm/utils/lit/tests/Inputs/diff-test-update/diff-tmp.test
new file mode 100644
index 0000000000000..042bf244ebaa1
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/diff-test-update/diff-tmp.test
@@ -0,0 +1,3 @@
+# RUN: cp %S/1.in %t.txt
+# RUN: cp %S/2.in %S/diff-t-out.txt
+# RUN: diff %t.txt %S/diff-t-out.txt
diff --git a/llvm/utils/lit/tests/Inputs/diff-test-update/lit.cfg b/llvm/utils/lit/tests/Inputs/diff-test-update/lit.cfg
new file mode 100644
index 0000000000000..9bd255276638a
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/diff-test-update/lit.cfg
@@ -0,0 +1,8 @@
+import lit.formats
+
+config.name = "diff-test-update"
+config.suffixes = [".test"]
+config.test_format = lit.formats.ShTest()
+config.test_source_root = None
+config.test_exec_root = None
+
diff --git a/llvm/utils/lit/tests/diff-test-update.py b/llvm/utils/lit/tests/diff-test-update.py
new file mode 100644
index 0000000000000..21b869d120655
--- /dev/null
+++ b/llvm/utils/lit/tests/diff-test-update.py
@@ -0,0 +1,10 @@
+# RUN: not %{lit} --update-tests -v %S/Inputs/diff-test-update | FileCheck %s
+
+# CHECK: # update-diff-test: could not deduce source and target from {{.*}}/Inputs/diff-test-update/1.in and {{.*}}/Inputs/diff-test-update/2.in
+# CHECK: # update-diff-test: could not deduce source and target from {{.*}}/diff-test-update/Output/diff-bail2.test.tmp/1.txt and {{.*}}/diff-test-update/Output/diff-bail2.test.tmp/2.txt
+# CHECK: # update-diff-test: copied {{.*}}/Output/diff-expected.test.tmp/my-file.txt to {{.*}}/Output/diff-expected.test.tmp/my-file.expected
+# CHECK: # update-diff-test: copied {{.*}}/Output/diff-tmp-dir.test.tmp/1.txt to {{.*}}/Inputs/diff-test-update/empty.txt
+# CHECK: # update-diff-test: copied {{.*}}/Inputs/diff-test-update/Output/diff-tmp.test.tmp.txt to {{.*}}/Inputs/diff-test-update/diff-t-out.txt
+
+
+# CHECK: Failed: 5 (100.00%)
diff --git a/llvm/utils/update_any_test_checks.py b/llvm/utils/update_any_test_checks.py
index e8eef1a46c504..76fe336593929 100755
--- a/llvm/utils/update_any_test_checks.py
+++ b/llvm/utils/update_any_test_checks.py
@@ -34,9 +34,12 @@ def find_utc_tool(search_path, utc_name):
return None
-def run_utc_tool(utc_name, utc_tool, testname):
+def run_utc_tool(utc_name, utc_tool, testname, environment):
result = subprocess.run(
- [utc_tool, testname], stdout=subprocess.PIPE, stderr=subprocess.PIPE
+ [utc_tool, testname],
+ stdout=subprocess.PIPE,
+ stderr=subprocess.PIPE,
+ env=environment,
)
return (result.returncode, result.stdout, result.stderr)
@@ -60,6 +63,42 @@ def expand_listfile_args(arg_list):
return exp_arg_list
+def utc_lit_plugin(result, test):
+ testname = test.getFilePath()
+ if not testname:
+ return None
+
+ script_name = os.path.abspath(__file__)
+ utc_search_path = os.path.join(os.path.dirname(script_name), os.path.pardir)
+
+ with open(testname, "r") as f:
+ header = f.readline().strip()
+
+ m = RE_ASSERTIONS.search(header)
+ if m is None:
+ return None
+
+ utc_name = m.group(1)
+ utc_tool = find_utc_tool([utc_search_path], utc_name)
+ if not utc_tool:
+ return f"update-utc-tests: {utc_name} not found"
+
+ return_code, stdout, stderr = run_utc_tool(
+ utc_name, utc_tool, testname, test.config.environment
+ )
+
+ stderr = stderr.decode(errors="replace")
+ if return_code != 0:
+ if stderr:
+ return f"update-utc-tests: {utc_name} exited with return code {return_code}\n{stderr.rstrip()}"
+ return f"update-utc-tests: {utc_name} exited with return code {return_code}"
+
+ stdout = stdout.decode(errors="replace")
+ if stdout:
+ return f"update-utc-tests: updated {testname}\n{stdout.rstrip()}"
+ return f"update-utc-tests: updated {testname}"
+
+
def main():
from argparse import RawTextHelpFormatter
@@ -78,6 +117,11 @@ def main():
nargs="*",
help="Additional directories to scan for update_*_test_checks scripts",
)
+ parser.add_argument(
+ "--path",
+ help="""Additional directories to scan for executables invoked by the update_*_test_checks scripts,
+separated by the platform path separator""",
+ )
parser.add_argument("tests", nargs="+")
config = parser.parse_args()
@@ -88,6 +132,10 @@ def main():
script_name = os.path.abspath(__file__)
utc_search_path.append(os.path.join(os.path.dirname(script_name), os.path.pardir))
+ local_env = os.environ.copy()
+ if config.path:
+ local_env["PATH"] = config.path + os.pathsep + local_env["PATH"]
+
not_autogenerated = []
utc_tools = {}
have_error = False
@@ -117,7 +165,7 @@ def main():
continue
future = executor.submit(
- run_utc_tool, utc_name, utc_tools[utc_name], testname
+ run_utc_tool, utc_name, utc_tools[utc_name], testname, local_env
)
jobs.append((testname, future))
|
|
This is stacked on top of #153821 |
|
✅ With the latest revision this PR passed the Python code formatter. |
|
ping :) |
|
I'm interpreting the silence as nobody having issues with this. Feel free to post-review if that's not the case! |
jroelofs
reviewed
Sep 8, 2025
| @@ -0,0 +1 @@ | |||
| *.txt | |||
There was a problem hiding this comment.
%t's are unique, so where does this take effect?
There was a problem hiding this comment.
llvm/utils/lit/tests/Inputs/diff-test-update/diff-tmp-dir.test creates an empty file at %S/empty.txt to have a file outside %t (to test the heuristic that if one file is in %t and the other isn't, the %t file is the one to copy from) that can be overwritten by the test without triggering a git diff. But now that you mention it I realise that needs to be erased if it has been written to by a previous test run... BRB creating a follow-up
There was a problem hiding this comment.
I guess I could also make it more explicit and mark only empty.txt as gitignored
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/46/builds/22989 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/94/builds/10633 Here is the relevant piece of the build log for the reference |
dyung
reviewed
Sep 9, 2025
| @@ -0,0 +1,10 @@ | |||
| # RUN: not %{lit} --update-tests -v %S/Inputs/diff-test-update | FileCheck %s | |||
|
|
|||
| # CHECK: # update-diff-test: could not deduce source and target from {{.*}}/Inputs/diff-test-update/1.in and {{.*}}/Inputs/diff-test-update/2.in | |||
There was a problem hiding this comment.
The CHECK lines need to be updated to account for Windows path separators.
# update-diff-test: could not deduce source and target from Z:\b\llvm-clang-x86_64-sie-win\build\utils\lit\tests\Inputs\diff-test-update/1.in and Z:\b\llvm-clang-x86_64-sie-win\build\utils\lit\tests\Inputs\diff-test-update/2.in
There was a problem hiding this comment.
yup, should be addressed in #157576
hnrklssn
added a commit
to hnrklssn/llvm-project
that referenced
this pull request
Sep 11, 2025
…vm#154147) This adds an updater to lit's --update-tests flag with support for `diff`. If a RUN line containing the `diff` command fails, this function will use heuristics to try to deduce which file is the "reference" file, and copy the contents of the other file to the reference. If it cannot deduce which file is the reference file, it does nothing. The heuristics are currently: - does one of the files end in .expected while the other does not? Then the .expected file is the reference. - does one of the file paths contain the substring ".tmp" while the other does not? Then the file not containing ".tmp" is the reference. This matches cases where one file path is constructed using the `%t` substitution. (cherry picked from commit 6c3f18e)
hnrklssn
added a commit
to hnrklssn/llvm-project
that referenced
this pull request
Sep 12, 2025
…vm#154147) This adds an updater to lit's --update-tests flag with support for `diff`. If a RUN line containing the `diff` command fails, this function will use heuristics to try to deduce which file is the "reference" file, and copy the contents of the other file to the reference. If it cannot deduce which file is the reference file, it does nothing. The heuristics are currently: - does one of the files end in .expected while the other does not? Then the .expected file is the reference. - does one of the file paths contain the substring ".tmp" while the other does not? Then the file not containing ".tmp" is the reference. This matches cases where one file path is constructed using the `%t` substitution. (cherry picked from commit 6c3f18e)
kateinoigakukun
pushed a commit
to kateinoigakukun/llvm-project
that referenced
this pull request
Sep 17, 2025
…vm#154147) This adds an updater to lit's --update-tests flag with support for `diff`. If a RUN line containing the `diff` command fails, this function will use heuristics to try to deduce which file is the "reference" file, and copy the contents of the other file to the reference. If it cannot deduce which file is the reference file, it does nothing. The heuristics are currently: - does one of the files end in .expected while the other does not? Then the .expected file is the reference. - does one of the file paths contain the substring ".tmp" while the other does not? Then the file not containing ".tmp" is the reference. This matches cases where one file path is constructed using the `%t` substitution. (cherry picked from commit 6c3f18e)
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.
This adds an updater to lit's --update-tests flag with support for
diff. If a RUN line containing thediffcommand fails, this functionwill use heuristics to try to deduce which file is the "reference" file,
and copy the contents of the other file to the reference. If it cannot
deduce which file is the reference file, it does nothing.
The heuristics are currently:
the .expected file is the reference.
other does not? Then the file not containing ".tmp" is the reference.
This matches cases where one file path is constructed using the
%tsubstitution.