[lit] Remove dead code not referenced in the LLVM SVN repo.

[dlj]

Summary

This change removes the intermediate 'FileBasedTest' format from lit. This
format is only ever used by the ShTest format, so the logic can be moved into
ShTest directly.

In order to better clarify what the TestFormat subclasses do, I fleshed out the
TestFormat base class with Python's notion of abstract methods, using
@abc.abstractmethod. This gives a convenient way to document the expected
interface, without the risk of instantiating an abstract class (that's what
ABCMeta does -- it raises an exception if you try to instantiate a class which
has abstract methods, but not if you instantiate a subclass that implements
them).

[modocache]

Awesome, thank you!

utils/lit/lit/formats/base.py

21 ↗

(On Diff #104553)

I greatly appreciate the documentation here. Thanks!

utils/lit/lit/formats/shtest.py

24 ↗

(On Diff #104553)

Question: The documentation for lit spells it with a lowercase 'l': http://llvm.org/docs/CommandGuide/lit.html. http://llvm.org/docs/TestingGuide.html, on the other hand, uses both capital and lowercase in some places. Which is correct?

I had always thought that lowercase "lit" was the proper name. If you agree, then this line and the "Args" section below should be updated.

[dlj]

Changed spelling: Lit -> lit.

utils/lit/lit/formats/base.py

21 ↗

(On Diff #104553)

Glad it helps!

utils/lit/lit/formats/shtest.py

24 ↗

(On Diff #104553)

I think I agree more with your logic... this was probably a typo, after typing LitConfig so many times.

[leandron]

Hi. There are some failures on LLVM test-suite[1] after this code was merged.

"FileBasedTest" is being imported at litsupport/test.py:10 (currently rev. 306655) and it is causing lnt to fail abruptly when running "lnt runtest test-suite".

It is not possible to observe failures on lnt public bots as they are running "lnt runtest nt", not "lnt runtest test-suite".

May I suggest to revert the commit or adjust so it doesn't break the existing test ?

[1] http://llvm.org/svn/llvm-project/test-suite/trunk/

[mzolotukhin]

We're seeing more issues (I think they are unrelated to the FileBasedTest issue mentioned before).

[1] is one example of the failure, the error is:

lit.py: /Users/buildslave/jenkins/sharedspace/phase1@2/llvm/utils/lit/lit/TestingConfig.py:101: fatal: unable to parse config file '/Users/buildslave/jenkins/sharedspace/phase1@2/llvm/projects/libcxx/test/lit.cfg', traceback: Traceback (most recent call last):
  File "/Users/buildslave/jenkins/sharedspace/phase1@2/llvm/utils/lit/lit/TestingConfig.py", line 88, in load_from_path
    exec(compile(data, path, 'exec'), cfg_globals, None)
  File "/Users/buildslave/jenkins/sharedspace/phase1@2/llvm/projects/libcxx/test/lit.cfg", line 52, in <module>
    configuration.configure()
  File "/Users/buildslave/jenkins/sharedspace/phase1@2/llvm/projects/libcxx/utils/libcxx/test/config.py", line 129, in configure
    self.configure_triple()
  File "/Users/buildslave/jenkins/sharedspace/phase1@2/llvm/projects/libcxx/utils/libcxx/test/config.py", line 1073, in configure_triple
    self.use_deployment = not self.use_target and self.can_use_deployment()
  File "/Users/buildslave/jenkins/sharedspace/phase1@2/llvm/projects/libcxx/utils/libcxx/test/config.py", line 1059, in can_use_deployment
    if not self.target_info.is_host_macosx():
  File "/Users/buildslave/jenkins/sharedspace/phase1@2/llvm/projects/libcxx/utils/libcxx/test/target_info.py", line 76, in is_host_macosx
    name = lit.util.capture(['sw_vers', '-productName']).strip()
AttributeError: 'module' object has no attribute 'capture'

Any ideas on how to fix it?

[1]: http://green.lab.llvm.org/green/job/clang-stage1-configure-RA_check/32959/consoleFull#-42777206a1ca8a51-895e-46c6-af87-ce24fa4cd561

[dlj]

In D34792#795937, @mzolotukhin wrote:

We're seeing more issues (I think they are unrelated to the FileBasedTest issue mentioned before).

[1] is one example of the failure, the error is:

lit.py: /Users/buildslave/jenkins/sharedspace/phase1@2/llvm/utils/lit/lit/TestingConfig.py:101: fatal: unable to parse config file '/Users/buildslave/jenkins/sharedspace/phase1@2/llvm/projects/libcxx/test/lit.cfg', traceback: Traceback (most recent call last):
  File "/Users/buildslave/jenkins/sharedspace/phase1@2/llvm/utils/lit/lit/TestingConfig.py", line 88, in load_from_path
    exec(compile(data, path, 'exec'), cfg_globals, None)
  File "/Users/buildslave/jenkins/sharedspace/phase1@2/llvm/projects/libcxx/test/lit.cfg", line 52, in <module>
    configuration.configure()
  File "/Users/buildslave/jenkins/sharedspace/phase1@2/llvm/projects/libcxx/utils/libcxx/test/config.py", line 129, in configure
    self.configure_triple()
  File "/Users/buildslave/jenkins/sharedspace/phase1@2/llvm/projects/libcxx/utils/libcxx/test/config.py", line 1073, in configure_triple
    self.use_deployment = not self.use_target and self.can_use_deployment()
  File "/Users/buildslave/jenkins/sharedspace/phase1@2/llvm/projects/libcxx/utils/libcxx/test/config.py", line 1059, in can_use_deployment
    if not self.target_info.is_host_macosx():
  File "/Users/buildslave/jenkins/sharedspace/phase1@2/llvm/projects/libcxx/utils/libcxx/test/target_info.py", line 76, in is_host_macosx
    name = lit.util.capture(['sw_vers', '-productName']).strip()
AttributeError: 'module' object has no attribute 'capture'

Any ideas on how to fix it?

[1]: http://green.lab.llvm.org/green/job/clang-stage1-configure-RA_check/32959/consoleFull#-42777206a1ca8a51-895e-46c6-af87-ce24fa4cd561

Indeed, that is different... that's a very surprising place for a dependency on lit. I don't think the utility is needed at all here.

This patch should fix it:
https://reviews.llvm.org/D34841

However, I don't believe I have access to a testing environment that can validate the forward fix (or a revert, for that matter). If you want, you can patch it and try it. Otherwise, let me know if I should go ahead and submit with crossed fingers.

[mgorny]

This broke two lit tests:

lit :: test-data.py
lit :: unit/TestRunner.py

They both explicitly use FileBasedTest. I'm surprised nobody noticed that because even a most trivial grep would point that out.

[mgorny]

I'm sorry, I was wrong. It breaks four tests:

lit :: test-data.py
lit :: test-output.py
lit :: unit/TestRunner.py
lit :: xunit-output.py

[modocache]

I submitted a revert for review: https://reviews.llvm.org/D35877. As described in my comments in https://bugs.llvm.org/show_bug.cgi?id=33704, the diff I submitted is one of three that should get the lit test suite passing again -- reviews greatly appreciated! :)

llvm/trunk/utils/lit/lit/formats/__init__.py

-from lit.formats.base import (  # noqa: F401
-    TestFormat,
-    FileBasedTest,
-    OneCommandPerFileTest
-)
-
+from lit.formats.base import TestFormat  # noqa: F401
 from lit.formats.googletest import GoogleTest  # noqa: F401
 from lit.formats.shtest import ShTest  # noqa: F401

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

-from __future__ import absolute_import
-import os
-
-import lit.Test
-import lit.util
+import abc
 
 class TestFormat(object):
-    pass
-
-###
-
-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
-
-            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)
-
-###
-
-import re
-import tempfile
-
-class OneCommandPerFileTest(TestFormat):
-    # FIXME: Refactor into generic test for running some command on a directory
-    # of inputs.
-
-    def __init__(self, command, dir, recursive=False,
-                 pattern=".*", useTempInput=False):
-        if isinstance(command, str):
-            self.command = [command]
-        else:
-            self.command = list(command)
-        if dir is not None:
-            dir = str(dir)
-        self.dir = dir
-        self.recursive = bool(recursive)
-        self.pattern = re.compile(pattern)
-        self.useTempInput = useTempInput
-
-    def getTestsInDirectory(self, testSuite, path_in_suite,
-                            litConfig, localConfig):
-        dir = self.dir
-        if dir is None:
-            dir = testSuite.getSourcePath(path_in_suite)
-
-        for dirname,subdirs,filenames in os.walk(dir):
-            if not self.recursive:
-                subdirs[:] = []
-
-            subdirs[:] = [d for d in subdirs
-                          if (d != '.svn' and
-                              d not in localConfig.excludes)]
-
-            for filename in filenames:
-                if (filename.startswith('.') or
-                    not self.pattern.match(filename) or
-                    filename in localConfig.excludes):
-                    continue
-
-                path = os.path.join(dirname,filename)
-                suffix = path[len(dir):]
-                if suffix.startswith(os.sep):
-                    suffix = suffix[1:]
-                test = lit.Test.Test(
-                    testSuite, path_in_suite + tuple(suffix.split(os.sep)),
-                    localConfig)
-                # FIXME: Hack?
-                test.source_path = path
-                yield test
+    """Base class for test formats.
 
-    def createTempInput(self, tmp, test):
-        raise NotImplementedError('This is an abstract method.')
+    A TestFormat encapsulates logic for finding and executing a certain type of
+    test. For example, a subclass FooTestFormat would contain the logic for
+    finding tests written in the 'Foo' format, and the logic for running a
+    single one.
+
+    TestFormat is an Abstract Base Class (ABC). It uses the Python abc.ABCMeta
+    type and associated @abc.abstractmethod decorator. Together, these provide
+    subclass behaviour which is notionally similar to C++ pure virtual classes:
+    only subclasses which implement all abstract methods can be instantiated
+    (the implementation may come from an intermediate base).
+
+    For details on ABCs, see: https://docs.python.org/2/library/abc.html. Note
+    that Python ABCs have extensive abilities beyond what is used here. For
+    TestFormat, we only care about enforcing that abstract methods are
+    implemented.
+    """
+
+    __metaclass__ = abc.ABCMeta
+
+    @abc.abstractmethod
+    def getTestsInDirectory(self, testSuite, path_in_suite, litConfig,
+                            localConfig):
+      """Finds tests of this format in the given directory.
+
+      Args:
+          testSuite: a Test.TestSuite object.
+          path_in_suite: the subpath under testSuite to look for tests.
+          litConfig: the LitConfig for the test suite.
+          localConfig: a LitConfig with local specializations.
+
+      Returns:
+          An iterable of Test.Test objects.
+      """
 
+    @abc.abstractmethod
     def execute(self, test, litConfig):
-        if test.config.unsupported:
-            return (lit.Test.UNSUPPORTED, 'Test is unsupported')
-
-        cmd = list(self.command)
-
-        # If using temp input, create a temporary file and hand it to the
-        # subclass.
-        if self.useTempInput:
-            tmp = tempfile.NamedTemporaryFile(suffix='.cpp')
-            self.createTempInput(tmp, test)
-            tmp.flush()
-            cmd.append(tmp.name)
-        elif hasattr(test, 'source_path'):
-            cmd.append(test.source_path)
-        else:
-            cmd.append(test.getSourcePath())
-
-        out, err, exitCode = lit.util.executeCommand(cmd)
-
-        diags = out + err
-        if not exitCode and not diags.strip():
-            return lit.Test.PASS,''
-
-        # Try to include some useful information.
-        report = """Command: %s\n""" % ' '.join(["'%s'" % a
-                                                 for a in cmd])
-        if self.useTempInput:
-            report += """Temporary File: %s\n""" % tmp.name
-            report += "--\n%s--\n""" % open(tmp.name).read()
-        report += """Output:\n--\n%s--""" % diags
+      """Runs the given 'test', which is of this format.
 
-        return lit.Test.FAIL, report
+      Args:
+          test: a Test.Test object describing the test to run.
+          litConfig: the LitConfig for the test suite.
+
+      Returns:
+          A tuple of (status:Test.ResultCode, message:str)
+      """

llvm/trunk/utils/lit/lit/formats/shtest.py

 from __future__ import absolute_import
 
+import os
+
+import lit.Test
 import lit.TestRunner
-from .base import FileBasedTest
+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:
+
+        http://llvm.org/docs/TestingGuide.html
+
+    The ShTest files contain some number of shell-like command pipelines, along
+    with assertions about what should be in the output.
+    """
 
-class ShTest(FileBasedTest):
     def __init__(self, execute_external = False):
+        """Initializer.
+
+        The 'execute_external' argument controls whether lit uses its internal
+        logic for command pipelines, or passes the command to a shell
+        subprocess.
+
+        Args:
+            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)
+
     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)