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

[Julian Lettner via Phabricator via llvm-commits]

yln accepted this revision.
yln added a comment.

Just a few question, change itself looks good!

LGTM, thanks!



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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151664