Fix FileWatcher DELETE event reporting and rearrange tests
#86
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
leandro-lucarella-frequenz
added
type:bug
github-actions
bot
added
part:docs
leandro-lucarella-frequenz
force-pushed
the
file-watcher-tests
branch
3 times, most recently
from
02bfb48 to
b9932fe
Compare
The existing tests are really integration tests, as they are not only testing `FileWatcher`, but also `Select` and `PeriodicTimer`, which is confusing when there is a bug in any of the other classes as we might get test failures here. This commit adds a a proper unit tests mocking `awatch`. Signed-off-by: Leandro Lucarella <[email protected]>
Since the integration tests in the `FileWatcher` tests are actually testing 3 different classes (and interacting with the OS), we move it to its own file, and we call it more generally. We also mark the tests with the pytest mark `integration` to make it more clear they are integration tests, and to enable an easy way to only run (exclude) integration tests. Signed-off-by: Leandro Lucarella <[email protected]>
Separate the function to filter events so it is easier to test separately. Also remove an unnecessary `type: ignore` whole at it. Signed-off-by: Leandro Lucarella <[email protected]>
leandro-lucarella-frequenz
force-pushed
the
file-watcher-tests
branch
from
b9932fe to
4eb089e
Compare
Contributor
Author
|
Rebased and added a note on how to test the new timer with I would make a release after this is merged. |
Contributor
daniel-zullo-frequenz
left a comment
daniel-zullo-frequenz
left a comment
There was a problem hiding this comment.
I have only 2 cosmetic comments to check for, LGTM otherwise
Contributor
@leandro-lucarella-frequenz can you please wait for a simple PR that addresses #53 which is based on the changes on this PR? |
The FileWatcher was skipping DELETE events, except on very weird cases where the file was deleted and re-created just before the event was triggered by awatch(). This also extends the integration test to test both situations (delete events when a file exist and when it doesn't). Because there is usually a considerable delay between a change in the filesystem happens and awatch() emits an event it is difficult to decide if we should filter events based on the file existing or not. For example, if a create event is received but before is triggered the file was removed, should we emit the create event or not? Or the other way around, should we emit a delete event if a file was created again before the event was triggered. It is probably best to leave this decision to the user, and let them deal with races. Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
Co-authored-by: daniel-zullo-frequenz <[email protected]> Signed-off-by: Leandro Lucarella <[email protected]>
leandro-lucarella-frequenz
force-pushed
the
file-watcher-tests
branch
from
38f8469 to
ebb99e6
Compare
daniel-zullo-frequenz
approved these changes
May 9, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request have 2 different but related main changes:
The
FileWatcherwas skippingDELETEevents, except on very weird cases where the file was deleted and re-created just before the event was triggered byawatch().This also extends the integration test to test both situations (delete events when a file exist and when it doesn't).
Because there is usually a considerable delay between a change in the filesystem happens and
awatch()emits an event it is difficult to decide if we should filter events based on the file existing or not. For example, if a create event is received but before is triggered the file was removed, should we emit the create event or not? Or the other way around, should we emit a delete event if a file was created again before the event was triggered.It is probably best to leave this decision to the user, and let them deal with races.
FileWatchertests were rearranged and split between unit and integration tests.The existing tests were really integration tests, as they are not only testing
FileWatcher, but alsoSelectandPeriodicTimer, which is confusing when there is a bug in any of the other classes as we might get test failures here (this actually happened during the development of Make the timer generic and support periodic timers #79).Because of this the current tests were moved to
test/util/test_integration.py. We also mark the tests with thepytestmarkintegrationto make it more clear they are integration tests, and to enable an easy way to only run (exclude) integration tests.New unit tests were added to the current test file, properly mocking
awatchto avoid interactions with the OS.There are many more integration tests posing as unit tests, but this PR only takes care of
FileWatcher. With time we should move the rest too.