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

[sammccall]

Why? workspaceSymbols depends on the index only.
ISTM the dynamic index + blockUntilIdle should be enough - what does the FS have to do with it?
Oh, is this a race condition due to async preamble builds?

---

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?

[kadircet]

In D82729#2120647, @sammccall wrote:

Why? workspaceSymbols depends on the index only.
ISTM the dynamic index + blockUntilIdle should be enough - what does the FS have to do with it?

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
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.

Oh, is this a race condition due to async preamble builds?

There was a race condition (due to modification of FS while ASTWorker was working and MockFS's lack of being thread-safe) that has been fixed in d26721776ff08e0ecdd73b8851c44032d7c0a366.
I suppose you are talking about the recent flakes on some build bots, it wasn't my intention to fix them in this patch (nor I think this has patch will have any effects on that flakiness, if you think so please
elaborate, as it likely means I am missing something)

---

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.

[sammccall]

In 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
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?

[kadircet]

abandoning in favor of D82944

clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp

addFile("foo.cpp", R"cpp(
         struct LocalClass {};
         int local_var;
       })cpp");
   EXPECT_THAT(getSymbols("l"), IsEmpty());
   EXPECT_THAT(getSymbols("p"), IsEmpty());
 }
 
 TEST_F(WorkspaceSymbolsTest, Globals) {
-  addFile("foo.h", R"cpp(
+  FS.Files["foo.h"] = R"cpp(
       int global_var;
 
       int global_func();
 
-      struct GlobalStruct {};)cpp");
+      struct GlobalStruct {};)cpp";
+
+  addFile("foo.h", FS.Files["foo.h"]);
   addFile("foo.cpp", R"cpp(
-      #include "foo.h"
-      )cpp");
+      #include "foo.h")cpp");
   EXPECT_THAT(getSymbols("global"),
               UnorderedElementsAre(
                   AllOf(QName("GlobalStruct"), WithKind(SymbolKind::Struct)),
                   AllOf(QName("global_func"), WithKind(SymbolKind::Function)),
                   AllOf(QName("global_var"), WithKind(SymbolKind::Variable))));
 }
 
 TEST_F(WorkspaceSymbolsTest, Unnamed) {
-  addFile("foo.h", R"cpp(
+  FS.Files["foo.h"] = R"cpp(
       struct {
         int InUnnamed;
-      } UnnamedStruct;)cpp");
+      } UnnamedStruct;)cpp";
+  addFile("foo.h", FS.Files["foo.h"]);
   addFile("foo.cpp", R"cpp(
       #include "foo.h"
       )cpp");
   EXPECT_THAT(getSymbols("UnnamedStruct"),
               ElementsAre(AllOf(QName("UnnamedStruct"),
                                 WithKind(SymbolKind::Variable))));
   EXPECT_THAT(getSymbols("InUnnamed"),
               ElementsAre(AllOf(QName("(anonymous struct)::InUnnamed"),
                                 WithKind(SymbolKind::Field))));
 }
 
 TEST_F(WorkspaceSymbolsTest, InMainFile) {
   addFile("foo.cpp", R"cpp(
       int test() {}
       static test2() {}
       )cpp");
   EXPECT_THAT(getSymbols("test"), ElementsAre(QName("test"), QName("test2")));
 }
 
 TEST_F(WorkspaceSymbolsTest, Namespaces) {
-  addFile("foo.h", R"cpp(
+  FS.Files["foo.h"] = R"cpp(
       namespace ans1 {
         int ai1;
       namespace ans2 {
         int ai2;
       }
-      }
-      )cpp");
+      })cpp";
+  addFile("foo.h", FS.Files["foo.h"]);
   addFile("foo.cpp", R"cpp(
       #include "foo.h"
       )cpp");
   EXPECT_THAT(getSymbols("a"),
               UnorderedElementsAre(QName("ans1"), QName("ans1::ai1"),
                                    QName("ans1::ans2"),
                                    QName("ans1::ans2::ai2")));
   EXPECT_THAT(getSymbols("::"), ElementsAre(QName("ans1")));
