Issue 12218102: Add built-in timeouts to scheduled_test.

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+563 lines, -29 lines)			Patch
	pkg/scheduled_test/lib/scheduled_test.dart	View	1	2 chunks	+13 lines, -1 line	0 comments	Download
M	pkg/scheduled_test/lib/src/schedule.dart	View	1	15 chunks	+152 lines, -19 lines	0 comments	Download
M	pkg/scheduled_test/lib/src/schedule_error.dart	View		3 chunks	+5 lines, -6 lines	0 comments	Download
A	pkg/scheduled_test/lib/src/substitute_future.dart	View		1 chunk	+54 lines, -0 lines	0 comments	Download
M	pkg/scheduled_test/test/scheduled_test_test.dart	View	1	2 chunks	+185 lines, -3 lines	0 comments	Download
A	pkg/scheduled_test/test/substitute_future_test.dart	View		1 chunk	+154 lines, -0 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

Bob Nystrom

I worry those tests will be brittle, but I don't have any better ideas. Bunch ...

7 years, 10 months ago (2013-02-12 00:34:17 UTC) #2

nweiz

7 years, 10 months ago (2013-02-12 01:15:56 UTC) #3

The tests *are* brittle. It's ridiculous that we have to rely on actual timers
and sleeps to test this stuff. But until we get support for legitimate
dependency injection, I don't see any way around it.

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

https://codereview.chromium.org/12218102/diff/1/pkg/scheduled_test/lib/src/sc...
pkg/scheduled_test/lib/src/schedule.dart:53: /// schedule has finished running.
On 2013/02/12 00:34:17, Bob Nystrom wrote:
> Wrong doc comment?

Done.

https://codereview.chromium.org/12218102/diff/1/pkg/scheduled_test/lib/src/sc...
pkg/scheduled_test/lib/src/schedule.dart:85: /// timeouts entirely.
On 2013/02/12 00:34:17, Bob Nystrom wrote:
> I think we should use zero for no timeout. We tend to treat numbers as
> non-nullable and "zero for no timeout" is a pretty common pattern already, I
> think.

See offline discussion.

https://codereview.chromium.org/12218102/diff/1/pkg/scheduled_test/lib/src/sc...
pkg/scheduled_test/lib/src/schedule.dart:89: /// out, that can only be handles
in [onComplete]; if [onComplete] times out,
On 2013/02/12 00:34:17, Bob Nystrom wrote:
> "handles" -> "handled".

Done.

https://codereview.chromium.org/12218102/diff/1/pkg/scheduled_test/lib/src/sc...
pkg/scheduled_test/lib/src/schedule.dart:93: /// will not be handlable. The user
will still be notified of it.
On 2013/02/12 00:34:17, Bob Nystrom wrote:
> "will not be handlable" -> "cannot be handled".

Done.

https://codereview.chromium.org/12218102/diff/1/pkg/scheduled_test/lib/src/sc...
pkg/scheduled_test/lib/src/schedule.dart:94: int get timeoutLength =>
_timeoutLength;
On 2013/02/12 00:34:17, Bob Nystrom wrote:
> The "length" seems unhelpful. How about "timeoutMs"?

Done.

https://codereview.chromium.org/12218102/diff/1/pkg/scheduled_test/lib/src/sc...
pkg/scheduled_test/lib/src/schedule.dart:215: // It's possible that the queue
timed out before this
On 2013/02/12 00:34:17, Bob Nystrom wrote:
> "."

Done.

https://codereview.chromium.org/12218102/diff/1/pkg/scheduled_test/lib/src/sc...
pkg/scheduled_test/lib/src/schedule.dart:252: void ping() {
On 2013/02/12 00:34:17, Bob Nystrom wrote:
> "ping" already means something else in common use. How about "heartbeat"?
That's
> what I've seen elsewhere for a keep-alive kick.

I was using it in the IRC sense of "are you still alive?", but sure, "heartbeat"
is also fine.

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

https://codereview.chromium.org/12218102/diff/1/pkg/scheduled_test/lib/src/sc...
pkg/scheduled_test/lib/src/schedule_error.dart:72: } else { //
_stateWhenDetected == ScheduleState.SET_UP
On 2013/02/12 00:34:17, Bob Nystrom wrote:
> How about doing an explicit else if here for this state and then have an
> 
> else {
>   assert(false); // Unreachable.
> }
> 
> at the end?

I'm really paranoid about getting into an unexpected state, since asynchronous
code is so unpredictable. I want to leave this as-is so that debugging is easier
if some poor user gets into a bad state.

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

https://codereview.chromium.org/12218102/diff/1/pkg/scheduled_test/test/sched...
pkg/scheduled_test/test/scheduled_test_test.dart:584:
expectTestsPass('currentSchedule.currentQueue is tasks before the schedule has '
On 2013/02/12 00:34:17, Bob Nystrom wrote:
> Long line.

Done.

https://codereview.chromium.org/12218102/diff/1/pkg/scheduled_test/test/sched...
pkg/scheduled_test/test/scheduled_test_test.dart:804: var end = new
DateTime.now().add(new Duration(milliseconds: 1000));
On 2013/02/12 00:34:17, Bob Nystrom wrote:
> 100?

Oops, that number was left over from my debugging code. Fixing.

https://codereview.chromium.org/12218102/diff/1/pkg/scheduled_test/test/sched...
pkg/scheduled_test/test/scheduled_test_test.dart:805: while (new DateTime.now()
< end) {}
On 2013/02/12 00:34:17, Bob Nystrom wrote:
> This looks so weird...

Yep, it sucks that we don't have the means to mock out Timer.

Expand Messages | Collapse Messages