Support nested tasks in scheduled_test.

Support nested tasks in scheduled_test.

BUG=8511
Committed: https://code.google.com/p/dart/source/detail?r=18800

[Bob Nystrom, 2013-02-19 23:15:04]

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/lib/schedu...
File pkg/scheduled_test/lib/scheduled_test.dart (right):

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/lib/schedu...
pkg/scheduled_test/lib/scheduled_test.dart:145: /// ensures that the task quque
doesn't complete until the out-of-band chain has
"quque" -> "queue"

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/lib/src/fu...
File pkg/scheduled_test/lib/src/future_group.dart (right):

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/lib/src/fu...
pkg/scheduled_test/lib/src/future_group.dart:15: final List<Future<T>> futures =
<Future<T>>[];
Is there a reason to store these?

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/lib/src/fu...
pkg/scheduled_test/lib/src/future_group.dart:28: if (completed) return;
It seems weird to swallow this. Doesn't this indicate something fishy happened?

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/lib/src/fu...
pkg/scheduled_test/lib/src/future_group.dart:38: if (completed) return;
Ditto here.

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/lib/src/fu...
pkg/scheduled_test/lib/src/future_group.dart:40: completed = true;
It seems like every place we use a completer we end up needing this flag. File a
bug to ask them to have the new lib_v2 completer expose this? It does store it
internally.

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/lib/src/sc...
File pkg/scheduled_test/lib/src/schedule.dart (right):

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/lib/src/sc...
pkg/scheduled_test/lib/src/schedule.dart:130: _state = ScheduleState.RUNNING;
Document why you're setting this state even though setup failed.

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/lib/src/sc...
pkg/scheduled_test/lib/src/schedule.dart:251: var doneCb = wrapAsync((_) =>
null);
Cb -> Callback

Or just inline this function in then() below?

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/lib/src/sc...
pkg/scheduled_test/lib/src/schedule.dart:376: _schedule.currentQueue == this;
Indent +2.

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/lib/src/ta...
File pkg/scheduled_test/lib/src/task.dart (right):

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/lib/src/ta...
pkg/scheduled_test/lib/src/task.dart:23: /// empty if this task is itself a
child task.
Does this mean child tasks cannot have their own child tasks?

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/lib/src/ta...
pkg/scheduled_test/lib/src/task.dart:54: : this._(fn, description, parent.queue,
parent, parent.children.length);
I think this means that ids are not unique. If top-level tasks A and B both have
one child, won't those children both have ID 0?

Either change this to make the IDs unique or update the docs for _id. One option
is to make the ID a string and then treat parents like a path, so the above
children would have IDs like "0/0" and "1/0" or something like that.

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/test/sched...
File pkg/scheduled_test/test/scheduled_test_test.dart (right):

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/test/sched...
pkg/scheduled_test/test/scheduled_test_test.dart:149:
wrapFuture(sleep(1).then((_) => expect('foo', equals('bar'))));
Using expect like this seems weird. How about just:

throw "fail"

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/test/sched...
pkg/scheduled_test/test/scheduled_test_test.dart:1031: 
Add expect(nestedScheduleRun, isFalse) here to test that it is async?

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/test/sched...
pkg/scheduled_test/test/scheduled_test_test.dart:1048: 
Ditto.

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/test/sched...
pkg/scheduled_test/test/scheduled_test_test.dart:1064: schedule(() =>
expect(sleepFinished, isFalse));
So child siblings are parallel but top-level siblings are sequential? You should
document that clearly somewhere.

[nweiz, 2013-02-20 00:23:12]

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/lib/schedu...
File pkg/scheduled_test/lib/scheduled_test.dart (right):

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/lib/schedu...
pkg/scheduled_test/lib/scheduled_test.dart:145: /// ensures that the task quque
doesn't complete until the out-of-band chain has
On 2013/02/19 23:15:04, Bob Nystrom wrote:
> "quque" -> "queue"

Done.

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/lib/src/fu...
File pkg/scheduled_test/lib/src/future_group.dart (right):

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/lib/src/fu...
pkg/scheduled_test/lib/src/future_group.dart:15: final List<Future<T>> futures =
<Future<T>>[];
On 2013/02/19 23:15:04, Bob Nystrom wrote:
> Is there a reason to store these?

I don't want to change this code here without changing it everywhere else it's
used. These are good comments, but they really belong on the code review that
comes out of issue 6626.

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/lib/src/fu...
pkg/scheduled_test/lib/src/future_group.dart:28: if (completed) return;
On 2013/02/19 23:15:04, Bob Nystrom wrote:
> It seems weird to swallow this. Doesn't this indicate something fishy
happened?

In this code, the futures passed to FutureGroup are guaranteed never to pass
errors through. For other code, see comment above.

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/lib/src/fu...
pkg/scheduled_test/lib/src/future_group.dart:40: completed = true;
On 2013/02/19 23:15:04, Bob Nystrom wrote:
> It seems like every place we use a completer we end up needing this flag. File
a
> bug to ask them to have the new lib_v2 completer expose this? It does store it
> internally.

