Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix PollingFileWatcher.ready for files that don't exist #157

Merged
merged 7 commits into from
Dec 1, 2023

Conversation

DanTup
Copy link
Contributor

@DanTup DanTup commented Nov 22, 2023

There were a few issues here:

  • FileWatcher.ready never fired for files that don't exist because of logic inside FileWatcher existing early if the modification time was null
  • The test I recently added trying to catch this was incorrectly passing because the mock timestamp code was set so that files that had not been created would return a 0-mtime whereas in the real implementation they return null

So this change

a) updates the mock to return null for uncreated files (to match the real implementation) which breaks a bunch of tests

b) fixes those tests by updating FileWatcher._poll() to handle null mtime separately from being the first poll.

  • Thanks for your contribution! Please replace this text with a description of what this PR is changing or adding and why, list any relevant issues, and review the contribution guidelines below.

  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.

There were a few issues here:

- FileWatcher.ready never fired for files that don't exist because of logic inside FileWatcher existing early if the modification time was `null`
- The test I recently added trying to catch this was incorrectly passing because the mock timestamp code was set so that files that had not been created would return a 0-mtime whereas in the real implementation they return `null`

So this change

a) updates the mock to return `null` for uncreated files (to match the real implementation) which breaks a bunch of tests

b) fixes those tests by updating FileWatcher._poll() to handle `null` mtime separately from being the first poll.
@DanTup DanTup changed the title Fix FileWatcher.ready for files that don't exist Fix PollingFileWatcher.ready for files that don't exist Nov 22, 2023
@DanTup
Copy link
Contributor Author

DanTup commented Nov 22, 2023

There's a failure here:

❌ test/directory_watcher/polling_test.dart: subdirectories notifies when a subdirectory is moved within the watched directory and then its contents are modified (failed)
  Expected: should emit an event that is modify new/file.txt
    Actual: <Instance of 'StreamQueue<WatchEvent>'>
     Which: emitted • add /tmp/dart_test_NLELVH/new/file.txt
  
  test/directory_watcher/shared.dart 250:7  sharedTests.<fn>.<fn>

This happens because now the timestamp code is "fixed", this rename looks like a new folder because renameDir() doesn't migrate the mock timestamps. I changed that but it broke many other tests so it need a little more work.

@DanTup
Copy link
Contributor Author

DanTup commented Nov 22, 2023

Pushed some additional changes:

  • renameDir moves the timestamps over for files it knows about
  • mock timestamps are now an increasing number starting from 1
    This is because otherwise, tests that delete and re-create a file could result in the same timestamp for the pre-delete file and the new file, and depending on timing that could look like the file never changed (whereas now it'll either by a delete+add or a modify, because of the timestamp change).
    This wasn't an issue before because deleted files didn't have their timestamps cleared and the re-add would still increase them (but now we delete timestamps for deleted files)

@DanTup
Copy link
Contributor Author

DanTup commented Nov 22, 2023

@jakemac53 this ended up touching a little more than originally planned, but I think it all makes sense and should result in the tests running with behaviour that much better matches the real fs (and ofc, doesn't mask issues like dart-lang/sdk#54116) :-)

Please review carefully - I'd like to roll this into the SDK once merged :-)

This would result in Remove events for files that don't still exist on the second poll
@jakemac53
Copy link
Contributor

I do think we should wait to land/publish this until after the thanksgiving break if that is alright

@DanTup
Copy link
Contributor Author

DanTup commented Nov 23, 2023

@jakemac53 yeah, that's fine by me. It isn't urgent - I'd just like to ensure I do some final testing once it gets into the analyzer and not forget about it after it lands :)

@DanTup
Copy link
Contributor Author

DanTup commented Nov 28, 2023

@jakemac53 how about now? :) still not urgent, just still on my list to not forget about 😄

@jakemac53
Copy link
Contributor

I will circle back soon, lots of things to catch up on 🤣

@jakemac53 jakemac53 merged commit dc45f19 into dart-lang:master Dec 1, 2023
10 checks passed
@jakemac53
Copy link
Contributor

@DanTup it sounds like you are going to try merging this into the SDK prior to publishing?

@DanTup
Copy link
Contributor Author

DanTup commented Dec 2, 2023

@jakemac53 I've opened https://dart-review.googlesource.com/c/sdk/+/339401 to roll this into the SDK, and have confirmed locally the failing test now passes.

I don't need the Pub package, so I'll leave it to you to decide when to publish. Thanks! :-)

@DanTup DanTup deleted the fix-not-found-file-issue branch December 2, 2023 12:24
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Dec 2, 2023
This updates the watcher package to include a fix for the `ready` Future not completing for non-existent folders on Windows:

dart-lang/watcher#157

This fixes a bug where the analyzer would wait indefinitely during initialisation if the workspace contained a folder that did not existing on disk (such as if you have a .code-workspace containing folders and one was subsequently deleted).

Fixes #54116

Change-Id: If283403681080477e497fe7e17f2faa217a5a643
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/339401
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
mosuem pushed a commit to dart-lang/tools that referenced this pull request Dec 11, 2024
…tcher#157)

There were a few issues here:

- FileWatcher.ready never fired for files that don't exist because of logic inside FileWatcher existing early if the modification time was `null`
- The test I recently added trying to catch this was incorrectly passing because the mock timestamp code was set so that files that had not been created would return a 0-mtime whereas in the real implementation they return `null`

So this change

a) updates the mock to return `null` for uncreated files (to match the real implementation) which breaks a bunch of tests

b) fixes those tests by updating FileWatcher._poll() to handle `null` mtime separately from being the first poll.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants