[clangd][Tweaks] Improve test infra for testing tweaks with an index #160391
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-clang-tools-extra Author: Nathan Ridge (HighCommander4) ChangesFull diff: https://github.com/llvm/llvm-project/pull/160391.diff 3 Files Affected:
diff --git a/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
index b5f09f9b14604..02133ec5d77a3 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
@@ -15,6 +15,8 @@ namespace clang {
namespace clangd {
namespace {
+using ::testing::UnorderedElementsAre;
+
TWEAK_TEST(DefineOutline);
TEST_F(DefineOutlineTest, TriggersOnFunctionDecl) {
@@ -747,6 +749,43 @@ TEST_F(DefineOutlineTest, FailsMacroSpecifier) {
}
}
+TWEAK_WORKSPACE_TEST(DefineOutline);
+
+// Test that DefineOutline's use of getCorrespondingHeaderOrSource()
+// to find the source file corresponding to a header file in which the
+// tweak is invoked is working as intended.
+TEST_F(DefineOutlineWorkspaceTest, FindsCorrespondingSource) {
+ llvm::Annotations HeaderBefore(R"cpp(
+class A {
+ void bar();
+ void f^oo(){}
+};
+)cpp");
+ std::string SourceBefore(R"cpp(
+#include "a.hpp"
+void A::bar(){}
+)cpp");
+ std::string HeaderAfter = R"cpp(
+class A {
+ void bar();
+ void foo();
+};
+)cpp";
+ std::string SourceAfter = R"cpp(
+#include "a.hpp"
+void A::bar(){}
+void A::foo(){}
+)cpp";
+ Workspace.addSource("a.hpp", HeaderBefore.code());
+ Workspace.addMainFile("a.cpp", SourceBefore);
+ auto Result = apply("a.hpp", {HeaderBefore.point(), HeaderBefore.point()});
+ EXPECT_THAT(Result,
+ AllOf(withStatus("success"),
+ editedFiles(UnorderedElementsAre(
+ FileWithContents(testPath("a.hpp"), HeaderAfter),
+ FileWithContents(testPath("a.cpp"), SourceAfter)))));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/clangd/unittests/tweaks/TweakTesting.cpp b/clang-tools-extra/clangd/unittests/tweaks/TweakTesting.cpp
index 81e65ede00781..c26fc21d7a01c 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/TweakTesting.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/TweakTesting.cpp
@@ -162,5 +162,37 @@ std::string TweakTest::decorate(llvm::StringRef Code,
.str();
}
+// TODO: Reuse more code between TweakTest::apply() and
+// TweakWorkspaceTest::apply().
+TweakResult
+TweakWorkspaceTest::apply(StringRef InvocationFile,
+ llvm::Annotations::Range InvocationRange) {
+ auto AST = Workspace.openFile(InvocationFile);
+ if (!AST) {
+ ADD_FAILURE() << "No file '" << InvocationFile << "' in workspace";
+ return TweakResult{"failed to setup"};
+ }
+
+ auto Index = Workspace.index();
+ auto Result = applyTweak(
+ *AST, InvocationRange, TweakID, Index.get(),
+ &AST->getSourceManager().getFileManager().getVirtualFileSystem());
+ if (!Result)
+ return TweakResult{"unavailable"};
+ if (!*Result)
+ return TweakResult{"fail: " + llvm::toString(Result->takeError())};
+ const auto &Effect = **Result;
+ if ((*Result)->ShowMessage)
+ return TweakResult{"message:\n" + *Effect.ShowMessage};
+
+ TweakResult Retval{"success"};
+ for (auto &It : Effect.ApplyEdits) {
+ auto NewText = It.second.apply();
+ if (!NewText)
+ return TweakResult{"bad edits: " + llvm::toString(NewText.takeError())};
+ Retval.EditedFiles.insert_or_assign(It.first(), *NewText);
+ }
+ return Retval;
+}
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h b/clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
index 9c6b1f9c000ae..867398513880c 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
+++ b/clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
@@ -10,6 +10,7 @@
#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_UNITTESTS_TWEAKS_TWEAKTESTING_H
#include "ParsedAST.h"
+#include "TestWorkspace.h"
#include "index/Index.h"
#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/StringRef.h"
@@ -123,6 +124,73 @@ MATCHER_P2(FileWithContents, FileName, Contents, "") {
#define EXPECT_AVAILABLE(MarkedCode) EXPECT_AVAILABLE_(MarkedCode, true)
#define EXPECT_UNAVAILABLE(MarkedCode) EXPECT_AVAILABLE_(MarkedCode, false)
+// A helper class to represent the return value of TweakWorkspaceTest::apply().
+struct TweakResult {
+ // A string representation the status of the operation.
+ // For failure cases, this is the same as the return value of
+ // TweakTest::apply() (see the comment above that for details).
+ // For success cases, this is "success".
+ std::string Status;
+ // The contents of all files changed by the tweak, including
+ // the file in which it was invoked. Keys are absolute paths.
+ llvm::StringMap<std::string> EditedFiles = {};
+};
+
+// GTest matchers to allow more easily writing assertions about the
+// expected value of a TweakResult.
+MATCHER_P(withStatus, S, "") { return arg.Status == S; }
+template <class EditedFilesMatcher>
+::testing::Matcher<TweakResult> editedFiles(EditedFilesMatcher M) {
+ return ::testing::Field(&TweakResult::EditedFiles, M);
+}
+
+// Used for formatting TweakResult objects in assertion failure messages,
+// so it's easier to understand what didn't match.
+inline llvm::raw_ostream &operator<<(llvm::raw_ostream &Stream,
+ const TweakResult &Result) {
+ Stream << "{ status: " << Result.Status << ", editedFiles: [";
+ for (const auto &F : Result.EditedFiles) {
+ Stream << F.first() << ":\n";
+ Stream << F.second;
+ }
+ return Stream << "] }";
+}
+
+// A version of TweakTest that makes it easier to create test cases that
+// involve multiple files which are indexed.
+// Usage:
+// - Call `Workspace.addMainFile(filename, contents)` to add
+// source files which are indexer entry points (e.g. would show
+// up in `compile_commands.json`).
+// - Call `Workspace.addSource(filename, contents)` to add other
+// source files (e.g. header files).
+// - Call `apply(filename, range)` to invoke the tweak on the
+// indicated file with the given range selected. Can be called
+// multiple times for the same set of added files.
+// The implementation takes care of building an index reflecting
+// all added source files, and making it available to the tweak.
+// Unlike TweakTest, this does not have a notion of a `CodeContext`
+// (i.e. the contents of all added files are interpreted as being
+// in a File context).
+class TweakWorkspaceTest : public ::testing::Test {
+ const char *TweakID;
+
+public:
+ TweakWorkspaceTest(const char *TweakID) : TweakID(TweakID) {}
+
+ TweakResult apply(StringRef InvocationFile,
+ llvm::Annotations::Range InvocationRange);
+
+protected:
+ TestWorkspace Workspace;
+};
+
+#define TWEAK_WORKSPACE_TEST(TweakID) \
+ class TweakID##WorkspaceTest : public ::clang::clangd::TweakWorkspaceTest { \
+ protected: \
+ TweakID##WorkspaceTest() : TweakWorkspaceTest(#TweakID) {} \
+ }
+
} // namespace clangd
} // namespace clang
|
|
@llvm/pr-subscribers-clangd Author: Nathan Ridge (HighCommander4) ChangesFull diff: https://github.com/llvm/llvm-project/pull/160391.diff 3 Files Affected:
diff --git a/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
index b5f09f9b14604..02133ec5d77a3 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
@@ -15,6 +15,8 @@ namespace clang {
namespace clangd {
namespace {
+using ::testing::UnorderedElementsAre;
+
TWEAK_TEST(DefineOutline);
TEST_F(DefineOutlineTest, TriggersOnFunctionDecl) {
@@ -747,6 +749,43 @@ TEST_F(DefineOutlineTest, FailsMacroSpecifier) {
}
}
+TWEAK_WORKSPACE_TEST(DefineOutline);
+
+// Test that DefineOutline's use of getCorrespondingHeaderOrSource()
+// to find the source file corresponding to a header file in which the
+// tweak is invoked is working as intended.
+TEST_F(DefineOutlineWorkspaceTest, FindsCorrespondingSource) {
+ llvm::Annotations HeaderBefore(R"cpp(
+class A {
+ void bar();
+ void f^oo(){}
+};
+)cpp");
+ std::string SourceBefore(R"cpp(
+#include "a.hpp"
+void A::bar(){}
+)cpp");
+ std::string HeaderAfter = R"cpp(
+class A {
+ void bar();
+ void foo();
+};
+)cpp";
+ std::string SourceAfter = R"cpp(
+#include "a.hpp"
+void A::bar(){}
+void A::foo(){}
+)cpp";
+ Workspace.addSource("a.hpp", HeaderBefore.code());
+ Workspace.addMainFile("a.cpp", SourceBefore);
+ auto Result = apply("a.hpp", {HeaderBefore.point(), HeaderBefore.point()});
+ EXPECT_THAT(Result,
+ AllOf(withStatus("success"),
+ editedFiles(UnorderedElementsAre(
+ FileWithContents(testPath("a.hpp"), HeaderAfter),
+ FileWithContents(testPath("a.cpp"), SourceAfter)))));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/clangd/unittests/tweaks/TweakTesting.cpp b/clang-tools-extra/clangd/unittests/tweaks/TweakTesting.cpp
index 81e65ede00781..c26fc21d7a01c 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/TweakTesting.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/TweakTesting.cpp
@@ -162,5 +162,37 @@ std::string TweakTest::decorate(llvm::StringRef Code,
.str();
}
+// TODO: Reuse more code between TweakTest::apply() and
+// TweakWorkspaceTest::apply().
+TweakResult
+TweakWorkspaceTest::apply(StringRef InvocationFile,
+ llvm::Annotations::Range InvocationRange) {
+ auto AST = Workspace.openFile(InvocationFile);
+ if (!AST) {
+ ADD_FAILURE() << "No file '" << InvocationFile << "' in workspace";
+ return TweakResult{"failed to setup"};
+ }
+
+ auto Index = Workspace.index();
+ auto Result = applyTweak(
+ *AST, InvocationRange, TweakID, Index.get(),
+ &AST->getSourceManager().getFileManager().getVirtualFileSystem());
+ if (!Result)
+ return TweakResult{"unavailable"};
+ if (!*Result)
+ return TweakResult{"fail: " + llvm::toString(Result->takeError())};
+ const auto &Effect = **Result;
+ if ((*Result)->ShowMessage)
+ return TweakResult{"message:\n" + *Effect.ShowMessage};
+
+ TweakResult Retval{"success"};
+ for (auto &It : Effect.ApplyEdits) {
+ auto NewText = It.second.apply();
+ if (!NewText)
+ return TweakResult{"bad edits: " + llvm::toString(NewText.takeError())};
+ Retval.EditedFiles.insert_or_assign(It.first(), *NewText);
+ }
+ return Retval;
+}
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h b/clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
index 9c6b1f9c000ae..867398513880c 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
+++ b/clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
@@ -10,6 +10,7 @@
#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_UNITTESTS_TWEAKS_TWEAKTESTING_H
#include "ParsedAST.h"
+#include "TestWorkspace.h"
#include "index/Index.h"
#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/StringRef.h"
@@ -123,6 +124,73 @@ MATCHER_P2(FileWithContents, FileName, Contents, "") {
#define EXPECT_AVAILABLE(MarkedCode) EXPECT_AVAILABLE_(MarkedCode, true)
#define EXPECT_UNAVAILABLE(MarkedCode) EXPECT_AVAILABLE_(MarkedCode, false)
+// A helper class to represent the return value of TweakWorkspaceTest::apply().
+struct TweakResult {
+ // A string representation the status of the operation.
+ // For failure cases, this is the same as the return value of
+ // TweakTest::apply() (see the comment above that for details).
+ // For success cases, this is "success".
+ std::string Status;
+ // The contents of all files changed by the tweak, including
+ // the file in which it was invoked. Keys are absolute paths.
+ llvm::StringMap<std::string> EditedFiles = {};
+};
+
+// GTest matchers to allow more easily writing assertions about the
+// expected value of a TweakResult.
+MATCHER_P(withStatus, S, "") { return arg.Status == S; }
+template <class EditedFilesMatcher>
+::testing::Matcher<TweakResult> editedFiles(EditedFilesMatcher M) {
+ return ::testing::Field(&TweakResult::EditedFiles, M);
+}
+
+// Used for formatting TweakResult objects in assertion failure messages,
+// so it's easier to understand what didn't match.
+inline llvm::raw_ostream &operator<<(llvm::raw_ostream &Stream,
+ const TweakResult &Result) {
+ Stream << "{ status: " << Result.Status << ", editedFiles: [";
+ for (const auto &F : Result.EditedFiles) {
+ Stream << F.first() << ":\n";
+ Stream << F.second;
+ }
+ return Stream << "] }";
+}
+
+// A version of TweakTest that makes it easier to create test cases that
+// involve multiple files which are indexed.
+// Usage:
+// - Call `Workspace.addMainFile(filename, contents)` to add
+// source files which are indexer entry points (e.g. would show
+// up in `compile_commands.json`).
+// - Call `Workspace.addSource(filename, contents)` to add other
+// source files (e.g. header files).
+// - Call `apply(filename, range)` to invoke the tweak on the
+// indicated file with the given range selected. Can be called
+// multiple times for the same set of added files.
+// The implementation takes care of building an index reflecting
+// all added source files, and making it available to the tweak.
+// Unlike TweakTest, this does not have a notion of a `CodeContext`
+// (i.e. the contents of all added files are interpreted as being
+// in a File context).
+class TweakWorkspaceTest : public ::testing::Test {
+ const char *TweakID;
+
+public:
+ TweakWorkspaceTest(const char *TweakID) : TweakID(TweakID) {}
+
+ TweakResult apply(StringRef InvocationFile,
+ llvm::Annotations::Range InvocationRange);
+
+protected:
+ TestWorkspace Workspace;
+};
+
+#define TWEAK_WORKSPACE_TEST(TweakID) \
+ class TweakID##WorkspaceTest : public ::clang::clangd::TweakWorkspaceTest { \
+ protected: \
+ TweakID##WorkspaceTest() : TweakWorkspaceTest(#TweakID) {} \
+ }
+
} // namespace clangd
} // namespace clang
|
|
@ckandeler Some context for this patch: I started reviewing #128164, and it occurred to me that as our tweaks start to make heavier use of the index, we could use some better test infrastructure for testing tweaks on a set of files that are indexed, without having to do the sort of manual index building that your patch is currently doing. |
ckandeler
approved these changes
Oct 8, 2025
svkeerthy
pushed a commit
that referenced
this pull request
Oct 9, 2025
clingfei
pushed a commit
to clingfei/llvm-project
that referenced
this pull request
Oct 10, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.