unittest: refactor ahead of matcher changes

Refactor with matcher

R=nweiz@google.com

Committed: https://github.com/dart-lang/unittest/commit/36c796b2d0c99ba0056a4d83a60c129b5a5f08e8

[kevmoo, 2015-01-26 20:40:00]

kevmoo@google.com changed reviewers:
+ nweiz@google.com

[nweiz, 2015-01-26 22:44:10]

https://codereview.chromium.org/869053002/diff/20001/CHANGELOG.md
File CHANGELOG.md (right):

https://codereview.chromium.org/869053002/diff/20001/CHANGELOG.md#newcode4
CHANGELOG.md:4: `configureExpectFailureHandler`, and
`getOrCreateExpectFailureHandler`. These
Indent +2 here and below.

https://codereview.chromium.org/869053002/diff/20001/CHANGELOG.md#newcode5
CHANGELOG.md:5: are no longer needed.
Explain why they're no longer needed. Imagine you're talking to a user who was
using them previously.

https://codereview.chromium.org/869053002/diff/20001/CHANGELOG.md#newcode7
CHANGELOG.md:7: * Moved a number of APIs from `matcher`, including:
"from `matcher`" -> "from `matcher` into `unittest`"

Remove ":"

https://codereview.chromium.org/869053002/diff/20001/CHANGELOG.md#newcode8
CHANGELOG.md:8: `completes`,
`completion`,`ErrorFormatter`,`expect`,`fail`,`prints`,
Add spaces after commas.

https://codereview.chromium.org/869053002/diff/20001/CHANGELOG.md#newcode13
CHANGELOG.md:13: * `expect` added an optional `ErrorFormatter` argument.
Including the name of the argument will be more useful than the type name.

https://codereview.chromium.org/869053002/diff/20001/lib/src/expect.dart
File lib/src/expect.dart (right):

https://codereview.chromium.org/869053002/diff/20001/lib/src/expect.dart#newc...
lib/src/expect.dart:1: // Copyright (c) 2012, the Dart project authors.  Please
see the AUTHORS file
Use the current year for copyright headers, even for mostly copy/pasted files.

https://codereview.chromium.org/869053002/diff/20001/lib/src/expect.dart#newc...
lib/src/expect.dart:9: /// The objects thrown by the default failure handler.
You removed any concept of a "failure handler".

I'd say "An exception thrown when a test assertion fails."