Filed as issue 8638.

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/lib/src/sc...
File pkg/scheduled_test/lib/src/schedule.dart (right):

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/lib/src/sc...
pkg/scheduled_test/lib/src/schedule.dart:130: _state = ScheduleState.RUNNING;
On 2013/02/19 23:15:04, Bob Nystrom wrote:
> Document why you're setting this state even though setup failed.

Done.

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/lib/src/sc...
pkg/scheduled_test/lib/src/schedule.dart:251: var doneCb = wrapAsync((_) =>
null);
On 2013/02/19 23:15:04, Bob Nystrom wrote:
> Cb -> Callback

Done.

> Or just inline this function in then() below?

wrapAsync needs to be called synchronously to ensure that the current queue
blocks until the wrapped callback is called.

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/lib/src/sc...
pkg/scheduled_test/lib/src/schedule.dart:376: _schedule.currentQueue == this;
On 2013/02/19 23:15:04, Bob Nystrom wrote:
> Indent +2.

Done.

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/lib/src/ta...
File pkg/scheduled_test/lib/src/task.dart (right):

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/lib/src/ta...
pkg/scheduled_test/lib/src/task.dart:23: /// empty if this task is itself a
child task.
On 2013/02/19 23:15:04, Bob Nystrom wrote:
> Does this mean child tasks cannot have their own child tasks?

Correct.

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/lib/src/ta...
pkg/scheduled_test/lib/src/task.dart:54: : this._(fn, description, parent.queue,
parent, parent.children.length);
On 2013/02/19 23:15:04, Bob Nystrom wrote:
> I think this means that ids are not unique. If top-level tasks A and B both
have
> one child, won't those children both have ID 0?

Yes.
 
> Either change this to make the IDs unique or update the docs for _id. One
option
> is to make the ID a string and then treat parents like a path, so the above
> children would have IDs like "0/0" and "1/0" or something like that.

I'll change the docs for "_id". Using "0/0" won't work well if the parent task
has a description rather than just an id.

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/test/sched...
File pkg/scheduled_test/test/scheduled_test_test.dart (right):

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/test/sched...
pkg/scheduled_test/test/scheduled_test_test.dart:149:
wrapFuture(sleep(1).then((_) => expect('foo', equals('bar'))));
On 2013/02/19 23:15:04, Bob Nystrom wrote:
> Using expect like this seems weird. How about just:
> 
> throw "fail"

I want to test against the failure conditions that the tests will actually
experience, which is more often going to be expectation failures than it is
random errors.

Also, using "throw" means I can't use "=>" if I ever want to run this on
dart2js.

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/test/sched...
pkg/scheduled_test/test/scheduled_test_test.dart:1031: 
On 2013/02/19 23:15:04, Bob Nystrom wrote:
> Add expect(nestedScheduleRun, isFalse) here to test that it is async?

Done.

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/test/sched...
pkg/scheduled_test/test/scheduled_test_test.dart:1048: 
On 2013/02/19 23:15:04, Bob Nystrom wrote:
> Ditto.

Done.

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/test/sched...
pkg/scheduled_test/test/scheduled_test_test.dart:1064: schedule(() =>
expect(sleepFinished, isFalse));
On 2013/02/19 23:15:04, Bob Nystrom wrote:
> So child siblings are parallel but top-level siblings are sequential? You
should
> document that clearly somewhere.

I thought that was pretty clear from the [schedule] docs, but I'll add some more
text to clarify it further.

[Bob Nystrom, 2013-02-20 17:30:25]

LGTM.

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/lib/src/ta...
File pkg/scheduled_test/lib/src/task.dart (right):

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/lib/src/ta...
pkg/scheduled_test/lib/src/task.dart:23: /// empty if this task is itself a
child task.
On 2013/02/20 00:23:12, nweiz wrote:
> On 2013/02/19 23:15:04, Bob Nystrom wrote:
> > Does this mean child tasks cannot have their own child tasks?
> 
> Correct.

Let's make sure that's documented somewhere. When I see "children" I usually
intuit an arbitrarily deep tree.

[nweiz, 2013-02-20 23:54:58]

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/lib/src/ta...
File pkg/scheduled_test/lib/src/task.dart (right):

https://codereview.chromium.org/12288061/diff/1/pkg/scheduled_test/lib/src/ta...
pkg/scheduled_test/lib/src/task.dart:23: /// empty if this task is itself a
child task.
On 2013/02/20 17:30:25, Bob Nystrom wrote:
> On 2013/02/20 00:23:12, nweiz wrote:
> > On 2013/02/19 23:15:04, Bob Nystrom wrote:
> > > Does this mean child tasks cannot have their own child tasks?
> > 
> > Correct.
> 
> Let's make sure that's documented somewhere. When I see "children" I usually
> intuit an arbitrarily deep tree.

Done.

[nweiz, 2013-02-20 23:57:39]

Committed patchset #3 manually as r18800 (presubmit successful).