pkg/scheduled_test: handle null currentSchedule due to timeout

pkg/scheduled_test: handle null currentSchedule due to timeout

R=nweiz@google.com

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

[kevmoo, 2014-04-06 01:07:19]

I crashed scheduled_test due to a null currentSchedule working through a weird
edge-case in shelf_io

I couldn't figure out how to create a minimal repro of the error I hit, but
using dependency_overrides, this fix resolved the crash.

If you want to run w/ it to add a test, pleas do...

[nweiz, 2014-04-07 18:02:21]

This needs a test. You should be able to reproduce it by doing something like

    test("whatever", () {
      pumpEventQueue().then((_) => throw 'error');
    });

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

https://codereview.chromium.org/227193002/diff/1/pkg/scheduled_test/lib/sched...
pkg/scheduled_test/lib/scheduled_test.dart:236: Schedule localSchedule;
Don't type-annotate local variables.

https://codereview.chromium.org/227193002/diff/1/pkg/scheduled_test/lib/sched...
pkg/scheduled_test/lib/scheduled_test.dart:239: localSchedule = _currentSchedule
= new Schedule();
We generally avoid multiple assignments on the same line.

[kevmoo, 2014-04-08 01:29:33]

PTAL

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

https://codereview.chromium.org/227193002/diff/1/pkg/scheduled_test/lib/sched...
pkg/scheduled_test/lib/scheduled_test.dart:236: Schedule localSchedule;
On 2014/04/07 18:02:21, nweiz wrote:
> Don't type-annotate local variables.

Done.

https://codereview.chromium.org/227193002/diff/1/pkg/scheduled_test/lib/sched...
pkg/scheduled_test/lib/scheduled_test.dart:239: localSchedule = _currentSchedule
= new Schedule();
On 2014/04/07 18:02:21, nweiz wrote:
> We generally avoid multiple assignments on the same line.

Done.

[nweiz, 2014-04-08 19:27:36]

https://codereview.chromium.org/227193002/diff/20001/pkg/scheduled_test/lib/s...
File pkg/scheduled_test/lib/scheduled_test.dart (right):

https://codereview.chromium.org/227193002/diff/20001/pkg/scheduled_test/lib/s...
pkg/scheduled_test/lib/scheduled_test.dart:236: var errorHandler;
Add a comment: "Capture this in a local variable in case we capture an
out-of-band error after the schedule completes."

https://codereview.chromium.org/227193002/diff/20001/pkg/scheduled_test/test/...
File pkg/scheduled_test/test/metatest.dart (right):

https://codereview.chromium.org/227193002/diff/20001/pkg/scheduled_test/test/...
pkg/scheduled_test/test/metatest.dart:56: /// to the `main` method of a test
file, and will be run in an isolate.
One-sentence first paragraph.

https://codereview.chromium.org/227193002/diff/20001/pkg/scheduled_test/test/...
pkg/scheduled_test/test/metatest.dart:60: Map<String, String> expectedResults) {
This never calls [_summarizeTests], which means that the error output will
likely be difficult to understand. Rather than using [expect] at all, consider
following [expectTestsPass] and calling [fail] explicitly.

https://codereview.chromium.org/227193002/diff/20001/pkg/scheduled_test/test/...
pkg/scheduled_test/test/metatest.dart:61: 
Nit: unnecessary newline.

https://codereview.chromium.org/227193002/diff/20001/pkg/scheduled_test/test/...
pkg/scheduled_test/test/metatest.dart:65: for (var t in results['results']) {
Nit: Use actual words for variables, not single characters.

https://codereview.chromium.org/227193002/diff/20001/pkg/scheduled_test/test/...
pkg/scheduled_test/test/metatest.dart:70: reason: '"$description" did not have
an expected result set.');
Instead of this, use `expect(expectedResults, containsKey(description))`.  That
will produce a nicer error message automatically.

https://codereview.chromium.org/227193002/diff/20001/pkg/scheduled_test/test/...
pkg/scheduled_test/test/metatest.dart:75: reason: 'The test "$description" not
not have the expected result.');
expect(t, containsPair('result', expectedResult))

https://codereview.chromium.org/227193002/diff/20001/pkg/scheduled_test/test/...
pkg/scheduled_test/test/metatest.dart:77: expect(expectedResults, isEmpty);
reason: "Unexpected additional test results."

https://codereview.chromium.org/227193002/diff/20001/pkg/scheduled_test/test/...
File pkg/scheduled_test/test/scheduled_test/current_schedule_errors_test.dart
(right):

https://codereview.chromium.org/227193002/diff/20001/pkg/scheduled_test/test/...
pkg/scheduled_test/test/scheduled_test/current_schedule_errors_test.dart:28: },
{'test 1': ERROR, 'test 2': PASS});
Where are ERROR and PASS defined?

