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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
