[PATCH] D77766: [clangd] Set up machinery for gtests of ClangdLSPServer.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

LGTM, mostly nits.

Unfortunately I don't see any way to make it simpler :/



================
Comment at: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp:104
+
+MATCHER_P(DiagMessage, M, "") {
+  if (const auto *O = arg.getAsObject()) {
----------------
nit: move this to top of the file


================
Comment at: clang-tools-extra/clangd/unittests/LSPClient.cpp:65
+
+  void reply(llvm::json::Value ID,
+             llvm::Expected<llvm::json::Value> V) override {
----------------
put methods before fields


================
Comment at: clang-tools-extra/clangd/unittests/LSPClient.cpp:68
+    if (V)
+      logBody("reply", *V, /*Send=*/false);
+    std::lock_guard<std::mutex> Lock(Mu);
----------------
i think logging errors here could help with debugging.


================
Comment at: clang-tools-extra/clangd/unittests/LSPClient.cpp:71
+    if (auto I = ID.getAsInteger()) {
+      if (*I >= 0 && *I < (int64_t)CallResults.size()) {
+        CallResults[*I].set(std::move(V));
----------------
nit: static_cast


================
Comment at: clang-tools-extra/clangd/unittests/LSPClient.cpp:99
+      Lock.unlock();
+      if (!Action) // stop!
+        return llvm::Error::success();
----------------
this check should happen before accessing Actions.front


================
Comment at: clang-tools-extra/clangd/unittests/LSPClient.cpp:106
+
+public:
+  std::pair<llvm::json::Value, CallResult *> addCallSlot() {
----------------
put public before private


================
Comment at: clang-tools-extra/clangd/unittests/LSPClient.cpp:157
+
+void LSPClient::stop() { T->enqueue(nullptr); }
+
----------------
ah, the `!Action` check for stop now makes sense :D maybe get rid of the `Stop` flag inside transportimpl and say `an empty action terminates the loop` or make this invoke a T->stop(); which sets the flag


================
Comment at: clang-tools-extra/clangd/unittests/LSPClient.cpp:197
+      auto U = PubDiagsParams->getString("uri");
+      if (!U || *U != uri(Path))
+        continue;
----------------
URI is not an optional field, so shouldn't `!U` -> ADD_FAILURE() ?


================
Comment at: clang-tools-extra/clangd/unittests/LSPClient.cpp:199
+        continue;
+      if (const auto *Diagnostics = PubDiagsParams->getArray("diagnostics"))
+        return std::vector<llvm::json::Value>(Diagnostics->begin(),
----------------
again `diagnostics` is not optional


================
Comment at: clang-tools-extra/clangd/unittests/LSPClient.h:44
+    friend TransportImpl;
+    void set(llvm::Expected<llvm::json::Value> V);
+  };
----------------
nit: put it before members, also some comments about "multiple call is failure" might be nice


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77766