[lit] Add a method to lit.TestFormat to get the list of tests associated to a path

[ldionne]

Summary

Lit TestFormat classes already needed to implement the getTestsInDirectory
method. However, some test formats may want to expand a single test path
to multiple Lit tests, for example in the case of a test that actually
generates other Lit tests.

To accommodate that, this commit adds the getTestsForFilename method to
TestFormat. This method can be used to turn a single path in a Lit test
suite into a list of tests associated to that path.

[ldionne]

For context, the problem this solves is that libc++ now has a special kind of test called .gen tests. Those tests are basically a script that generates other Lit tests on the fly. This means that if I run for example lit libcxx/test/libcxx/assertions/headers_declare_verbose_abort.gen.py, I need Lit to actually understand that this corresponds to more than one test, i.e. all the tests generated in that .gen.py file.

I think this abstraction is actually what we want most of the time instead of getTestsInDirectory -- indeed, I looked at several custom test formats and most seemed to define getTestsInDirectory in a way that defining getTestsForFilename would have worked better, and then getTestsInDirectory would have been implemented generically on top of that.

[Mordante]

Friendly ping, it would be great when people more familiar with lit can review this. This would make it easy to run lit on the generated files.

[yln]

Just a few question, change itself looks good!

LGTM, thanks!

llvm/utils/lit/lit/formats/base.py

9–16

Because of the method name getTestsForFilename this sounds like it should also be defined in FileBasedTest?! I am sure there is a reason you put into the TestFormat root class?

23

This is the default implementation, which is overridden in CxxStandardLibraryTest, right?

If I understand correctly, then existing TestFormats (even out of tree) ones should continue to work: if they override the old getTestsInDirectory, then they will continue to do their own thing (they will not know about/call into the new getTestsForFilename)!?

Should _getTestsForFilename be a "private" function (starting with`_`) of TestFormat? It can be overridden to customize test format/discovery , but the API that used by the LIT runner remains getTestsInDirectory!?

[Mordante]

In D151664#4429918, @yln wrote:

Just a few question, change itself looks good!

LGTM, thanks!

Thanks for the review!

[ldionne]

llvm/utils/lit/lit/formats/base.py

9–16

This needs to be defined in the TestFormat root class because llvm/utils/lit/lit/discovery.py needs to call it when it figures out all the tests in a TestSuite (in getTestsInSuite). This makes me think that getTestsForFilename might be the wrong name for this method. In a way, we're being passed a path_in_suite, which does not conceptually have to be a file name.

If we named this getTestsForPath, then we'd have:

class TestFormat(object):
  def getTestsForPath(self, testSuite, path_in_suite, litConfig, localConfig):
    yield lit.Test.Test(testSuite, path_in_suite, localConfig)

class FileBasedTest(TestFormat):
  def getTestsForPath(self, testSuite, path_in_suite, litConfig, localConfig):
    # Here we can assume paths are actual file names
    filename = path_in_suite[-1]

    # Ignore dot files and excluded tests.
    if filename.startswith(".") or filename in localConfig.excludes:
      return

    base, ext = os.path.splitext(filename)
    if ext in localConfig.suffixes:
      yield lit.Test.Test(testSuite, path_in_suite, localConfig)

  def getTestsInDirectory(self, testSuite, path_in_suite, litConfig, localConfig):
    # Use getTestsForPath
    ...

WDYT?

23

This is the default implementation, which is overridden in CxxStandardLibraryTest, right?

Yes. See my suggestion in the other comment.

If I understand correctly, then existing TestFormats (even out of tree) ones should continue to work: if they override the old getTestsInDirectory, then they will continue to do their own thing (they will not know about/call into the new getTestsForFilename)!?

Yes, they would continue doing their own thing. However, it would be possible for some test formats to end up with implementations of getTestsInDirectory and getTestsForPath that are inconsistent, in case their getTestsInDirectory implementation tries to map one file name to multiple Lit tests but they haven't overridden getTestsForPath. I don't think this would break down given the usage of getTestsForPath we do today, however that would technically be incorrect and could break down if we started relying on getTestsForPath more as a source of truth.

