Use stream matchers to unflake the mac OS watcher tests.

pkg/watcher/test/directory_watcher/shared.dart

Index: pkg/watcher/test/directory_watcher/shared.dart
diff --git a/pkg/watcher/test/directory_watcher/shared.dart b/pkg/watcher/test/directory_watcher/shared.dart
index 2e0186f5206b52c7b9c9173a0bce0b75e0ed194d..e1dd909762374946167c6e7aa2185ad48c69196f 100644
--- a/pkg/watcher/test/directory_watcher/shared.dart
+++ b/pkg/watcher/test/directory_watcher/shared.dart
@@ -3,6 +3,7 @@
 // BSD-style license that can be found in the LICENSE file.
 
 import 'package:scheduled_test/scheduled_test.dart';
+import 'package:watcher/src/utils.dart';
 
 import '../utils.dart';
 
@@ -58,10 +59,10 @@ sharedTests() {
 
     writeFile("a.txt", contents: "same");
     writeFile("b.txt", contents: "after");
-    inAnyOrder(() {
-      expectModifyEvent("a.txt");
-      expectModifyEvent("b.txt");
-    });
+    inAnyOrder([
+      isModifyEvent("a.txt"),
+      isModifyEvent("b.txt")
+    ]);
   });
 
   test('when the watched directory is deleted, removes all files', () {
@@ -71,10 +72,10 @@ sharedTests() {
     startWatcher(dir: "dir");
 
     deleteDir("dir");
-    inAnyOrder(() {
-      expectRemoveEvent("dir/a.txt");
-      expectRemoveEvent("dir/b.txt");
-    });
+    inAnyOrder([
+      isRemoveEvent("dir/a.txt"),
+      isRemoveEvent("dir/b.txt"),
+    ]);
   });
 
   group("moves", () {
@@ -83,10 +84,10 @@ sharedTests() {
       startWatcher();
       renameFile("old.txt", "new.txt");
 
-      inAnyOrder(() {
-        expectAddEvent("new.txt");
-        expectRemoveEvent("old.txt");
-      });
+      inAnyOrder([
+        isAddEvent("new.txt"),
+        isRemoveEvent("old.txt"),
+      ]);
     });
 
     test('notifies when a file is moved from outside the watched directory',
@@ -115,7 +116,12 @@ sharedTests() {
       writeFile("file.txt");
       deleteFile("file.txt");
 
-      // [startWatcher] will assert that no events were fired.
+      startClosingEventStream();
+
+      allowEvents(() {
+        expectAddEvent("file.txt");
+        expectRemoveEvent("file.txt");

[Bob Nystrom, 2014/01/09 00:34:22]

Seems like this contradicts the test description. If we only expect this to
happen with the mac watcher, maybe make this test platform-specific?

[nweiz, 2014/01/09 21:59:20]

In theory this could happen with any watcher; they all bundle changes together.

All of these additions contradict the test description. The whole issue here is
that we want to test behavior that nondeterministically happens 99% of the time,
but 1% of the time can reasonably do something else.

Another way to think about this is that we're testing that at least it doesn't
emit events that are completely off the wall... which is unfortunately not
outside the realm of possibility given the behavior of Directory.watch.

[Bob Nystrom, 2014/01/09 22:49:19]

Has it actually been seen to happen with the others? If not, we should keep the
test more precise on those platforms.
Maybe we should make the tests stochastic then? For each test case, run it a
bunch of times and verify that it did what we expect a certain % of the time?
Yeah, but that doesn't leave us in a good position. The watcher package is
critical for pub serve. I don't want to end up in the position of having a large
number of tests which actually validate very little, leaving us with a false
sense of security.

How about we set up a VC with Anders to talk about this?

[nweiz, 2014/01/10 01:05:31]

No, but it could in theory. I don't want to disallow it on other platforms when
it would be semantically valid output.

Also, it seems like it's possible for it to occur on linux or (especially) the
polling watcher, just much less likely than on mac. I don't want a 0.01% flake
either if I can avoid it.
That will also flake some percentage of the time, and would require additional
infrastructure that I don't want to spend time on.
These tests all validate that one of two sets of output occur. That rules out a
wide variety of possible outputs, including all of the erroneous ones that we've
seen in practice. If these tests start hitting the second case frequently,
that's also fine (albeit very unlikely) -- these tests exist to validate that
the batched events we get from dart:io are coalesced properly, and if the events
aren't getting batched in the tests, they won't be batched in real life.

These tests in practice validate what we're trying to validate, there's no
reason to believe they'll stop doing so, and even if they do it won't cause any
problems. This is the testing strategy we talked about and agreed on before the
break. In addition, I've already sunk a huge amount of time into getting these
tests running again, and I don't want to spend any more on a refactor with no
practical benefit.

+      });
     });
 
     test("reports a modification when a file is deleted and then immediately "
@@ -125,7 +131,13 @@ sharedTests() {
 
       deleteFile("file.txt");
       writeFile("file.txt", contents: "re-created");
-      expectModifyEvent("file.txt");
+
+      allowEither(() {
+        expectModifyEvent("file.txt");
+      }, () {
+        expectRemoveEvent("file.txt");
+        expectAddEvent("file.txt");
+      });
     });
 
     test("reports a modification when a file is moved and then immediately "
@@ -135,9 +147,16 @@ sharedTests() {
 
       renameFile("old.txt", "new.txt");
       writeFile("old.txt", contents: "re-created");
-      inAnyOrder(() {
-        expectModifyEvent("old.txt");
+
+      allowEither(() {
+        inAnyOrder([
+          isModifyEvent("old.txt"),
+          isAddEvent("new.txt"),
+        ]);
+      }, () {
+        expectRemoveEvent("old.txt");
         expectAddEvent("new.txt");
+        expectAddEvent("old.txt");

[Bob Nystrom, 2014/01/09 00:34:22]

Some docs explaining why/where this behavior can show up would be good. Here,
and pretty much everywhere in this file. :)

I'm fine with loosening the defined behavior of the package, but we should at
least document where it goes weird and why.

[nweiz, 2014/01/09 21:59:20]

Added a comment at the top of the "clustered changes" group.

       });
     });
 
@@ -148,6 +167,8 @@ sharedTests() {
 
       writeFile("file.txt", contents: "modified");
       deleteFile("file.txt");
+
+      allowModifyEvent("file.txt");
       expectRemoveEvent("file.txt");
     });
 
@@ -157,7 +178,10 @@ sharedTests() {
 
       writeFile("file.txt");
       writeFile("file.txt", contents: "modified");
+
       expectAddEvent("file.txt");
+      startClosingEventStream();
+      allowModifyEvent("file.txt");
     });
   });
 
