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

[Louis Dionne via Phabricator via llvm-commits]

ldionne added inline comments.


================
Comment at: llvm/utils/lit/lit/formats/base.py:9
 class TestFormat(object):
-    pass
-
+    def getTestsForFilename(self, testSuite, path_in_suite, litConfig, localConfig):
+        """
----------------
yln wrote:
> 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?
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?


================
Comment at: llvm/utils/lit/lit/formats/base.py:23
+        if ext in localConfig.suffixes:
+            yield lit.Test.Test(testSuite, path_in_suite, localConfig)
 
----------------
yln wrote:
> 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`!?
> 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.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151664/new/

https://reviews.llvm.org/D151664