[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

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

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

12

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

32

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

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

56

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

75

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

77

nit: Input->Filename

82

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

33

nit: I would move this near the stringmap.

39

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

EXPECT_THAT(getRefs(Idx, Common.ID),
               RefsAre({FileURI("unittest:///root/A.h"),
                        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(
-    #include "Base.h"
-    class A : public Base {};
-  )cpp";
-  FS.Files[testPath("root/B.cc")] = 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());
-
-  auto HeaderShard = MSS.loadShard(testPath("root/Base.h"));
-  EXPECT_NE(HeaderShard, nullptr);
-  SymbolID Base = findSymbol(*HeaderShard->Symbols, "Base").ID;
-
-  RelationsRequest Req;
-  Req.Subjects.insert(Base);
-  Req.Predicate = RelationKind::BaseOf;
-  uint32_t Results = 0;
-  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";
   FS.Files[testPath("root/A.cc")] =
       "#include \"A.h\"\nstatic void main_sym() { (void)header_sym; }";
 

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

 //===-- FileIndexTests.cpp  ---------------------------*- C++ -*-----------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "AST.h"
 #include "Annotations.h"
 #include "Compiler.h"
 #include "Headers.h"
 #include "ParsedAST.h"
 #include "SyncAPI.h"
 #include "TestFS.h"
 #include "TestTU.h"
+#include "TestWorkspace.h"
 #include "URI.h"
 #include "index/CanonicalIncludes.h"
 #include "index/FileIndex.h"
 #include "index/Index.h"
 #include "index/Ref.h"
 #include "index/Relation.h"
 #include "index/Serialization.h"
 #include "index/Symbol.h"
TEST(FileIndexTest, Relations) {
   uint32_t Results = 0;
   RelationsRequest Req;
   Req.Subjects.insert(A);
   Req.Predicate = RelationKind::BaseOf;
   Index.relations(Req, [&](const SymbolID &, const Symbol &) { ++Results; });
   EXPECT_EQ(Results, 1u);
 }
 
+TEST(FileIndexTest, RelationsMultiFile) {
+  TestWorkspace Workspace({{"Base.h", "class Base {};", false},
+                           {"A.cpp", R"cpp(
+    #include "Base.h"
+    class A : public Base {};
+  )cpp",
+                            true},
+                           {"B.cpp", R"cpp(
+    #include "Base.h"
+    class B : public Base {};
+  )cpp",
+                            true}});
+
+  auto 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; });
+  EXPECT_EQ(Results, 2u);
+}
+
 TEST(FileIndexTest, ReferencesInMainFileWithPreamble) {
   TestTU TU;
   TU.HeaderCode = "class Foo{};";
   Annotations Main(R"cpp(
     void f() {
       [[Foo]] foo;
     }
   )cpp");

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

struct TestTU {
   // eliminate the need for real file system here.
   // Please avoid using this for things other than implicit modules. The plan is
   // to eliminate this option some day.
   bool OverlayRealFileSystemForModules = false;
 
   // By default, build() will report Error diagnostics as GTest errors.
   // Suppress this behavior by adding an 'error-ok' comment to the code.
   ParsedAST build() const;
-  std::shared_ptr<const PreambleData> preamble() const;
+  std::shared_ptr<const PreambleData>
+  preamble(PreambleParsedCallback PreambleCallback = nullptr) const;
   ParseInputs inputs(MockFS &FS) const;
   SymbolSlab headerSymbols() const;
   RefSlab headerRefs() const;
   std::unique_ptr<SymbolIndex> index() const;
 };
 
 // Look up an index symbol by qualified name, which must be unique.
 const Symbol &findSymbol(const SymbolSlab &, llvm::StringRef QName);

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

 }
 
 void deleteModuleCache(const std::string ModuleCachePath) {
   if (!ModuleCachePath.empty()) {
     ASSERT_FALSE(llvm::sys::fs::remove_directories(ModuleCachePath));
   }
 }
 
