[clangd] Add a TestWorkspace utility

[nridge]

Summary

TestWorkspace allows easily writing tests involving multiple
files that can have inclusion relationships between them.

BackgroundIndexTest.RelationsMultiFile is refactored to use
TestWorkspace, and moved to FileIndexTest as it no longer
depends on BackgroundIndex.

[nridge]

This patch is a spinoff from my call hierarchy work (which I haven't posted for review yet, though I posted a draft at D89296).

I'd like to be able to write tests where I can:

• specify the contents of multiple header and source files, which may have #include relationships among them
• index the files
• create an AST for one or more of these files
• run operations that require an AST + the index and make assertions about their results.

The TestWorkspace utility in this patch is my attempt to allow writing tests of this form without a lot of boilerplate. I also use it in one existing test, BackgroundIndexTest.RelationsMultiFile, though this use is not the most interesting as it does not involve ASTs. There is a more interesting use in D89298 for writing a call hierarchy test that does use ASTs.

I am seeking guidance on the following:

• Is this a useful utility to have? I've certainly found it useful for writing call hierarchy tests.
• Is it OK that it depends on BackgroundIndex? In principle, it should be possible to make the same interface work with FileIndex under the hood, but I haven't figured out how. (Specifically, I haven't figured out how to get FileIndex to resolve #includes, because you can't pass a MockFS to it the way you can to BackgroundIndex.)
• If we keep the dependency on BackgroundIndex, is it OK to move the related dependencies (like MemoryShardStorage) to a header, so that TestWorkspace itself can live in a header and tests from different files (not just BackgroundIndexTest.cpp) can use it?

[nridge]

In D89297#2326838, @nridge wrote:

In principle, it should be possible to make the same interface work with FileIndex under the hood, but I haven't figured out how. (Specifically, I haven't figured out how to get FileIndex to resolve #includes, because you can't pass a MockFS to it the way you can to BackgroundIndex.)

I should mention for completeness that the way we did tests like this in Eclipse CDT is... we actually wrote the test files to disk :) But I'm guessing, given the efforts that have been made to use MockFS in the test suite so far, that we don't want to do this for clangd.

[kadircet]

This looks like a useful infra to have indeed, we currently don't have an easy way to setup a testcase that would require interactions between multiple files (e.g. a workspace), as you've noticed while working on callhierarchy tests (sorry for that).

A couple of suggestions from my side:

• Rather than enforcing files to come in header/source pairs, why not have a isTU flag. That way we can use the infra in a more flexible way.
• Instead of having a MockFS, maybe have a single TestTU with all the workspace files put into AdditionalFiles. Later on when building an AST, you can just replace the TestTU::Code and build the source as if it is in the workspace.
• Background or FileIndex is not the point handling include resolution (or AST build process). In theory during the construction time, after populating all the AdditionalFiles in a TestTU, you can have a single FileIndex and populated it for each TU using the AST coming from TestTU::build() and preamble from TestTU::preamble(). Currently TestTU::preamble() doesn't take a callback, but you can change that to populate the FileIndex using preamble information.
• In addition to having a constructor that takes all the files(or just some base files) at once, it might be better to have an interface like:

struct WorkspaceTest {
  void addSource(FilePath, Code);  // These are just sourcefiles, ASTs can be built for them, but they won't be MainFiles during indexing.
  void addMainFile(FilePath, Code); // These are entrypoints, will be marked as TU, and would be used for index builds.
  std::unique_ptr<SymbolIndex> index(); // Builds the index for whole forkspace, by indexing all the MainFiles and merging them under a single FileIndex.
  ParsedAST ast(FilePath);
};

[nridge]

Rework utility as suggested

[nridge]

Thanks for the suggestions! I implemented them and it seems to be working well.

[kadircet]

thanks this mostly looks good.

can you move implementations from TestWorkspace.h to TestWorkspace.cpp ?

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

431 ↗

