Add a scheduled test library.

Add a scheduled test library.

[nweiz, 2013-02-08 03:37:21]

This is a minimum-viable sort of thing; there are a lot of additions that need
to be made before it's ready to be used in pub (see the TODOs at the top of
scheduled_test.dart). However, it's in good enough shape to write some unit
tests for, so I thought I'd ship it out now for starters.

[Bob Nystrom, 2013-02-08 16:15:37]

LGTM.

Lots of questions and suggestions, and I only skimmed the metatest stuff, but
I'd like to land this so we can see how it helps pub and I won't be in today to
do more reviews.

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

https://codereview.chromium.org/12208081/diff/1/pkg/scheduled_test/lib/schedu...
pkg/scheduled_test/lib/scheduled_test.dart:114: /// they finish. When using
`unittest`, you wrap these callbacks with
<3 <3 <3

https://codereview.chromium.org/12208081/diff/1/pkg/scheduled_test/lib/schedu...
pkg/scheduled_test/lib/scheduled_test.dart:121: /// all of its
[wrapAsync]-wrapped functions have been called. This ensures that
We'll probably eventually want the ability to wrap things that are optionally
called or that should not be called at all (so you can trap an unexpected
error).

https://codereview.chromium.org/12208081/diff/1/pkg/scheduled_test/lib/schedu...
pkg/scheduled_test/lib/scheduled_test.dart:165: /// This is `null` when there's
no test currently running.
Maybe throw if the user tries to access this outside of a scheduled test?