-std::shared_ptr<const PreambleData> TestTU::preamble() const {
+std::shared_ptr<const PreambleData>
+TestTU::preamble(PreambleParsedCallback PreambleCallback) const {
   MockFS FS;
   auto Inputs = inputs(FS);
   IgnoreDiagnostics Diags;
   auto CI = buildCompilerInvocation(Inputs, Diags);
   assert(CI && "Failed to build compilation invocation.");
   if (OverlayRealFileSystemForModules)
     initializeModuleCache(*CI);
   auto ModuleCacheDeleter = llvm::make_scope_exit(
       std::bind(deleteModuleCache, CI->getHeaderSearchOpts().ModuleCachePath));
   return clang::clangd::buildPreamble(testPath(Filename), *CI, Inputs,
-                                      /*StoreInMemory=*/true,
-                                      /*PreambleCallback=*/nullptr);
+                                      /*StoreInMemory=*/true, PreambleCallback);
 }
 
 ParsedAST TestTU::build() const {
   MockFS FS;
   auto Inputs = inputs(FS);
   StoreDiags Diags;
   auto CI = buildCompilerInvocation(Inputs, Diags);
   assert(CI && "Failed to build compilation invocation.");

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

+//===--- TestWorkspace.h - Utility for writing multi-file tests --*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// TestWorkspace builds on TestTU to provide a way to write tests involving
+// several related files with inclusion relationships between them.
+//
+// The tests can exercise both index- and AST-based operations.
+//
+//===---------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTWORKSPACE_H
+#define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTWORKSPACE_H
+
+#include "TestFS.h"
+#include "TestTU.h"
+#include "index/FileIndex.h"
+#include "index/Index.h"
+#include "llvm/ADT/StringRef.h"
+#include <string>
+#include <vector>
+
+namespace clang {
+namespace clangd {
+
+class TestWorkspace {
+public:
+  struct SourceFile {
+    std::string Filename;
+    std::string Code;
+    bool IsMainFile = false;
+  };
+
+  TestWorkspace() {}
+  TestWorkspace(const std::vector<SourceFile> &Inputs) {
+    for (auto &Input : Inputs) {
+      addInput(Input);
+    }
+  }
+
+  void addSource(llvm::StringRef Filename, llvm::StringRef Code) {
+    addInput({std::string(Filename), std::string(Code), false});
+  }
+
+  void addMainFile(llvm::StringRef Filename, llvm::StringRef Code) {
+    addInput({std::string(Filename), std::string(Code), true});
+  }
+
+  std::unique_ptr<SymbolIndex> index() {
+    auto Index = std::make_unique<FileIndex>();
+    for (const auto &Input : Inputs) {
+      if (Input.IsMainFile) {
+        TU.Code = Input.Code;
+        TU.Filename = Input.Filename;
+        TU.preamble([&](ASTContext &Ctx,
+                        std::shared_ptr<clang::Preprocessor> PP,
+                        const CanonicalIncludes &CanonIncludes) {
+          Index->updatePreamble(testPath(Input.Filename), "null", Ctx, PP,
+                                CanonIncludes);
+        });
+        ParsedAST MainAST = TU.build();
+        Index->updateMain(testPath(Input.Filename), MainAST);
+      }
+    }
+    return Index;
+  }
+
+  Optional<ParsedAST> openFile(llvm::StringRef Filename) {
+    SourceFile *Input = findFile(Filename);
+    if (!Input)
+      return llvm::None;
+    TU.Code = Input->Code;
+    TU.Filename = std::string(Filename);
+    return TU.build();
+  }
+
+private:
+  std::vector<SourceFile> Inputs;
+  TestTU TU;
+
+  void addInput(const SourceFile &Input) {
+    Inputs.push_back(Input);
+    TU.AdditionalFiles.insert(std::make_pair(Input.Filename, Input.Code));
+  }
+
+  SourceFile *findFile(llvm::StringRef Filename) {
+    for (auto &Input : Inputs)
+      if (Filename == Input.Filename)
+        return &Input;
+    return nullptr;
+  }
+};
+
+} // namespace clangd
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTWORKSPACE_H