@@ -174,10 +198,10 @@ sharedTests() {
       startWatcher();
 
       renameDir("old", "new");
-      inAnyOrder(() {
-        expectRemoveEvent("old/file.txt");
-        expectAddEvent("new/file.txt");
-      });
+      inAnyOrder([
+        isRemoveEvent("old/file.txt"),
+        isAddEvent("new/file.txt"),

[Bob Nystrom, 2014/01/09 00:34:22]

Ditch trailing ",".

[nweiz, 2014/01/09 21:59:20]

Done.

+      ]);
 
       writeFile("new/file.txt", contents: "modified");
       expectModifyEvent("new/file.txt");
@@ -191,10 +215,8 @@ sharedTests() {
       startWatcher(dir: "dir");
       renameDir("sub", "dir/sub");
 
-      inAnyOrder(() {
-        withPermutations((i, j, k)  =>
-            expectAddEvent("dir/sub/sub-$i/sub-$j/file-$k.txt"));
-      });
+      inAnyOrder(withPermutations((i, j, k)  =>
+          isAddEvent("dir/sub/sub-$i/sub-$j/file-$k.txt")));
     });
 
     test('emits events for many nested files removed at once', () {
@@ -210,10 +232,8 @@ sharedTests() {
       // directory.
       renameDir("dir/sub", "sub");
 
-      inAnyOrder(() {
-        withPermutations((i, j, k) =>
-            expectRemoveEvent("dir/sub/sub-$i/sub-$j/file-$k.txt"));
-      });
+      inAnyOrder(withPermutations((i, j, k) =>
+          isRemoveEvent("dir/sub/sub-$i/sub-$j/file-$k.txt")));
     });
 
     test('emits events for many nested files moved at once', () {
@@ -224,12 +244,12 @@ sharedTests() {
       startWatcher(dir: "dir");
       renameDir("dir/old", "dir/new");
 
-      inAnyOrder(() {
-        withPermutations((i, j, k) {
-          expectRemoveEvent("dir/old/sub-$i/sub-$j/file-$k.txt");
-          expectAddEvent("dir/new/sub-$i/sub-$j/file-$k.txt");
-        });
-      });
+      inAnyOrder(unionAll(withPermutations((i, j, k) {
+        return new Set.from([
+          isRemoveEvent("dir/old/sub-$i/sub-$j/file-$k.txt"),
+          isAddEvent("dir/new/sub-$i/sub-$j/file-$k.txt"),
+        ]);
+      })));
     });
 
     test("emits events for many files added at once in a subdirectory with the "
@@ -241,11 +261,11 @@ sharedTests() {
 
       deleteFile("dir/sub");
       renameDir("old", "dir/sub");
-      inAnyOrder(() {
-        expectRemoveEvent("dir/sub");
-        withPermutations((i, j, k)  =>
-            expectAddEvent("dir/sub/sub-$i/sub-$j/file-$k.txt"));
-      });
+
+      var events = withPermutations((i, j, k)  =>
+          isAddEvent("dir/sub/sub-$i/sub-$j/file-$k.txt"));
+      events.add(isRemoveEvent("dir/sub"));
+      inAnyOrder(events);
     });
   });
 }