Should _getTestsForFilename be a "private" function (starting with`_`) of TestFormat? It can be overridden to customize test format/discovery , but the API that used by the LIT runner remains getTestsInDirectory!?

I am not sure I understand what would be the intent of making this a private function, since it would be called from outside the class from getTestsInSuite. IMO it makes more sense for it to be public.

[ldionne]

CC @bd1976llvm @thopre This is also related to D83069 and D94766. Specifically, after my patch, executing a test like

lit Inputs/standalone-tests/true.txt

using a config that doesn't mention .txt as a valid suffix would now result in the following warning:

lit.py: foo.py: warning: input 'Inputs/standalone-tests/true.txt' contained no tests

IMO this makes sense, and in fact I don't quite understand the motivation behind trying to allow running a test for which the config doesn't have a suffix configured. The original problem that D83069 was trying to solve remains solved after this patch, however: if you add a test with an invalid suffix and you try running it directly, Lit will not run it, it will complain that you're trying to run a path that "contains no tests". This seems to address the original issue that prompted D83069.

So I'd be inclined to basically remove the check that had been added by D83069 and the --no-indirectly-run-check flag (reading the reviews, it seems like this was perceived kind of as a hack anyway), and then we'd want to understand whether the "standalone test" use case truly requires being able to run arbitrary extensions that are not in the configured suffixes.

[ldionne]

Fix test issues, add test for the new feature and implement the changes suggested in the patch so far.

[ldionne]

Fix tests.

libcxx/utils/libcxx/test/format.py

def parseScript(test, preamble):
     # Perform substitutions in the script itself.
     script = lit.TestRunner.applySubstitutions(
         script, substitutions, recursion_limit=test.config.recursiveExpansionLimit
     )
 
     return script
 
 
-class CxxStandardLibraryTest(lit.formats.TestFormat):
+class CxxStandardLibraryTest(lit.formats.FileBasedTest):
     """
     Lit test format for the C++ Standard Library conformance test suite.
 
     This test format is based on top of the ShTest format -- it basically
     creates a shell script performing the right operations (compile/link/run)
     based on the extension of the test file it encounters. It supports files
     with the following extensions:
 
The test format will define the following substitutions for use inside tests:
             file with the %{flags}, %{compile_flags} and %{link_flags}
             substitutions, and that produces an executable named %t.exe.
 
         %{run}
             Equivalent to `%{exec} %t.exe`. This is intended to be used
             in conjunction with the %{build} substitution.
     """
 
-    def getTestsInDirectory(self, testSuite, pathInSuite, litConfig, localConfig):
+    def getTestsForPath(self, testSuite, pathInSuite, litConfig, localConfig):
         SUPPORTED_SUFFIXES = [
             "[.]pass[.]cpp$",
             "[.]pass[.]mm$",
             "[.]compile[.]pass[.]cpp$",
             "[.]compile[.]pass[.]mm$",
             "[.]compile[.]fail[.]cpp$",
             "[.]link[.]pass[.]cpp$",
             "[.]link[.]pass[.]mm$",
             "[.]link[.]fail[.]cpp$",
             "[.]sh[.][^.]+$",
             "[.]gen[.][^.]+$",
             "[.]verify[.]cpp$",
             "[.]fail[.]cpp$",
         ]
+
         sourcePath = testSuite.getSourcePath(pathInSuite)
