[PATCH] D77669: [clangd] Update TUStatus test to handle async PreambleThread
This revision was automatically updated to reflect the committed changes.
Closed by commit rG48d851a92e98: [clangd] Update TUStatus test to handle async
PreambleThread (authored by kadircet).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77669/new/
https://reviews.llvm.org/D77669
Files:
clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -8,6 +8,7 @@
#include "Annotations.h"
#include "Cancellation.h"
+#include "ClangdServer.h"
#include "Context.h"
#include "Diagnostics.h"
#include "Matchers.h"
@@ -829,18 +830,35 @@
class CaptureTUStatus : public ClangdServer::Callbacks {
public:
void onFileUpdated(PathRef File, const TUStatus ) override {
+ auto ASTAction = Status.ASTActivity.K;
+ auto PreambleAction = Status.PreambleActivity;
std::lock_guard Lock(Mutex);
- AllStatus.push_back(Status);
+ // Only push the action if it has changed. Since TUStatus can be published
+ // from either Preamble or AST thread and when one changes the other stays
+ // the same.
+ // Note that this can result in missing some updates when something other
+ // than action kind changes, e.g. when AST is built/reused the action kind
+ // stays as Building.
+ if (ASTActions.empty() || ASTActions.back() != ASTAction)
+ASTActions.push_back(ASTAction);
+ if (PreambleActions.empty() || PreambleActions.back() != PreambleAction)
+PreambleActions.push_back(PreambleAction);
}
-std::vector allStatus() {
+std::vector preambleStatuses() {
std::lock_guard Lock(Mutex);
- return AllStatus;
+ return PreambleActions;
+}
+
+std::vector astStatuses() {
+ std::lock_guard Lock(Mutex);
+ return ASTActions;
}
private:
std::mutex Mutex;
-std::vector AllStatus;
+std::vector ASTActions;
+std::vector PreambleActions;
} CaptureTUStatus;
MockFSProvider FS;
MockCompilationDatabase CDB;
@@ -850,35 +868,39 @@
// We schedule the following tasks in the queue:
// [Update] [GoToDefinition]
Server.addDocument(testPath("foo.cpp"), Code.code(), "1",
- WantDiagnostics::Yes);
+ WantDiagnostics::Auto);
+ ASSERT_TRUE(Server.blockUntilIdleForTest());
Server.locateSymbolAt(testPath("foo.cpp"), Code.point(),
[](Expected> Result) {
ASSERT_TRUE((bool)Result);
});
-
ASSERT_TRUE(Server.blockUntilIdleForTest());
- EXPECT_THAT(CaptureTUStatus.allStatus(),
+ EXPECT_THAT(CaptureTUStatus.preambleStatuses(),
+ ElementsAre(
+ // PreambleThread starts idle, as the update is first handled
+ // by ASTWorker.
+ PreambleAction::Idle,
+ // Then it starts building first preamble and releases that to
+ // ASTWorker.
+ PreambleAction::Building,
+ // Then goes idle and stays that way as we don't receive any
+ // more update requests.
+ PreambleAction::Idle));
+ EXPECT_THAT(CaptureTUStatus.astStatuses(),
ElementsAre(
- // Everything starts with ASTWorker starting to execute an
- // update
- TUState(PreambleAction::Idle, ASTAction::RunningAction),
- // We build the preamble
- TUState(PreambleAction::Building, ASTAction::RunningAction),
- // We built the preamble, and issued ast built on ASTWorker
- // thread. Preambleworker goes idle afterwards.
- TUState(PreambleAction::Idle, ASTAction::RunningAction),
- // Start task for building the ast, as a result of building
- // preamble, on astworker thread.
- TUState(PreambleAction::Idle, ASTAction::RunningAction),
- // AST build starts.
- TUState(PreambleAction::Idle, ASTAction::Building),
- // AST built finished successfully
- TUState(PreambleAction::Idle, ASTAction::Building),
- // Running go to def
- TUState(PreambleAction::Idle, ASTAction::RunningAction),
- // ASTWorker goes idle.
- TUState(PreambleAction::Idle, ASTAction::Idle)));
+ // Starts handling the update action and blocks until the
+ // first preamble is built.
+ ASTAction::RunningAction,
+ // Afterwqards it builds an AST for that preamble to publish
+ // diagnostics.
+
[PATCH] D77669: [clangd] Update TUStatus test to handle async PreambleThread
kadircet updated this revision to Diff 256875.
kadircet marked 6 inline comments as done.
kadircet added a comment.
- Record actions for each thread separately.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77669/new/
https://reviews.llvm.org/D77669
Files:
clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -8,6 +8,7 @@
#include "Annotations.h"
#include "Cancellation.h"
+#include "ClangdServer.h"
#include "Context.h"
#include "Diagnostics.h"
#include "Matchers.h"
@@ -829,18 +830,35 @@
class CaptureTUStatus : public ClangdServer::Callbacks {
public:
void onFileUpdated(PathRef File, const TUStatus ) override {
+ auto ASTAction = Status.ASTActivity.K;
+ auto PreambleAction = Status.PreambleActivity;
std::lock_guard Lock(Mutex);
- AllStatus.push_back(Status);
+ // Only push the action if it has changed. Since TUStatus can be published
+ // from either Preamble or AST thread and when one changes the other stays
+ // the same.
+ // Note that this can result in missing some updates when something other
+ // than action kind changes, e.g. when AST is built/reused the action kind
+ // stays as Building.
+ if (ASTActions.empty() || ASTActions.back() != ASTAction)
+ASTActions.push_back(ASTAction);
+ if (PreambleActions.empty() || PreambleActions.back() != PreambleAction)
+PreambleActions.push_back(PreambleAction);
}
-std::vector allStatus() {
+std::vector preambleStatuses() {
std::lock_guard Lock(Mutex);
- return AllStatus;
+ return PreambleActions;
+}
+
+std::vector astStatuses() {
+ std::lock_guard Lock(Mutex);
+ return ASTActions;
}
private:
std::mutex Mutex;
-std::vector AllStatus;
+std::vector ASTActions;
+std::vector PreambleActions;
} CaptureTUStatus;
MockFSProvider FS;
MockCompilationDatabase CDB;
@@ -850,35 +868,39 @@
// We schedule the following tasks in the queue:
// [Update] [GoToDefinition]
Server.addDocument(testPath("foo.cpp"), Code.code(), "1",
- WantDiagnostics::Yes);
+ WantDiagnostics::Auto);
+ ASSERT_TRUE(Server.blockUntilIdleForTest());
Server.locateSymbolAt(testPath("foo.cpp"), Code.point(),
[](Expected> Result) {
ASSERT_TRUE((bool)Result);
});
-
ASSERT_TRUE(Server.blockUntilIdleForTest());
- EXPECT_THAT(CaptureTUStatus.allStatus(),
+ EXPECT_THAT(CaptureTUStatus.preambleStatuses(),
+ ElementsAre(
+ // PreambleThread starts idle, as the update is first handled
+ // by ASTWorker.
+ PreambleAction::Idle,
+ // Then it starts building first preamble and releases that to
+ // ASTWorker.
+ PreambleAction::Building,
+ // Then goes idle and stays that way as we don't receive any
+ // more update requests.
+ PreambleAction::Idle));
+ EXPECT_THAT(CaptureTUStatus.astStatuses(),
ElementsAre(
- // Everything starts with ASTWorker starting to execute an
- // update
- TUState(PreambleAction::Idle, ASTAction::RunningAction),
- // We build the preamble
- TUState(PreambleAction::Building, ASTAction::RunningAction),
- // We built the preamble, and issued ast built on ASTWorker
- // thread. Preambleworker goes idle afterwards.
- TUState(PreambleAction::Idle, ASTAction::RunningAction),
- // Start task for building the ast, as a result of building
- // preamble, on astworker thread.
- TUState(PreambleAction::Idle, ASTAction::RunningAction),
- // AST build starts.
- TUState(PreambleAction::Idle, ASTAction::Building),
- // AST built finished successfully
- TUState(PreambleAction::Idle, ASTAction::Building),
- // Running go to def
- TUState(PreambleAction::Idle, ASTAction::RunningAction),
- // ASTWorker goes idle.
- TUState(PreambleAction::Idle, ASTAction::Idle)));
+ // Starts handling the update action and blocks until the
+ // first preamble is built.
+ ASTAction::RunningAction,
+ // Afterwqards it builds an AST for that preamble to publish
+ // diagnostics.
+ ASTAction::Building,
+ //
[PATCH] D77669: [clangd] Update TUStatus test to handle async PreambleThread
kadircet added a comment. going with separately recording each thread. that way we can also test building of diagnostics. Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:849 + auto Opts = ClangdServer::optsForTest(); + Opts.AsyncThreadsCount = 0; + ClangdServer Server(CDB, FS, Opts, ); sammccall wrote: > kadircet wrote: > > sammccall wrote: > > > This seems unfortunate because it doesn't test the production > > > configuration. > > > > > > How many possible sequences are there? Are there fewer if we > > > blockuntilidle between adddoc + locatesymbolat? > > > > > > Can we use testing::AnyOf to fudge around the nondeterminism? > > I didn't want to do that as it was rather asserting on action ordering of > > preamble and ast worker threads rather than checking for whether we emit > > TUStatuses, but I suppose this test was always trying to assert both. > > > > Introducing some more simplications by turning of diagnostics and also > > making use of the assumption that ASTWorker will block for first preamble > > build. > This makes sense, at the risk of being a massive pain do you think we should > have a separate test that (only) creates diagnostics? changed the test to record each thread separately while deduplicating, so we can also build the diags now. Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:891 + // action. + TUState(PreambleAction::Idle, ASTAction::RunningAction), + // Builds AST and serves the request, then goes idle. sammccall wrote: > Hmm... are you sure the preamble can't still be running at this point? What > stops it from pausing forever before exiting? it has to go idle before stopping, since we block after issuing the update. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77669/new/ https://reviews.llvm.org/D77669 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D77669: [clangd] Update TUStatus test to handle async PreambleThread
sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Thanks! Now having read it, I'm slightly nervous we're not catching all the interleavings. An alternate approach if this is too messy would be just to capture the two streams of enums into separate vectors, drop duplicates, and assert the sequence of each. Anyway, up to you what to do here, I'm happy if we're testing at least something in the async configuration. Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:849 + auto Opts = ClangdServer::optsForTest(); + Opts.AsyncThreadsCount = 0; + ClangdServer Server(CDB, FS, Opts, ); kadircet wrote: > sammccall wrote: > > This seems unfortunate because it doesn't test the production configuration. > > > > How many possible sequences are there? Are there fewer if we blockuntilidle > > between adddoc + locatesymbolat? > > > > Can we use testing::AnyOf to fudge around the nondeterminism? > I didn't want to do that as it was rather asserting on action ordering of > preamble and ast worker threads rather than checking for whether we emit > TUStatuses, but I suppose this test was always trying to assert both. > > Introducing some more simplications by turning of diagnostics and also making > use of the assumption that ASTWorker will block for first preamble build. This makes sense, at the risk of being a massive pain do you think we should have a separate test that (only) creates diagnostics? Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:885 + AnyOf(TUState(PreambleAction::Idle, ASTAction::RunningAction), +TUState(PreambleAction::Idle, ASTAction::RunningAction), +TUState(PreambleAction::Building, ASTAction::Idle)), haha, including the (idle,runningaction) case twice is fun :-) Maybe not necessary though Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:891 + // action. + TUState(PreambleAction::Idle, ASTAction::RunningAction), + // Builds AST and serves the request, then goes idle. Hmm... are you sure the preamble can't still be running at this point? What stops it from pausing forever before exiting? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77669/new/ https://reviews.llvm.org/D77669 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
