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

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 3 inline comments as done.
sammccall 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.
----------------
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.


================
Comment at: clang-tools-extra/trunk/clangd/unittests/lit.cfg.in:11
+# Point the dynamic loader at dynamic libraries in 'lib'.
+# XXX: it seems every project has a copy of this logic. Move it somewhere.
+import platform
----------------
thakis wrote:
> Did you mean to commit this XXX?
Yes. Probably should have been spelled FIXME.


================
Comment at: clang-tools-extra/trunk/clangd/unittests/lit.cfg.in:23
+
+
----------------
thakis wrote:
> Lots of trailing newlines.
Are these actually confusing, or significant in some way?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61187