pkg/unittest: Run each test in a Zone

pkg/unittest: Run each test in a Zone

added a change log
Deprecate methods with behavior subsumed by runZoned
removed rerunTests
updated async/future_timeout_test to not assume Zone.ROOT during test run

BUG= https://code.google.com/p/dart/issues/detail?id=13277
R=nweiz@google.com

Committed: https://code.google.com/p/dart/source/detail?r=32284

[Siggi Cherem (dart-lang), 2014-02-04 02:20:05]

https://codereview.chromium.org/130173010/diff/30001/pkg/unittest/lib/src/spr...
File pkg/unittest/lib/src/spread_args_helper.dart (right):

https://codereview.chromium.org/130173010/diff/30001/pkg/unittest/lib/src/spr...
pkg/unittest/lib/src/spread_args_helper.dart:124: _guardAsync(Function tryBody,
Function finallyBody, TestCase testCase) {
I thought we were going to get rid of this too (since the invoke0/1/2 are
created inside the zone, they should be guarded)

https://codereview.chromium.org/130173010/diff/30001/pkg/unittest/lib/unittes...
File pkg/unittest/lib/unittest.dart (right):

https://codereview.chromium.org/130173010/diff/30001/pkg/unittest/lib/unittes...
pkg/unittest/lib/unittest.dart:406: return callback;
not sure if this works. runZoned only intercepts 0 and 1 arg methods.

https://codereview.chromium.org/130173010/diff/30001/pkg/unittest/test/unitte...
File pkg/unittest/test/unittest_protect_async_test.dart (right):

https://codereview.chromium.org/130173010/diff/30001/pkg/unittest/test/unitte...
pkg/unittest/test/unittest_protect_async_test.dart:42: 'protectAsync2:Caught Bad
state: error during protectAsync2: arg1, arg2:'
can we combine this with the old message? I think it still is important that we
also mention that the callback is called after the test was done.

[kevmoo, 2014-02-04 02:26:17]

https://codereview.chromium.org/130173010/diff/30001/pkg/unittest/lib/src/spr...
File pkg/unittest/lib/src/spread_args_helper.dart (right):

https://codereview.chromium.org/130173010/diff/30001/pkg/unittest/lib/src/spr...
pkg/unittest/lib/src/spread_args_helper.dart:124: _guardAsync(Function tryBody,
Function finallyBody, TestCase testCase) {
On 2014/02/04 02:20:05, Siggi Cherem (dart-lang) wrote:
> I thought we were going to get rid of this too (since the invoke0/1/2 are
> created inside the zone, they should be guarded)

This is still used by the expectAsync logic. I'm leaving it in for the time
being while I peel the onion

https://codereview.chromium.org/130173010/diff/30001/pkg/unittest/lib/unittes...
File pkg/unittest/lib/unittest.dart (right):

https://codereview.chromium.org/130173010/diff/30001/pkg/unittest/lib/unittes...
pkg/unittest/lib/unittest.dart:406: return callback;
On 2014/02/04 02:20:05, Siggi Cherem (dart-lang) wrote:
> not sure if this works. runZoned only intercepts 0 and 1 arg methods.

This is basically a no-op. The assumption is that  callback (in this case) is a
two-argument method. It will be return as such and called as such.

Everything is run in a zone, so it's fine to make all of these no-ops

https://codereview.chromium.org/130173010/diff/30001/pkg/unittest/test/unitte...
File pkg/unittest/test/unittest_protect_async_test.dart (right):

https://codereview.chromium.org/130173010/diff/30001/pkg/unittest/test/unitte...
pkg/unittest/test/unittest_protect_async_test.dart:42: 'protectAsync2:Caught Bad
state: error during protectAsync2: arg1, arg2:'
On 2014/02/04 02:20:05, Siggi Cherem (dart-lang) wrote:
> can we combine this with the old message? I think it still is important that
we
> also mention that the callback is called after the test was done.

Good idea. I'll look into it.

[nweiz, 2014-02-04 02:34:25]

lgtm

https://codereview.chromium.org/130173010/diff/30001/pkg/unittest/CHANGELOG.md
File pkg/unittest/CHANGELOG.md (right):

https://codereview.chromium.org/130173010/diff/30001/pkg/unittest/CHANGELOG.m...
pkg/unittest/CHANGELOG.md:1: # Changelog - unittest
Mention the changes in this CL in the changelog as well.

https://codereview.chromium.org/130173010/diff/30001/pkg/unittest/CHANGELOG.m...
pkg/unittest/CHANGELOG.md:11: `solo_group` now throw a `StateError` if called
while tests are running.
Nit: indent this +2 spaces.

https://codereview.chromium.org/130173010/diff/30001/pkg/unittest/lib/unittes...
File pkg/unittest/lib/unittest.dart (right):

https://codereview.chromium.org/130173010/diff/30001/pkg/unittest/lib/unittes...
pkg/unittest/lib/unittest.dart:406: return callback;
On 2014/02/04 02:20:05, Siggi Cherem (dart-lang) wrote:
> not sure if this works. runZoned only intercepts 0 and 1 arg methods.

I don't think this is accurate. All asynchronous callbacks that are scheduled by
a dart: library are intercepted by [runZoned].

[kevmoo, 2014-02-04 02:41:34]

https://codereview.chromium.org/130173010/diff/30001/pkg/unittest/CHANGELOG.md
File pkg/unittest/CHANGELOG.md (right):

https://codereview.chromium.org/130173010/diff/30001/pkg/unittest/CHANGELOG.m...
pkg/unittest/CHANGELOG.md:1: # Changelog - unittest
On 2014/02/04 02:34:26, nweiz wrote:
> Mention the changes in this CL in the changelog as well.

Done.

https://codereview.chromium.org/130173010/diff/30001/pkg/unittest/CHANGELOG.m...
pkg/unittest/CHANGELOG.md:11: `solo_group` now throw a `StateError` if called
while tests are running.
On 2014/02/04 02:34:26, nweiz wrote:
> Nit: indent this +2 spaces.

Done.

[Siggi Cherem (dart-lang), 2014-02-04 02:42:01]

https://codereview.chromium.org/130173010/diff/30001/pkg/unittest/lib/src/spr...
File pkg/unittest/lib/src/spread_args_helper.dart (right):

https://codereview.chromium.org/130173010/diff/30001/pkg/unittest/lib/src/spr...
pkg/unittest/lib/src/spread_args_helper.dart:124: _guardAsync(Function tryBody,
Function finallyBody, TestCase testCase) {
On 2014/02/04 02:26:18, kevmoo wrote:
> On 2014/02/04 02:20:05, Siggi Cherem (dart-lang) wrote:
> > I thought we were going to get rid of this too (since the invoke0/1/2 are
> > created inside the zone, they should be guarded)
> 
> This is still used by the expectAsync logic. I'm leaving it in for the time
> being while I peel the onion

I meant, I thought you were going to get rid of it by changing expectAsync
above.

No worries, one layer at a time sounds good too :)

https://codereview.chromium.org/130173010/diff/30001/pkg/unittest/lib/unittes...
File pkg/unittest/lib/unittest.dart (right):

https://codereview.chromium.org/130173010/diff/30001/pkg/unittest/lib/unittes...
pkg/unittest/lib/unittest.dart:406: return callback;
On 2014/02/04 02:26:18, kevmoo wrote:
> On 2014/02/04 02:20:05, Siggi Cherem (dart-lang) wrote:
> > not sure if this works. runZoned only intercepts 0 and 1 arg methods.
> 
> This is basically a no-op. The assumption is that  callback (in this case) is
a
> two-argument method. It will be return as such and called as such.
> 
> Everything is run in a zone, so it's fine to make all of these no-ops

To clarify what my worry was. Zones used to only have logic to wrap 0 and 1 arg
callbacks. It seems that now they also wrap callbacks with 2 args. "Everything
is run in a zone" used to be true only if there were no 2-arg callbacks in the
entire system (dart:io/async/html). Turns out that 'dart:html' has some 2-arg
callbacks (e.g. mutation obsevers). Now that they have binary callbacks included
in zones, we only have to worry about 3-arg callbacks (for which we seem to have
only one in dart:html and I'm not even sure it is asynchronous)

[kevmoo, 2014-02-04 02:45:28]

https://codereview.chromium.org/130173010/diff/30001/pkg/unittest/lib/unittes...
File pkg/unittest/lib/unittest.dart (right):

https://codereview.chromium.org/130173010/diff/30001/pkg/unittest/lib/unittes...
pkg/unittest/lib/unittest.dart:406: return callback;
On 2014/02/04 02:42:01, Siggi Cherem (dart-lang) wrote:
> On 2014/02/04 02:26:18, kevmoo wrote:
> > On 2014/02/04 02:20:05, Siggi Cherem (dart-lang) wrote:
> > > not sure if this works. runZoned only intercepts 0 and 1 arg methods.
> > 
> > This is basically a no-op. The assumption is that  callback (in this case)
is
> a
> > two-argument method. It will be return as such and called as such.
> > 
> > Everything is run in a zone, so it's fine to make all of these no-ops
> 
> To clarify what my worry was. Zones used to only have logic to wrap 0 and 1
arg
> callbacks. It seems that now they also wrap callbacks with 2 args. "Everything
> is run in a zone" used to be true only if there were no 2-arg callbacks in the
> entire system (dart:io/async/html). Turns out that 'dart:html' has some 2-arg
> callbacks (e.g. mutation obsevers). Now that they have binary callbacks
included
> in zones, we only have to worry about 3-arg callbacks (for which we seem to
have
> only one in dart:html and I'm not even sure it is asynchronous)

I'm not sure how Zones handle callbacks with N arguments.

I'm side-stepping this issue by wrapping the call to runTest (which has zero
arguments) in the Zone.

I'm a bit confused where in the Zone logic there is a concept of the number of
arguments you speak of...

[kevmoo, 2014-02-04 17:52:26]

https://codereview.chromium.org/130173010/diff/30001/pkg/unittest/test/unitte...
File pkg/unittest/test/unittest_protect_async_test.dart (right):

https://codereview.chromium.org/130173010/diff/30001/pkg/unittest/test/unitte...
pkg/unittest/test/unittest_protect_async_test.dart:42: 'protectAsync2:Caught Bad
state: error during protectAsync2: arg1, arg2:'
On 2014/02/04 02:26:18, kevmoo wrote:
> On 2014/02/04 02:20:05, Siggi Cherem (dart-lang) wrote:
> > can we combine this with the old message? I think it still is important that
> we
> > also mention that the callback is called after the test was done.
> 
> Good idea. I'll look into it.

I added a TODO around this. We need to do a better way to capture and categorize
the classes of errors. During setUp, tearDown, and run. 'in-line' vs Future vs
async.

https://codereview.chromium.org/130173010

[kevmoo, 2014-02-04 17:52:50]

Committed patchset #6 manually as r32284 (presubmit successful).

[Siggi Cherem (dart-lang), 2014-02-04 17:59:36]

https://codereview.chromium.org/130173010/diff/30001/pkg/unittest/test/unitte...
File pkg/unittest/test/unittest_protect_async_test.dart (right):

https://codereview.chromium.org/130173010/diff/30001/pkg/unittest/test/unitte...
pkg/unittest/test/unittest_protect_async_test.dart:42: 'protectAsync2:Caught Bad
state: error during protectAsync2: arg1, arg2:'
On 2014/02/04 17:52:26, kevmoo wrote:
> On 2014/02/04 02:26:18, kevmoo wrote:
> > On 2014/02/04 02:20:05, Siggi Cherem (dart-lang) wrote:
> > > can we combine this with the old message? I think it still is important
that
> > we
> > > also mention that the callback is called after the test was done.
> > 
> > Good idea. I'll look into it.
> 
> I added a TODO around this. We need to do a better way to capture and
categorize
> the classes of errors. During setUp, tearDown, and run. 'in-line' vs Future vs
> async.
> 
> https://codereview.chromium.org/130173010

Is there a way to fix this part here to show the old message instead? I think
the real reason the test is failing here is not that there is an error in
'protectAsync', but the fact that the code is running after the test is done. 

What happens currently if the callback has no errors?