[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

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