This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Update TUStatus test to handle async PreambleThread
ClosedPublic

Authored by kadircet on Apr 7 2020, 11:55 AM.

Details

Summary

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.

Diff Detail

Event Timeline

kadircet created this revision.Apr 7 2020, 11:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 7 2020, 11:55 AM
sammccall added inline comments.Apr 7 2020, 6:01 PM
clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
866

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?

894

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

897

why do we never go idle at the end?

kadircet updated this revision to Diff 255914.Apr 8 2020, 12:43 AM
kadircet marked 4 inline comments as done.
  • Assert on the execution order instead with simplifications.
kadircet retitled this revision from [clangd] Run TUStatus test in sync mode to make output deterministic to [clangd] Update TUStatus to handle async PreambleThread.Apr 8 2020, 12:43 AM
kadircet added inline comments.
clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
866

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.

897

because astworker doesn't emit queue related notifications(running action, queued, idle) in RunSync mode.

kadircet retitled this revision from [clangd] Update TUStatus to handle async PreambleThread to [clangd] Update TUStatus test to handle async PreambleThread.Apr 9 2020, 12:05 PM
sammccall accepted this revision.Apr 9 2020, 2:15 PM

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.

clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
866

This makes sense, at the risk of being a massive pain do you think we should have a separate test that (only) creates diagnostics?

912

haha, including the (idle,runningaction) case twice is fun :-) Maybe not necessary though

918

Hmm... are you sure the preamble can't still be running at this point? What stops it from pausing forever before exiting?

This revision is now accepted and ready to land.Apr 9 2020, 2:15 PM
kadircet updated this revision to Diff 256875.Apr 12 2020, 12:42 PM
kadircet marked 6 inline comments as done.
  • Record actions for each thread separately.

going with separately recording each thread. that way we can also test building of diagnostics.

clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
866

changed the test to record each thread separately while deduplicating, so we can also build the diags now.

918

it has to go idle before stopping, since we block after issuing the update.

This revision was automatically updated to reflect the committed changes.