Added test setup/teardown.

Added test setup/teardown.
Made default matcher isTrue.
Made the exception matchers work with dart2js.
Added isIn matcher.
equals() now recurses.
Committed: https://code.google.com/p/dart/source/detail?r=8951

[Bob Nystrom, 2012-06-19 23:38:33]

Mostly looks good, mainly just an API design question.

http://codereview.chromium.org/10579008/diff/1/lib/unittest/collection_matche...
File lib/unittest/collection_matchers.dart (right):

http://codereview.chromium.org/10579008/diff/1/lib/unittest/collection_matche...
lib/unittest/collection_matchers.dart:53: Matcher _matcher;
Can you make these final?

http://codereview.chromium.org/10579008/diff/1/lib/unittest/collection_matche...
lib/unittest/collection_matchers.dart:59: bool matches(item) {
=> (item is Iterable) && _matcher.matches(item);

?

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

http://codereview.chromium.org/10579008/diff/1/lib/unittest/core_matchers.dar...
lib/unittest/core_matchers.dart:84: * recursion depth [limit can be pr]ovided.
The default limit is 100.
That ] is in a weird place.

http://codereview.chromium.org/10579008/diff/1/lib/unittest/core_matchers.dar...
lib/unittest/core_matchers.dart:96: // A utility function for comparing
iterators
This comment doesn't add a lot of value.

http://codereview.chromium.org/10579008/diff/1/lib/unittest/core_matchers.dar...
lib/unittest/core_matchers.dart:128: // If _limit is 1 we can only recurse one
level into object
Add a "."