(On Diff #298926)

nit: if you decide to keep the constructor can you prepend /*IsMainFile=*/ to last parameters?

clang-tools-extra/clangd/unittests/TestWorkspace.h

12 ↗

(On Diff #298926)

s/index-/index/
s/AST-based/AST based/

32 ↗

(On Diff #298926)

i think it would be better to move this struct to private, and only have addSoruce/addMainFile helpers with comments explaining only TUs rooted at mainfiles will be indexed.

if you prefer to keep this struct then we should probably have comments about it too. especially the IsMainFile bit.

46 ↗

(On Diff #298926)

nit: Filename.str() (same for Code and usages in addMainFile below.

56 ↗

(On Diff #298926)

nit: prefer early exit, if(!MainFile) continue;

75 ↗

(On Diff #298926)

should this be a failure instead? e.g. ADD_FAILURE() << "Accessing non-existing file: " << Filename;

77 ↗

(On Diff #298926)

nit: Input->Filename

82 ↗

(On Diff #298926)

nit: maybe move filename out of the struct, and keep a map here instead?

[nridge]

Address review comments

[kadircet]

thanks, lgtm!

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

17 ↗

(On Diff #299526)

nit: redundant braces

clang-tools-extra/clangd/unittests/TestWorkspace.h

32 ↗

(On Diff #299526)

nit: I would move this near the stringmap.

38 ↗

(On Diff #299526)

drop this.

llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn

99 ↗

(On Diff #299526)

these are usually picked up by gn-build-bots and auto-updated. but it shouldn't hurt to do that manually as well (i hope)

[nridge]

address final review comments

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

public:
   mutable llvm::StringSet<> AccessedPaths;
 };
 
 class BackgroundIndexTest : public ::testing::Test {
 protected:
   BackgroundIndexTest() { BackgroundQueue::preventThreadStarvationInTests(); }
 };
 
+class TestWorkspace {
+public:
+  struct HeaderSourcePair {
+    std::string BaseName;
+    std::string HeaderCode;
+    std::string SourceCode;
+
+    std::string headerFilename() const { return BaseName + ".h"; }
+    std::string sourceFilename() const { return BaseName + ".cc"; }
+  };
+
+  TestWorkspace(std::vector<HeaderSourcePair> &&Inputs)
+      : Inputs(std::move(Inputs)), MSS(Storage, CacheHits),
+        CDB(/*Base=*/nullptr) {
+    for (auto &Input : this->Inputs) {
+      FS.Files[testPath("root/" + Input.headerFilename())] = Input.HeaderCode;
+      FS.Files[testPath("root/" + Input.sourceFilename())] = Input.SourceCode;
+    }
+    Index = std::make_unique<BackgroundIndex>(
+        FS, CDB,
+        BackgroundIndexStorage::Factory([&](llvm::StringRef) { return &MSS; }),
+        BackgroundIndex::Options{});
+    for (auto &Input : this->Inputs) {
+      tooling::CompileCommand Cmd;
+      Cmd.Filename = testPath("root/" + Input.sourceFilename());
+      Cmd.Directory = testPath("root");
+      Cmd.CommandLine = {"clang++", Cmd.Filename};
+      CDB.setCompileCommand(Cmd.Filename, Cmd);
+      EXPECT_TRUE(Index->blockUntilIdleForTest());
+    }
+  }
+
+  SymbolIndex *index() const { return Index.get(); }
+
+  Optional<ParsedAST> openFile(llvm::StringRef Filename) {
+    HeaderSourcePair *Input = findFile(Filename);
+    if (!Input)
+      return llvm::None;
+    bool IsHeader = (Filename == Input->headerFilename());
+    TestTU TU;
+    TU.Code = IsHeader ? Input->HeaderCode : Input->SourceCode;
+    TU.Filename = "root/" + std::string(Filename);
+    for (auto &Input : Inputs) {
+      TU.AdditionalFiles.insert(
+          std::make_pair("root/" + Input.headerFilename(), Input.HeaderCode));
+    }
+    return TU.build();
+  }
+
+private:
+  std::vector<HeaderSourcePair> Inputs;
+  MockFS FS;
+  llvm::StringMap<std::string> Storage;
+  size_t CacheHits = 0;
+  MemoryShardStorage MSS;
+  OverlayCDB CDB;
+  std::unique_ptr<BackgroundIndex> Index;
+
+  HeaderSourcePair *findFile(llvm::StringRef Filename) {
+    for (auto &Input : Inputs)
+      if (Filename == Input.headerFilename() ||
+          Filename == Input.sourceFilename())
+        return &Input;
+    return nullptr;
+  }
+};
+
 TEST_F(BackgroundIndexTest, NoCrashOnErrorFile) {
   MockFS FS;
   FS.Files[testPath("root/A.cc")] = "error file";
   llvm::StringMap<std::string> Storage;
   size_t CacheHits = 0;
   MemoryShardStorage MSS(Storage, CacheHits);
   OverlayCDB CDB(/*Base=*/nullptr);
   BackgroundIndex Idx(FS, CDB, [&](llvm::StringRef) { return &MSS; },
EXPECT_THAT(getRefs(Idx, Common.ID),
                        FileURI("unittest:///root/A.cc"),
                        FileURI("unittest:///root/B.cc"),
                        FileURI("unittest:///root/B.cc"),
                        FileURI("unittest:///root/B.cc"),
                        FileURI("unittest:///root/B.cc")}));
 }
 
 TEST_F(BackgroundIndexTest, RelationsMultiFile) {
-  MockFS FS;
-  FS.Files[testPath("root/Base.h")] = "class Base {};";
-  FS.Files[testPath("root/A.cc")] = R"cpp(
+  TestWorkspace Workspace({{"Base", "class Base {};", ""},
+                           {"A", "", R"cpp(
     #include "Base.h"
     class A : public Base {};
-  )cpp";
-  FS.Files[testPath("root/B.cc")] = R"cpp(
+  )cpp"},
+                           {"B", "", R"cpp(
     #include "Base.h"
     class B : public Base {};
-  )cpp";
-
-  llvm::StringMap<std::string> Storage;
-  size_t CacheHits = 0;
-  MemoryShardStorage MSS(Storage, CacheHits);
-  OverlayCDB CDB(/*Base=*/nullptr);
-  BackgroundIndex Index(FS, CDB, [&](llvm::StringRef) { return &MSS; },
-                        /*Opts=*/{});
-
-  tooling::CompileCommand Cmd;
-  Cmd.Filename = testPath("root/A.cc");
-  Cmd.Directory = testPath("root");
-  Cmd.CommandLine = {"clang++", Cmd.Filename};
-  CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
-  ASSERT_TRUE(Index.blockUntilIdleForTest());
-
-  Cmd.Filename = testPath("root/B.cc");
-  Cmd.CommandLine = {"clang++", Cmd.Filename};
-  CDB.setCompileCommand(testPath("root/B.cc"), Cmd);
-  ASSERT_TRUE(Index.blockUntilIdleForTest());
+  )cpp"}});
 
-  auto HeaderShard = MSS.loadShard(testPath("root/Base.h"));
-  EXPECT_NE(HeaderShard, nullptr);
-  SymbolID Base = findSymbol(*HeaderShard->Symbols, "Base").ID;
+  SymbolIndex *Index = Workspace.index();
+  FuzzyFindRequest FFReq;
+  FFReq.Query = "Base";
+  FFReq.AnyScope = true;
+  SymbolID Base;
+  Index->fuzzyFind(FFReq, [&](const Symbol &S) { Base = S.ID; });
 
   RelationsRequest Req;
   Req.Subjects.insert(Base);
   Req.Predicate = RelationKind::BaseOf;
   uint32_t Results = 0;
-  Index.relations(Req, [&](const SymbolID &, const Symbol &) { ++Results; });
+  Index->relations(Req, [&](const SymbolID &, const Symbol &) { ++Results; });
   EXPECT_EQ(Results, 2u);
 }
 
 TEST_F(BackgroundIndexTest, MainFileRefs) {
   MockFS FS;
   FS.Files[testPath("root/A.h")] = R"cpp(
       void header_sym();
       )cpp";