Use stream matchers to unflake the mac OS watcher tests.

pkg/watcher/test/directory_watcher/shared.dart

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // 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';
 
 sharedTests() {
   test('does not notify for files that already exist when started', () {
     // Make some pre-existing files.
     writeFile("a.txt");
     writeFile("b.txt");
 
     startWatcher();
@@ skipping 35 matching lines @@
     expectModifyEvent("file.txt");
   });
 
   test('notifies even if the file contents are unchanged', () {
     writeFile("a.txt", contents: "same");
     writeFile("b.txt", contents: "before");
     startWatcher();
 
     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', () {
     writeFile("dir/a.txt");
     writeFile("dir/b.txt");
 
     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", () {
     test('notifies when a file is moved within the watched directory', () {
       writeFile("old.txt");
       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',
         () {
       writeFile("old.txt");
       createDir("dir");
       startWatcher(dir: "dir");
 
       renameFile("old.txt", "dir/new.txt");
       expectAddEvent("dir/new.txt");
     });
 
     test('notifies when a file is moved outside the watched directory', () {
       writeFile("dir/old.txt");
       startWatcher(dir: "dir");
 
       renameFile("dir/old.txt", "new.txt");
       expectRemoveEvent("dir/old.txt");
     });
   });
 
   group("clustered changes", () {
     test("doesn't notify when a file is created and then immediately removed",
         () {
       startWatcher();
       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 "
         "recreated", () {
       writeFile("file.txt");
       startWatcher();
 
       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 "
         "recreated", () {
       writeFile("old.txt");
       startWatcher();
 
       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.

       });
     });
 
     test("reports a removal when a file is modified and then immediately "
         "removed", () {
       writeFile("file.txt");
       startWatcher();
 
       writeFile("file.txt", contents: "modified");
       deleteFile("file.txt");
+
+      allowModifyEvent("file.txt");
       expectRemoveEvent("file.txt");
     });
 
     test("reports an add when a file is added and then immediately modified",
         () {
       startWatcher();
 
       writeFile("file.txt");
       writeFile("file.txt", contents: "modified");
+
       expectAddEvent("file.txt");
+      startClosingEventStream();
+      allowModifyEvent("file.txt");
     });
   });
 
   group("subdirectories", () {
     test('watches files in subdirectories', () {
       startWatcher();
       writeFile("a/b/c/d/file.txt");
       expectAddEvent("a/b/c/d/file.txt");
     });
 
     test('notifies when a subdirectory is moved within the watched directory '
         'and then its contents are modified', () {
       writeFile("old/file.txt");
       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");
     });
 
     test('emits events for many nested files added at once', () {
       withPermutations((i, j, k) =>
           writeFile("sub/sub-$i/sub-$j/file-$k.txt"));
 
       createDir("dir");
       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', () {
       withPermutations((i, j, k) =>
           writeFile("dir/sub/sub-$i/sub-$j/file-$k.txt"));
 
       createDir("dir");
       startWatcher(dir: "dir");
 
       // Rename the directory rather than deleting it because native watchers
       // report a rename as a single DELETE event for the directory, whereas
       // they report recursive deletion with DELETE events for every file in the
       // 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', () {
       withPermutations((i, j, k) =>
           writeFile("dir/old/sub-$i/sub-$j/file-$k.txt"));
 
       createDir("dir");
       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 "
         "same name as a removed file", () {
       writeFile("dir/sub");
       withPermutations((i, j, k) =>
           writeFile("old/sub-$i/sub-$j/file-$k.txt"));
       startWatcher(dir: "dir");
 
       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);
     });
   });
 }