Add CLANGD_INCLUDE_TESTS as a separate flag to control clangd tests

[hiraditya]

Summary

Keeping the default value ON to preserve the current behavior

Motivation:
1. Some of the clangd (TUSchedulerTests) tests intermittently fails. There are no internal changes to clang-tools-extra
directory. Either the test times out or it fails with mismatches.
2. It also makes sense to be able to disable clangd tests separately from the rest of clang-tools because clangd has it's own unittests directory.

Issue created: https://github.com/llvm/llvm-project/issues/64964
Previously reported issues: https://github.com/llvm/llvm-project/issues/59644, https://github.com/llvm/llvm-project/issues/50117, https://github.com/clangd/clangd/issues/1712

A typical output of ninja check-clang-tools

out/llvm-project/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:1354: Failure
Value of: S.blockUntilIdle(timeoutSeconds(10))
  Actual: false
Expected: true

Built preamble of size 207320 for file /clangd-test/foo.cpp version 3 in 5.44 seconds
Reusing preamble version 3 for version 4 of /clangd-test/foo.cpp
out/llvm-project/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:1318: Failure
Value of: Collector.diagVersions().back()
Expected: has a first field that is equal to "3", and has a second field that is equal to "3"
  Actual: ("3", "4"), whose second field does not match

out/llvm-project/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:1311
Value of: Collector.diagVersions().back()
Expected: has a first field that is equal to "3", and has a second field that is equal to "3"
  Actual: ("2", "3"), whose first field does not match
out/llvm-project/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:1318
Value of: Collector.diagVersions().back()
Expected: has a first field that is equal to "3", and has a second field that is equal to "3"
  Actual: ("3", "4"), whose second field does not match

[hiraditya]

Added previously reported issues shared by @nridge

[bogner]

I'm not entirely convinced by the motivation (wouldn't it be better to just fix the tests?), but this seems like a perfectly reasonable flag to have.

[ilya-biryukov]

Open question: I also feel like the best option here is to fix the tests, but I'm not sure how hard that would be. @sammccall any thoughts?
I suspect the particular tests are flaky is because they rely on timeouts, not sure it's easy to disentangle them. Therefore, some workaround seems reasonable

If we land this, we should ensure that when CLANG_INCLUDE_TESTS is off, Clangd tests don't run.
Clangd should respect the LLVM-wide options even if it has a more specific one for the project itself.

I'm marking as requiring changes mostly for the latter comment about CLANG_INCLUDE_TESTS.

[hiraditya]

I'm marking as requiring changes mostly for the latter comment about CLANG_INCLUDE_TESTS.

Added CLANG_INCLUDE_TESTS to the conditional. btw when CLANG_INCLUDE_TESTS is OFF, ninja check-clang-tools target doesn't get created so adding CLANG_INCLUDE_TESTS here didn't seem necessary, although tbh i'm not too familiar with the build system.

[kadircet]

In D158566#4616417, @ilya-biryukov wrote:

Open question: I also feel like the best option here is to fix the tests, but I'm not sure how hard that would be. @sammccall any thoughts?
I suspect the particular tests are flaky is because they rely on timeouts, not sure it's easy to disentangle them. Therefore, some workaround seems reasonable

FWIW, i've put some details in https://github.com/llvm/llvm-project/issues/64964#issuecomment-1702249740 and we had some previous discussions but unfortunately these tests have timeouts as a "poor-mans-deadlock-detection". i don't think we can get rid of the timeouts, without sacrificing that detection.
I can't remember how misleading buildbot outputs were, when deadlocks happened, before we introduced timeouts though. So one alternative is let the buildbots hang instead.

---

Regarding the approach in this patch, I don't feel strongly about it but I don't think it's a good idea to let people build clangd, without testing it on environments they care about. They might suppress legitimate issues. (there's also some value though, e.g. maybe they already performed testing before, and don't want to run tests again, but in such a scenario we've non-check equivalents of targets to only run builds).

Are you building clangd deliberately or is it just being pulled in via check-clang-tools? If you don't want to ship clangd with your toolchain at all, I think it's better to set CLANG_ENABLE_CLANGD to OFF.

[hiraditya]

In D158566#4633847, @kadircet wrote:

In D158566#4616417, @ilya-biryukov wrote:

Open question: I also feel like the best option here is to fix the tests, but I'm not sure how hard that would be. @sammccall any thoughts?
I suspect the particular tests are flaky is because they rely on timeouts, not sure it's easy to disentangle them. Therefore, some workaround seems reasonable

FWIW, i've put some details in https://github.com/llvm/llvm-project/issues/64964#issuecomment-1702249740 and we had some previous discussions but unfortunately these tests have timeouts as a "poor-mans-deadlock-detection". i don't think we can get rid of the timeouts, without sacrificing that detection.
I can't remember how misleading buildbot outputs were, when deadlocks happened, before we introduced timeouts though. So one alternative is let the buildbots hang instead.

---

Regarding the approach in this patch, I don't feel strongly about it but I don't think it's a good idea to let people build clangd, without testing it on environments they care about. They might suppress legitimate issues. (there's also some value though, e.g. maybe they already performed testing before, and don't want to run tests again, but in such a scenario we've non-check equivalents of targets to only run builds).

This option only allows more flexibility to the developers and by default it is on so anyone who doesn't care about switching off the test on constrained systems can continue to do so. Do you have specific concerns on why this will prevent developers from testing?

Are you building clangd deliberately or is it just being pulled in via check-clang-tools? If you don't want to ship clangd with your toolchain at all, I think it's better to set CLANG_ENABLE_CLANGD to OFF.

We want to build clangd and it gets tested on a regular basis but the timeout happens on mac builds that are overloaded at times. I don't want to disable the build of clangd just because 1 test is failing.

[kadircet]

hi @hiraditya , i believe your issues should disappear starting with 9a26d2c6d35f574d7a4b06a5a22f8a1c063cb664. LMK if you're still facing problems and want to move forward with such a patch

clang-tools-extra/clangd/CMakeLists.txt

 if ( CLANGD_BUILD_XPC )
   add_subdirectory(xpc)
 endif ()
 
 if (CLANGD_ENABLE_REMOTE)
   include(AddGRPC)
 endif()
 
-if(CLANG_INCLUDE_TESTS)
+if(CLANGD_INCLUDE_TESTS AND CLANG_INCLUDE_TESTS)
   add_subdirectory(test)
   add_subdirectory(unittests)
 endif()
 
 # FIXME(kirillbobyrev): Document this in the LLVM docs once remote index is stable.
 option(CLANGD_ENABLE_REMOTE "Use gRPC library to enable remote index support for Clangd" OFF)
+option(CLANGD_INCLUDE_TESTS "Include Clangd tests" ON)
 set(GRPC_INSTALL_PATH "" CACHE PATH "Path to gRPC library manual installation.")
 
 add_subdirectory(index/remote)
 add_subdirectory(index/dex/dexp)