[PATCH] D125103: [clangd] Speed up a slow sleeping testcase.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D125103#3497140 <https://reviews.llvm.org/D125103#3497140>, @ilya-biryukov wrote:

> Many thanks for improving the running time, this slow test has bugged me since the day I first ran it.
> I'd even be happy to remove it completely. It's slow, possibly flaky, so IMO the corresponding check does not carry its weight.

I agree in general, unfortunately this test is really valuable:

- it tests something critical
- it tests something tricky
- it's the only test of that functionality
- it's prevented real bugs in the past
- I can't see how to replace it with reasonable effort
- it doesn't seem to be flaky at all in practice => hard to justify an unreasonable effort

So I want to keep it, and I promise to feel bad about it.
(I think this is the only ridiculous sleeping test we have in clangd...)



================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:272
+                  [&](std::vector<Diag>) {
+                    ADD_FAILURE()
+                        << "auto should have been discarded (dead write)";
----------------
ilya-biryukov wrote:
> This is potentially a race in the test that we did not have before, right?
> There is probably much less work in the main test thread, but we probably can still see the failure if we are unlucky with how the OS schedules us.
> 
> My stance is it's better to have no tests for certain behaviours than flaky tests.
> But if you find it useful let's try to run it and see whether my suspicions are unfounded, don't want to block the change on it.
Yes-ish - it's the same thing as the race on the first updateWithDiags. In both cases we're hoping that the next event happens within 450/500ms so the debounce doesn't expire.

AFAIK this hasn't been flaky at all in practice (albeit at 1000ms), so until we have a better test (see above) I'd like to keep it :-(


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125103/new/

https://reviews.llvm.org/D125103