[kevmoo, 2014-04-08 20:17:50]

PTAL -- especially the new failure of unhandled_error_test

https://codereview.chromium.org/227193002/diff/20001/pkg/scheduled_test/lib/s...
File pkg/scheduled_test/lib/scheduled_test.dart (right):

https://codereview.chromium.org/227193002/diff/20001/pkg/scheduled_test/lib/s...
pkg/scheduled_test/lib/scheduled_test.dart:236: var errorHandler;
On 2014/04/08 19:27:36, nweiz wrote:
> Add a comment: "Capture this in a local variable in case we capture an
> out-of-band error after the schedule completes."

Done.

https://codereview.chromium.org/227193002/diff/20001/pkg/scheduled_test/test/...
File pkg/scheduled_test/test/metatest.dart (right):

https://codereview.chromium.org/227193002/diff/20001/pkg/scheduled_test/test/...
pkg/scheduled_test/test/metatest.dart:56: /// to the `main` method of a test
file, and will be run in an isolate.
On 2014/04/08 19:27:36, nweiz wrote:
> One-sentence first paragraph.

Done.

https://codereview.chromium.org/227193002/diff/20001/pkg/scheduled_test/test/...
pkg/scheduled_test/test/metatest.dart:60: Map<String, String> expectedResults) {
On 2014/04/08 19:27:36, nweiz wrote:
> This never calls [_summarizeTests], which means that the error output will
> likely be difficult to understand. Rather than using [expect] at all, consider
> following [expectTestsPass] and calling [fail] explicitly.

I added _summarizeTests to each reason: output, which should help

https://codereview.chromium.org/227193002/diff/20001/pkg/scheduled_test/test/...
pkg/scheduled_test/test/metatest.dart:61: 
On 2014/04/08 19:27:36, nweiz wrote:
> Nit: unnecessary newline.

Done.

https://codereview.chromium.org/227193002/diff/20001/pkg/scheduled_test/test/...
pkg/scheduled_test/test/metatest.dart:65: for (var t in results['results']) {
On 2014/04/08 19:27:36, nweiz wrote:
> Nit: Use actual words for variables, not single characters.

Done.

https://codereview.chromium.org/227193002/diff/20001/pkg/scheduled_test/test/...
pkg/scheduled_test/test/metatest.dart:70: reason: '"$description" did not have
an expected result set.');
On 2014/04/08 19:27:36, nweiz wrote:
> Instead of this, use `expect(expectedResults, containsKey(description))`. 
That
> will produce a nicer error message automatically.

Done.

https://codereview.chromium.org/227193002/diff/20001/pkg/scheduled_test/test/...
pkg/scheduled_test/test/metatest.dart:75: reason: 'The test "$description" not
not have the expected result.');
On 2014/04/08 19:27:36, nweiz wrote:
> expect(t, containsPair('result', expectedResult))

Done.

https://codereview.chromium.org/227193002/diff/20001/pkg/scheduled_test/test/...
pkg/scheduled_test/test/metatest.dart:77: expect(expectedResults, isEmpty);
On 2014/04/08 19:27:36, nweiz wrote:
> reason: "Unexpected additional test results."

Done.

https://codereview.chromium.org/227193002/diff/20001/pkg/scheduled_test/test/...
File pkg/scheduled_test/test/scheduled_test/current_schedule_errors_test.dart
(right):

https://codereview.chromium.org/227193002/diff/20001/pkg/scheduled_test/test/...
pkg/scheduled_test/test/scheduled_test/current_schedule_errors_test.dart:28: },
{'test 1': ERROR, 'test 2': PASS});
On 2014/04/08 19:27:36, nweiz wrote:
> Where are ERROR and PASS defined?

