-
Notifications
You must be signed in to change notification settings - Fork 4.9k
FileSystemWatcher fixes and test changes #8231
Conversation
I resolved the issues with OSX providing bogus Created events and ran the tests a few hundred times to (hopefully) catch any intermittent failures. The only failure I got was a timeout in the old unit tests that I mostly didn't touch, so I'm going to call this ready to merge (pending feedback). |
flags = FSEventStreamEventFlags.kFSEventStreamEventFlagItemInodeMetaMod | | ||
FSEventStreamEventFlags.kFSEventStreamEventFlagItemFinderInfoMod | | ||
FSEventStreamEventFlags.kFSEventStreamEventFlagItemModified | | ||
FSEventStreamEventFlags.kFSEventStreamEventFlagItemChangeOwner; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: the alignment here and below looks a bit off
Thanks for the great feedback, Steve! I pushed an update with responses to all of your comments. |
|
||
// Verify Changed | ||
bool Changed_expected = ((expectedEvents & WatcherChangeTypes.Changed) > 0); | ||
bool Changed_actual = changed.WaitOne(WaitForExpectedEventTimeout); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're waiting on the changed event even if a changed event may not be asked for? Won't that introduce unnecessary delays into the tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes to both questions. We wait for all events to ensure they are thrown (or not thrown) as expected. To make up for this, event waits after the first Changed are done using a shorter timeout value (10 ms vs 50 ms for Changed).
All of the timeout values are much lower than they were before this PR (30000/500 ms for expected/unexpected before) so there's still a net decrease in execution time. Adding the retry functionality lets us use a lower timeout without worrying as much about failures from CI machine slowdown, so the average execution is going to be faster.
I wouldn't say they're unnecessary delays though; we should be verifying that a test isn't throwing an unexpected event type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, sounds reasonable, thanks.
@stephentoub any final feedback before I merge this? |
Innerloop OSX Debug Build and Test :
|
- Filtering on the OSX FileSystemWatcher was letting through events that should have been filtered out due to the passed NotifyFilters. - The OSX FSW likes to set the Created flag for pretty much every event it throws. We explicitly disabled coalescing, so we can safely ignore that flag when it's combined with an actual flag that indicates a change.
Reworks the FSW tests to be 100% passing. - Adds path validation to most tests. - Adds a Retry mechanism to handle expected intermittent failures due to untrustworthy underlying API calls. - Adds more test cases to NotifyFilter and splits existing tests into separate dir/file tests to cover either behavior. - Enable test parallelization
go for it |
Thanks, Steve. |
FileSystemWatcher fixes and test changes Commit migrated from dotnet/corefx@acd7df3
Fixes to FileSystemWatcher tests to increase their reliability.
resolves #1165
resolves #1657
resolves #2011
resolves #2684
resolves #3215
resolves #3630
resolves #4916
resolves #5306
resolves #6226
resolves #6241
resolves #6732
resolves #7002
resolves #8023
resolves #8024
resolves #8154