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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet abandoned this revision.
kadircet added a comment.

abandoning in favor of D82944 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82729



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82729



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82729



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82729



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
kadircet added a reviewer: sammccall.
Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay, 
ilya-biryukov.
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D82729

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

Index: clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
===
--- clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
+++ clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
@@ -100,15 +100,16 @@
 }
 
 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)),
@@ -117,10 +118,11 @@
 }
 
 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");
@@ -141,14 +143,14 @@
 }
 
 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");
@@ -178,24 +180,20 @@
 }
 
 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 {
@@ -204,8 +202,8 @@
   namespace ns {
   int foo2() {
   }
-  }
-  )cpp");
+  })cpp";
+  addFile("foo.h", FS.Files["foo.h"]);
   addFile("foo.cpp", R"cpp(
   #include "foo.h"
   )cpp");
@@ -219,7 +217,7 @@
 }
 
 TEST_F(WorkspaceSymbolsTest, Enums) {
-  addFile("foo.h", R"cpp(
+  FS.Files["foo.h"] = R"cpp(
 enum {
   Red
 };
@@ -239,8 +237,8 @@
   enum class Color4 {
 White
   };
-}
-  )cpp");
+})cpp";
+  addFile("foo.h", FS.Files["foo.h"]);
   addFile("foo.cpp", R"cpp(
   #include "foo.h"
   )cpp");
@@ -259,21 +257,21 @@
 }
 
 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");
@@ -447,13 +445,13 @@
 }
 
 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());
 }
@@ -488,19 +486,17 @@
 }
 
 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