test-suite cmake: Detect test subdirectories when running cmake.

[MatzeB]

Summary

The intention here is that we can have test-suites in independent
repositories using the same infrastructure. Those test-suites can be
checked out into subdirectories of the test-suite and will be picked up
automatically when cmake runs.

I'd like to get some feedback about:

• Is this a good idea?
• Is it fine to search for */CMakeLists.txt on the toplevel test-suite directory? Should we also check the builddir for it? Do we need an extra configuration flag?
• We currently detect subdirectories only when there is no list of subdirs in the CMakeCache yet. This makes the behaviour deterministic across cmake runs but cmake will also miss newly checked test-suites when run a 2nd time in a configured build directory.

[tra]

Is this a good idea?

I could use it to experiment with tests I'm planning to run as part of CUDA build-bot without changing buildbot config every time I want to tweak something. I.e. I could configure buildbot with fixed set of parameters, with TEST_SUITE_SUBDIRS including directory *outside* of the tree checked out by buildbot. I could then change contents of that directory when I want to change what/how I test.

Is it fine to search for */CMakeLists.txt on the toplevel test-suite directory?

I don't see much point of doing that. For directories that are part of the source tree we already know they are there and can include them explicitly (assuming there are only few of them). External directories would also be explicitly specified via TEST_SUITE_SUBDIRS.

Do you have particular use case for wildcard search?

Should we also check the builddir for it?

Probably not. Build dir is something that can be nuked at any time -- not a good place for tests, IMO.

Do we need an extra configuration flag?

For wildcard search? Probably not, as I'm not sure we need wildcard search at all.

We currently detect subdirectories only when there is no list of subdirs in the CMakeCache yet. This makes the behaviour deterministic across cmake runs but cmake will also miss newly checked test-suites when run a 2nd time in a configured build directory.

My vote would be for deterministic behavior. Test setups can be rather heavy-weight. I would not want to re-check gazillion files/directories on every run just in case some new test was added there.

CMakeLists.txt

168–174

Assuming that we do end up with wildcard search, instead of explicitly excluding directories that do not contain tests (tools in this case), I'd instead explicitly mark directories that contain tests. For instance we may require each top-level test directory to have a test-description.txt and search for that file, instead of CMakeLists.txt.

[MatzeB]

In D21360#458243, @tra wrote:

Is it fine to search for */CMakeLists.txt on the toplevel test-suite directory?

I don't see much point of doing that. For directories that are part of the source tree we already know they are there and can include them explicitly (assuming there are only few of them). External directories would also be explicitly specified via TEST_SUITE_SUBDIRS.

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). Please also note that this will of course only pickup 1 level of directories and not recurse into subdirectories so it should not be a performance problem.

[jmolloy]

I like it. I also like the wildcard matching.

I've had to put third party test suite drivers in the LLVM test-suite (our local copy of it) for exactly this reason; it'd be amazing to decouple the drivers from the test-suite and store them alongside the test source.

[tra]

In 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".
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.

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.

Explicitly named directories should probably be cached.
Wildcard ones we should pick up without reconfiguration.

Does it make sense?

[MatzeB]

In D21360#459428, @tra wrote:

In 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).

[tra]

LGTM.

[cmatthews]

1. This is a great idea!!!

2. it is okay that it only scans once.

3. I dont think we should check the builddir: I find that counter intuitive.

4. I think a flag is best, it would be great if we can add more than one test suite at a time though. I bet that is done more easily through wildcards? The way I see this used is several test sets checked out side by side (to not muck with the checkouts), so passed via flag makes sense to me.

Ideally I don't want the test suite itself to run by default. The way I see myself using this is checking out a bunch of smaller tests, and combining them as a new bot. So, for instance in my O3 X86 bot, I checkout SPEC, and cool suite 1 and cool suite 2, and with one cmake invocation I have all those suites run in that configuration.

CMakeLists.txt

if(TEST_SUITE_TAKE_COMPILE_TIME)
   set(CMAKE_CXX_COMPILE_OBJECT "${CMAKE_BINARY_DIR}/tools/timeit --summary <OBJECT>.time ${CMAKE_CXX_COMPILE_OBJECT}")
   set(CMAKE_C_LINK_EXECUTABLE "${CMAKE_BINARY_DIR}/tools/timeit --summary <TARGET>.link.time ${CMAKE_C_LINK_EXECUTABLE}")
   set(CMAKE_CXX_LINK_EXECUTABLE "${CMAKE_BINARY_DIR}/tools/timeit --summary <TARGET>.link.time ${CMAKE_CXX_LINK_EXECUTABLE}")
 endif()
 
 set(TEST_SUITE_BENCHMARKING_ONLY "OFF" CACHE BOOL
   "Only run the benchmarking only subset")
 
-add_subdirectory(SingleSource)
-add_subdirectory(MultiSource)
-add_subdirectory(External)
+
+# Detect and include subdirectories
+if(NOT TEST_SUITE_SUBDIRS)
+  file(GLOB sub_cmakelists */CMakeLists.txt)
+  set(TEST_SUITE_SUBDIRS "")
+  foreach(entry ${sub_cmakelists})
+    get_filename_component(subdir ${entry} DIRECTORY)
+    if(NOT ${subdir} STREQUAL ${CMAKE_CURRENT_SOURCE_DIR}/tools)
+      list(APPEND TEST_SUITE_SUBDIRS ${subdir})
+    endif()
+  endforeach()
+  set(TEST_SUITE_SUBDIRS "${TEST_SUITE_SUBDIRS}" CACHE STRING
+      "Semicolon separated list of directories with CMakeLists.txt to include")
+endif()
+mark_as_advanced(TEST_SUITE_SUBDIRS)
+
+foreach(subdir ${TEST_SUITE_SUBDIRS})
+  message(STATUS "Adding directory ${subdir}")
+  add_subdirectory(${subdir})
+endforeach()
+
 
 set(LIT_MODULES "")
 list(APPEND LIT_MODULES run)
 list(APPEND LIT_MODULES codesize)
 list(APPEND LIT_MODULES hash)
 if(TEST_SUITE_TAKE_COMPILE_TIME)
   list(APPEND LIT_MODULES compiletime)
 endif()