[lit] Factor out listdir logic shared by different test formats.

[dlj]

Summary

The lit test formats use largely the same logic for discovering tests. There are
some superficial differences in the logic, which seem reasonable enough to
handle in a single routine.

At a high level, the common goal is "look for files that end with one of these
suffixes, and skip anything starting with a dot." The balance of the logic
specific to ShTest and GoogleTest collapses quite a bit, so that
getTestsInDirectory is only a couple of lines around a call to the new function.

[modocache]

utils/lit/lit/util.py

139

Note that this is a subtle change in behavior from os.path.splitext(). Consider a file without an extension, such as TARGETS:

>>> os.path.splitext('TARGETS')
('TARGETS', '')

The previous logic, which used os.path.splitext(), would not include the file, even if 'TARGETS' were included as a suffix. This new logic would include it, since TARGETS.endswith('TARGETS') returns True.

I'm not sure if any consumers of lit rely on this behavior... but if you didn't intend to make a change, perhaps keep the old behavior for now?

[dlj]

utils/lit/lit/util.py

139

Yup, you are correct... I think this is OK as-is, though, since splitext includes the trailing dot:

In [2]: os.path.splitext('TARGETS')
Out[2]: ('TARGETS', '')

In [3]: os.path.splitext('x.TARGETS')
Out[3]: ('x', '.TARGETS')

Any existing suffixes in lit configs would need to have a dot in order to match. For example:

https://reviews.llvm.org/diffusion/L/browse/lld/trunk/test/lit.cfg;306779$48

That anchors the suffix for both the endswith() and splitext() cases... so if the previous behaviour was to match TARGETS as a suffix, it would have needed to be specified as ".TARGETS" in order to work.

It's possible there are such bugs, but all my test counts locally line up before and after. (And I'll double-check again, just to be sure.) I believe the only cases where the behaviour would actually differ is if the old pattern had a latent bug.

[modocache]

utils/lit/lit/util.py

139

Ah OK. Great idea to make sure the test counts match. That sounds fine to me -- thanks!

[grimar]

Hi !

This (r306895) causes check-lld failtures under windows for me:

24>  Traceback (most recent call last):
24>    File "C:/access_softek/llvm/utils/lit/lit.py", line 6, in <module>
24>      main()
24>    File "C:\access_softek\llvm\utils\lit\lit\main.py", line 161, in main
24>      main_with_tmp(builtinParameters)
24>    File "C:\access_softek\llvm\utils\lit\lit\main.py", line 344, in main_with_tmp
24>      lit.discovery.find_tests_for_inputs(litConfig, inputs))
24>    File "C:\access_softek\llvm\utils\lit\lit\discovery.py", line 222, in find_tests_for_inputs
24>      test_suite_cache, local_config_cache)[1])
24>    File "C:\access_softek\llvm\utils\lit\lit\discovery.py", line 186, in getTestsInSuite
24>      for res in subiter:
24>    File "C:\access_softek\llvm\utils\lit\lit\discovery.py", line 144, in getTestsInSuite
24>      litConfig, lc):
24>    File "C:\access_softek\llvm\utils\lit\lit\formats\googletest.py", line 85, in getTestsInDirectory
24>      suffixes={self.test_suffix}):
24>    File "C:\access_softek\llvm\utils\lit\lit\util.py", line 136, in listdir_files
24>      for filename in os.listdir(dirname):
24>  WindowsError: [Error 3] : 'C:\\access_softek\\c_make_build_dir_x64\\tools\\lld\\unittests\\debug/*.*'

I think issue is that C:\\access_softek\\c_make_build_dir_x64\\tools\\lld\\unittests\\debug folder not exist.
I added some traces (D34944) and compared output from windows and ubuntu runs of check-lld:
(under ubuntu it works fine)

Ubuntu:

/home/umb/LLVM/llvm/tools/lld/test
Subdirs: .
BuildMode: .
Source: /home/umb/LLVM/llvm-build/tools/lld/unittests
Subdir: .
/home/umb/LLVM/llvm-build/tools/lld/unittests/.

Windows:

24>  BuildMode: Debug
24>  Subdirs: Debug
24>  Source: C:\access_softek\c_make_build_dir_x64\tools\lld\unittests
24>  Subdir: debug
24>  Dirname: C:\access_softek\c_make_build_dir_x64\tools\lld\unittests\debug

"BuildMode" above is config.llvm_build_mode.

So looks in my windows case config.llvm_build_mode is not ".", but "Debug" and is added to the path here and that is the reason of failture.
Not sure why Buildmode is "." under ubuntu though and where it comes from.

[grimar]

Ah, I just noticed the D34853, it fixes the issue I am observing.

[andrewng]

In D34855#798200, @grimar wrote:

Ah, I just noticed the D34853, it fixes the issue I am observing.

Hi,

I have also hit this issue. I don't think that D34853 is the correct solution (see my comment in that review).

