[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher
Jan Korous via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu May 23 18:49:28 PDT 2019
jkorous marked 6 inline comments as done.
jkorous added inline comments.
================
Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:131
+ // the async event handling picks them up. Can make this test flaky.
+ std::this_thread::sleep_for(std::chrono::milliseconds(100)); // 0.1s
+}
----------------
jkorous wrote:
> gribozavr wrote:
> > I'm certain this sleep will be flaky on heavily-loaded CI machines. If you are going to leave it as a sleep, please make it 1s. But is there really no better way?
> That was exactly my thinking! Honestly, I don't know - I wasn't able to come up with any reasonably simple, deterministic approach even on a single platform :(
> I eventually picked 0.1s as a tradeoff between slowing the test for everyone and getting less false positives.
>
> The problem as I understand it is that we're making changes and monitoring them asynchronously with no guarantee from the kernel API (true for FSEvents, not 100% about inotify) about when (if) we receive notifications.
>
> If you have any idea for robust testing approach I'd be totally happy to use it.
I found a way how to use more generous timeout without slowing down the test for every run - inspired by Argyrios' idea about using different thread + semaphore.
I am using 3 seconds for now. If that's not enough, just let me know.
================
Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:357
+
+ EXPECT_TRUE(eventConsumer.AreExpectedPresentInNonInitial(
+ {{DirectoryWatcher::Event::EventKind::WatcherGotInvalidated, ""}}));
----------------
gribozavr wrote:
> I would strongly prefer if you used the gmock matchers (like Contains); as written, when the test fails, the only error we would get would be like "expected: true, actual: false".
Since the tests are using are based on something like "eventual correctness" instead of one-time check I didn't use gmock matchers but implemented some custom diagnostics.
Example of the failed test:
```
/Users/jankorous/src/llvm-monorepo/llvm-project/clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:185: Failure
Value of: *TestConsumer.Result()
Actual: false
Expected: true
Expected but not seen non-initial events:
Removed a
Unexpected non-initial events seen:
Added a
```
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58418/new/
https://reviews.llvm.org/D58418
More information about the cfe-commits
mailing list