[PATCH] D61187: [clangd] Move clangd tests to clangd directory. check-clangd is no longer part of check-clang-tools.

[Nico Weber via Phabricator via llvm-commits]

thakis added inline comments.


================
Comment at: clang-tools-extra/trunk/clangd/unittests/lit.cfg.in:1
+ at LIT_SITE_CFG_IN_HEADER@
+# This is a shim to run the gtest unittests in ../unittests using lit.
----------------
sammccall wrote:
> MaskRay wrote:
> > thakis wrote:
> > > thakis wrote:
> > > > Every other LLVM project puts the lit.cfg.in for the unit tests in foo/test/Unit/lit.cfg.in. Any reason why clangd is different?
> > > > 
> > > > Also, almost all LLVM projects call their lit.cfg lit.cfg.py and this file lit.cfg.py.in since it helps Windows. Can you rename the lit.cfg(.in) files you added to have .py in them please?
> > > > 
> > > > Also, why did you merge the site.cfg.py.in file with the non-generated non-site file? This too is different from all other LLVM projects.
> > > One consequence of these differences (except things being harder to understand) is that running `bin/llvm-lit ../llvm-project/clang-tools-extra/clangd/test` doesn't do anything, and running a single test that way doesn't work either. This works fine for individual llvm, clang, lld, clang-tools-extra tests and it used to work for clangd tests.
> > I know little about lit configurations but not being able to run a single test (`$build/bin/llvm-lit clangd/test/path/to/some/test`) seems a big downside..
> > 
> > (offtopic: compiler-rt tests have two modes: i386 and x86_64. I wonder if llvm-lit can run a test in the i386 mode (if it defaults to x86_64))
> > Every other LLVM project puts the lit.cfg.in for the unit tests in foo/test/Unit/lit.cfg.in. Any reason why clangd is different? ... Also, why did you merge the site.cfg.py.in 
> 
> Most of these changes were motivated by reducing the number of moving parts, which inhibit understanding. It's likely to be harder to understand if you're knowledgeable about the way things are done in LLVM. My reasoning in weighing simplicity over matching LLVM precisely:
>  - the overlap in active contributors appears fairly small (maybe I'm missing other ways people interact with the code)
>  - average understanding of the fairly elaborate LLVM setup seems fairly low, I would describe the median response to e.g. the `Unit` directory as "baffled".
> 
> > Also, almost all LLVM projects call their lit.cfg lit.cfg.py and this file lit.cfg.py.in since it helps Windows
> 
> Can you elaborate, or point to some docs?
> 
> > One consequence of these differences (except things being harder to understand) is that running `bin/llvm-lit ../llvm-project/clang-tools-extra/clangd/test` doesn't do anything
> 
> Indeed. This works from the build directory though, which is where we typically run tests.
> It also works for `.../unittests`, which is (IMO) much more obvious than `.../test/Unit`.
> 
> > and running a single test that way doesn't work either
> AFAICS it never did for unit tests? lit tests are not a significant fraction of our tests.
> motivated by reducing the number of moving parts

That's a great motivation, and I agree the current state is hard to understand. But if every LLVM subsubproject does its own lit setup that they feel is simpler, the resulting whole will be even harder to understand than what we have now. Are you planning on changing the rest of c-t-e, clang, lld, llvm to use this new lit setup as well? If not, clangd should use the standard setup imho.

> Can you elaborate, or point to some docs?

It's just that Windows uses file extensions for file type associations heavily. If the file is called foo.py, you can double-click it in explorer and an editor will appear. See revision history of all the lit.(site.)cfg.py files in c-t-e, clang, llvm, lld for Zach's patches to move things to this world.

> AFAICS it never did for unit tests?

Yes, this only works for lit tests. LLVM in general tries to do most of its testing via lit tests.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61187