I think the easiest fix (and it's the workaround I currently have in place) is to test that the directory exists in GoogleTest.getTestsInDirectory before calling lit.util.listdir_files. I will put together a patch and review for this fix.

Cheers,
Andrew

utils/lit/lit/formats/googletest.py

 from __future__ import absolute_import
 import os
 import subprocess
 import sys
 
 import lit.Test
 import lit.TestRunner
 import lit.util
 from .base import TestFormat
 
 kIsWindows = sys.platform in ['win32', 'cygwin']
 
 class GoogleTest(TestFormat):
-    def __init__(self, test_sub_dir, test_suffix):
-        self.test_sub_dir = os.path.normcase(str(test_sub_dir)).split(';')
+    def __init__(self, test_sub_dirs, test_suffix):
+        self.test_sub_dirs = os.path.normcase(str(test_sub_dirs)).split(';')
         self.test_suffix = str(test_suffix)
 
         # On Windows, assume tests will also end in '.exe'.
         if kIsWindows:
             self.test_suffix += '.exe'
 
     def getGTestTests(self, path, litConfig, localConfig):
         """getGTestTests(path) - [name]
def getGTestTests(self, path, litConfig, localConfig):
             elif any([name.startswith('DISABLED_')
                       for name in nested_tests + [ln]]):
                 # Gtest will internally skip these tests. No need to launch a
                 # child process for it.
                 continue
             else:
                 yield ''.join(nested_tests) + ln
 
-    # Note: path_in_suite should not include the executable name.
-    def getTestsInExecutable(self, testSuite, path_in_suite, execpath,
-                             litConfig, localConfig):
-        if not execpath.endswith(self.test_suffix):
-            return
-        (dirname, basename) = os.path.split(execpath)
-        # Discover the tests in this executable.
-        for testname in self.getGTestTests(execpath, litConfig, localConfig):
-            testPath = path_in_suite + (basename, testname)
-            yield lit.Test.Test(testSuite, testPath, localConfig, file_path=execpath)
-
     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 os.path.isdir(filepath):
-                # Iterate over executables in a directory.
-                if not os.path.normcase(filename) in self.test_sub_dir:
-                    continue
-                dirpath_in_suite = path_in_suite + (filename, )
-                for subfilename in os.listdir(filepath):
-                    execpath = os.path.join(filepath, subfilename)
-                    for test in self.getTestsInExecutable(
-                            testSuite, dirpath_in_suite, execpath,
-                            litConfig, localConfig):
-                      yield test
-            elif ('.' in self.test_sub_dir):
-                for test in self.getTestsInExecutable(
-                        testSuite, path_in_suite, filepath,
-                        litConfig, localConfig):
-                    yield test
+        for subdir in self.test_sub_dirs:
+            for fn in lit.util.listdir_files(os.path.join(source_path, subdir),
+                                             suffixes={self.test_suffix}):
+                # Discover the tests in this executable.
+                execpath = os.path.join(source_path, subdir, fn)
+                testnames = self.getGTestTests(execpath, litConfig, localConfig)
+                for testname in testnames:
+                    testPath = path_in_suite + (subdir, fn, testname)
+                    yield lit.Test.Test(testSuite, testPath, localConfig,
+                                        file_path=execpath)
 
     def execute(self, test, litConfig):
         testPath,testName = os.path.split(test.getSourcePath())
         while not os.path.exists(testPath):
             # Handle GTest parametrized and typed tests, whose name includes
             # some '/'s.
             testPath, namePrefix = os.path.split(testPath)
             testName = namePrefix + '/' + testName

utils/lit/lit/formats/shtest.py

 from __future__ import absolute_import
 
 import os
 
 import lit.Test
 import lit.TestRunner
+import lit.util
 from .base import TestFormat
 
 class ShTest(TestFormat):
     """ShTest is a format with one file per test.
 
     This is the primary format for regression tests as described in the LLVM
     testing guide:
 
def __init__(self, execute_external = False):
             execute_external: (optional) If true, use shell subprocesses instead
                 of lit's internal pipeline logic.
         """
         self.execute_external = execute_external
 
     def getTestsInDirectory(self, testSuite, path_in_suite,
                             litConfig, localConfig):
         """Yields test files matching 'suffixes' from the 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
-
-            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)
+        file_matches = lit.util.listdir_files(
+            testSuite.getSourcePath(path_in_suite),
+            localConfig.suffixes, localConfig.excludes)
+        for filename in file_matches:
 
     def execute(self, test, litConfig):
         """Interprets and runs the given test file, and returns the result."""
         return lit.TestRunner.executeShTest(test, litConfig,
                                             self.execute_external)

utils/lit/lit/util.py

def mkdir_p(path):
     try:
         os.mkdir(path)
     except OSError:
         e = sys.exc_info()[1]
         # Ignore EEXIST, which may occur during a race condition.
         if e.errno != errno.EEXIST:
             raise
 
+def listdir_files(dirname, suffixes=None, exclude_filenames=None):
+    """Yields files in a directory.
+
+    Filenames that are not excluded by rules below are yielded one at a time, as
+    basenames (i.e., without dirname).
+
+    Files starting with '.' are always skipped.
+
+    If 'suffixes' is not None, then only filenames ending with one of its
+    members will be yielded. These can be extensions, like '.exe', or strings,
+    like 'Test'. (It is a lexicographic check; so an empty sequence will yield
+    nothing, but a single empty string will yield all filenames.)
+
+    If 'exclude_filenames' is not None, then none of the file basenames in it
+    will be yielded.
+
+    If specified, the containers for 'suffixes' and 'exclude_filenames' must
+    support membership checking for strs.
+
+    Args:
+        dirname: a directory path.
+        suffixes: (optional) a sequence of strings (set, list, etc.).
+        exclude_filenames: (optional) a sequence of strings.
+
+    Yields:
+        Filenames as returned by os.listdir (generally, str).
+    """
+    if exclude_filenames is None:
+        exclude_filenames = set()
+    if suffixes is None:
+        suffixes = {''}
+    for filename in os.listdir(dirname):
+        if (os.path.isdir(os.path.join(dirname, filename)) or
+            filename.startswith('.') or
+            filename in exclude_filenames or
+            not any(filename.endswith(sfx) for sfx in suffixes)):
+            continue
+        yield filename
+
 def which(command, paths = None):
     """which(command, [paths]) - Look up the given command in the paths string
     (or the PATH environment variable, if unspecified)."""
 
     if paths is None:
         paths = os.environ.get('PATH','')
 
     # Check for absolute match first.