[libcxx-commits] [PATCH] D143816: [libc++] Fixes a flaky test.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

In D143816#4131395 <https://reviews.llvm.org/D143816#4131395>, @huixie90 wrote:

> hmm. I am not sure if I understand why we need to change the test. According to the description, it is not the test that is flaky. Instead, it is the system under test (the dylib which contains the bug) that is flaky.
> So the test does exact what it should do: detect the bug. The fact that it fails very frequently probably means that it does a good job.
>
> the added assertion might actually makes the test flaky since that totally depends on the scheduler.

There are two reasons for the change:

1. The typical use case of `notify_all_at_thread_exit` is to issue it at the end of the thread. In the original case the worker thread was sleeping while holding a mutex. Something which is not a typical use case.
2. The test might never actually test that `notify_all_at_thread_exit` is called. Testing on Mac revealed it did happen, but was extremely rare. So if this fails 25% of the time on other platforms we should look at how to make the test more realistic, longer sleep, yielding at the start of the thread execution, add more thread, etc. So this is basically a test that the test tests what it expects.

I agree these improvements are not related to the "fix", so maybe it would be better to commit them separately.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143816