addFile("foo.cpp", R"cpp(
       namespace {
       void test() {}
       }
       )cpp");
   EXPECT_THAT(getSymbols("test"), ElementsAre(QName("test")));
 }
 
 TEST_F(WorkspaceSymbolsTest, MultiFile) {
-  addFile("foo.h", R"cpp(
-      int foo() {
-      }
-      )cpp");
-  addFile("foo2.h", R"cpp(
-      int foo2() {
-      }
-      )cpp");
+  FS.Files["foo.h"] = "int foo() {}";
+  FS.Files["foo2.h"] = "int foo2() {}";
+
+  addFile("foo.h", FS.Files["foo.h"]);
+  addFile("foo2.h", FS.Files["foo2.h"]);
   addFile("foo.cpp", R"cpp(
       #include "foo.h"
-      #include "foo2.h"
-      )cpp");
+      #include "foo2.h")cpp");
   EXPECT_THAT(getSymbols("foo"),
               UnorderedElementsAre(QName("foo"), QName("foo2")));
 }
 
 TEST_F(WorkspaceSymbolsTest, GlobalNamespaceQueries) {
-  addFile("foo.h", R"cpp(
+  FS.Files["foo.h"] = R"cpp(
       int foo() {
       }
       class Foo {
         int a;
       };
       namespace ns {
       int foo2() {
       }
-      }
-      )cpp");
+      })cpp";
+  addFile("foo.h", FS.Files["foo.h"]);
   addFile("foo.cpp", R"cpp(
       #include "foo.h"
       )cpp");
   EXPECT_THAT(getSymbols("::"),
               UnorderedElementsAre(
                   AllOf(QName("Foo"), WithKind(SymbolKind::Class)),
                   AllOf(QName("foo"), WithKind(SymbolKind::Function)),
                   AllOf(QName("ns"), WithKind(SymbolKind::Namespace))));
   EXPECT_THAT(getSymbols(":"), IsEmpty());
   EXPECT_THAT(getSymbols(""), IsEmpty());
 }
 
 TEST_F(WorkspaceSymbolsTest, Enums) {
-  addFile("foo.h", R"cpp(
+  FS.Files["foo.h"] = R"cpp(
     enum {
       Red
     };
     enum Color {
       Green
     };
     enum class Color2 {
       Yellow
     };
     namespace ns {
       enum {
         Black
       };
       enum Color3 {
         Blue
       };
       enum class Color4 {
         White
       };
-    }
-      )cpp");
+    })cpp";
+  addFile("foo.h", FS.Files["foo.h"]);
   addFile("foo.cpp", R"cpp(
       #include "foo.h"
       )cpp");
   EXPECT_THAT(getSymbols("Red"), ElementsAre(QName("Red")));
   EXPECT_THAT(getSymbols("::Red"), ElementsAre(QName("Red")));
   EXPECT_THAT(getSymbols("Green"), ElementsAre(QName("Green")));
   EXPECT_THAT(getSymbols("Green"), ElementsAre(QName("Green")));
   EXPECT_THAT(getSymbols("Color2::Yellow"),
               ElementsAre(QName("Color2::Yellow")));
   EXPECT_THAT(getSymbols("Yellow"), ElementsAre(QName("Color2::Yellow")));
 
   EXPECT_THAT(getSymbols("ns::Black"), ElementsAre(QName("ns::Black")));
   EXPECT_THAT(getSymbols("ns::Blue"), ElementsAre(QName("ns::Blue")));
   EXPECT_THAT(getSymbols("ns::Color4::White"),
               ElementsAre(QName("ns::Color4::White")));
 }
 
 TEST_F(WorkspaceSymbolsTest, Ranking) {
-  addFile("foo.h", R"cpp(
+  FS.Files["foo.h"] = R"cpp(
       namespace ns{}
       void func();
-      )cpp");
+      )cpp";
+  addFile("foo.h", FS.Files["foo.h"]);
   addFile("foo.cpp", R"cpp(
-      #include "foo.h"
-      )cpp");
+      #include "foo.h")cpp");
   EXPECT_THAT(getSymbols("::"), ElementsAre(QName("func"), QName("ns")));
 }
 
 TEST_F(WorkspaceSymbolsTest, WithLimit) {
-  addFile("foo.h", R"cpp(
+  FS.Files["foo.h"] = R"cpp(
       int foo;
-      int foo2;
-      )cpp");
+      int foo2;)cpp";
+  addFile("foo.h", FS.Files["foo.h"]);
   addFile("foo.cpp", R"cpp(
       #include "foo.h"
       )cpp");
   // Foo is higher ranked because of exact name match.
   EXPECT_THAT(getSymbols("foo"),
               UnorderedElementsAre(
                   AllOf(QName("foo"), WithKind(SymbolKind::Variable)),
                   AllOf(QName("foo2"), WithKind(SymbolKind::Variable))));
