[PATCH] D21360: test-suite cmake: Detect test subdirectories when running cmake.

[Matthias Braun via llvm-commits]

MatzeB added a comment.

In http://reviews.llvm.org/D21360#459428, @tra wrote:

> In http://reviews.llvm.org/D21360#458253, @MatzeB wrote:
>
> > > Do you have particular use case for wildcard search?
> >
> >
> > I guess this is a matter of preference. I personally find it very convenient to simply move additional test-suites as a subdirectory of my test-suite checkout. So I can already express the list of tests I want to run simply by moving directories in and out of the test-suite checkout.
> >
> > This is also similar to how the tools/ and projects/ directories in llvm work where you can drop stuff like compiler-rt, libcxx, clang etc. as you need them.
> >
> > My question was more about whether we should restrict the wildcard matching to a well-known subdirectory (say 'projects' to stay in line with llvm or maybe the existing 'External' directory).
>
>
> I think what we want to do is, as @jmolloy said, "decouple the drivers from the test-suite".


So far we all agree.

> Searching at the top of test-suite source tree is both unnecessarily wide (there are directories we don't want) and and may not even be the right place to search (tests we want may be somewhere else). What we do want is a way to tell where to find extra tests and pick them up with as little hassle as possible.


I think the current patch is fine in this respect:

- scanning a few extra directories is no problem in terms of performance. The only interesting question is whether there is a chance to pick up an unwanted directory by accident, which I don't see happening. Forcing checkouts to `External/Extra` seems like an unnecessarily deep path to me...
- Scanning the toplevel directory lets the todays "default" mode of including `SingleSource`, `MultiSoruce` and `External` happen naturally.
- If you want to run external tests then you can indeed specify TEST_SUITE_SUBDIRS at cmake time and the system will not touch it any and just use the directories you specified.
- I think of the wildcard searching the test-suite directory as a convenience helper instead of specifying TEST_SUITE_SUBDIRS! If someone is specifying TEST_SUITE_SUBDIRS anyway then I believe he knows what he is doing and doesn't need or want wildcard magic.

> Your patch is almost there. It allows specifying where tests are via TEST_SUITE_SUBDIRS, but does not do searching there.

>  It searches for tests in the source tree, but only if TEST_SUITE_SUBDIRS is not specified.

> 

> How about this:

>  for dir in TEST_SUITE_SUBDIRS:

> 

>   if $dir has(CMakeLists.txt)

>       add_subdirectory(dir)

>   else

>      find and add $dir/*/CMakeLists.txt

>    

> 

> Then we can add an empty External/Extra directory within source tree and set TEST_SUITE_SUBDIRS=External/Extra by default.

> 

> This way any extra tests placed in External/Extra will be picked up by default without any special configuration options.

>  If user wants something else he can add whatever additional directory he wants and will have a way to do it either one directory at a time or provide a place for wildcard search of directories.


That is already happening with my patch except that you place your additional directories in the test-suite root instead of `External/Extra`.

> Explicitly named directories should probably be cached.


They are.

> Wildcard ones we should pick up without reconfiguration.


This would not be happening after a build directory has been configured. I would argue that this gives you a more deterministic behaviour and is less surprising: Imagine you only wanted to change a single compilation flag but for unrelated reasons have a new module checked out in the test-suite sources, I would expect the cmake run to set the new compilation flag without picking up a set of directories that is different from the last build (unless I explicitely remove the TEST_SUITE_SUBDIRS setting from my cache or setup a new builddir of course).


Repository:
  rL LLVM

http://reviews.llvm.org/D21360