[PATCH] D82729: [clangd] Make sure headers are available in FS inside FindSymbolsTests

Tue Jun 30 05:23:19 PDT 2020

sammccall added a comment.

In D82729#2121999 <https://reviews.llvm.org/D82729#2121999>, @kadircet wrote:

> Yes it does, but tests that I updated are also adding a "foo.cpp" file, which tries to include some headers. Those headers can only be retrieved from FS and not from the dirty buffer (that has been populated via Server.AddDocument).
>  Hence even though this change should be invisible for the outcome of the tests (as symbols will still be populated due to indexing of headers), this ensures we've restored them to the proper state after d26721776ff08e0ecdd73b8851c44032d7c0a366 <https://reviews.llvm.org/rGd26721776ff08e0ecdd73b8851c44032d7c0a366>
>  I don't think these tests ever wanted to index the headers directly (as main files), they rather intended the headers to be indexed through `foo.cpp` including them, but I wasn't sure about that assumption hence didn't make that change.

Ah right, I think that change is safe and a good idea (assuming tests pass).
Essentially before the test is doing the wrong thing for setup, changing it to doing the right thing seems better than changing it to do both!

>> I think a slightly more principled fix here is that we should just use TestTU + clangd::getWorkspaceSymbols directly and stop using ClangdServer.
>>  We can land this as-is if tests are flaking and need a quick fix and TestTU is too invasive, but it seems it shouldn't be complicated. WDYT?
> 
> that option also SG, but I think there's value in having a couple tests that are invoked through TUScheduler to test the full production behaviour.
>  So I would at least copy some exemplar ones to ClangdTests.

Maybe one... isn't this covered in our lit tests though?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82729