[lit] Remove the --no-indirectly-run-check option

[ldionne]

Summary

This option had originally been added in D83069 to allow disabling the
check that something is going to get run at all when a specific test name
is used on the command-line. Since we now use getTestsForPath() (from D151664)
to get the tests to run for a specific path, we don't need a specific check
for this anymore -- Lit will produce the same complaint it would produce if
you provided a directory with no tests.

If one needs to run a specific test on the command-line and the Lit
configuration would normally not include that test, the configuration
should be set up as a "standalone" configuration or it should be fixed
to allow for that test to be found (i.e. probably fix the allowed test
suffixes).

[ldionne]

Rebase to poke CI

[ldionne]

Rebase onto main.

[ldionne]

Ping, I'd like to do this before the LLVM 17 release.

[yln]

I wasn't aware of the --no-indirectly-run-check flag or the problem it was trying to solve, but it sounds like it was a clutch in the first place. Glad to see that your change D151664 unified this case (running single test by name) so we can obsolete it!

[jdenny]

llvm/docs/CommandGuide/lit.rst

462

Needs to be updated.

llvm/utils/lit/lit/discovery.py

170–171

If I understand correctly, if a directory is in standalone mode because tests are always run individually, it is not possible to use a test format that overrides getTestsForPaths in order to generate multiple tests from each of those individual tests. Is that correct? I'm not saying that's problematic. I just want to be sure I understand.

[ldionne]

llvm/utils/lit/lit/discovery.py

170–171

Yes, that is correct. TBH I am also skeptical about the usefulness of "standalone" tests in the first place, it seems like they were introduced as a clutch for people not wanting to have appropriate suffixes and lit excludes in the first place.

[ldionne]

Update doc.

[jdenny]

Other than the comment I just requested, LGTM. Thanks for the cleanup.

llvm/utils/lit/lit/discovery.py

170–171

Yes, that is correct.

I feel like the inability to use standalone mode with getTestsForPaths in that manner ought to be documented somewhere. What about at a getTestsForPaths definition in llvm/utils/lit/lit/formats/base.py?

TBH I am also skeptical about the usefulness of "standalone" tests in the first place, it seems like they were introduced as a clutch for people not wanting to have appropriate suffixes and lit excludes in the first place.

My understanding is that it's for use cases where lit is not doing test discovery, so it seems pointless to require configuring test suffixes and exclusions for lit. However, it's not a use case I directly have experience with, so I might be misunderstanding.

[ldionne]

Add comment to address review feedback.

[ldionne]

Thanks for the reviews!

llvm/docs/CommandGuide/lit.rst

  Track the wall time individual tests take to execute and includes the results
  in the summary output.  This is useful for determining which tests in a test
  suite take the most time to execute.
 
 .. option:: --ignore-fail
 
  Exit with status zero even if some tests fail.
 
-.. option:: --no-indirectly-run-check
-
- Do not error if a test would not be run if the user had specified the
- containing directory instead of naming the test directly.
-
 .. _selection-options:
 
 SELECTION OPTIONS
 -----------------
 
 By default, `lit` will run failing tests first, then run tests in descending
 execution time order to optimize concurrency.  The execution order can be
 changed using the :option:`--order` option.

  the object directory.  This is where tests will be run and temporary output files
  placed.
 
  **environment** A dictionary representing the environment to use when executing
  tests in the suite.
 
  **standalone_tests** When true, mark a directory with tests expected to be run
  standalone. Test discovery is disabled for that directory and
  *--no-indirectly-run-check* is in effect. *lit.suffixes* and *lit.excludes*
  must be empty when this variable is true.
 
  **suffixes** For **lit** test formats which scan directories for tests, this
  variable is a list of suffixes to identify test files.  Used by: *ShTest*.
 
  **substitutions** For **lit** test formats which substitute variables into a test
  script, the list of substitutions to perform.  Used by: *ShTest*.
 

llvm/utils/lit/lit/LitTestCase.py

lit_config = lit.LitConfig.LitConfig(
         noExecute=False,
         debug=False,
         isWindows=windows,
         order="smart",
         params={},
     )
 
     # Perform test discovery.
-    tests = lit.discovery.find_tests_for_inputs(lit_config, inputs, False)
+    tests = lit.discovery.find_tests_for_inputs(lit_config, inputs)
     test_adaptors = [LitTestCase(t, lit_config) for t in tests]
 
     # Return a unittest test suite which just runs the tests in order.
     return unittest.TestSuite(test_adaptors)

