[PATCH] D87256: [clangd] Avoid relations being overwritten in a header shard

[Nathan Ridge via Phabricator via cfe-commits]

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