https://codereview.chromium.org/12208081/diff/1/pkg/scheduled_test/lib/schedu...
pkg/scheduled_test/lib/scheduled_test.dart:180: void _test(String description,
void body(), [Function testFn]) {
Why is testFn optional?

https://codereview.chromium.org/12208081/diff/1/pkg/scheduled_test/lib/schedu...
pkg/scheduled_test/lib/scheduled_test.dart:217: /// of [currentSchedule]. Tasks
will be run in the order they're scheduled. In
In -> If

https://codereview.chromium.org/12208081/diff/1/pkg/scheduled_test/lib/schedu...
pkg/scheduled_test/lib/scheduled_test.dart:261: throw 'There seems to be another
scheduled test "$description" still '
Throw a StateError.

https://codereview.chromium.org/12208081/diff/1/pkg/scheduled_test/lib/schedu...
pkg/scheduled_test/lib/scheduled_test.dart:279: throw "Unexpected call to
wrapAsync with no current schedule.";
Throw a StateError.

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

https://codereview.chromium.org/12208081/diff/1/pkg/scheduled_test/lib/src/sc...
pkg/scheduled_test/lib/src/schedule.dart:119: unittest.registerException(new
ExpectException(scheduleError.toString()));
Are we ever supposed to get here? I think this may do the wrong thing. If you've
already advanced to the next test, this will tell unittest to advance from that
one.

Maybe just throw a StateError or something here?

https://codereview.chromium.org/12208081/diff/1/pkg/scheduled_test/lib/src/sc...
pkg/scheduled_test/lib/src/schedule.dart:133: /// this.
Make this one private then?

https://codereview.chromium.org/12208081/diff/1/pkg/scheduled_test/lib/src/sc...
pkg/scheduled_test/lib/src/schedule.dart:134: Function wrapAsync(fn(arg)) {
Should check that _noPendingCallbacks isn't already complete. If it is, throw a
StateError.

https://codereview.chromium.org/12208081/diff/1/pkg/scheduled_test/lib/src/sc...
pkg/scheduled_test/lib/src/schedule.dart:153: if (_pendingCallbacks == 0) return
new Future.immediate(null);
What if callbacks are added after this is called?

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

https://codereview.chromium.org/12208081/diff/1/pkg/scheduled_test/lib/src/sc...
pkg/scheduled_test/lib/src/schedule_error.dart:58:
result.add(prefixLines(stackTrace.toString().trim()));
Might be cool to borrow some of the stacktrace cleaning code from
command_line_config here.

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

https://codereview.chromium.org/12208081/diff/1/pkg/scheduled_test/lib/src/ta...
pkg/scheduled_test/lib/src/task.dart:21: Function fn;
Make a typedef for this:

typedef Future TaskBody();

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

https://codereview.chromium.org/12208081/diff/1/pkg/scheduled_test/lib/src/ut...
pkg/scheduled_test/lib/src/utils.dart:25: }
Is this being used anywhere?

https://codereview.chromium.org/12208081/diff/1/pkg/scheduled_test/test/metat...
File pkg/scheduled_test/test/metatest.dart (right):

https://codereview.chromium.org/12208081/diff/1/pkg/scheduled_test/test/metat...
pkg/scheduled_test/test/metatest.dart:5: /// A test library for testing test
libraries? We have to go deeper.
"have to" -> "must"

[nweiz, 2013-02-08 22:14:38]

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

https://codereview.chromium.org/12208081/diff/1/pkg/scheduled_test/lib/schedu...
pkg/scheduled_test/lib/scheduled_test.dart:121: /// all of its
[wrapAsync]-wrapped functions have been called. This ensures that
On 2013/02/08 16:15:37, Bob Nystrom wrote:
> We'll probably eventually want the ability to wrap things that are optionally
> called or that should not be called at all (so you can trap an unexpected
> error).

Once that comes up, we can port guardAsync as well.

https://codereview.chromium.org/12208081/diff/1/pkg/scheduled_test/lib/schedu...
pkg/scheduled_test/lib/scheduled_test.dart:165: /// This is `null` when there's
no test currently running.
On 2013/02/08 16:15:37, Bob Nystrom wrote:
> Maybe throw if the user tries to access this outside of a scheduled test?

I think it's useful to allow users to check if a schedule exists. The
null-reference error should be clear enough here, hopefully.

https://codereview.chromium.org/12208081/diff/1/pkg/scheduled_test/lib/schedu...
pkg/scheduled_test/lib/scheduled_test.dart:180: void _test(String description,
void body(), [Function testFn]) {
On 2013/02/08 16:15:37, Bob Nystrom wrote:
> Why is testFn optional?

Good question. Fixed.

https://codereview.chromium.org/12208081/diff/1/pkg/scheduled_test/lib/schedu...
pkg/scheduled_test/lib/scheduled_test.dart:217: /// of [currentSchedule]. Tasks
will be run in the order they're scheduled. In
On 2013/02/08 16:15:37, Bob Nystrom wrote:
> In -> If

Done.

https://codereview.chromium.org/12208081/diff/1/pkg/scheduled_test/lib/schedu...
pkg/scheduled_test/lib/scheduled_test.dart:261: throw 'There seems to be another
scheduled test "$description" still '
On 2013/02/08 16:15:37, Bob Nystrom wrote:
> Throw a StateError.

Done.

https://codereview.chromium.org/12208081/diff/1/pkg/scheduled_test/lib/schedu...
pkg/scheduled_test/lib/scheduled_test.dart:279: throw "Unexpected call to
wrapAsync with no current schedule.";
On 2013/02/08 16:15:37, Bob Nystrom wrote:
> Throw a StateError.

Done.

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

https://codereview.chromium.org/12208081/diff/1/pkg/scheduled_test/lib/src/sc...
pkg/scheduled_test/lib/src/schedule.dart:119: unittest.registerException(new
ExpectException(scheduleError.toString()));
On 2013/02/08 16:15:37, Bob Nystrom wrote:
> Are we ever supposed to get here? I think this may do the wrong thing. If
you've
> already advanced to the next test, this will tell unittest to advance from
that
> one.
> 
> Maybe just throw a StateError or something here?

Yeah, a StateError is probably better here. I really hate top-levelling
exceptions in tests, but in this case I don't think there's a better
alternative.

https://codereview.chromium.org/12208081/diff/1/pkg/scheduled_test/lib/src/sc...
pkg/scheduled_test/lib/src/schedule.dart:133: /// this.
On 2013/02/08 16:15:37, Bob Nystrom wrote:
> Make this one private then?

"Usually", not necessarily "always". Also, scheduled_test.dart needs to access
this.

https://codereview.chromium.org/12208081/diff/1/pkg/scheduled_test/lib/src/sc...
pkg/scheduled_test/lib/src/schedule.dart:134: Function wrapAsync(fn(arg)) {
On 2013/02/08 16:15:37, Bob Nystrom wrote:
> Should check that _noPendingCallbacks isn't already complete. If it is, throw
a
> StateError.

It's possible to wait for pending callbacks multiple times, although the code
doesn't currently handle that correctly. Fixed.

https://codereview.chromium.org/12208081/diff/1/pkg/scheduled_test/lib/src/sc...
pkg/scheduled_test/lib/src/schedule.dart:153: if (_pendingCallbacks == 0) return
new Future.immediate(null);
On 2013/02/08 16:15:37, Bob Nystrom wrote:
> What if callbacks are added after this is called?

Then they're not pending :p.

In seriousness, this is called when you're otherwise done with the current
queue, but you want to wait for the existing callbacks to clear out. Future
callbacks belong to the next queue.

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

https://codereview.chromium.org/12208081/diff/1/pkg/scheduled_test/lib/src/sc...
pkg/scheduled_test/lib/src/schedule_error.dart:58:
result.add(prefixLines(stackTrace.toString().trim()));
On 2013/02/08 16:15:37, Bob Nystrom wrote:
> Might be cool to borrow some of the stacktrace cleaning code from
> command_line_config here.

That would be a nice thing to move into its own package, but let's save that for
a future change.

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

https://codereview.chromium.org/12208081/diff/1/pkg/scheduled_test/lib/src/ta...
pkg/scheduled_test/lib/src/task.dart:21: Function fn;
On 2013/02/08 16:15:37, Bob Nystrom wrote:
> Make a typedef for this:
> 
> typedef Future TaskBody();

Done.

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

https://codereview.chromium.org/12208081/diff/1/pkg/scheduled_test/lib/src/ut...
pkg/scheduled_test/lib/src/utils.dart:25: }
On 2013/02/08 16:15:37, Bob Nystrom wrote:
> Is this being used anywhere?

It's used a bunch in the tests.

https://codereview.chromium.org/12208081/diff/1/pkg/scheduled_test/test/metat...
File pkg/scheduled_test/test/metatest.dart (right):

https://codereview.chromium.org/12208081/diff/1/pkg/scheduled_test/test/metat...
pkg/scheduled_test/test/metatest.dart:5: /// A test library for testing test
libraries? We have to go deeper.
On 2013/02/08 16:15:37, Bob Nystrom wrote:
> "have to" -> "must"

Done.

[Bob Nystrom, 2013-02-11 18:46:14]

Double plus LGTM.