Windows: Fix filesystem subscription impl tests

[sunjayBhatia]

Commit Message:
Windows: Fix filesystem subscription impl tests

Replace dispatcher and watcher with mocks and orchestrate the test
harness to capture filesystem subscription impl file event callback and
invoke it directly. Test no longer relies on watching real filesystem
events and avoids timing discrepancies causing flakiness.

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

[sunjayBhatia]

cc @envoyproxy/windows-dev

[jmarantz]

Agreed that MemFileSystem would be larger than this change, but it would also have a lot more beneficial impact, for Windows in general but also across the testsuite, making tests faster and less flaky.

I suspect that the existing test actually covers something that this now mocks out, and I'm not sure if changing this would drop the coverage of it. But maybe someone who's more involved in the filesystem watcher can comment.

[jmarantz]

@envoyproxy/maintainers seeking help on reviewing this from someone who knows more about the filesystem watcher.

[wrowe]

I suspect that the existing test actually covers something that this now mocks out

Yes, this test is written expecting synchronous callback execution. In the real world, the NTFS kernel layer is not behaving synchronously. Solving this test design flaw requires a lot more code to anticipate watch notifications arriving asynch, and that isn't what this test is designed to exercise. We are simply exercising that a notification will cause a refresh of the config.

This seems better solved with the simplified mock than trying to anticipate the real-world execution of this test.

[jmarantz]

Seems like it should be possible to leave the test structure as is but allow for async vs sync notificaton; not even sure what that means, to be honest.

Shouldn't the intervening library synchronize the async notification so that callers can be expect the same behavior whether running on Windows or Linux or Mac?

If we have to be changing the calling code to one of our libraries to add ifdefs for subtle behavioral differences, we are going to have a bad time.

[sunjayBhatia]

Shouldn't the intervening library synchronize the async notification so that callers can be expect the same behavior whether running on Windows or Linux or Mac?

We are removing all async code from this test using this method of dependency injection. The class under test doesn't inherently need to react to asynchronous events. The watcher implementation is tested elsewhere: https://github.com/envoyproxy/envoy/blob/master/test/common/filesystem/watcher_impl_test.cc

We could continue to test the FilesystemSubscriptionImpl class with the dispatcher/libevent event loop and filesystem watcher with added synchronization that did not exist prior, however it seems simpler to remove any need for blocking or cross-thread synchronization etc. What is the approach the core maintainers want us to follow? @envoyproxy/senior-maintainers do you have any thoughts or direction for us?

If we have to be changing the calling code to one of our libraries to add ifdefs for subtle behavioral differences, we are going to have a bad time.

We shouldn't be adding any ifdefs for this class of issues, we are in fact trying to factor tests such that they are more correct across all platforms. Envoy relies on asynchronous event loops and callbacks and a number of tests make assumptions about timing and sequencing of events which are not definitive. We are trying to refactor tests so they eliminate assumptions or assertions are flexible enough to react correctly to these indefinite conditions.

Separate issue, which might be confusing things:

The different filesystem watcher implementations are all watching file events asynchronously by definition, when watching the local filesystem, our tests have relied on the fact that Linux+ext4 behaves synchronously/almost instantly. If we were to run the tests on Linux backed by NFS or other filesystems, e.g in GCP or other CI hosts, we may see similar filesystem event timing issues as we see on Windows. This design defect is commonly observed in implementations of locking strategies when tested only using one process on the local filesystem. Our recent fixes to the filesystem watcher tests patiently wait for asynchronous notices to fire (see: #12496).

[sunjayBhatia]

Seems like an unrelated coverage test failure, merged master to hopefully re-kick with success

[htuch]

Yeah, I agree that a memory/fake filesystem will be useful for a bunch of tests. In this specific case, for FilesystemSubscriptionImpl, I'm fine with the change as is. @sunjayBhatia do you anticipate having to do this for a bunch of other tests? This is probably the key weighing factor.

[wrowe]

@htuch at this point, following Matt's latest fix, some 27 integration tests and some 14 other tests are still to be resolved. As we discover more patterns, we can fix, and also go back over previously fixed tests which exhibited the same pattern. A memory filesystem would be a great addition, but I don't think it holds up merging this specific fix.

[jmarantz]

@envoyproxy/senior-maintainers

[sunjayBhatia]

Yeah, I agree that a memory/fake filesystem will be useful for a bunch of tests. In this specific case, for FilesystemSubscriptionImpl, I'm fine with the change as is. @sunjayBhatia do you anticipate having to do this for a bunch of other tests? This is probably the key weighing factor.

It seems like the dispatcher etc. is appropriately mocked in other tests etc. so I would guess we would not have to do this same exercise again (also b/c I think this is the lowest level abstraction on top of the filesystem watcher which shouldn't have too much more involvement).

[htuch]

LGTM, thanks!