Skip to content

Revert "[Utils] Add new --update-tests flag to llvm-lit" #110772

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 2, 2024

Conversation

hnrklssn
Copy link
Member

@hnrklssn hnrklssn commented Oct 2, 2024

Reverts #108425

@hnrklssn hnrklssn merged commit e495231 into main Oct 2, 2024
6 of 8 checks passed
@llvmbot llvmbot added clang Clang issues not falling into any other category llvm-lit testing-tools labels Oct 2, 2024
@hnrklssn hnrklssn deleted the revert-108425-lit-update-tests branch October 2, 2024 00:14
@llvmbot
Copy link
Member

llvmbot commented Oct 2, 2024

@llvm/pr-subscribers-clang

Author: Henrik G. Olsson (hnrklssn)

Changes

Reverts llvm/llvm-project#108425


Full diff: https://github.com/llvm/llvm-project/pull/110772.diff

9 Files Affected:

  • (modified) clang/test/lit.cfg.py (-10)
  • (modified) llvm/docs/CommandGuide/lit.rst (-5)
  • (modified) llvm/test/lit.cfg.py (-10)
  • (modified) llvm/utils/lit/lit/LitConfig.py (-3)
  • (modified) llvm/utils/lit/lit/TestRunner.py (-12)
  • (modified) llvm/utils/lit/lit/cl_arguments.py (-6)
  • (modified) llvm/utils/lit/lit/llvm/config.py (-5)
  • (modified) llvm/utils/lit/lit/main.py (-1)
  • (modified) llvm/utils/update_any_test_checks.py (+3-51)
diff --git a/clang/test/lit.cfg.py b/clang/test/lit.cfg.py
index 32ed5239b90795..92a3361ce672e2 100644
--- a/clang/test/lit.cfg.py
+++ b/clang/test/lit.cfg.py
@@ -362,13 +362,3 @@ 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 dadecef567b7c9..c9d5baba3e2f49 100644
--- a/llvm/docs/CommandGuide/lit.rst
+++ b/llvm/docs/CommandGuide/lit.rst
@@ -313,11 +313,6 @@ ADDITIONAL OPTIONS
 
  List all of the discovered tests and exit.
 
-.. option:: --update-tests
-
- Pass failing tests to functions in the ``lit_config.update_tests`` 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 1d5b2bcae1b766..5a03a85386e0aa 100644
--- a/llvm/test/lit.cfg.py
+++ b/llvm/test/lit.cfg.py
@@ -630,13 +630,3 @@ def have_ld64_plugin_support():
 
 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/LitConfig.py b/llvm/utils/lit/lit/LitConfig.py
index 198a2bf3172330..5dc712ae28370c 100644
--- a/llvm/utils/lit/lit/LitConfig.py
+++ b/llvm/utils/lit/lit/LitConfig.py
@@ -38,7 +38,6 @@ def __init__(
         parallelism_groups={},
         per_test_coverage=False,
         gtest_sharding=True,
-        update_tests=False,
     ):
         # The name of the test runner.
         self.progname = progname
@@ -90,8 +89,6 @@ 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 = []
 
     @property
     def maxIndividualTestTime(self):
diff --git a/llvm/utils/lit/lit/TestRunner.py b/llvm/utils/lit/lit/TestRunner.py
index 3a2cdc5026b0c2..a1785073547ad0 100644
--- a/llvm/utils/lit/lit/TestRunner.py
+++ b/llvm/utils/lit/lit/TestRunner.py
@@ -1190,18 +1190,6 @@ 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 dcbe553c6d4827..ed78256ee414b4 100644
--- a/llvm/utils/lit/lit/cl_arguments.py
+++ b/llvm/utils/lit/lit/cl_arguments.py
@@ -204,12 +204,6 @@ 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 c05ec1664d4c45..5f762ec7f3514a 100644
--- a/llvm/utils/lit/lit/llvm/config.py
+++ b/llvm/utils/lit/lit/llvm/config.py
@@ -64,17 +64,12 @@ 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 745e376de7d529..24ba804f0c363f 100755
--- a/llvm/utils/lit/lit/main.py
+++ b/llvm/utils/lit/lit/main.py
@@ -42,7 +42,6 @@ def main(builtin_params={}):
         config_prefix=opts.configPrefix,
         per_test_coverage=opts.per_test_coverage,
         gtest_sharding=opts.gtest_sharding,
