[clangd] Add lit tests for remote index

[kbobyrev]

Fix test except shutting down part :(

[kbobyrev]

Push the Franken-pytest. Benefits: doesn't fail, is platform independent
(to be determined).

[kbobyrev]

Don't change the existing Python formatting.

[kadircet]

clang-tools-extra/clangd/test/remote-index/pipeline.test

2

nit: / shouldn't be needed here.

the difference between %t and %/t is the latter is expressed with forward slashes on every platform.

3

nit: s/.in.dex/.idx/

nit: maybe create a new Inputs directory under test/remote-index and move the files there?

4

use %python instead to enable lit substitution, just in case there are weird build bots without python on the PATH or the default one pointing at a wrong version.

can we use 127.0.0.1:50051 instead? as starting a server on 0.0.0.0 might be blocked on some bots again. And also I would rather use a random port number (greater than 1024, possibly by letting the python script choose the number, and getting rid of the argument completely), to lower the chances of failing accidentally when 50051 is in use. (it might be because bot is running multiple tests in parallel, or bot itself has a grpc server running on default port for whatever reason).

clang-tools-extra/clangd/test/remote-index/pipeline_helper.py

26 ↗

(On Diff #301303)

can we rename test-directory to project-root and pass it directly from lit ?

this will also contain \\ when run on windows.

32 ↗

(On Diff #301303)

do we really need the abspath conversion here ? is it to get rid of .. ? i am uneasy about this as the call is likely to introduce \\.

36 ↗

(On Diff #301303)

can we instead wait until server prints Server listening on ... ?

47 ↗

(On Diff #301303)

can we just wait until clangd process shuts down ?

[ArcsinX]

clang-tools-extra/clangd/test/remote-index/pipeline.test

4

can we use 127.0.0.1:50051 instead?

I think we should do this, because we also pass this address into --remote-index-address= clangd parameter, which is illegal. Seems Linux and Mac allow to call connect with 0.0.0.0 as a destination address, but Windows does not and this test fails.

[kbobyrev]

Resolve review comments.

[kbobyrev]

Rebase on top of master.

[kbobyrev]

clang-tools-extra/clangd/test/remote-index/pipeline.test

4

Thanks for checking! Can you try run this test now and see if it works?

[ArcsinX]

clang-tools-extra/clangd/test/remote-index/pipeline.test

4

Is there any reason to use %/S in --project-root? We support --project-root with native slashes (at least --project-root=%S works for me on Windows + MS compiler)

It looks more reasonable to use %/S in paths like this %/S/Inputs/Source to avoid mixing slashes on Windows (but it works correctly with mixing slashes anyway)

4

It works ok now.

[kbobyrev]

Rebase on top of master, don't force forward slashes in project root,
format Python helper.

[kadircet]

thanks, lgtm!

clang-tools-extra/clangd/test/lit.cfg.py

26

we should rather call lit.llvm.llvm_config.use_default_substitutions(), preferably right after use_clang() on line 4.

sorry for the churn, i thought we were already doing this.

clang-tools-extra/clangd/test/remote-index/pipeline_helper.py

19 ↗

(On Diff #301619)

nit: maybe perform the magic after arg parsing ?

22 ↗

(On Diff #301619)

this still binds the socket to all interfaces, can we specify localhost or 127.0.0.1 explicitly for the interface address.

36 ↗

(On Diff #301619)

nit: formatting looks off, is this really what yapf offers?

39 ↗

(On Diff #301619)

i would rather scan through all lines and look for Server listening on $server_addres. We should be able to either hit the end of stream (i.e. server encounters a failure and shuts down) or find the log message we are looking for.

just to make test less reliant on log ordering.

51 ↗

(On Diff #301619)

can we rather use signal.SIGXXX here instead of 9 ?

Also rather than kill, SIGINT might be more applicable. https://docs.python.org/3/library/signal.html#signal.SIGKILL claims sigkill is not available on windows.

[kbobyrev]

Resolve post-LGTM commentts, increase index hot reload frequency.

clang-tools-extra/clangd/test/remote-index/pipeline_helper.py

36 ↗

(On Diff #301619)

Yep, I'm formatting with it :(

39 ↗

(On Diff #301619)

Unfortunately, it's not as easy :( Then I would have to set a time to wait for the server to warm up and read lines then only. The problem here is that the server runs, doesn't shut down but also doesn't print the message. We'll hang the buildbots then.

But I'll go with the timeout option. I don't think it's better than checking the first line as the initialize one but still.

[kbobyrev]

clang-tools-extra/clangd/test/remote-index/pipeline_helper.py

51 ↗

(On Diff #301619)

The problem with SIGINT is that it needs to wait for the hot reload sync to finish :) I'll add a patch to modify the frequency via CLI to avoid long test terminaiton.

[kadircet]

clang-tools-extra/clangd/test/remote-index/pipeline_helper.py

42 ↗

(On Diff #302213)

so if we think .readline() is blocking but this is not, can't we just keep calling readline instead?

39 ↗

(On Diff #301619)

Unfortunately, it's not as easy :( Then I would have to set a time to wait for the server to warm up and read lines then only

Why? isn't the readline() on stderr blocking ? If not how was this code working without sleep before?

The problem here is that the server runs, doesn't shut down but also doesn't print the message. We'll hang the buildbots then.

If the argument above is correct, this was the case already, i.e. stderr.readline() would block until a line is available or server had shutdown.

To overcome this problem I would suggest having a separate thread that would kill the server if it is still running after N seconds.

[hans]

clang-tools-extra/clangd/test/lit.site.cfg.py.in

11

Could this use PYTHON_EXECUTABLE instead? I see other tests using that, and Python3_EXECUTABLE might not always be set.

[kadircet]

clang-tools-extra/clangd/test/lit.site.cfg.py.in

11

i was checking llvm's lit.site.cfg.py.in for reference and it was using python3 too.

any specific reasons for using PYTHON_EXECUTABLE instead of python3? I can't say this was a strong preference, but the python script included might not be python2 compliant, hence the need for 3.

If there is a compelling reason for using a possibly python2 executable, we can try to make the script both 2 and 3 compliant.

[hans]

clang-tools-extra/clangd/test/lit.site.cfg.py.in

11

i was checking llvm's lit.site.cfg.py.in for reference and it was using python3 too.

Oh interesting, I hadn't seen that. Well then using it here too makes sense I suppose. Sorry for the noise :)

clang-tools-extra/clangd/test/CMakeLists.txt

 # Set CLANG_TOOLS_DIR to buildtree/bin, or buildtree/%(build_mode)s/bin if the
 # location is dynamic. The latter must be interpolated by lit configs.
 # FIXME: this is duplicated in many places.
 if (CMAKE_CFG_INTDIR STREQUAL ".")
   set(LLVM_BUILD_MODE ".")
 else ()
   set(LLVM_BUILD_MODE "%(build_mode)s")
 endif ()
 string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} CLANG_TOOLS_DIR ${LLVM_RUNTIME_OUTPUT_INTDIR})
 
 set(CLANGD_TEST_DEPS
   clangd
   ClangdTests
-  # No tests for these, but we should still make sure they build.
   clangd-indexer
+  # No tests for it, but we should still make sure they build.
   dexp
   )
 
 if(CLANGD_BUILD_XPC)
   list(APPEND CLANGD_TEST_DEPS clangd-xpc-test-client)
   list(APPEND CLANGD_TEST_DEPS ClangdXpcUnitTests)
 endif()
 

clang-tools-extra/clangd/test/Inputs/remote-index/Header.h

+namespace clang {
+namespace clangd {
+namespace remote {
+int getFoo(bool Argument);
+char Character;
+} // namespace remote
+} // namespace clangd
+} // namespace clang
+
+namespace llvm {} // namespace llvm

clang-tools-extra/clangd/test/Inputs/remote-index/Source.cpp

+#include "Header.h"
+
+namespace {} // namespace

clang-tools-extra/clangd/test/lit.cfg.py

 import lit.llvm
 
 lit.llvm.initialize(lit_config, config)
 lit.llvm.llvm_config.use_clang()
 
 config.name = 'Clangd'
 config.suffixes = ['.test']
 config.excludes = ['Inputs']
 config.test_format = lit.formats.ShTest(not lit.llvm.llvm_config.use_lit_shell)
 config.test_source_root = config.clangd_source_dir + "/test"
 config.test_exec_root = config.clangd_binary_dir + "/test"
 
 
 # Used to enable tests based on the required targets. Can be queried with e.g.
 #    REQUIRES: x86-registered-target
 def calculate_arch_features(arch_string):
     return [arch.lower() + '-registered-target' for arch in arch_string.split()]
 
 
 lit.llvm.llvm_config.feature_config([('--targets-built',
                                       calculate_arch_features)])
 
 # Clangd-specific lit environment.
 config.substitutions.append(('%clangd-benchmark-dir',
                              config.clangd_binary_dir + "/benchmarks"))
 
 if config.clangd_build_xpc:
     config.available_features.add('clangd-xpc-support')
+
+if config.clangd_enable_remote:
+    config.available_features.add('clangd-remote-index')

clang-tools-extra/clangd/test/lit.site.cfg.py.in

 @LIT_SITE_CFG_IN_HEADER@
 
 # Variables needed for common clang config.
 config.lit_tools_dir = "@LLVM_LIT_TOOLS_DIR@"
 config.clang_tools_dir = "@CLANG_TOOLS_DIR@"
 config.llvm_tools_dir = "@LLVM_TOOLS_DIR@"
 config.clang_libs_dir = "@CLANG_LIBS_DIR@"
 config.llvm_libs_dir = "@LLVM_LIBS_DIR@"
 config.target_triple = "@TARGET_TRIPLE@"
 config.host_triple = "@LLVM_HOST_TRIPLE@"
 
 # Support substitution of the tools and libs dirs with user parameters. This is
 # used when we can't determine the tool dir at configuration time.
 try:
     config.clang_tools_dir = config.clang_tools_dir % lit_config.params
     config.llvm_tools_dir = config.llvm_tools_dir % lit_config.params
     config.llvm_libs_dir = config.llvm_libs_dir % lit_config.params
 except KeyError:
     e = sys.exc_info()[1]
     key, = e.args
     lit_config.fatal("unable to find %r parameter, use '--param=%s=VALUE'" % (key,key))
 
 config.clangd_source_dir = "@CMAKE_CURRENT_SOURCE_DIR@/.."
 config.clangd_binary_dir = "@CMAKE_CURRENT_BINARY_DIR@/.."
 config.clangd_build_xpc = @CLANGD_BUILD_XPC@
+config.clangd_enable_remote = @CLANGD_ENABLE_REMOTE@
 
 # Delegate logic to lit.cfg.py.
 lit_config.load_config(config, "@CMAKE_CURRENT_SOURCE_DIR@/lit.cfg.py")

clang-tools-extra/clangd/test/remote-index/pipeline.test

+# FIXME: Unsupported on Windows because of POSIX features (export, &, kill).
+# UNSUPPORTED: darwin, windows
+# RUN: rm -rf %/t
+# RUN: export PROJECT_ROOT=$(readlink -f %/S/../Inputs/remote-index/)
+# RUN: clangd-indexer $PROJECT_ROOT/Source.cpp > %t.in.dex
+# FIXME: Check that default address (0.0.0.0:50051) is available.
+# RUN: export SERVER_ADDRESS="0.0.0.0:50051"
+# RUN: clangd-index-server --server-address=$SERVER_ADDRESS %t.in.dex $PROJECT_ROOT & export PID=$!
+# RUN: sleep 5
+# RUN: clangd --remote-index-address=$SERVER_ADDRESS --project-root=$PROJECT_ROOT --lit-test --sync < %s | FileCheck %s
+# RUN: kill -SIGINT $PID
+# REQUIRES: clangd-remote-index
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+#      CHECK:  "id": 0,
+# CHECK-NEXT:  "jsonrpc": "2.0",
+---
+{"jsonrpc":"2.0","id":1,"method":"workspace/symbol","params":{"query":"Character"}}
+#      CHECK:  "id": 1,
+# CHECK-NEXT:  "jsonrpc": "2.0",
+# CHECK-NEXT:    "result": [
+# CHECK-NEXT:      {
+# CHECK-NEXT:        "containerName": "clang::clangd::remote",
+# CHECK-NEXT:        "kind": 13,
+# CHECK-NEXT:        "location": {
+# CHECK-NEXT:          "range": {
+# CHECK-NEXT:            "end": {
+# CHECK-NEXT:              "character": {{.*}},
+# CHECK-NEXT:              "line": {{.*}}
+# CHECK-NEXT:            },
+# CHECK-NEXT:            "start": {
+# CHECK-NEXT:              "character": {{.*}},
+# CHECK-NEXT:              "line": {{.*}}
+# CHECK-NEXT:            }
+# CHECK-NEXT:          },
+# CHECK-NEXT:          "uri": "file://{{.*}}/Header.h"
+# CHECK-NEXT:        },
+# CHECK-NEXT:        "name": "Character",
+# CHECK-NEXT:        "score": {{.*}}
+# CHECK-NEXT:      }
+# CHECK-NEXT:    ]
+# CHECK-NEXT:}
+---
+{"jsonrpc":"2.0","id":4,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
+---