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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/test/remote-index/pipeline.test:1
+# RUN: rm -rf %/t
+# RUN: clangd-indexer %/S/../Inputs/remote-index/Source.cpp > %t.in.dex
----------------
nit: `/` shouldn't be needed here.

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


================
Comment at: clang-tools-extra/clangd/test/remote-index/pipeline.test:2
+# RUN: rm -rf %/t
+# RUN: clangd-indexer %/S/../Inputs/remote-index/Source.cpp > %t.in.dex
+# RUN: python %/S/pipeline_helper.py --input-file-name=%s --server-address=0.0.0.0:50051 --test-directory=%/S --index-file=%t.in.dex | FileCheck %s
----------------
nit: s/.in.dex/.idx/

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


================
Comment at: clang-tools-extra/clangd/test/remote-index/pipeline.test:3
+# RUN: clangd-indexer %/S/../Inputs/remote-index/Source.cpp > %t.in.dex
+# RUN: python %/S/pipeline_helper.py --input-file-name=%s --server-address=0.0.0.0:50051 --test-directory=%/S --index-file=%t.in.dex | FileCheck %s
+# REQUIRES: clangd-remote-index
----------------
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).


================
Comment at: clang-tools-extra/clangd/test/remote-index/pipeline_helper.py:26
+
+  project_root = os.path.join(args.test_directory, '..', 'Inputs',
+                              'remote-index')
----------------
can we rename `test-directory` to `project-root` and pass it directly from lit ?

this will also contain `\\` when run on windows.


================
Comment at: clang-tools-extra/clangd/test/remote-index/pipeline_helper.py:32
+      os.path.abspath(args.index_file),
+      os.path.abspath(project_root)
+  ])
----------------
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 `\\`.


================
Comment at: clang-tools-extra/clangd/test/remote-index/pipeline_helper.py:36
+  # Wait for the server to warm-up.
+  time.sleep(2)
+
----------------
can we instead wait until server prints `Server listening on ...` ?


================
Comment at: clang-tools-extra/clangd/test/remote-index/pipeline_helper.py:47
+  # Keep the server alive until request is complete.
+  time.sleep(1)
+
----------------
can we just wait until `clangd` process shuts down ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90291