Change package scheduled_test to throw TestFailures rather than strings.

Change package scheduled_test to throw TestFailures rather than strings.

BUG=
R=nweiz@google.com

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

[Bill Hesse, 2013-09-20 21:23:08]

It seems a little weird to have a package that throws an internal exception
class, that has no public interface, only Object (_DirectoryException).

[nweiz, 2013-09-20 21:45:40]

_DirectoryValidationError exists to keep some metadata around internally so that
the library can properly format the exception when the user sees it. If Dart had
a class that all exceptions inherited from, _DirectoryValidationError would
inherit from that, but it doesn't. Instead it just has the implicit contract
that exception objects should implement a reasonable [toString], which
_DirectoryValidationError does.

https://codereview.chromium.org/24276010/diff/1/pkg/scheduled_test/test/descr...
File pkg/scheduled_test/test/descriptor/async_test.dart (right):

https://codereview.chromium.org/24276010/diff/1/pkg/scheduled_test/test/descr...
pkg/scheduled_test/test/descriptor/async_test.dart:75:
expect(errors.first.error, new isInstanceOf<TestFailure>());
The type of this exception isn't an important part of the interface. We don't
expect users of the library to catch it, and it may change in the future. As
such, we shouldn't test for it (here or elsewhere).

https://codereview.chromium.org/24276010/diff/1/pkg/scheduled_test/test/descr...
pkg/scheduled_test/test/descriptor/async_test.dart:76:
expect(errors.first.error.message,
In keeping with the above, we should test [error.toString()] rather than
[error.message].

https://codereview.chromium.org/24276010/diff/1/pkg/scheduled_test/test/descr...
File pkg/scheduled_test/test/descriptor/directory_test.dart (right):

https://codereview.chromium.org/24276010/diff/1/pkg/scheduled_test/test/descr...
pkg/scheduled_test/test/descriptor/directory_test.dart:130: // Error is
_DirectoryValidationError.
I don't think these comments are useful. The tests are written from the
perspective of a user of the library, who wouldn't know about the type of the
exception.

[Bill Hesse, 2013-09-23 17:15:25]

https://codereview.chromium.org/24276010/diff/1/pkg/scheduled_test/test/descr...
File pkg/scheduled_test/test/descriptor/async_test.dart (right):

https://codereview.chromium.org/24276010/diff/1/pkg/scheduled_test/test/descr...
pkg/scheduled_test/test/descriptor/async_test.dart:75:
expect(errors.first.error, new isInstanceOf<TestFailure>());
On 2013/09/20 21:45:41, nweiz wrote:
> The type of this exception isn't an important part of the interface. We don't
> expect users of the library to catch it, and it may change in the future. As
> such, we shouldn't test for it (here or elsewhere).

Done.

https://codereview.chromium.org/24276010/diff/1/pkg/scheduled_test/test/descr...
pkg/scheduled_test/test/descriptor/async_test.dart:76:
expect(errors.first.error.message,
On 2013/09/20 21:45:41, nweiz wrote:
> In keeping with the above, we should test [error.toString()] rather than
> [error.message].

Done.

https://codereview.chromium.org/24276010/diff/1/pkg/scheduled_test/test/descr...
File pkg/scheduled_test/test/descriptor/directory_test.dart (right):

https://codereview.chromium.org/24276010/diff/1/pkg/scheduled_test/test/descr...
pkg/scheduled_test/test/descriptor/directory_test.dart:130: // Error is
_DirectoryValidationError.
On 2013/09/20 21:45:41, nweiz wrote:
> I don't think these comments are useful. The tests are written from the
> perspective of a user of the library, who wouldn't know about the type of the
> exception.

Done.

https://codereview.chromium.org/24276010/diff/7001/pkg/scheduled_test/test/de...
File pkg/scheduled_test/test/descriptor/async_test.dart (right):

https://codereview.chromium.org/24276010/diff/7001/pkg/scheduled_test/test/de...
pkg/scheduled_test/test/descriptor/async_test.dart:77: });
Should we even just drop the length check, and write

errors.single.error.toString(), matches..

or even

(errors.single as ScheduleError).error.toString(), matches...
?
Maybe the separate checks are clearer.

[nweiz, 2013-09-23 22:14:34]

lgtm

https://codereview.chromium.org/24276010/diff/7001/pkg/scheduled_test/test/de...
File pkg/scheduled_test/test/descriptor/async_test.dart (right):

https://codereview.chromium.org/24276010/diff/7001/pkg/scheduled_test/test/de...
pkg/scheduled_test/test/descriptor/async_test.dart:77: });
On 2013/09/23 17:15:25, Bill Hesse wrote:
> Should we even just drop the length check, and write
> 
> errors.single.error.toString(), matches..
> 
> or even
> 
> (errors.single as ScheduleError).error.toString(), matches...
> ?
> Maybe the separate checks are clearer.

I like this idea (probably with two checks rather than "as ScheduleError"). You
can make that change or not, as you prefer.

[Bill Hesse, 2013-09-23 23:31:31]

Committed patchset #4 manually as r27787 (presubmit successful).

[nweiz, 2013-09-24 01:47:16]

lgtm

[nweiz, 2013-09-24 01:47:30]

On 2013/09/24 01:47:16, nweiz wrote:
> lgtm

Whoops, that was supposed to be on https://codereview.chromium.org/23447054/.