[PATCH] D89297: [clangd] Add a TestWorkspace utility

[Nathan Ridge via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGaaa8b44d1991: [clangd] Add a TestWorkspace utility (authored 
by nridge).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89297

Files:
  clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp
  clang-tools-extra/clangd/unittests/TestTU.h
  clang-tools-extra/clangd/unittests/TestWorkspace.cpp
  clang-tools-extra/clangd/unittests/TestWorkspace.h
  llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
@@ -96,6 +96,7 @@
 "TestFS.cpp",
 "TestIndex.cpp",
 "TestTU.cpp",
+"TestWorkspace.cpp",
 "TweakTesting.cpp",
 "TweakTests.cpp",
 "TypeHierarchyTests.cpp",
Index: clang-tools-extra/clangd/unittests/TestWorkspace.h
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/TestWorkspace.h
@@ -0,0 +1,59 @@
+//===--- 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 
+#include 
+
+namespace clang {
+namespace clangd {
+
+class TestWorkspace {
+public:
+  // The difference between addSource() and addMainFile() is that only main
+  // files will be indexed.
+  void addSource(llvm::StringRef Filename, llvm::StringRef Code) {
+addInput(Filename.str(), {Code.str(), /*IsMainFile=*/false});
+  }
+  void addMainFile(llvm::StringRef Filename, llvm::StringRef Code) {
+addInput(Filename.str(), {Code.str(), /*IsMainFile=*/true});
+  }
+
+  std::unique_ptr index();
+
+  Optional openFile(llvm::StringRef Filename);
+
+private:
+  struct SourceFile {
+std::string Code;
+bool IsMainFile = false;
+  };
+  llvm::StringMap Inputs;
+  TestTU TU;
+
+  void addInput(llvm::StringRef Filename, const SourceFile );
+};
+
+} // namespace clangd
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTWORKSPACE_H
Index: clang-tools-extra/clangd/unittests/TestWorkspace.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/TestWorkspace.cpp
@@ -0,0 +1,49 @@
+//===--- TestWorkspace.cpp - 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
+//
+//===--===//
+
+#include "TestWorkspace.h"
+
+namespace clang {
+namespace clangd {
+
+std::unique_ptr TestWorkspace::index() {
+  auto Index = std::make_unique();
+  for (const auto  : Inputs) {
+if (!Input.second.IsMainFile)
+  continue;
+TU.Code = Input.second.Code;
+TU.Filename = Input.first().str();
+TU.preamble([&](ASTContext , std::shared_ptr PP,
+const CanonicalIncludes ) {
+  Index->updatePreamble(testPath(Input.first()), "null", Ctx, PP,
+CanonIncludes);
+});
+ParsedAST MainAST = TU.build();
+Index->updateMain(testPath(Input.first()), MainAST);
+  }
+  return Index;
+}
+
+Optional TestWorkspace::openFile(llvm::StringRef Filename) {
+  auto It = Inputs.find(Filename);
+  if (It == Inputs.end()) {
+ADD_FAILURE() << "Accessing non-existing file: " << Filename;
+return llvm::None;
+  }
+  TU.Code = It->second.Code;
+  TU.Filename = It->first().str();
+  return TU.build();
+}
+
+void TestWorkspace::addInput(llvm::StringRef Filename,
+ const SourceFile ) {
+  

[PATCH] D89297: [clangd] Add a TestWorkspace utility

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 300513.
nridge marked 4 inline comments as done.
nridge added a comment.

address final review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89297

Files:
  clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp
  clang-tools-extra/clangd/unittests/TestTU.h
  clang-tools-extra/clangd/unittests/TestWorkspace.cpp
  clang-tools-extra/clangd/unittests/TestWorkspace.h
  llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
@@ -96,6 +96,7 @@
 "TestFS.cpp",
 "TestIndex.cpp",
 "TestTU.cpp",
+"TestWorkspace.cpp",
 "TweakTesting.cpp",
 "TweakTests.cpp",
 "TypeHierarchyTests.cpp",
Index: clang-tools-extra/clangd/unittests/TestWorkspace.h
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/TestWorkspace.h
@@ -0,0 +1,59 @@
+//===--- 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 
+#include 
+
+namespace clang {
+namespace clangd {
+
+class TestWorkspace {
+public:
+  // The difference between addSource() and addMainFile() is that only main
+  // files will be indexed.
+  void addSource(llvm::StringRef Filename, llvm::StringRef Code) {
+addInput(Filename.str(), {Code.str(), /*IsMainFile=*/false});
+  }
+  void addMainFile(llvm::StringRef Filename, llvm::StringRef Code) {
+addInput(Filename.str(), {Code.str(), /*IsMainFile=*/true});
+  }
+
+  std::unique_ptr index();
+
+  Optional openFile(llvm::StringRef Filename);
+
+private:
+  struct SourceFile {
+std::string Code;
+bool IsMainFile = false;
+  };
+  llvm::StringMap Inputs;
+  TestTU TU;
+
+  void addInput(llvm::StringRef Filename, const SourceFile );
+};
+
+} // namespace clangd
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTWORKSPACE_H
Index: clang-tools-extra/clangd/unittests/TestWorkspace.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/TestWorkspace.cpp
@@ -0,0 +1,49 @@
+//===--- TestWorkspace.cpp - 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
+//
+//===--===//
+
+#include "TestWorkspace.h"
+
+namespace clang {
+namespace clangd {
+
+std::unique_ptr TestWorkspace::index() {
+  auto Index = std::make_unique();
+  for (const auto  : Inputs) {
+if (!Input.second.IsMainFile)
+  continue;
+TU.Code = Input.second.Code;
+TU.Filename = Input.first().str();
+TU.preamble([&](ASTContext , std::shared_ptr PP,
+const CanonicalIncludes ) {
+  Index->updatePreamble(testPath(Input.first()), "null", Ctx, PP,
+CanonIncludes);
+});
+ParsedAST MainAST = TU.build();
+Index->updateMain(testPath(Input.first()), MainAST);
+  }
+  return Index;
+}
+
+Optional TestWorkspace::openFile(llvm::StringRef Filename) {
+  auto It = Inputs.find(Filename);
+  if (It == Inputs.end()) {
+ADD_FAILURE() << "Accessing non-existing file: " << Filename;
+return llvm::None;
+  }
+  TU.Code = It->second.Code;
+  TU.Filename = It->first().str();
+  return TU.build();
+}
+
+void TestWorkspace::addInput(llvm::StringRef Filename,
+ const SourceFile ) {
+  Inputs.insert(std::make_pair(Filename, Input));
+  

[PATCH] D89297: [clangd] Add a TestWorkspace utility

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

thanks, lgtm!




Comment at: clang-tools-extra/clangd/unittests/TestWorkspace.cpp:17
+  for (const auto  : Inputs) {
+if (!Input.second.IsMainFile) {
+  continue;

nit: redundant braces



Comment at: clang-tools-extra/clangd/unittests/TestWorkspace.h:32
+private:
+  struct SourceFile {
+std::string Code;

nit: I would move this near the stringmap.



Comment at: clang-tools-extra/clangd/unittests/TestWorkspace.h:38
+public:
+  TestWorkspace() {}
+

drop this.



Comment at: 
llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn:99
 "TestTU.cpp",
+"TestWorkspace.cpp",
 "TweakTesting.cpp",

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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89297

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

[PATCH] D89297: [clangd] Add a TestWorkspace utility

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 299526.
nridge marked 8 inline comments as done.
nridge added a comment.
Herald added subscribers: llvm-commits, mgorny.
Herald added a project: LLVM.

Address review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89297

Files:
  clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp
  clang-tools-extra/clangd/unittests/TestTU.h
  clang-tools-extra/clangd/unittests/TestWorkspace.cpp
  clang-tools-extra/clangd/unittests/TestWorkspace.h
  llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
@@ -96,6 +96,7 @@
 "TestFS.cpp",
 "TestIndex.cpp",
 "TestTU.cpp",
+"TestWorkspace.cpp",
 "TweakTesting.cpp",
 "TweakTests.cpp",
 "TypeHierarchyTests.cpp",
Index: clang-tools-extra/clangd/unittests/TestWorkspace.h
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/TestWorkspace.h
@@ -0,0 +1,63 @@
+//===--- 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 
+#include 
+
+namespace clang {
+namespace clangd {
+
+class TestWorkspace {
+private:
+  struct SourceFile {
+std::string Code;
+bool IsMainFile = false;
+  };
+
+public:
+  TestWorkspace() {}
+
+  // The difference between addSource() and addMainFile() is that only main
+  // files will be indexed.
+  void addSource(llvm::StringRef Filename, llvm::StringRef Code) {
+addInput(Filename.str(), {Code.str(), /*IsMainFile=*/false});
+  }
+  void addMainFile(llvm::StringRef Filename, llvm::StringRef Code) {
+addInput(Filename.str(), {Code.str(), /*IsMainFile=*/true});
+  }
+
+  std::unique_ptr index();
+
+  Optional openFile(llvm::StringRef Filename);
+
+private:
+  llvm::StringMap Inputs;
+  TestTU TU;
+
+  void addInput(llvm::StringRef Filename, const SourceFile );
+};
+
+} // namespace clangd
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTWORKSPACE_H
Index: clang-tools-extra/clangd/unittests/TestWorkspace.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/TestWorkspace.cpp
@@ -0,0 +1,50 @@
+//===--- TestWorkspace.cpp - 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
+//
+//===--===//
+
+#include "TestWorkspace.h"
+
+namespace clang {
+namespace clangd {
+
+std::unique_ptr TestWorkspace::index() {
+  auto Index = std::make_unique();
+  for (const auto  : Inputs) {
+if (!Input.second.IsMainFile) {
+  continue;
+}
+TU.Code = Input.second.Code;
+TU.Filename = Input.first().str();
+TU.preamble([&](ASTContext , std::shared_ptr PP,
+const CanonicalIncludes ) {
+  Index->updatePreamble(testPath(Input.first()), "null", Ctx, PP,
+CanonIncludes);
+});
+ParsedAST MainAST = TU.build();
+Index->updateMain(testPath(Input.first()), MainAST);
+  }
+  return Index;
+}
+
+Optional TestWorkspace::openFile(llvm::StringRef Filename) {
+  auto It = Inputs.find(Filename);
+  if (It == Inputs.end()) {
+ADD_FAILURE() << "Accessing non-existing file: " << Filename;
+return llvm::None;
+  }
+  TU.Code = It->second.Code;
+  TU.Filename = It->first().str();
+  return TU.build();
+}
+
+void TestWorkspace::addInput(llvm::StringRef Filename,
+ 

[PATCH] D89297: [clangd] Add a TestWorkspace utility

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

thanks this mostly looks good.

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




Comment at: clang-tools-extra/clangd/unittests/FileIndexTests.cpp:431
+TEST(FileIndexTest, RelationsMultiFile) {
+  TestWorkspace Workspace({{"Base.h", "class Base {};", false},
+   {"A.cpp", R"cpp(

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



Comment at: clang-tools-extra/clangd/unittests/TestWorkspace.h:12
+//
+// The tests can exercise both index- and AST-based operations.
+//

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



Comment at: clang-tools-extra/clangd/unittests/TestWorkspace.h:32
+public:
+  struct SourceFile {
+std::string Filename;

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.



Comment at: clang-tools-extra/clangd/unittests/TestWorkspace.h:46
+  void addSource(llvm::StringRef Filename, llvm::StringRef Code) {
+addInput({std::string(Filename), std::string(Code), false});
+  }

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



Comment at: clang-tools-extra/clangd/unittests/TestWorkspace.h:56
+for (const auto  : Inputs) {
+  if (Input.IsMainFile) {
+TU.Code = Input.Code;

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



Comment at: clang-tools-extra/clangd/unittests/TestWorkspace.h:75
+if (!Input)
+  return llvm::None;
+TU.Code = Input->Code;

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



Comment at: clang-tools-extra/clangd/unittests/TestWorkspace.h:77
+TU.Code = Input->Code;
+TU.Filename = std::string(Filename);
+return TU.build();

nit: `Input->Filename`



Comment at: clang-tools-extra/clangd/unittests/TestWorkspace.h:82
+private:
+  std::vector Inputs;
+  TestTU TU;

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89297

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

[PATCH] D89297: [clangd] Add a TestWorkspace utility

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a reviewer: kadircet.
nridge added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89297

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

[PATCH] D89297: [clangd] Add a TestWorkspace utility

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 298926.
nridge added a comment.

Rework utility as suggested


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89297

Files:
  clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp
  clang-tools-extra/clangd/unittests/TestTU.h
  clang-tools-extra/clangd/unittests/TestWorkspace.h

Index: clang-tools-extra/clangd/unittests/TestWorkspace.h
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/TestWorkspace.h
@@ -0,0 +1,101 @@
+//===--- 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 
+#include 
+
+namespace clang {
+namespace clangd {
+
+class TestWorkspace {
+public:
+  struct SourceFile {
+std::string Filename;
+std::string Code;
+bool IsMainFile = false;
+  };
+
+  TestWorkspace() {}
+  TestWorkspace(const std::vector ) {
+for (auto  : 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 index() {
+auto Index = std::make_unique();
+for (const auto  : Inputs) {
+  if (Input.IsMainFile) {
+TU.Code = Input.Code;
+TU.Filename = Input.Filename;
+TU.preamble([&](ASTContext ,
+std::shared_ptr PP,
+const CanonicalIncludes ) {
+  Index->updatePreamble(testPath(Input.Filename), "null", Ctx, PP,
+CanonIncludes);
+});
+ParsedAST MainAST = TU.build();
+Index->updateMain(testPath(Input.Filename), MainAST);
+  }
+}
+return Index;
+  }
+
+  Optional 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 Inputs;
+  TestTU TU;
+
+  void addInput(const SourceFile ) {
+Inputs.push_back(Input);
+TU.AdditionalFiles.insert(std::make_pair(Input.Filename, Input.Code));
+  }
+
+  SourceFile *findFile(llvm::StringRef Filename) {
+for (auto  : Inputs)
+  if (Filename == Input.Filename)
+return 
+return nullptr;
+  }
+};
+
+} // namespace clangd
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTWORKSPACE_H
Index: clang-tools-extra/clangd/unittests/TestTU.h
===
--- clang-tools-extra/clangd/unittests/TestTU.h
+++ clang-tools-extra/clangd/unittests/TestTU.h
@@ -79,7 +79,8 @@
   // 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 preamble() const;
+  std::shared_ptr
+  preamble(PreambleParsedCallback PreambleCallback = nullptr) const;
   ParseInputs inputs(MockFS ) const;
   SymbolSlab headerSymbols() const;
   RefSlab headerRefs() const;
Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -80,7 +80,8 @@
   }
 }
 
-std::shared_ptr TestTU::preamble() const {
+std::shared_ptr
+TestTU::preamble(PreambleParsedCallback PreambleCallback) const {
   MockFS FS;
   auto Inputs = inputs(FS);
   IgnoreDiagnostics Diags;
@@ -91,8 +92,7 @@
   auto ModuleCacheDeleter = llvm::make_scope_exit(
   std::bind(deleteModuleCache, CI->getHeaderSearchOpts().ModuleCachePath));
   return clang::clangd::buildPreamble(testPath(Filename), *CI, Inputs,
-  

[PATCH] D89297: [clangd] Add a TestWorkspace utility

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

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 index(); // Builds the index for whole 
forkspace, by indexing all the MainFiles and merging them under a single 
FileIndex.
ParsedAST ast(FilePath);
  };


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89297

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

[PATCH] D89297: [clangd] Add a TestWorkspace utility

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

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 `#include`s, 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89297

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

[PATCH] D89297: [clangd] Add a TestWorkspace utility

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

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 `#include`s, 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89297

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

[PATCH] D89297: [clangd] Add a TestWorkspace utility

[Nathan Ridge via Phabricator via cfe-commits]

nridge created this revision.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman.
Herald added a project: clang.
nridge requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.

Use it in BackgroundIndexTest.RelationsMultiFile


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89297

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

Index: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
===
--- clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
+++ clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
@@ -89,6 +89,73 @@
   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 &)
+  : Inputs(std::move(Inputs)), MSS(Storage, CacheHits),
+CDB(/*Base=*/nullptr) {
+for (auto  : this->Inputs) {
+  FS.Files[testPath("root/" + Input.headerFilename())] = Input.HeaderCode;
+  FS.Files[testPath("root/" + Input.sourceFilename())] = Input.SourceCode;
+}
+Index = std::make_unique(
+FS, CDB,
+BackgroundIndexStorage::Factory([&](llvm::StringRef) { return  }),
+BackgroundIndex::Options{});
+for (auto  : 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 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  : Inputs) {
+  TU.AdditionalFiles.insert(
+  std::make_pair("root/" + Input.headerFilename(), Input.HeaderCode));
+}
+return TU.build();
+  }
+
+private:
+  std::vector Inputs;
+  MockFS FS;
+  llvm::StringMap Storage;
+  size_t CacheHits = 0;
+  MemoryShardStorage MSS;
+  OverlayCDB CDB;
+  std::unique_ptr Index;
+
+  HeaderSourcePair *findFile(llvm::StringRef Filename) {
+for (auto  : Inputs)
+  if (Filename == Input.headerFilename() ||
+  Filename == Input.sourceFilename())
+return 
+return nullptr;
+  }
+};
+
 TEST_F(BackgroundIndexTest, NoCrashOnErrorFile) {
   MockFS FS;
   FS.Files[testPath("root/A.cc")] = "error file";
@@ -230,45 +297,28 @@
 }
 
 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 Storage;
-  size_t CacheHits = 0;
-  MemoryShardStorage MSS(Storage, CacheHits);
-  OverlayCDB CDB(/*Base=*/nullptr);
-  BackgroundIndex Index(FS, CDB, [&](llvm::StringRef) { return  },
-/*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 ) { 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);
 }
 
___
cfe-commits mailing list