https://codereview.chromium.org/869053002/diff/20001/lib/src/expect.dart#newc...
lib/src/expect.dart:10: class TestFailure extends Error {
I don't think this should extend Error. It's not a "program failure that the
programmer should have avoided".

https://codereview.chromium.org/869053002/diff/20001/lib/src/expect.dart#newc...
lib/src/expect.dart:19: /// failures in [expect].
"The used" -> "The type used"

It seems very strange to have this tied to [expect] rather than the
configuration, but that's something we can deal with later.

https://codereview.chromium.org/869053002/diff/20001/lib/src/expect.dart#newc...
lib/src/expect.dart:26: /// not supplied, as a reason is generated from the
matcher; if [reason]
"the matcher" -> "[matcher]"

https://codereview.chromium.org/869053002/diff/20001/lib/src/expect.dart#newc...
lib/src/expect.dart:36: /// [verbose] should be specified as true;
"specified as true;" -> "`true`.".

https://codereview.chromium.org/869053002/diff/20001/lib/src/expect.dart#newc...
lib/src/expect.dart:56: // The default error formatter implementation.
"formatter implementation." -> "formatter."

https://codereview.chromium.org/869053002/diff/20001/lib/src/future_matchers....
File lib/src/future_matchers.dart (right):

https://codereview.chromium.org/869053002/diff/20001/lib/src/future_matchers....
lib/src/future_matchers.dart:34: /// in error messages.
While we're making backwards-incompatible changes, let's change this to a named
argument called [description].

https://codereview.chromium.org/869053002/diff/20001/lib/src/simple_configura...
File lib/src/simple_configuration.dart (right):

https://codereview.chromium.org/869053002/diff/20001/lib/src/simple_configura...
lib/src/simple_configuration.dart:5: library unittest.simple_configuration;
I don't understand the changes to this code or why they're in this CL.

https://codereview.chromium.org/869053002/diff/20001/lib/src/throws_matcher.dart
File lib/src/throws_matcher.dart (right):

https://codereview.chromium.org/869053002/diff/20001/lib/src/throws_matcher.d...
lib/src/throws_matcher.dart:15: ///   * A [Function] that throws an exception
when called. The function cannot
Why are the "*"s indented here?

https://codereview.chromium.org/869053002/diff/20001/lib/src/throws_matcher.d...
lib/src/throws_matcher.dart:38: /// In both cases, when an exception is thrown,
this will test that the exception
Long line.

https://codereview.chromium.org/869053002/diff/20001/lib/unittest.dart
File lib/unittest.dart (right):

https://codereview.chromium.org/869053002/diff/20001/lib/unittest.dart#newcod...
lib/unittest.dart:503: /// This Function is used by certain Matchers to catch
certain exceptions.
"Function" -> "function"

https://codereview.chromium.org/869053002/diff/20001/lib/unittest.dart#newcod...
lib/unittest.dart:512: Function wrapAsync = (Function f, [id]) => f;
Since this is now defined in unittest, why doesn't it default to expectAsync?

I'd be okay with getting rid of this entirely, actually.

https://codereview.chromium.org/869053002/diff/20001/test/prints_matcher_test...
File test/prints_matcher_test.dart (right):

https://codereview.chromium.org/869053002/diff/20001/test/prints_matcher_test...
test/prints_matcher_test.dart:120: r'''Expected future to complete successfully,
but it failed with Expected: 'Goodbye, world!\n'
Long lines.

[kevmoo, 2015-01-27 00:11:30]

PTAL

https://codereview.chromium.org/869053002/diff/20001/CHANGELOG.md
File CHANGELOG.md (right):

https://codereview.chromium.org/869053002/diff/20001/CHANGELOG.md#newcode4
CHANGELOG.md:4: `configureExpectFailureHandler`, and
`getOrCreateExpectFailureHandler`. These
On 2015/01/26 22:44:09, nweiz wrote:
> Indent +2 here and below.

Done.

https://codereview.chromium.org/869053002/diff/20001/CHANGELOG.md#newcode5
CHANGELOG.md:5: are no longer needed.
On 2015/01/26 22:44:09, nweiz wrote:
> Explain why they're no longer needed. Imagine you're talking to a user who was
> using them previously.

Done.

https://codereview.chromium.org/869053002/diff/20001/CHANGELOG.md#newcode7
CHANGELOG.md:7: * Moved a number of APIs from `matcher`, including:
On 2015/01/26 22:44:09, nweiz wrote:
> "from `matcher`" -> "from `matcher` into `unittest`"
> 
> Remove ":"

Done.

https://codereview.chromium.org/869053002/diff/20001/CHANGELOG.md#newcode8
CHANGELOG.md:8: `completes`,
`completion`,`ErrorFormatter`,`expect`,`fail`,`prints`,
On 2015/01/26 22:44:09, nweiz wrote:
> Add spaces after commas.

Done.

https://codereview.chromium.org/869053002/diff/20001/CHANGELOG.md#newcode13
CHANGELOG.md:13: * `expect` added an optional `ErrorFormatter` argument.
On 2015/01/26 22:44:09, nweiz wrote:
> Including the name of the argument will be more useful than the type name.

Done.

https://codereview.chromium.org/869053002/diff/20001/lib/src/expect.dart
File lib/src/expect.dart (right):

https://codereview.chromium.org/869053002/diff/20001/lib/src/expect.dart#newc...
lib/src/expect.dart:1: // Copyright (c) 2012, the Dart project authors.  Please
see the AUTHORS file
On 2015/01/26 22:44:09, nweiz wrote:
> Use the current year for copyright headers, even for mostly copy/pasted files.

Done.

https://codereview.chromium.org/869053002/diff/20001/lib/src/expect.dart#newc...
lib/src/expect.dart:9: /// The objects thrown by the default failure handler.
On 2015/01/26 22:44:09, nweiz wrote:
> You removed any concept of a "failure handler".
> 
> I'd say "An exception thrown when a test assertion fails."

Done.

https://codereview.chromium.org/869053002/diff/20001/lib/src/expect.dart#newc...
lib/src/expect.dart:10: class TestFailure extends Error {
On 2015/01/26 22:44:09, nweiz wrote:
> I don't think this should extend Error. It's not a "program failure that the
> programmer should have avoided".

Done.

https://codereview.chromium.org/869053002/diff/20001/lib/src/expect.dart#newc...
lib/src/expect.dart:19: /// failures in [expect].
On 2015/01/26 22:44:09, nweiz wrote:
> "The used" -> "The type used"
> 
> It seems very strange to have this tied to [expect] rather than the
> configuration, but that's something we can deal with later.

Done.

https://codereview.chromium.org/869053002/diff/20001/lib/src/expect.dart#newc...
lib/src/expect.dart:26: /// not supplied, as a reason is generated from the
matcher; if [reason]
On 2015/01/26 22:44:09, nweiz wrote:
> "the matcher" -> "[matcher]"

Done.

https://codereview.chromium.org/869053002/diff/20001/lib/src/expect.dart#newc...
lib/src/expect.dart:36: /// [verbose] should be specified as true;
On 2015/01/26 22:44:09, nweiz wrote:
> "specified as true;" -> "`true`.".

Done.

https://codereview.chromium.org/869053002/diff/20001/lib/src/expect.dart#newc...
lib/src/expect.dart:56: // The default error formatter implementation.
On 2015/01/26 22:44:09, nweiz wrote:
> "formatter implementation." -> "formatter."

Done.

https://codereview.chromium.org/869053002/diff/20001/lib/src/future_matchers....
File lib/src/future_matchers.dart (right):

https://codereview.chromium.org/869053002/diff/20001/lib/src/future_matchers....
lib/src/future_matchers.dart:34: /// in error messages.
On 2015/01/26 22:44:09, nweiz wrote:
> While we're making backwards-incompatible changes, let's change this to a
named
> argument called [description].

Done.

https://codereview.chromium.org/869053002/diff/20001/lib/src/simple_configura...
File lib/src/simple_configuration.dart (right):

https://codereview.chromium.org/869053002/diff/20001/lib/src/simple_configura...
lib/src/simple_configuration.dart:5: library unittest.simple_configuration;
On 2015/01/26 22:44:10, nweiz wrote:
> I don't understand the changes to this code or why they're in this CL.

Done.

https://codereview.chromium.org/869053002/diff/20001/lib/src/throws_matcher.dart
File lib/src/throws_matcher.dart (right):

https://codereview.chromium.org/869053002/diff/20001/lib/src/throws_matcher.d...
lib/src/throws_matcher.dart:15: ///   * A [Function] that throws an exception
when called. The function cannot
On 2015/01/26 22:44:10, nweiz wrote:
> Why are the "*"s indented here?

No clue. copy-paste. Fixed

https://codereview.chromium.org/869053002/diff/20001/lib/src/throws_matcher.d...
lib/src/throws_matcher.dart:38: /// In both cases, when an exception is thrown,
this will test that the exception
On 2015/01/26 22:44:10, nweiz wrote:
> Long line.

Done.

https://codereview.chromium.org/869053002/diff/20001/lib/unittest.dart
File lib/unittest.dart (right):

https://codereview.chromium.org/869053002/diff/20001/lib/unittest.dart#newcod...
lib/unittest.dart:503: /// This Function is used by certain Matchers to catch
certain exceptions.
On 2015/01/26 22:44:10, nweiz wrote:
> "Function" -> "function"

Done.

https://codereview.chromium.org/869053002/diff/20001/lib/unittest.dart#newcod...
lib/unittest.dart:512: Function wrapAsync = (Function f, [id]) => f;
On 2015/01/26 22:44:10, nweiz wrote:
> Since this is now defined in unittest, why doesn't it default to expectAsync?
> 
> I'd be okay with getting rid of this entirely, actually.

Done.

https://codereview.chromium.org/869053002/diff/20001/test/prints_matcher_test...
File test/prints_matcher_test.dart (right):

https://codereview.chromium.org/869053002/diff/20001/test/prints_matcher_test...
test/prints_matcher_test.dart:120: r'''Expected future to complete successfully,
but it failed with Expected: 'Goodbye, world!\n'
On 2015/01/26 22:44:10, nweiz wrote:
> Long lines.

These are long string literals. To make it easy to track changes in the printed
output, I'd like to keep them as blocks

[nweiz, 2015-01-27 00:24:56]

https://codereview.chromium.org/869053002/diff/20001/lib/src/future_matchers....
File lib/src/future_matchers.dart (right):

https://codereview.chromium.org/869053002/diff/20001/lib/src/future_matchers....
lib/src/future_matchers.dart:34: /// in error messages.
On 2015/01/27 00:11:30, kevmoo wrote:
> On 2015/01/26 22:44:09, nweiz wrote:
> > While we're making backwards-incompatible changes, let's change this to a
> named
> > argument called [description].
> 
> Done.

This should be named.

https://codereview.chromium.org/869053002/diff/40001/lib/src/expect.dart
File lib/src/expect.dart (right):

https://codereview.chromium.org/869053002/diff/40001/lib/src/expect.dart#newc...
lib/src/expect.dart:18: /// The type type used for functions that can be used to
build up error reports
"type type" -> "type"

[nweiz, 2015-01-27 00:25:05]

lgtm

[kevmoo, 2015-01-27 00:42:11]

https://codereview.chromium.org/869053002/diff/40001/lib/src/expect.dart
File lib/src/expect.dart (right):

https://codereview.chromium.org/869053002/diff/40001/lib/src/expect.dart#newc...
lib/src/expect.dart:18: /// The type type used for functions that can be used to
build up error reports
On 2015/01/27 00:24:56, nweiz wrote:
> "type type" -> "type"

Done.

[kevmoo, 2015-01-27 00:42:27]

Committed patchset #4 (id:60001) manually as
36c796b2d0c99ba0056a4d83a60c129b5a5f08e8 (presubmit successful).