-        update_tests=opts.update_tests,
     )
 
     discovered_tests = lit.discovery.find_tests_for_inputs(
diff --git a/llvm/utils/update_any_test_checks.py b/llvm/utils/update_any_test_checks.py
index 76fe3365939290..e8eef1a46c504f 100755
--- a/llvm/utils/update_any_test_checks.py
+++ b/llvm/utils/update_any_test_checks.py
@@ -34,12 +34,9 @@ def find_utc_tool(search_path, utc_name):
     return None
 
 
-def run_utc_tool(utc_name, utc_tool, testname, environment):
+def run_utc_tool(utc_name, utc_tool, testname):
     result = subprocess.run(
-        [utc_tool, testname],
-        stdout=subprocess.PIPE,
-        stderr=subprocess.PIPE,
-        env=environment,
+        [utc_tool, testname], stdout=subprocess.PIPE, stderr=subprocess.PIPE
     )
     return (result.returncode, result.stdout, result.stderr)
 
@@ -63,42 +60,6 @@ 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
 
@@ -117,11 +78,6 @@ 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()
 
@@ -132,10 +88,6 @@ 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
@@ -165,7 +117,7 @@ def main():
                         continue
 
                 future = executor.submit(
-                    run_utc_tool, utc_name, utc_tools[utc_name], testname, local_env
+                    run_utc_tool, utc_name, utc_tools[utc_name], testname
                 )
                 jobs.append((testname, future))
 

@llvmbot
Copy link
Member

llvmbot commented Oct 2, 2024

@llvm/pr-subscribers-testing-tools

Author: Henrik G. Olsson (hnrklssn)

Changes

Reverts llvm/llvm-project#108425


Full diff: https://github.com/llvm/llvm-project/pull/110772.diff

9 Files Affected:

  • (modified) clang/test/lit.cfg.py (-10)
  • (modified) llvm/docs/CommandGuide/lit.rst (-5)
  • (modified) llvm/test/lit.cfg.py (-10)
  • (modified) llvm/utils/lit/lit/LitConfig.py (-3)
  • (modified) llvm/utils/lit/lit/TestRunner.py (-12)
  • (modified) llvm/utils/lit/lit/cl_arguments.py (-6)
  • (modified) llvm/utils/lit/lit/llvm/config.py (-5)
  • (modified) llvm/utils/lit/lit/main.py (-1)
  • (modified) llvm/utils/update_any_test_checks.py (+3-51)
diff --git a/clang/test/lit.cfg.py b/clang/test/lit.cfg.py
index 32ed5239b90795..92a3361ce672e2 100644
--- a/clang/test/lit.cfg.py
+++ b/clang/test/lit.cfg.py
@@ -362,13 +362,3 @@ 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 dadecef567b7c9..c9d5baba3e2f49 100644
--- a/llvm/docs/CommandGuide/lit.rst
+++ b/llvm/docs/CommandGuide/lit.rst
@@ -313,11 +313,6 @@ ADDITIONAL OPTIONS
 
  List all of the discovered tests and exit.
 
-.. option:: --update-tests
-
- Pass failing tests to functions in the ``lit_config.update_tests`` 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 1d5b2bcae1b766..5a03a85386e0aa 100644
--- a/llvm/test/lit.cfg.py
+++ b/llvm/test/lit.cfg.py
@@ -630,13 +630,3 @@ def have_ld64_plugin_support():
 
 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/LitConfig.py b/llvm/utils/lit/lit/LitConfig.py
index 198a2bf3172330..5dc712ae28370c 100644
--- a/llvm/utils/lit/lit/LitConfig.py
+++ b/llvm/utils/lit/lit/LitConfig.py
@@ -38,7 +38,6 @@ def __init__(
         parallelism_groups={},
         per_test_coverage=False,
         gtest_sharding=True,
-        update_tests=False,
     ):
         # The name of the test runner.
         self.progname = progname
@@ -90,8 +89,6 @@ 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 = []
 
     @property
     def maxIndividualTestTime(self):
diff --git a/llvm/utils/lit/lit/TestRunner.py b/llvm/utils/lit/lit/TestRunner.py
index 3a2cdc5026b0c2..a1785073547ad0 100644
--- a/llvm/utils/lit/lit/TestRunner.py
+++ b/llvm/utils/lit/lit/TestRunner.py
@@ -1190,18 +1190,6 @@ 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 dcbe553c6d4827..ed78256ee414b4 100644
--- a/llvm/utils/lit/lit/cl_arguments.py
+++ b/llvm/utils/lit/lit/cl_arguments.py
@@ -204,12 +204,6 @@ 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 c05ec1664d4c45..5f762ec7f3514a 100644
--- a/llvm/utils/lit/lit/llvm/config.py
+++ b/llvm/utils/lit/lit/llvm/config.py
@@ -64,17 +64,12 @@ 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 745e376de7d529..24ba804f0c363f 100755
--- a/llvm/utils/lit/lit/main.py
+++ b/llvm/utils/lit/lit/main.py
@@ -42,7 +42,6 @@ def main(builtin_params={}):
         config_prefix=opts.configPrefix,
         per_test_coverage=opts.per_test_coverage,
         gtest_sharding=opts.gtest_sharding,
-        update_tests=opts.update_tests,
     )
 
     discovered_tests = lit.discovery.find_tests_for_inputs(
diff --git a/llvm/utils/update_any_test_checks.py b/llvm/utils/update_any_test_checks.py
index 76fe3365939290..e8eef1a46c504f 100755
--- a/llvm/utils/update_any_test_checks.py
+++ b/llvm/utils/update_any_test_checks.py
@@ -34,12 +34,9 @@ def find_utc_tool(search_path, utc_name):
     return None
 
 
-def run_utc_tool(utc_name, utc_tool, testname, environment):
+def run_utc_tool(utc_name, utc_tool, testname):
     result = subprocess.run(
-        [utc_tool, testname],
-        stdout=subprocess.PIPE,
-        stderr=subprocess.PIPE,
-        env=environment,
+        [utc_tool, testname], stdout=subprocess.PIPE, stderr=subprocess.PIPE
     )
     return (result.returncode, result.stdout, result.stderr)
 
@@ -63,42 +60,6 @@ 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
 
@@ -117,11 +78,6 @@ 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()
 
@@ -132,10 +88,6 @@ 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
@@ -165,7 +117,7 @@ def main():
                         continue
 
                 future = executor.submit(
-                    run_utc_tool, utc_name, utc_tools[utc_name], testname, local_env
+                    run_utc_tool, utc_name, utc_tools[utc_name], testname
                 )
                 jobs.append((testname, future))
 

Sterling-Augustine pushed a commit to Sterling-Augustine/llvm-project that referenced this pull request Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang Clang issues not falling into any other category llvm-lit testing-tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants