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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
