Skip to content

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

Merged
dio merged 1 commit intoenvoyproxy:masterfrom
greenhouse-org:fix-filesystem-watch-tests
Aug 5, 2020
Merged

Windows: Fix //test/common/filesystem:watcher_impl_test#12496
dio merged 1 commit intoenvoyproxy:masterfrom
greenhouse-org:fix-filesystem-watch-tests

Conversation

@sunjayBhatia
Copy link
Copy Markdown
Member

@sunjayBhatia sunjayBhatia commented Aug 5, 2020

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

@wrowe @sunjayBhatia
Fix watcher_impl_test on Windows
8212fa2 
- 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 <wrowe@vmware.com>
Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com>
@sunjayBhatia
Copy link
Copy Markdown
Member Author

sunjayBhatia commented Aug 5, 2020

cc @envoyproxy/windows-dev

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

sunjayBhatia commented Aug 5, 2020

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

/wait

@sunjayBhatia
Copy link
Copy Markdown
Member Author

sunjayBhatia commented Aug 5, 2020

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
Copy Markdown
Member Author

sunjayBhatia commented Aug 5, 2020

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

@antoniovicente
Copy link
Copy Markdown
Contributor

antoniovicente commented Aug 5, 2020

/assign @dio for maintainer review

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Aug 5, 2020

🙀 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
Copy Markdown
Contributor

antoniovicente commented Aug 5, 2020

/assign @dio

dio
dio approved these changes Aug 5, 2020
View reviewed changes
Copy link
Copy Markdown
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
@sunjayBhatia @wrowe
test: Fix watcher_impl_test on Windows (envoyproxy#12496)
0ca1ebc 
- 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 <wrowe@vmware.com>
Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com>
Co-authored-by: William A Rowe Jr <wrowe@vmware.com>
chaoqin-li1123 pushed a commit to chaoqin-li1123/envoy that referenced this pull request Aug 7, 2020
@sunjayBhatia @wrowe
test: Fix watcher_impl_test on Windows (envoyproxy#12496)
9f206e4 
- 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 <wrowe@vmware.com>
Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com>
Co-authored-by: William A Rowe Jr <wrowe@vmware.com>
Signed-off-by: chaoqinli <chaoqinli@google.com>
@sunjayBhatia sunjayBhatia mentioned this pull request Aug 13, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@dio dio dio approved these changes

@antoniovicente antoniovicente antoniovicente approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@dio dio

@antoniovicente antoniovicente

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sunjayBhatia @antoniovicente @dio @wrowe