nridge marked an inline comment as done. nridge added inline comments.
================ Comment at: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp:232 +TEST_F(BackgroundIndexTest, RelationsMultiFile) { + MockFS FS; ---------------- kadircet wrote: > kadircet wrote: > > nridge wrote: > > > kadircet wrote: > > > > do we still need this test? > > > I think it's still useful, yes. If someone makes a change to the way > > > relations are stored in the future, they could regress this use case > > > without a test specifically for it. > > this one was marked as resolved but i still don't see the reasoning. can we > > test this in fileindextests instead? > > > > we already test the sharding logic, we need to test the merging logic now. > > can we rather test it at FileSymbols layer directly? or is there something > > extra that i am misisng? > okay, i still think it is better to test on FileSymbols layer but not that > important. The reason I'd like to have a test at this layer is so that we have a test that closely approximates the steps to reproduce: create files with certain contents and inclusion relationships between them, index them, and perform a particular query. I feel like internal abstractions like `FileSymbols` are liable to change over time through refactorings, and so tests written against them are less robust. Note, I don't think using `BackgroundIndex` is important for the kind of test I'd like; `FileIndex` would be fine too. However, I could not see how to use `FileIndex` with files that contain `#include` statements (didn't see a way to provide a `MockFS` or another way to get the includes to be resolved). That said, I did try to write an additional test using `FileSymbols`, but I found that: * it only exercises the merging logic, not the sharding logic * I couldn't get the test to fail even if I omit the merging logic of this patch, because both `MemIndex` and `Dex` build a `DenseMap` for relations and therefore end up implicitly deduplicating anyways Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87256/new/ https://reviews.llvm.org/D87256 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits