[PATCH] D77669: [clangd] Run TUStatus test in sync mode to make output deterministic

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
kadircet added a reviewer: sammccall.
Herald added subscribers: cfe-commits, usaxena95, arphaman, mgrang, jkorous, 
MaskRay, javed.absar, ilya-biryukov.
Herald added a project: clang.

Currently it doesn't matter because we run PreambleThread in sync mode.
Once we start running it in async, test order will become non-deterministic.


Repository:
  rG LLVM Github Monorepo

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"
@@ -844,7 +845,9 @@
   } CaptureTUStatus;
   MockFSProvider FS;
   MockCompilationDatabase CDB;
-  ClangdServer Server(CDB, FS, ClangdServer::optsForTest(), &CaptureTUStatus);
+  auto Opts = ClangdServer::optsForTest();
+  Opts.AsyncThreadsCount = 0;
+  ClangdServer Server(CDB, FS, Opts, &CaptureTUStatus);
   Annotations Code("int m^ain () {}");
 
   // We schedule the following tasks in the queue:
@@ -858,27 +861,12 @@
 
   ASSERT_TRUE(Server.blockUntilIdleForTest());
 
-  EXPECT_THAT(CaptureTUStatus.allStatus(),
-  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)));
+  EXPECT_THAT(
+  CaptureTUStatus.allStatus(),
+  ElementsAre(TUState(PreambleAction::Building, ASTAction::Idle),
+  TUState(PreambleAction::Building, ASTAction::Building),
+  TUState(PreambleAction::Building, ASTAction::Building),
+  TUState(PreambleAction::Idle, ASTAction::Building)));
 }
 
 TEST_F(TUSchedulerTests, CommandLineErrors) {


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"
@@ -844,7 +845,9 @@
   } CaptureTUStatus;
   MockFSProvider FS;
   MockCompilationDatabase CDB;
-  ClangdServer Server(CDB, FS, ClangdServer::optsForTest(), &CaptureTUStatus);
+  auto Opts = ClangdServer::optsForTest();
+  Opts.AsyncThreadsCount = 0;
+  ClangdServer Server(CDB, FS, Opts, &CaptureTUStatus);
   Annotations Code("int m^ain () {}");
 
   // We schedule the following tasks in the queue:
@@ -858,27 +861,12 @@
 
   ASSERT_TRUE(Server.blockUntilIdleForTest());
 
-  EXPECT_THAT(CaptureTUStatus.allStatus(),
-  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, ASTA

[PATCH] D77669: [clangd] Run TUStatus test in sync mode to make output deterministic

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:849
+  auto Opts = ClangdServer::optsForTest();
+  Opts.AsyncThreadsCount = 0;
+  ClangdServer Server(CDB, FS, Opts, &CaptureTUStatus);

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?



Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:866
+  CaptureTUStatus.allStatus(),
+  ElementsAre(TUState(PreambleAction::Building, ASTAction::Idle),
+  TUState(PreambleAction::Building, ASTAction::Building),

FWIW, I found the comments useful, and don't understand e.g. why we have two 
identical lines anymore.



Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:869
+  TUState(PreambleAction::Building, ASTAction::Building),
+  TUState(PreambleAction::Idle, ASTAction::Building)));
 }

why do we never go idle at the end?


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