-        for filename in os.listdir(sourcePath):
-            # Ignore dot files and excluded tests.
-            if filename.startswith(".") or filename in localConfig.excludes:
-                continue
-
-            filepath = os.path.join(sourcePath, filename)
-            if not os.path.isdir(filepath):
-                if any([re.search(ext, filename) for ext in SUPPORTED_SUFFIXES]):
-                    # If this is a generated test, run the generation step and add
-                    # as many Lit tests as necessary.
-                    if re.search('[.]gen[.][^.]+$', filename):
-                        for test in self._generateGenTest(testSuite, pathInSuite + (filename,), litConfig, localConfig):
-                            yield test
-                    else:
-                        yield lit.Test.Test(testSuite, pathInSuite + (filename,), localConfig)
+        filename = os.path.basename(sourcePath)
+
+        # Ignore dot files, excluded tests and tests with an unsupported suffix
+        hasSupportedSuffix = lambda f: any([re.search(ext, f) for ext in SUPPORTED_SUFFIXES])
+        if filename.startswith(".") or filename in localConfig.excludes or not hasSupportedSuffix(filename):
+            return
+
+            for test in self._generateGenTest(testSuite, pathInSuite, litConfig, localConfig):
+            yield lit.Test.Test(testSuite, pathInSuite, localConfig)
 
     def execute(self, test, litConfig):
         VERIFY_FLAGS = (
             "-Xclang -verify -Xclang -verify-ignore-unexpected=note -ferror-limit=0"
         )
         supportsVerify = "verify-support" in test.config.available_features
         filename = test.path_in_suite[-1]
 

llvm/utils/lit/examples/many-tests/ManyTests.py

-from lit import Test
+from lit import Test, TestFormat
 
 
-class ManyTests(object):
+class ManyTests(TestFormat):
     def __init__(self, N=10000):
         self.N = N
 
     def getTestsInDirectory(self, testSuite, path_in_suite, litConfig, localConfig):
         for i in range(self.N):
             test_name = "test-%04d" % (i,)
             yield Test.Test(testSuite, path_in_suite + (test_name,), localConfig)
 
     def execute(self, test, litConfig):
         # Do a "non-trivial" amount of Python work.
         sum = 0
         for i in range(10000):
             sum += i
         return Test.PASS, ""

llvm/utils/lit/lit/discovery.py

):
     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)
-        test = Test.Test(ts, path_in_suite, lc)
 
+        # 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 \
+                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.
     if lc.standalone_tests:

llvm/utils/lit/lit/formats/base.py

 from __future__ import absolute_import
 import os
 
 import lit.Test
 import lit.util
 
 
 class TestFormat(object):
-    pass
-
+    def getTestsForPath(self, testSuite, path_in_suite, litConfig, localConfig):
+        """
+        Given the path to a test in the test suite, generates the Lit tests associated
+        to that path. There can be zero, one or more tests. For example, some testing
+        formats allow expanding a single path in the test suite into multiple Lit tests
+        (e.g. they are generated on the fly).
+        """
+        yield lit.Test.Test(testSuite, path_in_suite, localConfig)
 
 ###
 
 
 class FileBasedTest(TestFormat):
-    def getTestsInDirectory(self, testSuite, path_in_suite, litConfig, localConfig):
-        source_path = testSuite.getSourcePath(path_in_suite)
-        for filename in os.listdir(source_path):
-            # Ignore dot files and excluded tests.
-            if filename.startswith(".") or filename in localConfig.excludes:
-                continue
+    def getTestsForPath(self, testSuite, path_in_suite, litConfig, localConfig):
+        """
+        Expand each path in a test suite to a Lit test using that path and assuming
+        it is a file containing the test. File extensions excluded by the configuration
+        or not contained in the allowed extensions are ignored.
+        """
+        filename = path_in_suite[-1]
+
+            return
 
-            filepath = os.path.join(source_path, filename)
-            if not os.path.isdir(filepath):
-                base, ext = os.path.splitext(filename)
-                if ext in localConfig.suffixes:
-                    yield lit.Test.Test(
-                        testSuite, path_in_suite + (filename,), localConfig
-                    )
+            yield lit.Test.Test(testSuite, path_in_suite, localConfig)
+
+    def getTestsInDirectory(self, testSuite, path_in_suite, litConfig, localConfig):
+        source_path = testSuite.getSourcePath(path_in_suite)
+        for filename in os.listdir(source_path):
+            filepath = os.path.join(source_path, filename)
+            if not os.path.isdir(filepath):
+                for t in self.getTestsForPath(testSuite, path_in_suite + (filename,), litConfig, localConfig):
+                    yield t
 
 
 ###
 
 import re
 import tempfile
 
 

