Add a ScheduledProcess class to pkg/scheduled_test.

Add a ScheduledProcess class to pkg/scheduled_test.

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

[Bob Nystrom, 2013-03-04 23:52:00]

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

https://codereview.chromium.org/12377093/diff/1/pkg/scheduled_test/lib/schedu...
pkg/scheduled_test/lib/scheduled_process.dart:80: {options, String description,
Encoding encoding: Encoding.UTF_8})
This is quite a large method with a bunch of asynchrony, futures, and closures.
Can you split it up? Below are some suggestions.

https://codereview.chromium.org/12377093/diff/1/pkg/scheduled_test/lib/schedu...
pkg/scheduled_test/lib/scheduled_process.dart:90: }
How about pulling this into a separate _updateDescription() method?

https://codereview.chromium.org/12377093/diff/1/pkg/scheduled_test/lib/schedu...
pkg/scheduled_test/lib/scheduled_process.dart:105: _process.then((p) =>
p.exitCode).then((exitCode) {
Asynchronously accessing the result of a future stored in a field that is set as
the return value of the asynchronous function in question makes my head hurt.

Move this to a separate fn?

https://codereview.chromium.org/12377093/diff/1/pkg/scheduled_test/lib/schedu...
pkg/scheduled_test/lib/scheduled_process.dart:135: _description = "$executable
${arguments.map((a) => '"$a"').join(' ')}";
Call _updateDescription() here.

https://codereview.chromium.org/12377093/diff/1/pkg/scheduled_test/lib/schedu...
pkg/scheduled_test/lib/scheduled_process.dart:154:
currentSchedule.onException.schedule(() {
Move this to a separate fn.

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

https://codereview.chromium.org/12377093/diff/1/pkg/scheduled_test/lib/src/sc...
pkg/scheduled_test/lib/src/schedule.dart:401: // Avoid top-leveling errors that
are passed to onTasksComplete if there are no
Long line.

https://codereview.chromium.org/12377093/diff/1/pkg/scheduled_test/lib/src/ut...
File pkg/scheduled_test/lib/src/utils.dart (right):

https://codereview.chromium.org/12377093/diff/1/pkg/scheduled_test/lib/src/ut...
pkg/scheduled_test/lib/src/utils.dart:145: if (object is Collection) {
Iterable?

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

https://codereview.chromium.org/12377093/diff/1/pkg/scheduled_test/test/sched...
pkg/scheduled_test/test/scheduled_process_test.dart:346: return new
Directory('').createTemp().then((dir) => dir.path);
Create this synchronously.

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

https://codereview.chromium.org/12377093/diff/1/pkg/scheduled_test/test/sched...
pkg/scheduled_test/test/scheduled_test_test.dart:1254: });
What's the motivation for this?

[nweiz, 2013-03-05 02:16:09]

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

https://codereview.chromium.org/12377093/diff/1/pkg/scheduled_test/lib/schedu...
pkg/scheduled_test/lib/scheduled_process.dart:90: }
On 2013/03/04 23:52:00, Bob Nystrom wrote:
> How about pulling this into a separate _updateDescription() method?

Done.

https://codereview.chromium.org/12377093/diff/1/pkg/scheduled_test/lib/schedu...
pkg/scheduled_test/lib/scheduled_process.dart:105: _process.then((p) =>
p.exitCode).then((exitCode) {
On 2013/03/04 23:52:00, Bob Nystrom wrote:
> Asynchronously accessing the result of a future stored in a field that is set
as
> the return value of the asynchronous function in question makes my head hurt.
> 
> Move this to a separate fn?

Done.

https://codereview.chromium.org/12377093/diff/1/pkg/scheduled_test/lib/schedu...
pkg/scheduled_test/lib/scheduled_process.dart:135: _description = "$executable
${arguments.map((a) => '"$a"').join(' ')}";
On 2013/03/04 23:52:00, Bob Nystrom wrote:
> Call _updateDescription() here.

Done.

https://codereview.chromium.org/12377093/diff/1/pkg/scheduled_test/lib/schedu...
pkg/scheduled_test/lib/scheduled_process.dart:154:
currentSchedule.onException.schedule(() {
On 2013/03/04 23:52:00, Bob Nystrom wrote:
> Move this to a separate fn.

Done.

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

https://codereview.chromium.org/12377093/diff/1/pkg/scheduled_test/lib/src/sc...
pkg/scheduled_test/lib/src/schedule.dart:401: // Avoid top-leveling errors that
are passed to onTasksComplete if there are no
On 2013/03/04 23:52:00, Bob Nystrom wrote:
> Long line.

Done.

https://codereview.chromium.org/12377093/diff/1/pkg/scheduled_test/lib/src/ut...
File pkg/scheduled_test/lib/src/utils.dart (right):

https://codereview.chromium.org/12377093/diff/1/pkg/scheduled_test/lib/src/ut...
pkg/scheduled_test/lib/src/utils.dart:145: if (object is Collection) {
On 2013/03/04 23:52:00, Bob Nystrom wrote:
> Iterable?

Done.

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

https://codereview.chromium.org/12377093/diff/1/pkg/scheduled_test/test/sched...
pkg/scheduled_test/test/scheduled_process_test.dart:346: return new
Directory('').createTemp().then((dir) => dir.path);
On 2013/03/04 23:52:00, Bob Nystrom wrote:
> Create this synchronously.

Why? It wouldn't clean up the code at all.

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

https://codereview.chromium.org/12377093/diff/1/pkg/scheduled_test/test/sched...
pkg/scheduled_test/test/scheduled_test_test.dart:1254: });
On 2013/03/04 23:52:00, Bob Nystrom wrote:
> What's the motivation for this?

It's catching the error thrown by the return value of schedule() so we can
examine it in the next test.

[Bob Nystrom, 2013-03-05 17:35:06]

LGTM.

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

https://codereview.chromium.org/12377093/diff/1/pkg/scheduled_test/test/sched...
pkg/scheduled_test/test/scheduled_process_test.dart:346: return new
Directory('').createTemp().then((dir) => dir.path);
On 2013/03/05 02:16:09, nweiz wrote:
> On 2013/03/04 23:52:00, Bob Nystrom wrote:
> > Create this synchronously.
> 
> Why? It wouldn't clean up the code at all.

Couldn't you save a schedule() step here and create the directory in the same
schedule() call where you use it below?

Either way, my thinking is that we should consistently move towards sync APIs
where available and only use async ones if we have to.

[nweiz, 2013-03-08 22:38:09]

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

https://codereview.chromium.org/12377093/diff/1/pkg/scheduled_test/test/sched...
pkg/scheduled_test/test/scheduled_process_test.dart:346: return new
Directory('').createTemp().then((dir) => dir.path);
On 2013/03/05 17:35:06, Bob Nystrom wrote:
> On 2013/03/05 02:16:09, nweiz wrote:
> > On 2013/03/04 23:52:00, Bob Nystrom wrote:
> > > Create this synchronously.
> > 
> > Why? It wouldn't clean up the code at all.
> 
> Couldn't you save a schedule() step here and create the directory in the same
> schedule() call where you use it below?

No, we need to access it from multiple schedule blocks after this, so we'll need
to wrap it in a Future anyway.

> Either way, my thinking is that we should consistently move towards sync APIs
> where available and only use async ones if we have to.

I like this philosophy for Pub, but I'm less excited about it in library code,
especially when it doesn't make the code cleaner.

[nweiz, 2013-03-08 23:02:20]

Committed patchset #2 manually as r19742 (presubmit successful).