llvm/utils/lit/lit/cl_arguments.py

execution_group.add_argument(
         action="store_true",
     )
     execution_group.add_argument(
         "--ignore-fail",
         dest="ignoreFail",
         action="store_true",
         help="Exit with status zero even if some tests fail",
     )
-    execution_group.add_argument(
-        "--no-indirectly-run-check",
-        dest="indirectlyRunCheck",
-        help="Do not error if a test would not be run if the user had "
-        "specified the containing directory instead of naming the "
-        "test directly.",
-        action="store_false",
-    )
 
     selection_group = parser.add_argument_group("Test Selection")
     selection_group.add_argument(
         "--max-tests",
         metavar="N",
         help="Maximum number of tests to run",
         type=_positive_int,
     )

llvm/utils/lit/lit/discovery.py

def search(path_in_suite):
         res = cache.get(key)
         if res is None:
             cache[key] = res = search1(path_in_suite)
         return res
 
     return search(path_in_suite)
 
 
-def getTests(path, litConfig, testSuiteCache, localConfigCache, indirectlyRunCheck):
+def getTests(path, litConfig, testSuiteCache, localConfigCache):
     # Find the test suite for this input and its relative path.
     ts, path_in_suite = getTestSuite(path, litConfig, testSuiteCache)
     if ts is None:
         litConfig.warning("unable to find test suite for %r" % path)
         return (), ()
 
     if litConfig.debug:
         litConfig.note("resolved input %r to %r::%r" % (path, ts.name, path_in_suite))
 
     return ts, getTestsInSuite(
         ts,
         path_in_suite,
         litConfig,
         testSuiteCache,
         localConfigCache,
-        indirectlyRunCheck,
     )
 
 
 def getTestsInSuite(
-    ts, path_in_suite, litConfig, testSuiteCache, localConfigCache, indirectlyRunCheck
+    ts, path_in_suite, litConfig, testSuiteCache, localConfigCache
 ):
     # Check that the source path exists (errors here are reported by the
     # caller).
     source_path = ts.getSourcePath(path_in_suite)
     if not os.path.exists(source_path):
         return
 
     # Check if the user named a test directly.
     if not os.path.isdir(source_path):
         test_dir_in_suite = path_in_suite[:-1]
         lc = getLocalConfig(ts, test_dir_in_suite, litConfig, localConfigCache)
 
-        # TODO: Stop checking for indirectlyRunCheck and lc.standalone_tests here
-        #       once we remove --no-indirectly-run-check, which is not needed anymore
-        #       now that we error out when trying to run a test that wouldn't be
-        #       discovered in the directory.
-        fallbackOnSingleTest = lc.test_format is None or not indirectlyRunCheck or lc.standalone_tests
-        tests = [Test.Test(ts, path_in_suite, lc)] if fallbackOnSingleTest else \
+        # If we don't have a test format or if we are running standalone tests,
+        # always "find" the test itself. Otherwise, we might find no tests at
+        # all, which is considered an error but isn't an error with standalone
+        # tests.
+        tests = [Test.Test(ts, path_in_suite, lc)] if lc.test_format is None or lc.standalone_tests else \
                 lc.test_format.getTestsForPath(ts, path_in_suite, litConfig, lc)
 
         for test in tests:
-            # Issue a error if the specified test would not be run if
-            # the user had specified the containing directory instead of
-            # of naming the test directly. This helps to avoid writing
-            # tests which are not executed. The check adds some performance
-            # overhead which might be important if a large number of tests
-            # are being run directly.
-            # This check can be disabled by using --no-indirectly-run-check or
-            # setting the standalone_tests variable in the suite's configuration.
-            if (
-                indirectlyRunCheck
-                and lc.test_format is not None
-                and not lc.standalone_tests
-            ):
-                found = False
-                for res in lc.test_format.getTestsInDirectory(
-                    ts, test_dir_in_suite, litConfig, lc
-                ):
-                    if test.getFullName() == res.getFullName():
-                        found = True
-                        break
-                if not found:
-                    litConfig.error(
-                        "%r would not be run indirectly: change name or LIT config"
-                        "(e.g. suffixes or standalone_tests variables)" % test.getFullName()
-                    )
-
             yield test
         return
 
     # Otherwise we have a directory to search for tests, start by getting the
     # local configuration.
     lc = getLocalConfig(ts, path_in_suite, litConfig, localConfigCache)
 
     # Directory contains tests to be run standalone. Do not try to discover.
