[PATCH] D79862: [clangd-remote] Replace YAML serialization with proper Protobuf messages

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Just more stuff about simplifying the tests :-(

This is really just about converting between a struct and a proto, if we have to add indexing-related fixtures to shared libraries something seems to have gone a bit off the rails.
In the worst case we could just create a few symbols with fields populated by hand, it's probably easier to see what parts of the code are being covered by the test...



================
Comment at: clang-tools-extra/clangd/unittests/TestTU.cpp:128
+  SymbolSlab::Builder Merger;
+  for (const auto &Sym : MainCodeSymbols)
+    Merger.insert(Sym);
----------------
can you structure the tests to avoid doing this (untested) merge?

Why do we need the example symbols split over multiple files at all?
If we need such symbols can we just construct them manually instead?


================
Comment at: clang-tools-extra/clangd/unittests/TestTU.h:73
   SymbolSlab headerSymbols() const;
+  RefSlab headerRefs() const;
+  // Returns all symbols from the header and main file.
----------------
please don't add these for a single test, we can do this inline in the test unless it becomes a common thing


================
Comment at: clang-tools-extra/clangd/unittests/remote/CMakeLists.txt:2
+set(LLVM_LINK_COMPONENTS
+  Support
+  )
----------------
Can we conditionally include these tests into the main ClangdTests, instead of setting up another set of targets?


================
Comment at: clang-tools-extra/clangd/unittests/remote/CMakeLists.txt:18
+
+  ../TestTU.cpp
+  ../TestFS.cpp
----------------
I don't think including the same source files in multiple targets is a great path to go down.


================
Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:20
+
+class RemoteMarshallingTest : public ::testing::Test {
+public:
----------------
Using inheritance to define test *cases* is a bit of a trap, and not particularly idiomatic.
Can we just write a function `indexSampleData()` that returns a `SlabTuple`, and call it from the tests?


================
Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:62
+  TestTU TU;
+  llvm::BumpPtrAllocator NewArena;
+};
----------------
similarly i'd consider just inlining the stringsaver/arena into the tests, seems easier to read


================
Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:66
+TEST_F(RemoteMarshallingTest, SymbolSerialization) {
+  for (auto &Sym : symbols()) {
+    const auto ProtobufMeessage = toProtobuf(Sym);
----------------
if you want to define the symbols indirectly like this, you probably also want to assert that there are more than 5 or something, to guard against the test fixture being broken


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79862