http://codereview.chromium.org/10579008/diff/1/lib/unittest/core_matchers.dar...
lib/unittest/core_matchers.dart:130: if (expected != actual) {
This is some painfully nested flow control. Can you flatten it out some?

Maybe do:

if (expected == actual) {
  // Do nothing.
} else if (depth > _limit) { 
  ...
}

http://codereview.chromium.org/10579008/diff/1/lib/unittest/core_matchers.dar...
lib/unittest/core_matchers.dart:132: reason =  new StringDescription('recursion
depth limit exceeded');
Extra space before "new".

http://codereview.chromium.org/10579008/diff/1/lib/unittest/unittest.dart
File lib/unittest/unittest.dart (right):

http://codereview.chromium.org/10579008/diff/1/lib/unittest/unittest.dart#new...
lib/unittest/unittest.dart:514: [Function setupTest, Function teardownTest]) {
Instead of taking these functions as arguments, how about making separate
setUp() and tearDown() functions to register them, like:

group('some group', () {
  setUp(() {
    print('setting up...');
  });

  tearDown(() {
    print('tearing down...');
  });

  test('some test', () {
    print('this will print between "setting up" and "tearing down"');
  });
});

http://codereview.chromium.org/10579008/diff/1/lib/unittest/unittest.dart#new...
lib/unittest/unittest.dart:528: // settings for test setup/teardown
.

[gram, 2012-06-20 17:44:14]

PTAL

http://codereview.chromium.org/10579008/diff/1/lib/unittest/collection_matche...
File lib/unittest/collection_matchers.dart (right):

http://codereview.chromium.org/10579008/diff/1/lib/unittest/collection_matche...
lib/unittest/collection_matchers.dart:53: Matcher _matcher;
On 2012/06/19 23:38:33, Bob Nystrom wrote:
> Can you make these final?

_expected yes, _matcher no

http://codereview.chromium.org/10579008/diff/1/lib/unittest/collection_matche...
lib/unittest/collection_matchers.dart:59: bool matches(item) {
On 2012/06/19 23:38:33, Bob Nystrom wrote:
> => (item is Iterable) && _matcher.matches(item);
> 
> ?

Done.

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

http://codereview.chromium.org/10579008/diff/1/lib/unittest/core_matchers.dar...
lib/unittest/core_matchers.dart:84: * recursion depth [limit can be pr]ovided.
The default limit is 100.
On 2012/06/19 23:38:33, Bob Nystrom wrote:
> That ] is in a weird place.

Telekinesis

http://codereview.chromium.org/10579008/diff/1/lib/unittest/core_matchers.dar...
lib/unittest/core_matchers.dart:96: // A utility function for comparing
iterators
On 2012/06/19 23:38:33, Bob Nystrom wrote:
> This comment doesn't add a lot of value.

Removed

http://codereview.chromium.org/10579008/diff/1/lib/unittest/core_matchers.dar...
lib/unittest/core_matchers.dart:128: // If _limit is 1 we can only recurse one
level into object
On 2012/06/19 23:38:33, Bob Nystrom wrote:
> Add a "."

Done.

http://codereview.chromium.org/10579008/diff/1/lib/unittest/core_matchers.dar...
lib/unittest/core_matchers.dart:130: if (expected != actual) {
On 2012/06/19 23:38:33, Bob Nystrom wrote:
> This is some painfully nested flow control. Can you flatten it out some?
> 
> Maybe do:
> 
> if (expected == actual) {
>   // Do nothing.
> } else if (depth > _limit) { 
>   ...
> }

Done.

http://codereview.chromium.org/10579008/diff/1/lib/unittest/core_matchers.dar...
lib/unittest/core_matchers.dart:132: reason =  new StringDescription('recursion
depth limit exceeded');
On 2012/06/19 23:38:33, Bob Nystrom wrote:
> Extra space before "new".

Done.

http://codereview.chromium.org/10579008/diff/1/lib/unittest/unittest.dart
File lib/unittest/unittest.dart (right):

http://codereview.chromium.org/10579008/diff/1/lib/unittest/unittest.dart#new...
lib/unittest/unittest.dart:514: [Function setupTest, Function teardownTest]) {
On 2012/06/19 23:38:33, Bob Nystrom wrote:
> Instead of taking these functions as arguments, how about making separate
> setUp() and tearDown() functions to register them, like:
> 
> group('some group', () {
>   setUp(() {
>     print('setting up...');
>   });
> 
>   tearDown(() {
>     print('tearing down...');
>   });
> 
>   test('some test', () {
>     print('this will print between "setting up" and "tearing down"');
>   });
> });

Done.

http://codereview.chromium.org/10579008/diff/1/lib/unittest/unittest.dart#new...
lib/unittest/unittest.dart:528: // settings for test setup/teardown
On 2012/06/19 23:38:33, Bob Nystrom wrote:
> .

Done.

[Sean Eagan, 2012-06-20 17:58:40]

Looks great!

[Sean Eagan, 2012-06-20 18:00:22]

https://chromiumcodereview.appspot.com/10579008/diff/1/lib/unittest/core_matc...
File lib/unittest/core_matchers.dart (right):

https://chromiumcodereview.appspot.com/10579008/diff/1/lib/unittest/core_matc...
lib/unittest/core_matchers.dart:552: 
Thanks for adding this!

Just a small gripe, I think "in" is better than "isIn" because the "is" prefix
is elsewhere used for Matcher constants ("isTrue", "isNull", "isZero" etc.) as
opposed to Matcher-returning methods such as this.

Also see http://dartbug.com/3661#c1 which notes that this matcher could subsume
all of the existing range based matchers if Dart had a Range/Interval interface.

[gram, 2012-06-20 18:01:49]

Unfortunately 'in' is a reserved word so we can't use it.

On 2012/06/20 18:00:22, seaneagan1 wrote:
>
https://chromiumcodereview.appspot.com/10579008/diff/1/lib/unittest/core_matc...
> File lib/unittest/core_matchers.dart (right):
> 
>
https://chromiumcodereview.appspot.com/10579008/diff/1/lib/unittest/core_matc...
> lib/unittest/core_matchers.dart:552: 
> Thanks for adding this!
> 
> Just a small gripe, I think "in" is better than "isIn" because the "is" prefix
> is elsewhere used for Matcher constants ("isTrue", "isNull", "isZero" etc.) as
> opposed to Matcher-returning methods such as this.
> 
> Also see http://dartbug.com/3661#c1 which notes that this matcher could
subsume
> all of the existing range based matchers if Dart had a Range/Interval
interface.

[Sean Eagan, 2012-06-20 18:37:06]

On 2012/06/20 18:01:49, gram wrote:
> Unfortunately 'in' is a reserved word so we can't use it.

Sorry, didn't realize that.  Now that I think about it, I wonder if
"containedBy" would make the association with "contains" more obvious.

[gram, 2012-06-20 18:45:06]

On 2012/06/20 18:37:06, seaneagan1 wrote:
> On 2012/06/20 18:01:49, gram wrote:
> > Unfortunately 'in' is a reserved word so we can't use it.
> 
> Sorry, didn't realize that.  Now that I think about it, I wonder if
> "containedBy" would make the association with "contains" more obvious.

Or we could go the other way, and change 'contains' to 'has'. 

'containedBy' does make the association clearer but I can't say I like it. isIn
scans better IMO.

[Sean Eagan, 2012-06-20 19:57:29]

On 2012/06/20 18:45:06, gram wrote:
> On 2012/06/20 18:37:06, seaneagan1 wrote:
> > On 2012/06/20 18:01:49, gram wrote:
> > > Unfortunately 'in' is a reserved word so we can't use it.
> > 
> > Sorry, didn't realize that.  Now that I think about it, I wonder if
> > "containedBy" would make the association with "contains" more obvious.
> 
> Or we could go the other way, and change 'contains' to 'has'. 
> 
> 'containedBy' does make the association clearer but I can't say I like it.
isIn
> scans better IMO.

Yeah, "isContainedBy" would read more naturally, but that's getting pretty long.

Also just noticed the "predicate" matcher.  You could probably get away with
just extending the "matches" matcher to accept predicates instead of adding a
new matcher.

[gram, 2012-06-20 20:03:39]

On 2012/06/20 19:57:29, seaneagan1 wrote:
> On 2012/06/20 18:45:06, gram wrote:
> > On 2012/06/20 18:37:06, seaneagan1 wrote:
> > > On 2012/06/20 18:01:49, gram wrote:
> > > > Unfortunately 'in' is a reserved word so we can't use it.
> > > 
> > > Sorry, didn't realize that.  Now that I think about it, I wonder if
> > > "containedBy" would make the association with "contains" more obvious.
> > 
> > Or we could go the other way, and change 'contains' to 'has'. 
> > 
> > 'containedBy' does make the association clearer but I can't say I like it.
> isIn
> > scans better IMO.
> 
> Yeah, "isContainedBy" would read more naturally, but that's getting pretty
long.
> 
> Also just noticed the "predicate" matcher.  You could probably get away with
> just extending the "matches" matcher to accept predicates instead of adding a
> new matcher.

Bob, what do you think of that idea?

[Bob Nystrom, 2012-06-20 20:21:22]

A couple of nits, but LGTM after you re-upload the changes to
core_matchers.dart.

https://chromiumcodereview.appspot.com/10579008/diff/5001/lib/unittest/unitte...
File lib/unittest/unittest.dart (right):

https://chromiumcodereview.appspot.com/10579008/diff/5001/lib/unittest/unitte...
lib/unittest/unittest.dart:519: [Function setupTest, Function teardownTest]) {
Since you added setUp() and tearDown(), do you still think it's useful to have
these here too?

https://chromiumcodereview.appspot.com/10579008/diff/5001/lib/unittest/unitte...
lib/unittest/unittest.dart:552: * are nested only the most locally scoped
[setUp] function will be run.
It might be useful at some point to actually allow nested setups and call all of
them (probably outermost first for setup and innermost first for teardown).

https://chromiumcodereview.appspot.com/10579008/diff/5001/lib/unittest/unitte...
lib/unittest/unittest.dart:557: void setUp(Function setupTest) {
Remove blank line above this.

https://chromiumcodereview.appspot.com/10579008/diff/5001/lib/unittest/unitte...
lib/unittest/unittest.dart:569: void tearDown(Function teardownTest) {
Remove blank line above this.

[gram, 2012-06-20 21:11:47]

https://chromiumcodereview.appspot.com/10579008/diff/5001/lib/unittest/unitte...
File lib/unittest/unittest.dart (right):

https://chromiumcodereview.appspot.com/10579008/diff/5001/lib/unittest/unitte...
lib/unittest/unittest.dart:519: [Function setupTest, Function teardownTest]) {
On 2012/06/20 20:21:22, Bob Nystrom wrote:
> Since you added setUp() and tearDown(), do you still think it's useful to have
> these here too?

Done.

https://chromiumcodereview.appspot.com/10579008/diff/5001/lib/unittest/unitte...
lib/unittest/unittest.dart:552: * are nested only the most locally scoped
[setUp] function will be run.
On 2012/06/20 20:21:22, Bob Nystrom wrote:
> It might be useful at some point to actually allow nested setups and call all
of
> them (probably outermost first for setup and innermost first for teardown).

I think this could be tricky. There may be cases where people want the the outer
ones run only once for the group, not once per inner test. The inner ones can
always call the outer ones so I think this restriction is okay.

https://chromiumcodereview.appspot.com/10579008/diff/5001/lib/unittest/unitte...
lib/unittest/unittest.dart:557: void setUp(Function setupTest) {
On 2012/06/20 20:21:22, Bob Nystrom wrote:
> Remove blank line above this.

Done.

https://chromiumcodereview.appspot.com/10579008/diff/5001/lib/unittest/unitte...
lib/unittest/unittest.dart:569: void tearDown(Function teardownTest) {
On 2012/06/20 20:21:22, Bob Nystrom wrote:
> Remove blank line above this.

Done.