Improved the way we generate mismatch descriptions.

Improved the way we generate mismatch descriptions.
matches() methods can now return additional state that is used by describeMismatch(). This adds a small tax to the happy path, but makes the unhappy path quite a bit more efficient as well as providing better error messages.

Related to this, expect() can now be passed a verbose flag. Currently this is used by throwsA/returnsNormally to provide stack traces from the actual exception but could be used for other things too.

The tests for the Mock classes have been changed to be normal tests, not 'meta' tests. 

A couple of other minor fixes.
Committed: https://code.google.com/p/dart/source/detail?r=10078

[Siggi Cherem (dart-lang), 2012-07-31 17:16:40]

No need to split them now, but for the future reference I'd prefer to have
separate CLs when the changes are so unrelated to each other (in this case
MatcherState vs Mock tests).

A couple general questions:
- I assume the mock tests are unchanged (just moved over) is that right?
- on MatcherState, can we use this to get more output on orderedEquals? (e.g.
when the length is a mismatch, I still want to see the full list in the error
message)

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

http://codereview.chromium.org/10832058/diff/4001/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:26: 'state': matchState.state
so state is a list of states?

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

http://codereview.chromium.org/10832058/diff/4001/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:235: *     or twice upon failure (the second
time to get the failure description).
Regarding this part - is it still true that we call the function twice? with
MatcherState you no longer need to do this, right?

http://codereview.chromium.org/10832058/diff/4001/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:307: } else if (_matcher.matches(e, matchState))
{
seems you can keep it as
if (_matcher == null || _matcher.matches(e, matchState)) {
  return true;
} else {
 ...
}

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

http://codereview.chromium.org/10832058/diff/4001/lib/unittest/interfaces.dar...
lib/unittest/interfaces.dart:59: bool matches(item, MatchState matchState);
should matchState be optional?

http://codereview.chromium.org/10832058/diff/4001/lib/unittest/interfaces.dar...
lib/unittest/interfaces.dart:75: MatchState matchState, bool verbose);
should matchState + verbose be optional?

http://codereview.chromium.org/10832058/diff/4001/tests/lib/unittest/matchers...
File tests/lib/unittest/matchers_test.dart (right):

http://codereview.chromium.org/10832058/diff/4001/tests/lib/unittest/matchers...
tests/lib/unittest/matchers_test.dart:415: "Expected: every element <1> but: was
<2> at position 1.");
I felt i had more information in the old message (it helps to see the whole list
as context)

http://codereview.chromium.org/10832058/diff/4001/tests/lib/unittest/mock_tes...
File tests/lib/unittest/mock_test.dart (right):

http://codereview.chromium.org/10832058/diff/4001/tests/lib/unittest/mock_tes...
tests/lib/unittest/mock_test.dart:117: m.foo();
I prefer the line breaks you had in the previous test. 

expect(() { m.foo(); }, returnsNormally);

In this case, because the body is just a single expression, the style preference
is to use => :

expect(() => m.foo, returnsNormally);

(here and throughout this file)

http://codereview.chromium.org/10832058/diff/4001/tests/lib/unittest/mock_tes...
tests/lib/unittest/mock_test.dart:612: 
remove empty lines...

[gram, 2012-07-31 17:56:01]

PTAL

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

http://codereview.chromium.org/10832058/diff/4001/lib/unittest/collection_mat...
lib/unittest/collection_matchers.dart:26: 'state': matchState.state
On 2012/07/31 17:16:40, sigmund wrote:
> so state is a list of states?

No; we fail fast, so it has the mismatch state just for the first failure.

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

http://codereview.chromium.org/10832058/diff/4001/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:235: *     or twice upon failure (the second
time to get the failure description).
On 2012/07/31 17:16:40, sigmund wrote:
> Regarding this part - is it still true that we call the function twice? with
> MatcherState you no longer need to do this, right?

Actually the double call was removed some time back, but in the process we lost
the details of whether the failure was due to not throwing or due to a
mismatched throw. We can now bring that detail (and more) back.

http://codereview.chromium.org/10832058/diff/4001/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:307: } else if (_matcher.matches(e, matchState))
{
On 2012/07/31 17:16:40, sigmund wrote:
> seems you can keep it as
> if (_matcher == null || _matcher.matches(e, matchState)) {
>   return true;
> } else {
>  ...
> }

Done.

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

http://codereview.chromium.org/10832058/diff/4001/lib/unittest/interfaces.dar...
lib/unittest/interfaces.dart:59: bool matches(item, MatchState matchState);
On 2012/07/31 17:16:40, sigmund wrote:
> should matchState be optional?

I decided not to do this because the initial call to matches happens only in one
place (expect) which is going to pass a value in, and that value is used
recursively. So there is no need right now for it to be optional, and making it
optional  means we have to add tests for null wherever we have a failure and are
going to use it. SO there would be cost with no benefit.

http://codereview.chromium.org/10832058/diff/4001/lib/unittest/interfaces.dar...
lib/unittest/interfaces.dart:75: MatchState matchState, bool verbose);
On 2012/07/31 17:16:40, sigmund wrote:
> should matchState + verbose be optional?

Similarly, there is currently only one place where we pass this in initially, so
there is no need to make it optional.

http://codereview.chromium.org/10832058/diff/4001/tests/lib/unittest/matchers...
File tests/lib/unittest/matchers_test.dart (right):

http://codereview.chromium.org/10832058/diff/4001/tests/lib/unittest/matchers...
tests/lib/unittest/matchers_test.dart:415: "Expected: every element <1> but: was
<2> at position 1.");
On 2012/07/31 17:16:40, sigmund wrote:
> I felt i had more information in the old message (it helps to see the whole
list
> as context)

I will add this detail for the verbose flag.

[Siggi Cherem (dart-lang), 2012-07-31 17:59:43]

Thanks! lgtm!

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

http://codereview.chromium.org/10832058/diff/4001/lib/unittest/core_matchers....
lib/unittest/core_matchers.dart:235: *     or twice upon failure (the second
time to get the failure description).
On 2012/07/31 17:56:01, gram wrote:
> On 2012/07/31 17:16:40, sigmund wrote:
> > Regarding this part - is it still true that we call the function twice? with
> > MatcherState you no longer need to do this, right?
> 
> Actually the double call was removed some time back, but in the process we
lost
> the details of whether the failure was due to not throwing or due to a
> mismatched throw. We can now bring that detail (and more) back.
> 

Cool, let's fix the comment then :)