TEST_F(DocumentSymbolsTest, Concepts) {
   std::string FilePath = testPath("foo.cpp");
   addFile(FilePath,
           "template <typename T> concept C = requires(T t) { t.foo(); };");
 
   EXPECT_THAT(getSymbols(FilePath), ElementsAre(WithName("C")));
 }
 
 TEST_F(DocumentSymbolsTest, ExternSymbol) {
+  FS.Files["foo.h"] = R"cpp(
+      extern int var;)cpp";
+
+  addFile(testPath("foo.h"), FS.Files["foo.h"]);
   std::string FilePath = testPath("foo.cpp");
-  addFile(testPath("foo.h"), R"cpp(
-      extern int var;
-      )cpp");
   addFile(FilePath, R"cpp(
-      #include "foo.h"
-      )cpp");
+      #include "foo.h")cpp");
 
   EXPECT_THAT(getSymbols(FilePath), IsEmpty());
 }
 
 TEST_F(DocumentSymbolsTest, NoLocals) {
   std::string FilePath = testPath("foo.cpp");
   addFile(FilePath,
           R"cpp(
EXPECT_THAT(
           AllOf(WithName("(anonymous struct)"), WithKind(SymbolKind::Struct),
                 Children(AllOf(WithName("InUnnamed"),
                                WithKind(SymbolKind::Field), Children()))),
           AllOf(WithName("UnnamedStruct"), WithKind(SymbolKind::Variable),
                 Children())));
 }
 
 TEST_F(DocumentSymbolsTest, InHeaderFile) {
-  addFile(testPath("bar.h"), R"cpp(
-      int foo() {
-      }
-      )cpp");
-  std::string FilePath = testPath("foo.h");
-  addFile(FilePath, R"cpp(
+  FS.Files["bar.h"] = R"cpp(
+      int foo() {})cpp";
+  FS.Files["foo.h"] = R"cpp(
       #include "bar.h"
-      int test() {
-      }
-      )cpp");
+      int test() {})cpp";
+
+  std::string FilePath = testPath("foo.h");
+  addFile(testPath("bar.h"), FS.Files["bar.h"]);
+  addFile(FilePath, FS.Files["foo.h"]);
   addFile(testPath("foo.cpp"), R"cpp(
-      #include "foo.h"
-      )cpp");
+      #include "foo.h")cpp");
   EXPECT_THAT(getSymbols(FilePath), ElementsAre(WithName("test")));
 }
 
 TEST_F(DocumentSymbolsTest, Template) {
   std::string FilePath = testPath("foo.cpp");
   addFile(FilePath, R"(
     template <class T> struct Tmpl {T x = 0;};
     template <> struct Tmpl<int> {