[PATCH] D90291: [clangd] Add lit tests for remote index

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

thanks, lgtm!



================
Comment at: clang-tools-extra/clangd/test/lit.cfg.py:26
                              config.clangd_binary_dir + "/benchmarks"))
+config.substitutions.append(('%python',
+                             config.python_executable))
----------------
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.


================
Comment at: clang-tools-extra/clangd/test/remote-index/pipeline_helper.py:19
+def main():
+  server_address = 'localhost:'
+  # Grab an available port.
----------------
nit: maybe perform the magic after arg parsing ?


================
Comment at: clang-tools-extra/clangd/test/remote-index/pipeline_helper.py:22
+  with socket() as s:
+    s.bind(('', 0))
+    server_address += str(s.getsockname()[1])
----------------
this still binds the socket to all interfaces, can we specify `localhost` or `127.0.0.1` explicitly for the interface address.


================
Comment at: clang-tools-extra/clangd/test/remote-index/pipeline_helper.py:36
+  ],
+                                          stderr=subprocess.PIPE)
+
----------------
nit: formatting looks off, is this really what yapf offers?


================
Comment at: clang-tools-extra/clangd/test/remote-index/pipeline_helper.py:39
+  # Wait for the server to warm-up.
+  if not b'Server listening' in index_server_process.stderr.readline():
+    sys.exit(1)
----------------
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.


================
Comment at: clang-tools-extra/clangd/test/remote-index/pipeline_helper.py:51
+  clangd_process.wait()
+  os.kill(index_server_process.pid, 9)
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90291