[PATCH] D77669: [clangd] Update TUStatus test to handle async PreambleThread

Sun Apr 12 12:48:49 PDT 2020

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, &CaptureTUStatus);
----------------
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