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
