Use stream matchers to unflake the mac OS watcher tests.

Use stream matchers to unflake the mac OS watcher tests.

R=rnystrom@google.com
BUG=15024
Committed: https://code.google.com/p/dart/source/detail?r=31705

[Bob Nystrom, 2014-01-09 00:34:21]

https://codereview.chromium.org/129473003/diff/1/pkg/watcher/test/directory_w...
File pkg/watcher/test/directory_watcher/shared.dart (right):

https://codereview.chromium.org/129473003/diff/1/pkg/watcher/test/directory_w...
pkg/watcher/test/directory_watcher/shared.dart:123:
expectRemoveEvent("file.txt");
Seems like this contradicts the test description. If we only expect this to
happen with the mac watcher, maybe make this test platform-specific?

https://codereview.chromium.org/129473003/diff/1/pkg/watcher/test/directory_w...
pkg/watcher/test/directory_watcher/shared.dart:159: expectAddEvent("old.txt");
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.

https://codereview.chromium.org/129473003/diff/1/pkg/watcher/test/directory_w...
pkg/watcher/test/directory_watcher/shared.dart:203: isAddEvent("new/file.txt"),
Ditch trailing ",".

[nweiz, 2014-01-09 21:59:19]

I've marked the one Mac OS test case that was failing on your machine as
failing.

https://codereview.chromium.org/129473003/diff/1/pkg/watcher/test/directory_w...
File pkg/watcher/test/directory_watcher/shared.dart (right):

https://codereview.chromium.org/129473003/diff/1/pkg/watcher/test/directory_w...
pkg/watcher/test/directory_watcher/shared.dart:123:
expectRemoveEvent("file.txt");
On 2014/01/09 00:34:22, Bob Nystrom wrote:
> Seems like this contradicts the test description. If we only expect this to
> happen with the mac watcher, maybe make this test platform-specific?

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.

https://codereview.chromium.org/129473003/diff/1/pkg/watcher/test/directory_w...
pkg/watcher/test/directory_watcher/shared.dart:159: expectAddEvent("old.txt");
On 2014/01/09 00:34:22, Bob Nystrom wrote:
> 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.

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

https://codereview.chromium.org/129473003/diff/1/pkg/watcher/test/directory_w...
pkg/watcher/test/directory_watcher/shared.dart:203: isAddEvent("new/file.txt"),
On 2014/01/09 00:34:22, Bob Nystrom wrote:
> Ditch trailing ",".

Done.

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

https://codereview.chromium.org/129473003/diff/1/pkg/watcher/test/directory_w...
File pkg/watcher/test/directory_watcher/shared.dart (right):

https://codereview.chromium.org/129473003/diff/1/pkg/watcher/test/directory_w...
pkg/watcher/test/directory_watcher/shared.dart:123:
expectRemoveEvent("file.txt");
On 2014/01/09 21:59:20, nweiz wrote:
> On 2014/01/09 00:34:22, Bob Nystrom wrote:
> > Seems like this contradicts the test description. If we only expect this to
> > happen with the mac watcher, maybe make this test platform-specific?
> 
> In theory this could happen with any watcher; they all bundle changes
together.

Has it actually been seen to happen with the others? If not, we should keep the
test more precise on those platforms.

> 
> 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.

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?

> 
> 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.

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?

https://codereview.chromium.org/129473003/diff/70001/pkg/watcher/test/directo...
File pkg/watcher/test/directory_watcher/mac_os_test.dart (right):

https://codereview.chromium.org/129473003/diff/70001/pkg/watcher/test/directo...
pkg/watcher/test/directory_watcher/mac_os_test.dart:42: return;
I think this generates an unreachable code warning in the Editor. Also, this
will still print the test in the output I'd just comment out the whole test.

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

https://codereview.chromium.org/129473003/diff/1/pkg/watcher/test/directory_w...
File pkg/watcher/test/directory_watcher/shared.dart (right):

https://codereview.chromium.org/129473003/diff/1/pkg/watcher/test/directory_w...
pkg/watcher/test/directory_watcher/shared.dart:123:
expectRemoveEvent("file.txt");
On 2014/01/09 22:49:19, Bob Nystrom wrote:
> On 2014/01/09 21:59:20, nweiz wrote:
> > On 2014/01/09 00:34:22, Bob Nystrom wrote:
> > > Seems like this contradicts the test description. If we only expect this
to
> > > happen with the mac watcher, maybe make this test platform-specific?
> > 
> > In theory this could happen with any watcher; they all bundle changes
> together.
> 
> Has it actually been seen to happen with the others? If not, we should keep
the
> test more precise on those platforms.

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.

> > 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.
> 
> 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?

That will also flake some percentage of the time, and would require additional
infrastructure that I don't want to spend time on.

> > 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.
> 
> 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?

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.

https://codereview.chromium.org/129473003/diff/70001/pkg/watcher/test/directo...
File pkg/watcher/test/directory_watcher/mac_os_test.dart (right):

https://codereview.chromium.org/129473003/diff/70001/pkg/watcher/test/directo...
pkg/watcher/test/directory_watcher/mac_os_test.dart:42: return;
On 2014/01/09 22:49:20, Bob Nystrom wrote:
> I think this generates an unreachable code warning in the Editor. Also, this
> will still print the test in the output I'd just comment out the whole test.

Done.

[Bob Nystrom, 2014-01-10 18:00:17]

LGTM. The explanatory comment is really helpful.

[nweiz, 2014-01-10 20:36:39]

Committed patchset #4 manually as r31705 (presubmit successful).