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

Windows: Fix //test/common/filesystem:watcher_impl_test #12496

Merged
merged 1 commit into from
Aug 5, 2020
Merged

Windows: Fix //test/common/filesystem:watcher_impl_test #12496

merged 1 commit into from
Aug 5, 2020

Conversation

sunjayBhatia
Copy link
Member

Commit Message:
Windows: Fix //test/common/filesystem:watcher_impl_test

  • Tests that used a non-blocking libevent event loop are flaky on
    Windows (and would be flaky on other platforms if event notifications
    routinely took longer to be propagated) since the event loop could exit
    before an event notification. Switching to use a blocking event loop
    in places where we expect a callback to be fired prevents early exit before
    filesystem events are evaluated.
  • Skip SymlinkAtomicRename test as Windows does not have an atomic file
    move API that can move a directory/symlink where the new name is a non-empty
    existing directory/symlink (MoveFileEx can atomically replace a file
    with a file however).

Additional Description:
Risk Level: Low
Testing: Modifies unit tests, checked on Windows locally
Docs Changes: N/A
Release Notes: N/A

- Tests that used a non-blocking libevent event loop are flaky on
  Windows (and would be flaky on other platforms if event notifications
  routinely took longer to be propagated) since the event loop could exit
  before an event notification. Switching to use a blocking event loop
  prevents early exit before filesystem events are evaluated.
- Skip SymlinkAtomicRename test as Windows does not have an atomic file
  move API that can move a directory/symlink where the new name is a non-empty
  existing directory/symlink (MoveFileEx can atomically replace a file
  with a file however).

Signed-off-by: William A Rowe Jr <[email protected]>
Co-authored-by: Sunjay Bhatia <[email protected]>
@sunjayBhatia
Copy link
Member Author

cc @envoyproxy/windows-dev

@dio dio assigned wrowe and antoniovicente and unassigned wrowe Aug 5, 2020
@sunjayBhatia
Copy link
Member Author

There might be something we can do about the atomic move test

/wait

@sunjayBhatia
Copy link
Member Author

There might be something we can do about the atomic move test

Trying a delete and then move we're not seeing any events get generated, the PR description still stands

@sunjayBhatia
Copy link
Member Author

If someone has power to remove the waiting tag, this no longer needs it

@antoniovicente
Copy link
Contributor

/assign @dio for maintainer review

@repokitteh-read-only
Copy link

🙀 Error while processing event:

evaluation error
error: function _github_call error: GET https://api.github.com/repos/envoyproxy/envoy/assignees/for: 404 Not Found []:
 Traceback (most recent call last):
  bootstrap:143: in <toplevel>
  bootstrap:135: in _main
  bootstrap:62: in _call
  bootstrap:15: in _call1
  github.com/repokitteh/modules/assign.star:18: in _assign
  github:374: in issue_check_assignee
  github:134: in call
Error: function _github_call error: GET https://api.github.com/repos/envoyproxy/envoy/assignees/for: 404 Not Found []
🐱

Caused by: a #12496 (comment) was created by @antoniovicente.

see: more, trace.

@antoniovicente
Copy link
Contributor

/assign @dio

Copy link
Member

@dio dio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@dio dio removed the waiting label Aug 5, 2020
@dio dio merged commit 0cb1e86 into envoyproxy:master Aug 5, 2020
@sunjayBhatia sunjayBhatia deleted the fix-filesystem-watch-tests branch August 6, 2020 14:59
chaoqin-li1123 pushed a commit to chaoqin-li1123/envoy that referenced this pull request Aug 7, 2020
- Tests that used a non-blocking libevent event loop are flaky on
   Windows (and would be flaky on other platforms if event notifications
   routinely took longer to be propagated) since the event loop could exit
   before an event notification. Switching to use a blocking event loop
   prevents early exit before filesystem events are evaluated.
- Skip SymlinkAtomicRename test as Windows does not have an atomic file
   move API that can move a directory/symlink where the new name is a non-empty
   existing directory/symlink (MoveFileEx can atomically replace a file
   with a file, however).

Signed-off-by: William A Rowe Jr <[email protected]>
Co-authored-by: Sunjay Bhatia <[email protected]>
Co-authored-by: William A Rowe Jr <[email protected]>
chaoqin-li1123 pushed a commit to chaoqin-li1123/envoy that referenced this pull request Aug 7, 2020
- Tests that used a non-blocking libevent event loop are flaky on
   Windows (and would be flaky on other platforms if event notifications
   routinely took longer to be propagated) since the event loop could exit
   before an event notification. Switching to use a blocking event loop
   prevents early exit before filesystem events are evaluated.
- Skip SymlinkAtomicRename test as Windows does not have an atomic file
   move API that can move a directory/symlink where the new name is a non-empty
   existing directory/symlink (MoveFileEx can atomically replace a file
   with a file, however).

Signed-off-by: William A Rowe Jr <[email protected]>
Co-authored-by: Sunjay Bhatia <[email protected]>
Co-authored-by: William A Rowe Jr <[email protected]>
Signed-off-by: chaoqinli <[email protected]>
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.

4 participants