New expectation functions plus convert old tests to use these.

New expectation functions plus convert old tests to use these.
Committed: https://code.google.com/p/dart/source/detail?r=8355

[gram, 2012-05-30 20:45:22]

May as well take advantage of the pain meds...

[Bob Nystrom, 2012-05-30 23:23:51]

This has to be a record for number of comments on a single patch.

I really like this new expectation stuff. Most of my comments are about style
stuff, which you will internalize quickly. Some are about getting rid of members
that I think might not be worth their weight.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
File lib/unittest/collection_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:5: // Collection matchers match against a
collection. We add this intermediate
Make this a doc comment for the class:

/**
 * Collection matchers...
 */

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:10: if (!(item is Collection)) {
if (item is !Collection)

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:19:
//-----------------------------------------------------------
I don't think we need a comments for structure. Whitespace is usually
sufficient. Here and elsewhere.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:21: * This returns a matcher that matches
empty collections.
"This returns" -> "Returns"

Here and elsewhere. What you have isn't wrong, but I think we usually tend to
word things like "Returns..."

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:23: IMatcher emptyCollection() => new
_EmptyCollection();
Make this a variable so you don't need the ():

final emptyCollection = const _EmptyCollection();

then:

expect(foo, emptyCollection);

Also, rename to "isEmpty" or "isEmptyCollection"?

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:26: bool matches(collection) {
Annotate this to type Collection?

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:29: } catch (var e) {
Why are you swallowing exceptions here?

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:40: * This returns a matcher that matches
collection that contains [obj].
"collection" -> "a collection"

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:46: var _val;
"_val" -> "_value"

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:52: return item.some( (e) => e == _val );
Space like:

return item.some((e) => e == _val);

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:57: IDescription describe(IDescription
description) =>
Maybe a blank line before this one?

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:71: _EveryElement(Matcher this._matcher);
We don't type annotate "this." constructor params since their type is obvious
from the field.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:75: return item.every( (e) =>
_matcher.matches(e) );
No spaces around lambda fn.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:115: IMatcher orderedEquals(expected) =>
new _OrderedEquals(new List.from(expected));
Why new List.from() here?

Can "expected" be annotated to Iterable?

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:124: List actual = new List.from(item);
"List" -> "var".

We rarely annotate locals.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:125: if (_expected.length ==
actual.length) {
It would be nice if this worked with any Iterable instead of requiring .length.
Can you do this just manually iterating both in stepwise fashion?

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:149: IMatcher unorderedEquals(expected) =>
Can this be type annotated?

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:150: new _UnorderedEquals(new
Set.from(expected));
Does this mean all items in expected must be hashable? If so, we should document
that.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:159: Set items = new Set.from(item);
"Set" -> "var".

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
File lib/unittest/core_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:6: IMatcher isNull() => new _IsNull();
Make it a var:

final isNull = const _IsNull();

Pretty much any matcher that doesn't take args can (and should, I think) be a
constant like this.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:12: bool matches(item) => (item == null);
Remove parens:

 bool matches(item) => item == null;

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:23: 
Delete this blank line.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:24: bool matches(item) => (item == true);
Remove unneeded parens.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:29: /** Returns a matches that matches if the
value is the same instance
Can you format this with the text on the next line? We don't seem to often put
text on the first line of a doc comment. Also, subsequent lines should only have
one space:

/**
 * Returns...
 * as [
 */

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:30: * as [object] (===).
Put === in backticks to code format it: as [object] (`===`).

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:32: IMatcher same(object) => new
_IsSame(object);
"isSameAs"?

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:36: var _object;
Other matcher classes use "_val" for this. Should pick a name and use it
consistently.

Also, it can be made final. We tend to make fields final whenever possible.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:40: bool matches(item) => (item === _object);
Unnecessary ().

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:42: // If all types were hashable we could show
a hash here
True. :(

Add a "." at the end of the sentence.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:58: //if (_object is IMatcher) {
Remove commented out code.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:69: IMatcher anything([description =
'anything']) => new _IsAnything(description);
Can we get rid of description and make this a constant? expect() has a reason
param, so it seems redundant here.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:89: IMatcher throwsException() => new _Throws();
"throwsException" -> "throws"?

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:101: /**
Add blank line above comment.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:108: IMatcher returnsNormally() => new
_ReturnsNormally();
Maybe I don't the intent here, but it seems to me that all expectations are
implicitly "returns normally", so I don't think we need an expectation that
calls this out. Am I missing something?

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:114: if (_matcher != null && !(_matcher is
Matcher)) {
You can delete this. Dart will validate this for you by virtue of the type
annotation in checked mode.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:132: return description.append('throws an
exception which needs to match ').
"which needs to match" -> "which matches"?

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:166: * of a (super)[type].
This is worded a bit confusingly. Maybe:

"if an object is an instance of [type] (or a subtype)."

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:169: * approximate this test, by using a generic
wrapper named Type.
Delete ","

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:183: class Type<T> {
How about making this implement IMatcher directly so you can do:

expect(bar, new InstanceOf<Foo>());

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:192: Type _expected_type;
"_expectedType"

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:202: // The description here is lame :-(
Why?

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:204: description.append('an instance of
${_expected_type.toString()}');
You don't need .toString() here. Interpolation does that implicitly.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:209: * that matches [matcher].
This doesn't seem to add a lot of value. Why not just do:

expect(foo.length, equals(123));

?

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:219: var _len_matcher;
"_lenMatcher"

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:254: IMatcher hasString(matcher) => new
_HasString(wrapMatcher(matcher));
This name is confusing to me, and it doesn't seem that useful. Can we just
remove this?

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:270: * with built-in types. Note that cyclic
structures will never terminate!
Wouldn't this also work with user-defined types that implement Collection or
Map?

Also, non-termination in tests seems bad. Can you make it handle cycles
gracefully? If I remember right, the toString() implementations for List and Map
do, so you could borrow code from that.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:289: 'at position ${i} ${location}');
Indent another 2 spaces. Continued lines are indented at least +4.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:296: if (!(actual is Map)) {
actual is !Map

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/description.dart
File lib/unittest/description.dart (right):

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/description.da...
lib/unittest/description.dart:10: 
Remove blank line.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/description.da...
lib/unittest/description.dart:22: 
Remove blank line, here and elsewhere.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/description.da...
lib/unittest/description.dart:23: IDescription append(text) {
"add" to be consistent with List and StringBuffer.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/description.da...
lib/unittest/description.dart:29: * Append a description of given [value]. If it
is a string just
"Append" -> "appends". Remove "given".

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/description.da...
lib/unittest/description.dart:30: * append it with some control characters
escaped. Else try
This sentence needs some grammar work.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/description.da...
lib/unittest/description.dart:37: value.describe(this);
Instead of just trying to call describe(), why not check for is IDescription?

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart
File lib/unittest/expect.dart (right):

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart#ne...
lib/unittest/expect.dart:8: *   expect(value, matcher, reason)
Do five spaces here and elsewhere. The first one is for the doc comment, and the
other four tell markdown this is a code block.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart#ne...
lib/unittest/expect.dart:18: * the matcher [arg2] for the reason [arg3]. In the
second form expect()
Do we use the second form anywhere? If not, we could remove it and just require:

    expect(foo, isTrue);

That would let us give nice types for expect(). arg1 is not that user-friendly.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart#ne...
lib/unittest/expect.dart:22: * ExpectException, but this behavior can be changed
by calling
Put square brackets around ExpectException, configureExpectHandler and
IFailureHandler.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart#ne...
lib/unittest/expect.dart:26: * expect is a 3rd generation assertion mechanism,
drawing
`expect()`

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart#ne...
lib/unittest/expect.dart:27: * inspiration from Hamcrest and Ladislav Thon's
matcher
Make these markdown links:

[Hamcrest][] and Ladislav Thon's [dart-matchers][]...

[hamcrest]: http://http://code.google.com/p/hamcrest/
[dart-matchers]: https://github.com/Ladicek/dart-matchers

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart#ne...
lib/unittest/expect.dart:43: // _assertMatch handles expect() calls that use a
matcher.
This comment is probably not necessary.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart#ne...
lib/unittest/expect.dart:56: void _assertBool(assertion, [String reason=null]) {
Type annotate assertion.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart#ne...
lib/unittest/expect.dart:67: // wrapMatcher takes an argument and returns a
equivalent matcher.
"Takes an argument and returns an equivalent matcher."

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart#ne...
lib/unittest/expect.dart:69: // generates a equals matcher for the argument.
"a equals" -> "an equals"

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart#ne...
lib/unittest/expect.dart:70: IMatcher wrapMatcher(x) => ((x is IMatcher) ? x :
equals(x));
What is this used for?

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart#ne...
lib/unittest/expect.dart:73: // The reference to the failure handler for failed
asserts
This comment doesn't add much.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart#ne...
lib/unittest/expect.dart:89: * IFailureHandler); if this is omitted or null then
the failure handler
Remove "(that must implement IFailureHandler)" That's what type annotations are
for. :)

Also ";" -> "."

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart#ne...
lib/unittest/expect.dart:90: * is reset to the default, which throws
ExpectExceptions on expect()
Square brackets around [ExpectException]s and [expect()].

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart#ne...
lib/unittest/expect.dart:93: void configureExpectHandler([IFailureHandler
handler = null]) {
Why make this optional?

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart#ne...
lib/unittest/expect.dart:102: IErrorFormatter _assertErrorFormatter = null;
This comment doesn't add much.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart#ne...
lib/unittest/expect.dart:109: append('\nExpected: ').
Indent these +4.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart#ne...
lib/unittest/expect.dart:120: * [formatter] is a reference to the new formatter
(that must implement
Remove parenthetical.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/interfaces.dart
File lib/unittest/interfaces.dart (right):

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/interfaces.dar...
lib/unittest/interfaces.dart:10: * can be used to build up error reports upon
expect() failures.
Square brackets around [expect()], [StringDescription].

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/interfaces.dar...
lib/unittest/interfaces.dart:18: String format(actual, IMatcher matcher, String
reason);
Should this just be a function type? Why make it an interface?

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/interfaces.dar...
lib/unittest/interfaces.dart:29: IDescription append(text);
"append" -> "add"

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/interfaces.dar...
lib/unittest/interfaces.dart:31: IDescription appendList(start, separator, end,
list);
Can these be typed?

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/interfaces.dar...
lib/unittest/interfaces.dart:36: * are three methods: match, which does the
matching;
Instead of describing the methods here, just put comments with the methods.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/map_matchers.dart
File lib/unittest/map_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/map_matchers.d...
lib/unittest/map_matchers.dart:8: IMatcher emptyMap() => new _EmptyMap();
Can we just have a single isEmpty matcher? Why also test the type?

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/map_matchers.d...
lib/unittest/map_matchers.dart:26: bool matches(item) => item is Map &&
item.containsKey(_key);
Does it add value to test the type here, or should this work with anything that
has a containsKey method?

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/map_matchers.d...
lib/unittest/map_matchers.dart:65: IDescription describe(IDescription
description) =>
I would use a full {} body for this since it's multiple lines.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/map_matchers.d...
lib/unittest/map_matchers.dart:75: appendDescriptionOf(item).
Indent another 2.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/map_matchers.d...
lib/unittest/map_matchers.dart:80: append(' contains key ').
Ditto.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/map_matchers.d...
lib/unittest/map_matchers.dart:97: IMatcher mapLength(matcher) => new
_MapLength(wrapMatcher(matcher));
This feels unnecessary to me. Can we remove it?

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/map_matchers.d...
lib/unittest/map_matchers.dart:111: // Map matchers match against a Map. We add
this intermediate
Make this a doc comment.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/matcher.dart
File lib/unittest/matcher.dart (right):

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/matcher.dart#n...
lib/unittest/matcher.dart:13: class Matcher implements IMatcher {
I would get rid of the separate interface and just keep this class. Most users
will subclass. If they don't want to inherit the implementation, they can always
implement the class's implicit interface.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/matcher.dart#n...
lib/unittest/matcher.dart:19: bool matches(item) {
Just make this abstract.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/matcher.dart#n...
lib/unittest/matcher.dart:27: IDescription describe(IDescription
mismatchDescription) {
This too.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/matcher.dart#n...
lib/unittest/matcher.dart:32: * This generates a description of the matcher
failed for a particular
"Generates a description of why the matcher failed..."

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/numeric_matche...
File lib/unittest/numeric_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/numeric_matche...
lib/unittest/numeric_matchers.dart:30: IMatcher lessThanOrEqualTo(value) =>
Is this really better than doing:

expect(foo > bar)

?

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/numeric_matche...
lib/unittest/numeric_matchers.dart:36: IMatcher isZero() {
Make these constants.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/numeric_matche...
lib/unittest/numeric_matchers.dart:64: return value is int || value is double;
"is num"

But I would just get rid of this. You can always do:

expect(foo is num)

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/numeric_matche...
lib/unittest/numeric_matchers.dart:69: var _value; // Expected value.
Make these doc comments. Make the fields final.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/numeric_matche...
lib/unittest/numeric_matchers.dart:78: bool this._equalValue,
Remove these type annotations.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/numeric_matche...
lib/unittest/numeric_matchers.dart:90: if (!_isNumeric(item)) {
Should we allow this on user-defined types that implement comparison operators?

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/numeric_matche...
lib/unittest/numeric_matchers.dart:102: IDescription describe(IDescription
description) =>
I would use a {} body here.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/numeric_matche...
lib/unittest/numeric_matchers.dart:134: if (diff < 0) diff = -diff;
var diff = Math.abs(item - _value);

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/numeric_matche...
lib/unittest/numeric_matchers.dart:160: * than or equal to a [low] and less than
or equal to [high].
"to a" -> "to"

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/numeric_matche...
lib/unittest/numeric_matchers.dart:168: IMatcher inExclusiveRange(low, high) =>
new _InRange(low, high, true, true);
Add a blank line after each member.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/numeric_matche...
lib/unittest/numeric_matchers.dart:187: if (!_isNumeric(_low) ||
!_isNumeric(_high)) {
Delete these checks. That's what checked mode is for.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/numeric_matche...
lib/unittest/numeric_matchers.dart:214: "$_low
(${_lowExclusive?'exclusive':'inclusive'}) to "
Spaces around ? and :

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/operator_match...
File lib/unittest/operator_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/operator_match...
lib/unittest/operator_matchers.dart:24: * arguments (up to 7) match. Instead of
passing the matchers separately
Why 7?

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/string_matcher...
File lib/unittest/string_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/string_matcher...
lib/unittest/string_matchers.dart:6: IMatcher isEmptyString() => new _Empty();
Does this add a lot of value over just doing:

expect(foo, equals(''))

?

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/string_matcher...
lib/unittest/string_matchers.dart:28: IMatcher isInterpolated(matcher) => new
_IsInterpolated(wrapMatcher(matcher));
I think this is equivalent to toString(). Do we need this?

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/string_matcher...
lib/unittest/string_matchers.dart:49: IMatcher equalToIgnoringCase(String value)
=> new _IsEqualIgnoringCase(value);
"equalTo" -> "equals"

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/string_matcher...
lib/unittest/string_matchers.dart:57: if (!(_value is String)) {
Remove this check.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/string_matcher...
lib/unittest/string_matchers.dart:71: // Utility functions
Remove this comment?

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/string_matcher...
lib/unittest/string_matchers.dart:73: bool isWhiteSpace(String ch) => ('
\n\r\t'.indexOf(ch) >= 0);
There is some debate here, but I think this should be "isWhitespace". At least
in the context of programming, I usually see whitespace as a single word. Also,
make private?

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/string_matcher...
lib/unittest/string_matchers.dart:75: String _stripspace(_string) {
"_normalizeWhitespace"?

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/string_matcher...
lib/unittest/string_matchers.dart:95: * is equal to [value] when compared with
all runs of white space
"whitespace"

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/string_matcher...
lib/unittest/string_matchers.dart:98: * For example, equalToIgnoringCase("hello
world") will match
Put inline code in backticks: `equalToIgnoringWhitespace("hello world")`.

Also, "Case" -> "Whitespace".

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/string_matcher...
lib/unittest/string_matchers.dart:101: IMatcher
equalToIgnoringWhitespace(_string) =>
"equalTo" -> "equals"

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/string_matcher...
lib/unittest/string_matchers.dart:110: if (!(_value is String)) {
Remove check.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/string_matcher...
lib/unittest/string_matchers.dart:117: bool matches(item) => (item is String &&
_matchValue == _stripspace(item));
Unneeded ().

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/string_matcher...
lib/unittest/string_matchers.dart:120:
description.appendDescriptionOf(_matchValue).
I would use a {} here. If not, +2 indent.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/string_matcher...
lib/unittest/string_matchers.dart:170: IllegalArgumentException('endsWith
requires a string');
Indent +2.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/string_matcher...
lib/unittest/string_matchers.dart:185: IMatcher containsString(substring) => new
_StringContains(substring);
Can we do a single unified contains() that works with collections and strings?

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/string_matcher...
lib/unittest/string_matchers.dart:273: IDescription  describe(IDescription
description) =>
Extra space.

http://codereview.chromium.org/10441104/diff/1044/tests/html/history_test.dart
File tests/html/history_test.dart (right):

http://codereview.chromium.org/10441104/diff/1044/tests/html/history_test.dar...
tests/html/history_test.dart:12:
expect(window.location.href.endsWith('foo=bar'), isTrue());
Use endsWith matcher here?

http://codereview.chromium.org/10441104/diff/1044/tests/html/history_test.dar...
tests/html/history_test.dart:16:
expect(window.location.href.endsWith('foo=baz'), isTrue());
And here?

http://codereview.chromium.org/10441104/diff/1044/tests/json/json_test.dart
File tests/json/json_test.dart (right):

http://codereview.chromium.org/10441104/diff/1044/tests/json/json_test.dart#n...
tests/json/json_test.dart:66: allOf([mapContains('w', null), mapLength(1)]));
Use recursivelyMatches here?

http://codereview.chromium.org/10441104/diff/1044/tests/lib/args/args_test.dart
File tests/lib/args/args_test.dart (right):

http://codereview.chromium.org/10441104/diff/1044/tests/lib/args/args_test.da...
tests/lib/args/args_test.dart:542: throwsIllegalArg(function) {
What do you think about making this a matcher and moving it into unittest? I
find myself using this all the time.

http://codereview.chromium.org/10441104/diff/1044/tests/utils/dartdoc_test.dart
File tests/utils/dartdoc_test.dart (right):

http://codereview.chromium.org/10441104/diff/1044/tests/utils/dartdoc_test.da...
tests/utils/dartdoc_test.dart:109: expect(doc.relativePath('other/file.html'),
equalsTo(
"equalsTo"?

http://codereview.chromium.org/10441104/diff/1044/tests/utils/dartdoc_test.da...
tests/utils/dartdoc_test.dart:115:
expect(doc.relativePath('other/sub/file.html'), equalsTo(
Ditto.

http://codereview.chromium.org/10441104/diff/1044/tests/utils/unittest_test.dart
File tests/utils/unittest_test.dart (right):

http://codereview.chromium.org/10441104/diff/1044/tests/utils/unittest_test.d...
tests/utils/unittest_test.dart:2: // for details. All rights reserved. Use of
this source code is governed by a
Yay unit tests for unittest! This is awesome.

http://codereview.chromium.org/10441104/diff/1044/tests/utils/unittest_test.d...
tests/utils/unittest_test.dart:5: #library('unittestTest');
unittest_test

http://codereview.chromium.org/10441104/diff/1044/tests/utils/unittest_test.d...
tests/utils/unittest_test.dart:10: foo() {}
"doesNotThrow"

http://codereview.chromium.org/10441104/diff/1044/tests/utils/unittest_test.d...
tests/utils/unittest_test.dart:11: bar() { throw 'X'; }
"doesThrow"

http://codereview.chromium.org/10441104/diff/1044/tests/utils/unittest_test.d...
tests/utils/unittest_test.dart:15: var myhandler;
myHandler

Or, ideally, a more meaningful name.

http://codereview.chromium.org/10441104/diff/1044/tests/utils/unittest_test.d...
tests/utils/unittest_test.dart:26: f = new DefaultErrorFormatter();
Put all on one line or use {}.

http://codereview.chromium.org/10441104/diff/1044/tests/utils/unittest_test.d...
tests/utils/unittest_test.dart:54: // Core matchers
Instead of comments, use group() and test(). That way the structure shows up in
the test results and it's easier to track down failures.

http://codereview.chromium.org/10441104/diff/1044/tests/utils/unittest_test.d...
tests/utils/unittest_test.dart:89: "Expected: throws an exception but: no
exception");
Indent another 2, here and elsewhere.

http://codereview.chromium.org/10441104/diff/1044/tests/utils/unittest_test.d...
tests/utils/unittest_test.dart:270: shouldPass('goodbye cruel world',
stringContainsInOrder(['goodbye', 'cruel']));
Long line.

http://codereview.chromium.org/10441104/diff/1044/tests/utils/unittest_test.d...
tests/utils/unittest_test.dart:291: var d = [ 1, 2 ];
No spaces after [ or before ]

[gram, 2012-06-01 17:33:15]

PTAL, thanks!

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
File lib/unittest/collection_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:5: // Collection matchers match against a
collection. We add this intermediate
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Make this a doc comment for the class:
> 
> /**
>  * Collection matchers...
>  */

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:10: if (!(item is Collection)) {
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> if (item is !Collection)

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:19:
//-----------------------------------------------------------
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> I don't think we need a comments for structure. Whitespace is usually
> sufficient. Here and elsewhere.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:21: * This returns a matcher that matches
empty collections.
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> "This returns" -> "Returns"
> 
> Here and elsewhere. What you have isn't wrong, but I think we usually tend to
> word things like "Returns..."

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:23: IMatcher emptyCollection() => new
_EmptyCollection();
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Make this a variable so you don't need the ():
> 
> final emptyCollection = const _EmptyCollection();
> 
> then:
> 
> expect(foo, emptyCollection);
> 
> Also, rename to "isEmpty" or "isEmptyCollection"?
> 

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:26: bool matches(collection) {
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Annotate this to type Collection?

None of the matches() methods are typed, as they are implementations of an
interface method that is untyped.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:29: } catch (var e) {
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Why are you swallowing exceptions here?

The idea is that you can pass anything in, and that part of the matching process
is to make sure that what you passed in was sane. That is why the exception is
caught and returns false.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:40: * This returns a matcher that matches
collection that contains [obj].
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> "collection" -> "a collection"

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:46: var _val;
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> "_val" -> "_value"

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:52: return item.some( (e) => e == _val );
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Space like:
> 
> return item.some((e) => e == _val);

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:57: IDescription describe(IDescription
description) =>
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Maybe a blank line before this one?

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:71: _EveryElement(Matcher this._matcher);
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> We don't type annotate "this." constructor params since their type is obvious
> from the field.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:75: return item.every( (e) =>
_matcher.matches(e) );
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> No spaces around lambda fn.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:115: IMatcher orderedEquals(expected) =>
new _OrderedEquals(new List.from(expected));
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Why new List.from() here?
> 
> Can "expected" be annotated to Iterable?

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:115: IMatcher orderedEquals(expected) =>
new _OrderedEquals(new List.from(expected));
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Why new List.from() here?
> 
> Can "expected" be annotated to Iterable?

We need at least one of the two to be indexable. I have changed the code so the
item to be tested can be iterable and we don't make a list from it, but the
expected value is still turned into a list.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:124: List actual = new List.from(item);
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> "List" -> "var".
> 
> We rarely annotate locals.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:125: if (_expected.length ==
actual.length) {
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> It would be nice if this worked with any Iterable instead of requiring
.length.
> Can you do this just manually iterating both in stepwise fashion?

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:149: IMatcher unorderedEquals(expected) =>
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Can this be type annotated?

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:150: new _UnorderedEquals(new
Set.from(expected));
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Does this mean all items in expected must be hashable? If so, we should
document
> that.

I rewrote it so that isn't necessary. It is O(n^2) but that may have been the
case before.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:159: Set items = new Set.from(item);
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> "Set" -> "var".

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
File lib/unittest/core_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:6: IMatcher isNull() => new _IsNull();
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Make it a var:
> 
> final isNull = const _IsNull();
> 
> Pretty much any matcher that doesn't take args can (and should, I think) be a
> constant like this.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:12: bool matches(item) => (item == null);
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Remove parens:
> 
>  bool matches(item) => item == null;

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:23: 
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Delete this blank line.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:24: bool matches(item) => (item == true);
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Remove unneeded parens.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:29: /** Returns a matches that matches if the
value is the same instance
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Can you format this with the text on the next line? We don't seem to often put
> text on the first line of a doc comment. Also, subsequent lines should only
have
> one space:
> 
> /**
>  * Returns...
>  * as [
>  */

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:30: * as [object] (===).
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Put === in backticks to code format it: as [object] (`===`).

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:32: IMatcher same(object) => new
_IsSame(object);
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> "isSameAs"?

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:36: var _object;
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Other matcher classes use "_val" for this. Should pick a name and use it
> consistently.
> 
> Also, it can be made final. We tend to make fields final whenever possible.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:40: bool matches(item) => (item === _object);
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Unnecessary ().

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:42: // If all types were hashable we could show
a hash here
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> True. :(
> 
> Add a "." at the end of the sentence.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:58: //if (_object is IMatcher) {
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Remove commented out code.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:69: IMatcher anything([description =
'anything']) => new _IsAnything(description);
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Can we get rid of description and make this a constant? expect() has a reason
> param, so it seems redundant here.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:89: IMatcher throwsException() => new _Throws();
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> "throwsException" -> "throws"?

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:101: /**
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Add blank line above comment.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:108: IMatcher returnsNormally() => new
_ReturnsNormally();
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Maybe I don't the intent here, but it seems to me that all expectations are
> implicitly "returns normally", so I don't think we need an expectation that
> calls this out. Am I missing something?

Yes - this will catch an exception and return false, allowing the failure
handler to deal with it appropriately.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:114: if (_matcher != null && !(_matcher is
Matcher)) {
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> You can delete this. Dart will validate this for you by virtue of the type
> annotation in checked mode.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:114: if (_matcher != null && !(_matcher is
Matcher)) {
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> You can delete this. Dart will validate this for you by virtue of the type
> annotation in checked mode.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:132: return description.append('throws an
exception which needs to match ').
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> "which needs to match" -> "which matches"?

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:166: * of a (super)[type].
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> This is worded a bit confusingly. Maybe:
> 
> "if an object is an instance of [type] (or a subtype)."

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:169: * approximate this test, by using a generic
wrapper named Type.
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Delete ","

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:183: class Type<T> {
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> How about making this implement IMatcher directly so you can do:
> 
> expect(bar, new InstanceOf<Foo>());

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:192: Type _expected_type;
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> "_expectedType"

No longer applicable.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:202: // The description here is lame :-(
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Why?

Because we don't have the type name unless it is explicitly provided.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:204: description.append('an instance of
${_expected_type.toString()}');
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> You don't need .toString() here. Interpolation does that implicitly.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:209: * that matches [matcher].
You can make this a bit shorter, with:

expect(foo, hasLength(123));

But if you prefer I will remove this.

On 2012/05/30 23:23:51, Bob Nystrom wrote:
> This doesn't seem to add a lot of value. Why not just do:
> 
> expect(foo.length, equals(123));
> 
> ?

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:219: var _len_matcher;
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> "_lenMatcher"

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:254: IMatcher hasString(matcher) => new
_HasString(wrapMatcher(matcher));
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> This name is confusing to me, and it doesn't seem that useful. Can we just
> remove this?

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:270: * with built-in types. Note that cyclic
structures will never terminate!
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Wouldn't this also work with user-defined types that implement Collection or
> Map?
> 
> Also, non-termination in tests seems bad. Can you make it handle cycles
> gracefully? If I remember right, the toString() implementations for List and
Map
> do, so you could borrow code from that.

I made it a bit more general, and added an item count limit to deal with cycles;
that seems the most lightweight way without the ability to annotate objects.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:289: 'at position ${i} ${location}');
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Indent another 2 spaces. Continued lines are indented at least +4.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:296: if (!(actual is Map)) {
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> actual is !Map

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/description.dart
File lib/unittest/description.dart (right):

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/description.da...
lib/unittest/description.dart:10: 
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Remove blank line.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/description.da...
lib/unittest/description.dart:22: 
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Remove blank line, here and elsewhere.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/description.da...
lib/unittest/description.dart:23: IDescription append(text) {
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> "add" to be consistent with List and StringBuffer.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/description.da...
lib/unittest/description.dart:29: * Append a description of given [value]. If it
is a string just
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> "Append" -> "appends". Remove "given".

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/description.da...
lib/unittest/description.dart:37: value.describe(this);
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Instead of just trying to call describe(), why not check for is IDescription?

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart
File lib/unittest/expect.dart (right):

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart#ne...
lib/unittest/expect.dart:18: * the matcher [arg2] for the reason [arg3]. In the
second form expect()
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Do we use the second form anywhere? If not, we could remove it and just
require:
> 
>     expect(foo, isTrue);
> 
> That would let us give nice types for expect(). arg1 is not that
user-friendly.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart#ne...
lib/unittest/expect.dart:22: * ExpectException, but this behavior can be changed
by calling
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Put square brackets around ExpectException, configureExpectHandler and
> IFailureHandler.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart#ne...
lib/unittest/expect.dart:26: * expect is a 3rd generation assertion mechanism,
drawing
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> `expect()`

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart#ne...
lib/unittest/expect.dart:27: * inspiration from Hamcrest and Ladislav Thon's
matcher
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Make these markdown links:
> 
> [Hamcrest][] and Ladislav Thon's [dart-matchers][]...
> 
> [hamcrest]: http://http://code.google.com/p/hamcrest/
> [dart-matchers]: https://github.com/Ladicek/dart-matchers

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart#ne...
lib/unittest/expect.dart:43: // _assertMatch handles expect() calls that use a
matcher.
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> This comment is probably not necessary.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart#ne...
lib/unittest/expect.dart:56: void _assertBool(assertion, [String reason=null]) {
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Type annotate assertion.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart#ne...
lib/unittest/expect.dart:67: // wrapMatcher takes an argument and returns a
equivalent matcher.
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> "Takes an argument and returns an equivalent matcher."

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart#ne...
lib/unittest/expect.dart:69: // generates a equals matcher for the argument.
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> "a equals" -> "an equals"

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart#ne...
lib/unittest/expect.dart:70: IMatcher wrapMatcher(x) => ((x is IMatcher) ? x :
equals(x));
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> What is this used for?

It allows matchers that themselves expect matcher arguments to be passed
scalars. E.g. instead of isNot(equals(3)) you can say isNot(3).

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart#ne...
lib/unittest/expect.dart:73: // The reference to the failure handler for failed
asserts
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> This comment doesn't add much.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart#ne...
lib/unittest/expect.dart:89: * IFailureHandler); if this is omitted or null then
the failure handler
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Remove "(that must implement IFailureHandler)" That's what type annotations
are
> for. :)
> 
> Also ";" -> "."

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart#ne...
lib/unittest/expect.dart:90: * is reset to the default, which throws
ExpectExceptions on expect()
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Square brackets around [ExpectException]s and [expect()].

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart#ne...
lib/unittest/expect.dart:93: void configureExpectHandler([IFailureHandler
handler = null]) {
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Why make this optional?

For the reset to default case.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart#ne...
lib/unittest/expect.dart:102: IErrorFormatter _assertErrorFormatter = null;
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> This comment doesn't add much.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart#ne...
lib/unittest/expect.dart:109: append('\nExpected: ').
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Indent these +4.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart#ne...
lib/unittest/expect.dart:120: * [formatter] is a reference to the new formatter
(that must implement
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Remove parenthetical.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/interfaces.dart
File lib/unittest/interfaces.dart (right):

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/interfaces.dar...
lib/unittest/interfaces.dart:18: String format(actual, IMatcher matcher, String
reason);
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Should this just be a function type? Why make it an interface?

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/interfaces.dar...
lib/unittest/interfaces.dart:29: IDescription append(text);
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> "append" -> "add"

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/interfaces.dar...
lib/unittest/interfaces.dart:31: IDescription appendList(start, separator, end,
list);
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Can these be typed?

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/interfaces.dar...
lib/unittest/interfaces.dart:36: * are three methods: match, which does the
matching;
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Instead of describing the methods here, just put comments with the methods.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/map_matchers.dart
File lib/unittest/map_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/map_matchers.d...
lib/unittest/map_matchers.dart:8: IMatcher emptyMap() => new _EmptyMap();
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Can we just have a single isEmpty matcher? Why also test the type?

I combined the matchers for string, map and collection into one - so I do have
to test the type, but I think teh result is better.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/map_matchers.d...
lib/unittest/map_matchers.dart:26: bool matches(item) => item is Map &&
item.containsKey(_key);
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Does it add value to test the type here, or should this work with anything
that
> has a containsKey method?

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/map_matchers.d...
lib/unittest/map_matchers.dart:65: IDescription describe(IDescription
description) =>
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> I would use a full {} body for this since it's multiple lines.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/map_matchers.d...
lib/unittest/map_matchers.dart:75: appendDescriptionOf(item).
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Indent another 2.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/map_matchers.d...
lib/unittest/map_matchers.dart:80: append(' contains key ').
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Ditto.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/map_matchers.d...
lib/unittest/map_matchers.dart:97: IMatcher mapLength(matcher) => new
_MapLength(wrapMatcher(matcher));
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> This feels unnecessary to me. Can we remove it?

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/map_matchers.d...
lib/unittest/map_matchers.dart:111: // Map matchers match against a Map. We add
this intermediate
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Make this a doc comment.

Removed.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/matcher.dart
File lib/unittest/matcher.dart (right):

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/matcher.dart#n...
lib/unittest/matcher.dart:13: class Matcher implements IMatcher {
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> I would get rid of the separate interface and just keep this class. Most users
> will subclass. If they don't want to inherit the implementation, they can
always
> implement the class's implicit interface.

I'd rather not, if that's okay with you. Implementing the implicit interface is
a horrible kludge that presumably won't work in checked mode. Matcher is so
ubiquitous in this library I feel better using an interface.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/matcher.dart#n...
lib/unittest/matcher.dart:19: bool matches(item) {
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Just make this abstract.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/matcher.dart#n...
lib/unittest/matcher.dart:27: IDescription describe(IDescription
mismatchDescription) {
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> This too.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/matcher.dart#n...
lib/unittest/matcher.dart:32: * This generates a description of the matcher
failed for a particular
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> "Generates a description of why the matcher failed..."

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/numeric_matche...
File lib/unittest/numeric_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/numeric_matche...
lib/unittest/numeric_matchers.dart:30: IMatcher lessThanOrEqualTo(value) =>
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Is this really better than doing:
> 
> expect(foo > bar)
> 
> ?

It gives better error messages. Either can be used. See the header comments in
expect.dart.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/numeric_matche...
lib/unittest/numeric_matchers.dart:36: IMatcher isZero() {
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Make these constants.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/numeric_matche...
lib/unittest/numeric_matchers.dart:64: return value is int || value is double;
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> "is num"
> 
> But I would just get rid of this. You can always do:
> 
> expect(foo is num)

This isn't a matcher - it's a utility function used by the matchers.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/numeric_matche...
lib/unittest/numeric_matchers.dart:69: var _value; // Expected value.
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Make these doc comments. Make the fields final.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/numeric_matche...
lib/unittest/numeric_matchers.dart:78: bool this._equalValue,
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Remove these type annotations.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/numeric_matche...
lib/unittest/numeric_matchers.dart:90: if (!_isNumeric(item)) {
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Should we allow this on user-defined types that implement comparison
operators?

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/numeric_matche...
lib/unittest/numeric_matchers.dart:102: IDescription describe(IDescription
description) =>
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> I would use a {} body here.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/numeric_matche...
lib/unittest/numeric_matchers.dart:134: if (diff < 0) diff = -diff;
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> var diff = Math.abs(item - _value);

There is no Math.abs

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/numeric_matche...
lib/unittest/numeric_matchers.dart:160: * than or equal to a [low] and less than
or equal to [high].
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> "to a" -> "to"

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/numeric_matche...
lib/unittest/numeric_matchers.dart:168: IMatcher inExclusiveRange(low, high) =>
new _InRange(low, high, true, true);
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Add a blank line after each member.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/numeric_matche...
lib/unittest/numeric_matchers.dart:187: if (!_isNumeric(_low) ||
!_isNumeric(_high)) {
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Delete these checks. That's what checked mode is for.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/numeric_matche...
lib/unittest/numeric_matchers.dart:214: "$_low
(${_lowExclusive?'exclusive':'inclusive'}) to "
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Spaces around ? and :

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/operator_match...
File lib/unittest/operator_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/operator_match...
lib/unittest/operator_matchers.dart:24: * arguments (up to 7) match. Instead of
passing the matchers separately
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Why 7?

Because it is greater than 6 and less than 8 ;-)

Thumb suck. Seemed like enough.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/string_matcher...
File lib/unittest/string_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/string_matcher...
lib/unittest/string_matchers.dart:28: IMatcher isInterpolated(matcher) => new
_IsInterpolated(wrapMatcher(matcher));
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> I think this is equivalent to toString(). Do we need this?

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/string_matcher...
lib/unittest/string_matchers.dart:49: IMatcher equalToIgnoringCase(String value)
=> new _IsEqualIgnoringCase(value);
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> "equalTo" -> "equals"

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/string_matcher...
lib/unittest/string_matchers.dart:57: if (!(_value is String)) {
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Remove this check.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/string_matcher...
lib/unittest/string_matchers.dart:71: // Utility functions
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Remove this comment?

I removed the --- at least.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/string_matcher...
lib/unittest/string_matchers.dart:73: bool isWhiteSpace(String ch) => ('
\n\r\t'.indexOf(ch) >= 0);
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> There is some debate here, but I think this should be "isWhitespace". At least
> in the context of programming, I usually see whitespace as a single word.
Also,
> make private?

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/string_matcher...
lib/unittest/string_matchers.dart:75: String _stripspace(_string) {
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> "_normalizeWhitespace"?

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/string_matcher...
lib/unittest/string_matchers.dart:75: String _stripspace(_string) {
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> "_normalizeWhitespace"?

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/string_matcher...
lib/unittest/string_matchers.dart:95: * is equal to [value] when compared with
all runs of white space
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> "whitespace"

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/string_matcher...
lib/unittest/string_matchers.dart:98: * For example, equalToIgnoringCase("hello
world") will match
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Put inline code in backticks: `equalToIgnoringWhitespace("hello world")`.
> 
> Also, "Case" -> "Whitespace".

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/string_matcher...
lib/unittest/string_matchers.dart:101: IMatcher
equalToIgnoringWhitespace(_string) =>
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> "equalTo" -> "equals"

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/string_matcher...
lib/unittest/string_matchers.dart:110: if (!(_value is String)) {
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Remove check.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/string_matcher...
lib/unittest/string_matchers.dart:117: bool matches(item) => (item is String &&
_matchValue == _stripspace(item));
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Unneeded ().

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/string_matcher...
lib/unittest/string_matchers.dart:120:
description.appendDescriptionOf(_matchValue).
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> I would use a {} here. If not, +2 indent.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/string_matcher...
lib/unittest/string_matchers.dart:170: IllegalArgumentException('endsWith
requires a string');
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Indent +2.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/string_matcher...
lib/unittest/string_matchers.dart:185: IMatcher containsString(substring) => new
_StringContains(substring);
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Can we do a single unified contains() that works with collections and strings?

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/string_matcher...
lib/unittest/string_matchers.dart:273: IDescription  describe(IDescription
description) =>
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Extra space.

Done.

[Bob Nystrom, 2012-06-01 18:22:22]

Looks much better. A small(er) number of comments but this is close to being
done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
File lib/unittest/collection_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:26: bool matches(collection) {
On 2012/06/01 17:33:15, gram wrote:
> On 2012/05/30 23:23:51, Bob Nystrom wrote:
> > Annotate this to type Collection?
> 
> None of the matches() methods are typed, as they are implementations of an
> interface method that is untyped.

That makes sense.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:29: } catch (var e) {
On 2012/06/01 17:33:15, gram wrote:
> On 2012/05/30 23:23:51, Bob Nystrom wrote:
> > Why are you swallowing exceptions here?
> 
> The idea is that you can pass anything in, and that part of the matching
process
> is to make sure that what you passed in was sane. That is why the exception is
> caught and returns false.

Is that helpful to users? If I do:

expect(foo, isEmptyCollection)

and foo throws an exception from isEmpty(), I'd rather see a helpful stack trace
than just a "fail", right?

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:115: IMatcher orderedEquals(expected) =>
new _OrderedEquals(new List.from(expected));
On 2012/06/01 17:33:15, gram wrote:
> On 2012/05/30 23:23:51, Bob Nystrom wrote:
> > Why new List.from() here?
> > 
> > Can "expected" be annotated to Iterable?
> 
> We need at least one of the two to be indexable. I have changed the code so
the
> item to be tested can be iterable and we don't make a list from it, but the
> expected value is still turned into a list.

Is this comment still accurate? It looks like the code allows iterables for both
(which is good!).

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
File lib/unittest/core_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:108: IMatcher returnsNormally() => new
_ReturnsNormally();
On 2012/06/01 17:33:15, gram wrote:
> On 2012/05/30 23:23:51, Bob Nystrom wrote:
> > Maybe I don't the intent here, but it seems to me that all expectations are
> > implicitly "returns normally", so I don't think we need an expectation that
> > calls this out. Am I missing something?
> 
> Yes - this will catch an exception and return false, allowing the failure
> handler to deal with it appropriately.
> 

What happens when exceptions are thrown outside of this?

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:209: * that matches [matcher].
On 2012/06/01 17:33:15, gram wrote:
> You can make this a bit shorter, with:
> 
> expect(foo, hasLength(123));
> 
> But if you prefer I will remove this.
> 
> On 2012/05/30 23:23:51, Bob Nystrom wrote:
> > This doesn't seem to add a lot of value. Why not just do:
> > 
> > expect(foo.length, equals(123));
> > 
> > ?
> 

I like terseness, but I also like simplicity and minimizing the surface area of
the API. If it's OK with you, I would remove this.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart
File lib/unittest/expect.dart (right):

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/expect.dart#ne...
lib/unittest/expect.dart:27: * inspiration from Hamcrest and Ladislav Thon's
matcher
On 2012/06/01 17:33:15, gram wrote:
> On 2012/05/30 23:23:51, Bob Nystrom wrote:
> > Make these markdown links:
> > 
> > [Hamcrest][] and Ladislav Thon's [dart-matchers][]...
> > 
> > [hamcrest]: http://http://code.google.com/p/hamcrest/
> > [dart-matchers]: https://github.com/Ladicek/dart-matchers
> 
> Done.

Almost done. :) The markdown syntax is very specific here. It needs to be:

expect() is a 3rd generation assertion mechanism, drawing
inspiration from [Hamcrest][] and Ladislav Thon's [dart-matchers][]
library.

[Hamcrest]: http://en.wikipedia.org/wiki/Hamcrest
[dart-matchers]: https://github.com/Ladicek/dart-matchers

That will make "Hamcrest" and "dart-matchers" inline links to the provided URLs.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/map_matchers.dart
File lib/unittest/map_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/map_matchers.d...
lib/unittest/map_matchers.dart:8: IMatcher emptyMap() => new _EmptyMap();
On 2012/06/01 17:33:15, gram wrote:
> On 2012/05/30 23:23:51, Bob Nystrom wrote:
> > Can we just have a single isEmpty matcher? Why also test the type?
> 
> I combined the matchers for string, map and collection into one - so I do have
> to test the type, but I think teh result is better.

Agreed. We do lots of other type tests in this lib, so that sounds fine to me.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/matcher.dart
File lib/unittest/matcher.dart (right):

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/matcher.dart#n...
lib/unittest/matcher.dart:13: class Matcher implements IMatcher {
On 2012/06/01 17:33:15, gram wrote:
> On 2012/05/30 23:23:51, Bob Nystrom wrote:
> > I would get rid of the separate interface and just keep this class. Most
users
> > will subclass. If they don't want to inherit the implementation, they can
> always
> > implement the class's implicit interface.
> 
> I'd rather not, if that's okay with you. Implementing the implicit interface
is
> a horrible kludge that presumably won't work in checked mode.

No, it works fine in checked mode. Implicit interfaces are a deep part of the
language. They exist in part so that you don't have to do the Java anti-pattern
of wrapping every class in an interface just in case you end up needing to
abstract it later. If there's only a single implementation, it's fine to just
start with a class.

> Matcher is so ubiquitous in this library I feel better using an interface.

If it makes you feel better, that's fine. Will both the class and interface be
exposed by the library? Can they subclass the class?

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/numeric_matche...
File lib/unittest/numeric_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/numeric_matche...
lib/unittest/numeric_matchers.dart:64: return value is int || value is double;
On 2012/06/01 17:33:15, gram wrote:
> On 2012/05/30 23:23:51, Bob Nystrom wrote:
> > "is num"
> > 
> > But I would just get rid of this. You can always do:
> > 
> > expect(foo is num)
> 
> This isn't a matcher - it's a utility function used by the matchers.

Oh, right. Sorry. In either case, you can still simplify its implementation to
"value is num". "int" and "double" are both subinterfaces of that and it has no
other subinterfaces.

This is only being called in one place now (isCloseTo). Just get rid of this and
do "is num" there?

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/numeric_matche...
lib/unittest/numeric_matchers.dart:134: if (diff < 0) diff = -diff;
On 2012/06/01 17:33:15, gram wrote:
> On 2012/05/30 23:23:51, Bob Nystrom wrote:
> > var diff = Math.abs(item - _value);
> 
> There is no Math.abs

Heh, whoops! Every now and then, the corelib designers throw some Smalltalk in
instead of the usual Java. It's actually:

diff = (item - _value).abs();

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/operator_match...
File lib/unittest/operator_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/operator_match...
lib/unittest/operator_matchers.dart:24: * arguments (up to 7) match. Instead of
passing the matchers separately
On 2012/06/01 17:33:15, gram wrote:
> On 2012/05/30 23:23:51, Bob Nystrom wrote:
> > Why 7?
> 
> Because it is greater than 6 and less than 8 ;-)
> 
> Thumb suck. Seemed like enough.

Works for me.

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/collection_ma...
File lib/unittest/collection_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/collection_ma...
lib/unittest/collection_matchers.dart:114: }
If you like, you can make this a single line:

if (item is !Iterable) return 'not iterable';

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/collection_ma...
lib/unittest/collection_matchers.dart:147: mask |= (1 << actualPosition);
This may be the first Dart code I've seen that relies on arbitrary-precision
ints. This is cool, but it won't work right now on dart2js/frog. Thoughts on
what we should do here?

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/collection_ma...
lib/unittest/collection_matchers.dart:176: /*abstract*/ class _CollectionMatcher
extends Matcher {
I think Dart does actually allow abstract here (which I didn't know until
yesterday). What happens if this isn't commented out?

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/core_matchers...
File lib/unittest/core_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/core_matchers...
lib/unittest/core_matchers.dart:14: if (item is Map || item is Collection)
{} for multi-line ifs.

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/core_matchers...
lib/unittest/core_matchers.dart:120: IMatcher throwsA(IMatcher matcher) => new
_Throws(matcher);
Nice name. :)

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/core_matchers...
lib/unittest/core_matchers.dart:285: if (expectedIterator.hasNext()) {
collection_matchers has a near identical chunk of code. Refactor this out into a
function they both use?

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/core_matchers...
lib/unittest/core_matchers.dart:352: IMatcher contains(expected) => new
_Contains(expected);
For the non-string case, can expected be a matcher? Should this be supported?

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/description.dart
File lib/unittest/description.dart (right):

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/description.d...
lib/unittest/description.dart:55: IDescription addList(String start, String
separator, String end,
This doesn't require a list, so rename it "addAll"? That's what List and
StringBuffer have.

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/expect.dart
File lib/unittest/expect.dart (right):

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/expect.dart#n...
lib/unittest/expect.dart:7: * matches the [matcher]. [reason] is an optional and
is typically
"an optional" -> "optional".

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/expect.dart#n...
lib/unittest/expect.dart:36: // Make sure we have a failure handler and
formatter configured
"." at the end of the comment. Google style is that comments should be full
sentences.

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/expect.dart#n...
lib/unittest/expect.dart:44: // Make sure we have a failure handler configured
"."

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/interfaces.dart
File lib/unittest/interfaces.dart (right):

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/interfaces.da...
lib/unittest/interfaces.dart:25: interface IDescription {
No "I" in interface names, here and elsewhere. Everything is an interface in
Dart, even classes, so naming them differently doesn't add a lot of value.
(Also, no other Dart code names interfaces with I.)

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/map_matchers....
File lib/unittest/map_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/map_matchers....
lib/unittest/map_matchers.dart:6: * Returns a matcher which matches objects
containing the given [value].
Might want to clarify that this is for maps, since it reads like it would work
on lists too.

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/numeric_match...
File lib/unittest/numeric_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/numeric_match...
lib/unittest/numeric_matchers.dart:179: if (value is !num) { //
!_isNumeric(value)) {
Remove dead code.

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/operator_matc...
File lib/unittest/operator_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/operator_matc...
lib/unittest/operator_matchers.dart:122: expect(arg1, isNull);
This does look pretty nice. I like the new API you have.

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/string_matche...
File lib/unittest/string_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/string_matche...
lib/unittest/string_matchers.dart:36: class _IsEqualIgnoringWhiteSpace extends
_StringMatcher {
This is private so it matters less, but for consistency's sake this should
probably be _IsEqualIgnoringWhitespace now too.

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/string_matche...
lib/unittest/string_matchers.dart:59: // Utility function to collapse whitespace
runs
Doc comment?

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/string_matche...
lib/unittest/string_matchers.dart:120: * For example,
`stringContainsInOrder(["a", "e", "i", "o", "u"])` will match
Yay markdown!

http://codereview.chromium.org/10441104/diff/12001/tests/utils/unittest_test....
File tests/utils/unittest_test.dart (right):

http://codereview.chromium.org/10441104/diff/12001/tests/utils/unittest_test....
tests/utils/unittest_test.dart:1: // Copyright (c) 2012, the Dart project
authors.  Please see the AUTHORS file
Doesn't look like you got to the comments on this file. :)

[gram, 2012-06-01 22:21:59]

PTAL

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
File lib/unittest/collection_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:26: bool matches(collection) {
On 2012/06/01 18:22:22, Bob Nystrom wrote:
> On 2012/06/01 17:33:15, gram wrote:
> > On 2012/05/30 23:23:51, Bob Nystrom wrote:
> > > Annotate this to type Collection?
> > 
> > None of the matches() methods are typed, as they are implementations of an
> > interface method that is untyped.
> 
> That makes sense.

Done.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:29: } catch (var e) {
On 2012/06/01 18:22:22, Bob Nystrom wrote:
> On 2012/06/01 17:33:15, gram wrote:
> > On 2012/05/30 23:23:51, Bob Nystrom wrote:
> > > Why are you swallowing exceptions here?
> > 
> > The idea is that you can pass anything in, and that part of the matching
> process
> > is to make sure that what you passed in was sane. That is why the exception
is
> > caught and returns false.
> 
> Is that helpful to users? If I do:
> 
> expect(foo, isEmptyCollection)
> 
> and foo throws an exception from isEmpty(), I'd rather see a helpful stack
trace
> than just a "fail", right?

I think we're both partly right. I do think that expect should not throws
exceptions unless the expect error handler  is configured as such. So there
should be a try/catch but this is the wrong place; it should be around the call
to matches() in expect(), and that should be treated as a test failure, ideally
with stack information. I'll make that change.

http://codereview.chromium.org/10441104/diff/1044/tests/utils/unittest_test.dart
File tests/utils/unittest_test.dart (right):

http://codereview.chromium.org/10441104/diff/1044/tests/utils/unittest_test.d...
tests/utils/unittest_test.dart:54: // Core matchers
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Instead of comments, use group() and test(). That way the structure shows up
in
> the test results and it's easier to track down failures.

Done.

http://codereview.chromium.org/10441104/diff/1044/tests/utils/unittest_test.d...
tests/utils/unittest_test.dart:89: "Expected: throws an exception but: no
exception");
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Indent another 2, here and elsewhere.

Done.

http://codereview.chromium.org/10441104/diff/1044/tests/utils/unittest_test.d...
tests/utils/unittest_test.dart:270: shouldPass('goodbye cruel world',
stringContainsInOrder(['goodbye', 'cruel']));
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> Long line.

Done.

http://codereview.chromium.org/10441104/diff/1044/tests/utils/unittest_test.d...
tests/utils/unittest_test.dart:291: var d = [ 1, 2 ];
On 2012/05/30 23:23:51, Bob Nystrom wrote:
> No spaces after [ or before ]

Done.

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/collection_ma...
File lib/unittest/collection_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/collection_ma...
lib/unittest/collection_matchers.dart:114: }
On 2012/06/01 18:22:23, Bob Nystrom wrote:
> If you like, you can make this a single line:
> 
> if (item is !Iterable) return 'not iterable';

I think I prefer it split, especially as it has a return.

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/collection_ma...
lib/unittest/collection_matchers.dart:147: mask |= (1 << actualPosition);
On 2012/06/01 18:22:23, Bob Nystrom wrote:
> This may be the first Dart code I've seen that relies on arbitrary-precision
> ints. This is cool, but it won't work right now on dart2js/frog. Thoughts on
> what we should do here?

I was concerned about that. In one respect I want to leave it, if only to call
out the absurdity of having an int type  in a language that is going to be
compiled to Javascript :-) But I'll change it to List<bool>

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/collection_ma...
lib/unittest/collection_matchers.dart:176: /*abstract*/ class _CollectionMatcher
extends Matcher {
On 2012/06/01 18:22:23, Bob Nystrom wrote:
> I think Dart does actually allow abstract here (which I didn't know until
> yesterday). What happens if this isn't commented out?

frog barfs when generating docs.

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/core_matchers...
File lib/unittest/core_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/core_matchers...
lib/unittest/core_matchers.dart:14: if (item is Map || item is Collection)
On 2012/06/01 18:22:23, Bob Nystrom wrote:
> {} for multi-line ifs.

Done.

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/core_matchers...
lib/unittest/core_matchers.dart:120: IMatcher throwsA(IMatcher matcher) => new
_Throws(matcher);
On 2012/06/01 18:22:23, Bob Nystrom wrote:
> Nice name. :)

Done.

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/core_matchers...
lib/unittest/core_matchers.dart:285: if (expectedIterator.hasNext()) {
On 2012/06/01 18:22:23, Bob Nystrom wrote:
> collection_matchers has a near identical chunk of code. Refactor this out into
a
> function they both use?

Done.

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/core_matchers...
lib/unittest/core_matchers.dart:352: IMatcher contains(expected) => new
_Contains(expected);
On 2012/06/01 18:22:23, Bob Nystrom wrote:
> For the non-string case, can expected be a matcher? Should this be supported?

Good idea!

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/description.dart
File lib/unittest/description.dart (right):

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/description.d...
lib/unittest/description.dart:55: IDescription addList(String start, String
separator, String end,
On 2012/06/01 18:22:23, Bob Nystrom wrote:
> This doesn't require a list, so rename it "addAll"? That's what List and
> StringBuffer have.

Done.

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/expect.dart
File lib/unittest/expect.dart (right):

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/expect.dart#n...
lib/unittest/expect.dart:7: * matches the [matcher]. [reason] is an optional and
is typically
On 2012/06/01 18:22:23, Bob Nystrom wrote:
> "an optional" -> "optional".

Done.

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/expect.dart#n...
lib/unittest/expect.dart:36: // Make sure we have a failure handler and
formatter configured
On 2012/06/01 18:22:23, Bob Nystrom wrote:
> "." at the end of the comment. Google style is that comments should be full
> sentences.

Done.

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/expect.dart#n...
lib/unittest/expect.dart:44: // Make sure we have a failure handler configured
On 2012/06/01 18:22:23, Bob Nystrom wrote:
> "."

Done.

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/interfaces.dart
File lib/unittest/interfaces.dart (right):

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/interfaces.da...
lib/unittest/interfaces.dart:25: interface IDescription {
On 2012/06/01 18:22:23, Bob Nystrom wrote:
> No "I" in interface names, here and elsewhere. Everything is an interface in
> Dart, even classes, so naming them differently doesn't add a lot of value.
> (Also, no other Dart code names interfaces with I.)

Done.

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/map_matchers....
File lib/unittest/map_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/map_matchers....
lib/unittest/map_matchers.dart:6: * Returns a matcher which matches objects
containing the given [value].
On 2012/06/01 18:22:23, Bob Nystrom wrote:
> Might want to clarify that this is for maps, since it reads like it would work
> on lists too.

Done.

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/numeric_match...
File lib/unittest/numeric_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/numeric_match...
lib/unittest/numeric_matchers.dart:179: if (value is !num) { //
!_isNumeric(value)) {
On 2012/06/01 18:22:23, Bob Nystrom wrote:
> Remove dead code.

Done.

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/operator_matc...
File lib/unittest/operator_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/operator_matc...
lib/unittest/operator_matchers.dart:122: expect(arg1, isNull);
On 2012/06/01 18:22:23, Bob Nystrom wrote:
> This does look pretty nice. I like the new API you have.

Done.

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/string_matche...
File lib/unittest/string_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/string_matche...
lib/unittest/string_matchers.dart:36: class _IsEqualIgnoringWhiteSpace extends
_StringMatcher {
On 2012/06/01 18:22:23, Bob Nystrom wrote:
> This is private so it matters less, but for consistency's sake this should
> probably be _IsEqualIgnoringWhitespace now too.

Done.

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/string_matche...
lib/unittest/string_matchers.dart:59: // Utility function to collapse whitespace
runs
On 2012/06/01 18:22:23, Bob Nystrom wrote:
> Doc comment?

Done.

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/string_matche...
lib/unittest/string_matchers.dart:120: * For example,
`stringContainsInOrder(["a", "e", "i", "o", "u"])` will match
On 2012/06/01 18:22:23, Bob Nystrom wrote:
> Yay markdown!

Done.

http://codereview.chromium.org/10441104/diff/12001/tests/utils/unittest_test....
File tests/utils/unittest_test.dart (right):

http://codereview.chromium.org/10441104/diff/12001/tests/utils/unittest_test....
tests/utils/unittest_test.dart:1: // Copyright (c) 2012, the Dart project
authors.  Please see the AUTHORS file
On 2012/06/01 18:22:23, Bob Nystrom wrote:
> Doesn't look like you got to the comments on this file. :)

Done.

[Bob Nystrom, 2012-06-01 22:29:00]

LGTM!

When this is in and working, we should tell the public about the new hotness.

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
File lib/unittest/collection_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/1044/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:29: } catch (var e) {
On 2012/06/01 22:21:59, gram wrote:
> On 2012/06/01 18:22:22, Bob Nystrom wrote:
> > On 2012/06/01 17:33:15, gram wrote:
> > > On 2012/05/30 23:23:51, Bob Nystrom wrote:
> > > > Why are you swallowing exceptions here?
> > > 
> > > The idea is that you can pass anything in, and that part of the matching
> > process
> > > is to make sure that what you passed in was sane. That is why the
exception
> is
> > > caught and returns false.
> > 
> > Is that helpful to users? If I do:
> > 
> > expect(foo, isEmptyCollection)
> > 
> > and foo throws an exception from isEmpty(), I'd rather see a helpful stack
> trace
> > than just a "fail", right?
> 
> I think we're both partly right. I do think that expect should not throws
> exceptions unless the expect error handler  is configured as such. So there
> should be a try/catch but this is the wrong place; it should be around the
call
> to matches() in expect(), and that should be treated as a test failure,
ideally
> with stack information. I'll make that change.

Perfect. I love it.

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/collection_ma...
File lib/unittest/collection_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/collection_ma...
lib/unittest/collection_matchers.dart:147: mask |= (1 << actualPosition);
On 2012/06/01 22:22:00, gram wrote:
> On 2012/06/01 18:22:23, Bob Nystrom wrote:
> > This may be the first Dart code I've seen that relies on arbitrary-precision
> > ints. This is cool, but it won't work right now on dart2js/frog. Thoughts on
> > what we should do here?
> 
> I was concerned about that. In one respect I want to leave it, if only to call
> out the absurdity of having an int type  in a language that is going to be
> compiled to Javascript :-) But I'll change it to List<bool>

Maybe leave a TODO: use int when all Dart implementations support
arbitrary-size?

http://codereview.chromium.org/10441104/diff/12001/lib/unittest/collection_ma...
lib/unittest/collection_matchers.dart:176: /*abstract*/ class _CollectionMatcher
extends Matcher {
On 2012/06/01 22:22:00, gram wrote:
> On 2012/06/01 18:22:23, Bob Nystrom wrote:
> > I think Dart does actually allow abstract here (which I didn't know until
> > yesterday). What happens if this isn't commented out?
> 
> frog barfs when generating docs.
> 

Ah. Please leave a TODO then saying "uncomment this when dartdoc supports it."
so we know what it's gating on.

[gram, 2012-06-02 00:29:12]

I made a few more changes. If you don't mind take a look at the changes between
patch set 4 and 5 for the lib/unittest/* and tests/utils/unitest_tests.dart

Basically this was to:

- move user-supplied reason to end of mismatch description
- remove try/catch in some places as expect will handle it
- add stack trace to description if an exception was thrown
- make the whitespace compressor utility function public
- fix the expect(0, hasLength()) test failure check now that a stack trace is
included
- add limit parameter to recursiveMatch factory function

[Bob Nystrom, 2012-06-04 20:04:09]

Nits but LGTM.

http://codereview.chromium.org/10441104/diff/39001/lib/unittest/core_matchers...
File lib/unittest/core_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/39001/lib/unittest/core_matchers...
lib/unittest/core_matchers.dart:235: if (item.length * item.length >= 0) { //
hack to will throw if no length
This comment no grammar. Why is this hack needed?

http://codereview.chromium.org/10441104/diff/39001/tests/utils/unittest_test....
File tests/utils/unittest_test.dart (right):

http://codereview.chromium.org/10441104/diff/39001/tests/utils/unittest_test....
tests/utils/unittest_test.dart:58: return  item is String &&
Extra space after "return".

[gram, 2012-06-05 16:25:45]

Take a look at the comment I added in core_matchers about the length test and
see if you are okay with it.

I also expanded isPositive/isNegative into four cases (>0, >=0, <0 <=0) after
Eli pointed out to me that it is a religious argument whether 0 is a positive
number.

http://codereview.chromium.org/10441104/diff/39001/lib/unittest/core_matchers...
File lib/unittest/core_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/39001/lib/unittest/core_matchers...
lib/unittest/core_matchers.dart:235: if (item.length * item.length >= 0) { //
hack to will throw if no length
On 2012/06/04 20:04:09, Bob Nystrom wrote:
> This comment no grammar. Why is this hack needed?

Basically we want to generate a different description if there is no length
property. So I wanted some harmless code that will throw if no length property
but that would be subtle enough that an optimizer wouldn't strip it out.

http://codereview.chromium.org/10441104/diff/39001/tests/utils/unittest_test....
File tests/utils/unittest_test.dart (right):

http://codereview.chromium.org/10441104/diff/39001/tests/utils/unittest_test....
tests/utils/unittest_test.dart:58: return  item is String &&
On 2012/06/04 20:04:09, Bob Nystrom wrote:
> Extra space after "return".

Done.

[gram, 2012-06-05 16:30:07]

Adding Siggy

[Bob Nystrom, 2012-06-05 16:50:36]

LGTM!

http://codereview.chromium.org/10441104/diff/39001/lib/unittest/core_matchers...
File lib/unittest/core_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/39001/lib/unittest/core_matchers...
lib/unittest/core_matchers.dart:235: if (item.length * item.length >= 0) { //
hack to will throw if no length
On 2012/06/05 16:25:46, gram wrote:
> On 2012/06/04 20:04:09, Bob Nystrom wrote:
> > This comment no grammar. Why is this hack needed?
> 
> Basically we want to generate a different description if there is no length
> property. So I wanted some harmless code that will throw if no length property
> but that would be subtle enough that an optimizer wouldn't strip it out.

OK. Can you move that comment to the preceding line and make it a complete
grammatical sentence?

[gram, 2012-06-05 16:59:53]

http://codereview.chromium.org/10441104/diff/39001/lib/unittest/core_matchers...
File lib/unittest/core_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/39001/lib/unittest/core_matchers...
lib/unittest/core_matchers.dart:235: if (item.length * item.length >= 0) { //
hack to will throw if no length
Was done already :-)
On 2012/06/05 16:50:37, Bob Nystrom wrote:
> On 2012/06/05 16:25:46, gram wrote:
> > On 2012/06/04 20:04:09, Bob Nystrom wrote:
> > > This comment no grammar. Why is this hack needed?
> > 
> > Basically we want to generate a different description if there is no length
> > property. So I wanted some harmless code that will throw if no length
property
> > but that would be subtle enough that an optimizer wouldn't strip it out.
> 
> OK. Can you move that comment to the preceding line and make it a complete
> grammatical sentence?

[gram, 2012-06-05 18:02:13]

I have added some missing tests for the new isNonNegative/isNonPositive
matchers, and fixed an omission in the description generation for these so that
they say, for example, 

expected a positive value

instead of the both ugly and confusing:

expected a value positive <0>

[gram, 2012-06-05 21:54:06]

A last set of changes - removed a few cases of trailing whitespace on lines, and
changed the comments at the start of unittest.dart to use the new expect()
syntax.

[Bob Nystrom, 2012-06-05 22:26:35]

On 2012/06/05 21:54:06, gram wrote:
> A last set of changes - removed a few cases of trailing whitespace on lines,
and
> changed the comments at the start of unittest.dart to use the new expect()
> syntax.

Still LGTM! Stop teasing me and get this checked in! :)

- bob

[Siggi Cherem (dart-lang), 2012-06-06 00:26:08]

LGTM!!

This looks amazingly cool!

After such a thorough review from Bob (+1 on wow!), I have very few comments
(mostly little nits).

http://codereview.chromium.org/10441104/diff/60002/lib/unittest/collection_ma...
File lib/unittest/collection_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/60002/lib/unittest/collection_ma...
lib/unittest/collection_matchers.dart:44: * Returns a matcher which matches
Iterables that have the same
dart-doc style: Iterables => [Iterable]s
(here and below)

http://codereview.chromium.org/10441104/diff/60002/lib/unittest/core_matchers...
File lib/unittest/core_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/60002/lib/unittest/core_matchers...
lib/unittest/core_matchers.dart:207: bool matches(obj) => obj is T;
FYI - I believe frog fails incorrectly here (returns true always), I hope
dart2js works in this case.

http://codereview.chromium.org/10441104/diff/60002/lib/unittest/description.dart
File lib/unittest/description.dart (right):

http://codereview.chromium.org/10441104/diff/60002/lib/unittest/description.d...
lib/unittest/description.dart:83: */
style nit: for comments that fit in a single line, use this form:
/** Single-line comment. */

http://codereview.chromium.org/10441104/diff/60002/lib/unittest/interfaces.dart
File lib/unittest/interfaces.dart (right):

http://codereview.chromium.org/10441104/diff/60002/lib/unittest/interfaces.da...
lib/unittest/interfaces.dart:28: /** This is used to add a meaningful
description of a value. */
style nit: add empty line above this comment (here and in all members below)

http://codereview.chromium.org/10441104/diff/60002/lib/unittest/matcher.dart
File lib/unittest/matcher.dart (right):

http://codereview.chromium.org/10441104/diff/60002/lib/unittest/matcher.dart#...
lib/unittest/matcher.dart:37: 
nit: empty line

http://codereview.chromium.org/10441104/diff/60002/lib/unittest/unittest.dart
File lib/unittest/unittest.dart (right):

http://codereview.chromium.org/10441104/diff/60002/lib/unittest/unittest.dart...
lib/unittest/unittest.dart:11: * ##Concepts##
could you add another item introducing 'Matchers: .... '?

http://codereview.chromium.org/10441104/diff/60002/samples/tests/samples/lib/...
File samples/tests/samples/lib/observable/observable_list_tests.dart (right):

http://codereview.chromium.org/10441104/diff/60002/samples/tests/samples/lib/...
samples/tests/samples/lib/observable/observable_list_tests.dart:62:
expect(arr.removeFirstElement(3), isTrue);// 1 6( )  3 7 5 6 7 42
nit: fix alignment of comments (here and a couple lines below)

http://codereview.chromium.org/10441104/diff/60002/tests/dom/canvas_test.dart
File tests/dom/canvas_test.dart (right):

http://codereview.chromium.org/10441104/diff/60002/tests/dom/canvas_test.dart...
tests/dom/canvas_test.dart:37: expect(context.fillStyle is CanvasGradient,
isTrue());
isTrue() => isTrue (remove extra parenthesis)

http://codereview.chromium.org/10441104/diff/60002/tests/utils/unittest_test....
File tests/utils/unittest_test.dart (right):

http://codereview.chromium.org/10441104/diff/60002/tests/utils/unittest_test....
tests/utils/unittest_test.dart:1: // Copyright (c) 2012, the Dart project
authors.  Please see the AUTHORS file
I'm very excited that we now have tests for the unittest library! woo hoo!

A couple file-general comments:
- I guess this is only testing matchers, maybe we should make this
'matcher_test.dart' and have a separate test file later to actually test
unittest functionality (which should probably be written without using unittest
itself)

- I think we should put this under tests/lib/unittest/... (create the directory)

http://codereview.chromium.org/10441104/diff/60002/tests/utils/unittest_test....
tests/utils/unittest_test.dart:5: #library('unittestTest');
rename to unittest_test?

http://codereview.chromium.org/10441104/diff/60002/tests/utils/unittest_test....
tests/utils/unittest_test.dart:134: shouldFail(0, new
isInstanceOf<String>('String'),
does this fail in frog?

[gram, 2012-06-06 16:23:55]

PTAL at a couple of the issues below.

http://codereview.chromium.org/10441104/diff/60002/lib/unittest/collection_ma...
File lib/unittest/collection_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/60002/lib/unittest/collection_ma...
lib/unittest/collection_matchers.dart:44: * Returns a matcher which matches
Iterables that have the same
On 2012/06/06 00:26:08, sigmund wrote:
> dart-doc style: Iterables => [Iterable]s
> (here and below)

Done.

http://codereview.chromium.org/10441104/diff/60002/lib/unittest/core_matchers...
File lib/unittest/core_matchers.dart (right):

http://codereview.chromium.org/10441104/diff/60002/lib/unittest/core_matchers...
lib/unittest/core_matchers.dart:207: bool matches(obj) => obj is T;
On 2012/06/06 00:26:08, sigmund wrote:
> FYI - I believe frog fails incorrectly here (returns true always), I hope
> dart2js works in this case.

I'll test this. If it fails I'll file a bug against dart2js.

http://codereview.chromium.org/10441104/diff/60002/lib/unittest/description.dart
File lib/unittest/description.dart (right):

http://codereview.chromium.org/10441104/diff/60002/lib/unittest/description.d...
lib/unittest/description.dart:83: */
On 2012/06/06 00:26:08, sigmund wrote:
> style nit: for comments that fit in a single line, use this form:
> /** Single-line comment. */

Done.

http://codereview.chromium.org/10441104/diff/60002/lib/unittest/interfaces.dart
File lib/unittest/interfaces.dart (right):

http://codereview.chromium.org/10441104/diff/60002/lib/unittest/interfaces.da...
lib/unittest/interfaces.dart:28: /** This is used to add a meaningful
description of a value. */
On 2012/06/06 00:26:08, sigmund wrote:
> style nit: add empty line above this comment (here and in all members below)

Done.

http://codereview.chromium.org/10441104/diff/60002/lib/unittest/matcher.dart
File lib/unittest/matcher.dart (right):

http://codereview.chromium.org/10441104/diff/60002/lib/unittest/matcher.dart#...
lib/unittest/matcher.dart:37: 
On 2012/06/06 00:26:08, sigmund wrote:
> nit: empty line

Done.

http://codereview.chromium.org/10441104/diff/60002/lib/unittest/unittest.dart
File lib/unittest/unittest.dart (right):

http://codereview.chromium.org/10441104/diff/60002/lib/unittest/unittest.dart...
lib/unittest/unittest.dart:11: * ##Concepts##
On 2012/06/06 00:26:08, sigmund wrote:
> could you add another item introducing 'Matchers: .... '?

Done.

http://codereview.chromium.org/10441104/diff/60002/samples/tests/samples/lib/...
File samples/tests/samples/lib/observable/observable_list_tests.dart (right):

http://codereview.chromium.org/10441104/diff/60002/samples/tests/samples/lib/...
samples/tests/samples/lib/observable/observable_list_tests.dart:62:
expect(arr.removeFirstElement(3), isTrue);// 1 6( )  3 7 5 6 7 42
On 2012/06/06 00:26:08, sigmund wrote:
> nit: fix alignment of comments (here and a couple lines below)

Done.

http://codereview.chromium.org/10441104/diff/60002/tests/dom/canvas_test.dart
File tests/dom/canvas_test.dart (right):

http://codereview.chromium.org/10441104/diff/60002/tests/dom/canvas_test.dart...
tests/dom/canvas_test.dart:37: expect(context.fillStyle is CanvasGradient,
isTrue());
On 2012/06/06 00:26:08, sigmund wrote:
> isTrue() => isTrue (remove extra parenthesis)

Done.

http://codereview.chromium.org/10441104/diff/60002/tests/utils/unittest_test....
File tests/utils/unittest_test.dart (right):

http://codereview.chromium.org/10441104/diff/60002/tests/utils/unittest_test....
tests/utils/unittest_test.dart:1: // Copyright (c) 2012, the Dart project
authors.  Please see the AUTHORS file
On 2012/06/06 00:26:08, sigmund wrote:
> I'm very excited that we now have tests for the unittest library! woo hoo!
> 
> A couple file-general comments:
> - I guess this is only testing matchers, maybe we should make this
> 'matcher_test.dart' and have a separate test file later to actually test
> unittest functionality (which should probably be written without using
unittest
> itself)
> 
> - I think we should put this under tests/lib/unittest/... (create the
directory)

I can add some tests for unit test framework itself later, and just add them to
the same file; I'm not sure we need multiple files.

As for the location, there seems to be no clear rule here. Some libs have tests
under tests/lib/<name>, some are under tests/<name>, and some under
tests/utils/<name>. I followed uri and utf and used utils. I'm not moving it now
unless you feel strongly about this; let me know.

http://codereview.chromium.org/10441104/diff/60002/tests/utils/unittest_test....
tests/utils/unittest_test.dart:5: #library('unittestTest');
On 2012/06/06 00:26:08, sigmund wrote:
> rename to unittest_test?

We aren't consistent here. I took the camel case from uri_tests.dart, but I
notice dartdoc_test.dart uses underscore. Leaving unchanged for now and kicking
this back to you - tell me which you prefer and I'll fix them all to be
consistent.

http://codereview.chromium.org/10441104/diff/60002/tests/utils/unittest_test....
tests/utils/unittest_test.dart:134: shouldFail(0, new
isInstanceOf<String>('String'),
On 2012/06/06 00:26:08, sigmund wrote:
> does this fail in frog?

Will test. I confess I tested only in Dartium.

[Siggi Cherem (dart-lang), 2012-06-06 22:32:45]

lgtm