[PATCH] D45332: [LIT] Add new LLDB test format

[Pavel Labath via Phabricator via llvm-commits]

labath added a comment.

In https://reviews.llvm.org/D45332#1058976, @JDevlieghere wrote:

> In https://reviews.llvm.org/D45332#1058970, @zturner wrote:
>
> > I don't think `sys.path` is set up correctly to be able to find the lldbtest package from the `lldb/lit` folder.
> >
> > These things kind of evolved separately, and the `lldb/lit` folder was created as a place to start iterating on LLVM-style lit / FileCheck tests.  These kind of tests -- by definition -- don't really use the SB API, so no work was ever done to set up paths correctly so that it could write `import lldb` or to re-use any of the other stuff from `packages/Python`.
> >
> > I'm not sure what the best thing to do is, but usually the canonical structuring is to have the test files in the same tree as the lit configuration.  So perhaps you could put a lit configuration file in `lldb/packages/Python/lldbsuite` and have that be separate from `lldb/lit`, with the goal of eventually (possibly) merging them.  Then have a separate CMake target so you'd still have `check-lldb-lit` which goes into the `lldb/lit` directory, and another one like `check-lldb-lit-dotest` which starts from the `lldb/packages/Python/lldbsuite` directory.
> >
> > On the other hand, if you want to see how `dotest.py` sets up its `sys.path`, have a look at `lldb/test/dotest.py`  The magic is in this `use_lldb_suite` function, which walks backwards through the tree until it finds the root, then dives into the `lldbsuite` folder to manually add it to `sys.path`.
>
>
> Do you feel all that outweighs the alternative of just having the format in `llvm/Utils` as is the case in this diff? We already have some LLDB specific stuff there and I would argue that conceptually it makes (at least a little) sense to have all the format living together.


I don't think it needs to be that complex.

You already have LLDB_SOURCE_DIR from `lit.site.cfg.in`, so it should only be a matter of doing something like this in `lit.cfg`:

  sys.path.append(os.path.join(config.lldb_src_root, "whereever_is_the_format_file")
  from something import LLDBTest
  config.test_format = LLDBTest(...)

If we can make things as simple as this, then I think we should move this to the lldb repo.



================
Comment at: utils/lit/lit/formats/lldbtest.py:32
+                base, ext = os.path.splitext(filename)
+                if ext in localConfig.suffixes:
+                    yield lit.Test.Test(testSuite, path_in_suite +
----------------
Do we need the suffixes to be configurable? The whole testsuite is very specific to python and I don't see us running non-python files through this method any time soon.


================
Comment at: utils/lit/lit/formats/lldbtest.py:60
+
+        passing_test_line = 'RESULT: PASSED'
+        if passing_test_line not in out:
----------------
This line only appears if you run dotest in `--no-multiprocess` mode, but I don't see where you are adding that. Am I missing something?


Repository:
  rL LLVM

https://reviews.llvm.org/D45332