toplevel consts in unittest

[nweiz, 2014-04-09 01:20:41]

https://codereview.chromium.org/227193002/diff/20001/pkg/scheduled_test/lib/s...
File pkg/scheduled_test/lib/scheduled_test.dart (right):

https://codereview.chromium.org/227193002/diff/20001/pkg/scheduled_test/lib/s...
pkg/scheduled_test/lib/scheduled_test.dart:253: }, onError: errorHandler);
I figured out the issue: this is being passed in when [errorHandler] is still
null. Just write "(error, stackTrace) => errorHandler(error, stackTrace)" and
it's all good.

[kevmoo, 2014-04-09 17:56:58]

On 2014/04/09 01:20:41, nweiz wrote:
>
https://codereview.chromium.org/227193002/diff/20001/pkg/scheduled_test/lib/s...
> File pkg/scheduled_test/lib/scheduled_test.dart (right):
> 
>
https://codereview.chromium.org/227193002/diff/20001/pkg/scheduled_test/lib/s...
> pkg/scheduled_test/lib/scheduled_test.dart:253: }, onError: errorHandler);
> I figured out the issue: this is being passed in when [errorHandler] is still
> null. Just write "(error, stackTrace) => errorHandler(error, stackTrace)" and
> it's all good.

I've tried to create a test that fails without this patch and passes with it,
but no luck.

Let's talk more about this today...

[kevmoo, 2014-04-10 00:41:34]

PTAL

[nweiz, 2014-04-10 00:43:50]

https://codereview.chromium.org/227193002/diff/80001/pkg/scheduled_test/test/...
File pkg/scheduled_test/test/scheduled_test/current_schedule_errors_test.dart
(right):

https://codereview.chromium.org/227193002/diff/80001/pkg/scheduled_test/test/...
pkg/scheduled_test/test/scheduled_test/current_schedule_errors_test.dart:20: new
Future.delayed(const Duration(milliseconds: 10), () => throw 'error');
[Future.delayed] is unreliable. Use mock_clock instead. See tests below for
examples.

https://codereview.chromium.org/227193002/diff/80001/pkg/scheduled_test/test/...
pkg/scheduled_test/test/scheduled_test/current_schedule_errors_test.dart:22:
return new Future.delayed(const Duration(milliseconds: 1));
There's no reason to return a Future here.

[kevmoo, 2014-04-10 00:50:38]

PTAL

https://codereview.chromium.org/227193002/diff/80001/pkg/scheduled_test/test/...
File pkg/scheduled_test/test/scheduled_test/current_schedule_errors_test.dart
(right):

https://codereview.chromium.org/227193002/diff/80001/pkg/scheduled_test/test/...
pkg/scheduled_test/test/scheduled_test/current_schedule_errors_test.dart:20: new
Future.delayed(const Duration(milliseconds: 10), () => throw 'error');
On 2014/04/10 00:43:51, nweiz wrote:
> [Future.delayed] is unreliable. Use mock_clock instead. See tests below for
> examples.

Done.

https://codereview.chromium.org/227193002/diff/80001/pkg/scheduled_test/test/...
pkg/scheduled_test/test/scheduled_test/current_schedule_errors_test.dart:22:
return new Future.delayed(const Duration(milliseconds: 1));
On 2014/04/10 00:43:51, nweiz wrote:
> There's no reason to return a Future here.

Done.

[nweiz, 2014-04-10 00:58:15]

lgtm

https://codereview.chromium.org/227193002/diff/100001/pkg/scheduled_test/test...
File pkg/scheduled_test/test/scheduled_test/current_schedule_errors_test.dart
(right):

https://codereview.chromium.org/227193002/diff/100001/pkg/scheduled_test/test...
pkg/scheduled_test/test/scheduled_test/current_schedule_errors_test.dart:20:
sleep(5).then((_) => throw 'error');
If you're going to use [sleep] like this, you need to call
`mock_clock.mock().run()` at the beginning of the block.

Also, there's no reason to sleep for 5 and 10 increments; just 1 and 2 will do.
Each increment corresponds to a full [pumpEventQueue] iteration.

[kevmoo, 2014-04-10 01:08:22]

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