llvm/utils/lit/tests/Inputs/discovery-getTestsForPath/custom_format.py

+import os
+import lit.formats
+
+class CustomFormat(lit.formats.ShTest):
+    def getTestsForPath(self, testSuite, path_in_suite, litConfig, localConfig):
+        for sub in ['one.test', 'two.test']:
+            basePath = os.path.dirname(testSuite.getExecPath(path_in_suite))
+            os.makedirs(basePath, exist_ok=True)
+            generatedFile = os.path.join(basePath, sub)
+            with open(generatedFile, 'w') as f:
+                f.write('RUN: true')
+            yield lit.Test.Test(testSuite, (generatedFile, ), localConfig)

llvm/utils/lit/tests/Inputs/discovery-getTestsForPath/lit.cfg

+import sys, os
+sys.path.append(os.path.dirname(__file__))
+from custom_format import CustomFormat
+
+config.name = "discovery-getTestsForPath-suite"
+config.suffixes = [".test"]
+config.test_format = CustomFormat()
+config.test_source_root = None
+config.test_exec_root = None

llvm/utils/lit/tests/discovery.py

 # RUN: %{lit} \
 # RUN:     %{inputs}/exec-discovery/subdir/test-three.py \
 # RUN:   --show-tests --show-suites -v > %t.out
 # RUN: FileCheck --check-prefix=CHECK-ASEXEC-DIRECT-TEST < %t.out %s
 #
 # CHECK-ASEXEC-DIRECT-TEST: -- Available Tests --
 # CHECK-ASEXEC-DIRECT-TEST: top-level-suite :: subdir/test-three
 
-# Check an error is emitted when the directly named test would not be run
-# indirectly (e.g. when the directory containing the test is specified).
+# Check that an error is emitted when the directly named test does not satisfy
+# the test config's requirements.
 #
 # RUN: not %{lit} \
 # RUN:     %{inputs}/discovery/test.not-txt 2>%t.err
-# RUN: FileCheck --check-prefix=CHECK-ERROR-INDIRECT-RUN-CHECK < %t.err %s
+# RUN: FileCheck --check-prefix=CHECK-ERROR-INPUT-CONTAINED-NO-TESTS < %t.err %s
 #
-# CHECK-ERROR-INDIRECT-RUN-CHECK: error: 'top-level-suite :: test.not-txt' would not be run indirectly
+# 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.
 #

 
 # Check that no discovery is done for testsuite with standalone tests.
 #
 # RUN: not %{lit} %{inputs}/standalone-tests 2>%t.err
 # RUN: FileCheck --check-prefix=CHECK-STANDALONE-DISCOVERY < %t.err %s
 #
 # CHECK-STANDALONE-DISCOVERY: error: did not discover any tests for provided path(s)
 
+# Check that a single file path can result in multiple tests being discovered if
+# the test format implements those semantics.
+#
+# RUN: %{lit} %{inputs}/discovery-getTestsForPath/x.test > %t.out
+# RUN: FileCheck --check-prefix=CHECK-getTestsForPath < %t.out %s
+#
+# CHECK-getTestsForPath: PASS: discovery-getTestsForPath-suite :: {{.+}}one.test
+# CHECK-getTestsForPath: PASS: discovery-getTestsForPath-suite :: {{.+}}two.test
+
 # Check that we don't recurse infinitely when loading an site specific test
 # suite located inside the test source root.
 #
 # RUN: %{lit} \
 # RUN:     %{inputs}/exec-discovery-in-tree/obj/ \
 # RUN:   --show-tests --show-suites -v > %t.out
 # RUN: FileCheck --check-prefix=CHECK-ASEXEC-INTREE < %t.out %s
 #