for filename in os.listdir(source_path):
         # Otherwise, load from the nested test suite, if present.
         if sub_ts is not None:
             subiter = getTestsInSuite(
                 sub_ts,
                 subpath_in_suite,
                 litConfig,
                 testSuiteCache,
                 localConfigCache,
-                indirectlyRunCheck,
             )
         else:
             subiter = getTestsInSuite(
                 ts,
                 subpath,
                 litConfig,
                 testSuiteCache,
                 localConfigCache,
-                indirectlyRunCheck,
             )
 
         N = 0
         for res in subiter:
             N += 1
             yield res
         if sub_ts and not N:
             litConfig.warning("test suite %r contained no tests" % sub_ts.name)
 
 
-def find_tests_for_inputs(lit_config, inputs, indirectlyRunCheck):
+def find_tests_for_inputs(lit_config, inputs):
     """
     find_tests_for_inputs(lit_config, inputs) -> [Test]
 
     Given a configuration object and a list of input specifiers, find all the
     tests to execute.
     """
 
     # Expand '@...' form in inputs.
def find_tests_for_inputs(lit_config, inputs):
     for input in actual_inputs:
         prev = len(tests)
         tests.extend(
             getTests(
                 input,
                 lit_config,
                 test_suite_cache,
                 local_config_cache,
-                indirectlyRunCheck,
             )[1]
         )
         if prev == len(tests):
             lit_config.warning("input %r contained no tests" % input)
 
     # This data is no longer needed but keeping it around causes awful
     # performance problems while the test suites run.
     for k, suite in test_suite_cache.items():

llvm/utils/lit/lit/main.py

lit_config = lit.LitConfig.LitConfig(
         isWindows=is_windows,
         order=opts.order,
         params=params,
         config_prefix=opts.configPrefix,
         echo_all_commands=opts.echoAllCommands,
     )
 
     discovered_tests = lit.discovery.find_tests_for_inputs(
-        lit_config, opts.test_paths, opts.indirectlyRunCheck
+        lit_config, opts.test_paths
     )
     if not discovered_tests:
         sys.stderr.write("error: did not discover any tests for provided path(s)\n")
         sys.exit(2)
 
     if opts.show_suites or opts.show_tests:
         print_discovered(discovered_tests, opts.show_suites, opts.show_tests)
         sys.exit(0)

llvm/utils/lit/tests/discovery.py

 #
 # RUN: not %{lit} \
 # RUN:     %{inputs}/discovery/test.not-txt 2>%t.err
 # RUN: FileCheck --check-prefix=CHECK-ERROR-INPUT-CONTAINED-NO-TESTS < %t.err %s
 #
 # CHECK-ERROR-INPUT-CONTAINED-NO-TESTS: warning: input 'Inputs/discovery/test.not-txt' contained no tests
 # CHECK-ERROR-INPUT-CONTAINED-NO-TESTS: error: did not discover any tests for provided path(s)
 
-# Check that no error is emitted with --no-indirectly-run-check.
-#
-# RUN: %{lit} \
-# RUN:     %{inputs}/discovery/test.not-txt --no-indirectly-run-check
-
 # Check that a standalone test with no suffixes set is run without any errors.
 #
 # RUN: %{lit} %{inputs}/standalone-tests/true.txt > %t.out
 # RUN: FileCheck --check-prefix=CHECK-STANDALONE < %t.out %s
 #
 # CHECK-STANDALONE: PASS: Standalone tests :: true.txt
 
 # Check that an error is produced if suffixes variable is set for a suite with

llvm/utils/lit/tests/unit/TestRunner.py

def load_keyword_parser_lit_tests():
             order="smart",
             params={},
         )
         TestIntegratedTestKeywordParser.litConfig = lit_config
         # Perform test discovery.
         test_path = os.path.dirname(os.path.dirname(__file__))
         inputs = [os.path.join(test_path, "Inputs/testrunner-custom-parsers/")]
         assert os.path.isdir(inputs[0])
-        tests = lit.discovery.find_tests_for_inputs(lit_config, inputs, False)
+        tests = lit.discovery.find_tests_for_inputs(lit_config, inputs)
         assert len(tests) == 1 and "there should only be one test"
         TestIntegratedTestKeywordParser.inputTestCase = tests[0]
 
     @staticmethod
     def make_parsers():
         def custom_parse(line_number, line, output):
             if output is None:
                 output = []