Add pretty-printing to unittest and more thoroughly print values.

Add pretty-printing to unittest and more thoroughly print values.

This adds a prettyPrint function that knows to respect line lengths and avoid
printing too much of very large objects. Because that function has built-in
guards against over-printing, the default error formatter is changed to print
the actual value by default.

This also tweaks the output of some matchers to take advantage of the assumption
that the full actual value will always be printed.

R=gram@google.com

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

[gram, 2013-05-17 01:00:30]

https://codereview.chromium.org/14600029/diff/1/pkg/unittest/lib/src/core_mat...
File pkg/unittest/lib/src/core_matchers.dart (left):

https://codereview.chromium.org/14600029/diff/1/pkg/unittest/lib/src/core_mat...
pkg/unittest/lib/src/core_matchers.dart:178: var includeTypes = eType != aType;
What was the motivation in taking this out? John requested a while back that we
display mismatched types so that when things print the same but the types are
different it can still be detected.

https://codereview.chromium.org/14600029/diff/1/pkg/unittest/lib/src/pretty_p...
File pkg/unittest/lib/src/pretty_print.dart (right):

https://codereview.chromium.org/14600029/diff/1/pkg/unittest/lib/src/pretty_p...
pkg/unittest/lib/src/pretty_print.dart:28: 
Please add a short comment at the start of each of these logical blocks of code
here to explain their function.

https://codereview.chromium.org/14600029/diff/1/pkg/unittest/test/matchers_te...
File pkg/unittest/test/matchers_test.dart (right):

https://codereview.chromium.org/14600029/diff/1/pkg/unittest/test/matchers_te...
pkg/unittest/test/matchers_test.dart:207: "but:  exception <Exception> does not
match "
I think now that we have Expected and Actual both capitalized we should
capitalize But: too.

https://codereview.chromium.org/14600029/diff/1/pkg/unittest/test/matchers_te...
pkg/unittest/test/matchers_test.dart:653: });
You seem to have established a new pattern for output of the form:

Expected: <something>
But: <reason>
Actual: <value>

This and some other matchers are not following that pattern but I think they
should; i.e. in this case something like:

Expected: contains value 'ba'
But: value 'ba' not found
Actual: {'foo': 'bar'}.

There are more cases above and below.

https://codereview.chromium.org/14600029/diff/1/pkg/unittest/test/matchers_te...
pkg/unittest/test/matchers_test.dart:693: "but: was <-1> (a value greater than
<0>).");
Another inconsistent case.

https://codereview.chromium.org/14600029/diff/1/pkg/unittest/test/matchers_te...
pkg/unittest/test/matchers_test.dart:705: isAsync: true);
Inconsistent pattern here and more below

[nweiz, 2013-05-17 19:50:56]

https://codereview.chromium.org/14600029/diff/1/pkg/unittest/lib/src/core_mat...
File pkg/unittest/lib/src/core_matchers.dart (left):

https://codereview.chromium.org/14600029/diff/1/pkg/unittest/lib/src/core_mat...
pkg/unittest/lib/src/core_matchers.dart:178: var includeTypes = eType != aType;
On 2013/05/17 01:00:30, gram wrote:
> What was the motivation in taking this out? John requested a while back that
we
> display mismatched types so that when things print the same but the types are
> different it can still be detected.

This functionality is in prettyPrint now. For objects where the types aren't
obvious -- anything other than a num, bool, Function, null, List, Map, or String
-- the type will be printed as part of the pretty-printing (as long as it's
public).

https://codereview.chromium.org/14600029/diff/1/pkg/unittest/lib/src/pretty_p...
File pkg/unittest/lib/src/pretty_print.dart (right):

https://codereview.chromium.org/14600029/diff/1/pkg/unittest/lib/src/pretty_p...
pkg/unittest/lib/src/pretty_print.dart:28: 
On 2013/05/17 01:00:30, gram wrote:
> Please add a short comment at the start of each of these logical blocks of
code
> here to explain their function.

Done.

https://codereview.chromium.org/14600029/diff/1/pkg/unittest/test/matchers_te...
File pkg/unittest/test/matchers_test.dart (right):

https://codereview.chromium.org/14600029/diff/1/pkg/unittest/test/matchers_te...
pkg/unittest/test/matchers_test.dart:207: "but:  exception <Exception> does not
match "
On 2013/05/17 01:00:30, gram wrote:
> I think now that we have Expected and Actual both capitalized we should
> capitalize But: too.

Done.

https://codereview.chromium.org/14600029/diff/1/pkg/unittest/test/matchers_te...
pkg/unittest/test/matchers_test.dart:653: });
On 2013/05/17 01:00:30, gram wrote:
> You seem to have established a new pattern for output of the form:
> 
> Expected: <something>
> But: <reason>
> Actual: <value>
> 
> This and some other matchers are not following that pattern but I think they
> should; i.e. in this case something like:
> 
> Expected: contains value 'ba'
> But: value 'ba' not found
> Actual: {'foo': 'bar'}.
> 
> There are more cases above and below.

It seems weird to me when the "But" clause would just be "the thing we expected
to be true wasn't." If that doesn't provide any additional information, I think
it makes sense just to display the actual value.

For example, in this case, saying "But: value 'ba' not found" doesn't provide
any information that "Expected: contains value 'ba'" doesn't.

https://codereview.chromium.org/14600029/diff/1/pkg/unittest/test/matchers_te...
pkg/unittest/test/matchers_test.dart:693: "but: was <-1> (a value greater than
<0>).");
On 2013/05/17 01:00:30, gram wrote:
> Another inconsistent case.

This case is weird. If all of the uses of [allOf] looked just like this, I'd
agree; something like "but: wasn't a value greater than <0>" would look better.
However, [allOf] needs to call [describeMismatch] on the matcher that failed, to
give it a change to provide its own description of the failure if it's more
complex. It's also important that we provide enough information for the user to
tell which of the matchers failed. So I think something like this current format
is necessary.

https://codereview.chromium.org/14600029/diff/1/pkg/unittest/test/matchers_te...
pkg/unittest/test/matchers_test.dart:705: isAsync: true);
On 2013/05/17 01:00:30, gram wrote:
> Inconsistent pattern here and more below

I think the future matchers just work completely differently than the stuff I'm
changing.

[gram, 2013-05-17 20:38:32]

lgtm with one nit about 'Unknown" types.

https://codereview.chromium.org/14600029/diff/1/pkg/unittest/lib/src/core_mat...
File pkg/unittest/lib/src/core_matchers.dart (left):

https://codereview.chromium.org/14600029/diff/1/pkg/unittest/lib/src/core_mat...
pkg/unittest/lib/src/core_matchers.dart:178: var includeTypes = eType != aType;
Yes, sorry, I should have deleted this; I spotted the change in prettyPrint
later in the review.

On 2013/05/17 19:50:56, nweiz wrote:
> On 2013/05/17 01:00:30, gram wrote:
> > What was the motivation in taking this out? John requested a while back that
> we
> > display mismatched types so that when things print the same but the types
are
> > different it can still be detected.
> 
> This functionality is in prettyPrint now. For objects where the types aren't
> obvious -- anything other than a num, bool, Function, null, List, Map, or
String
> -- the type will be printed as part of the pretty-printing (as long as it's
> public).

https://codereview.chromium.org/14600029/diff/1/pkg/unittest/test/matchers_te...
File pkg/unittest/test/matchers_test.dart (right):

https://codereview.chromium.org/14600029/diff/1/pkg/unittest/test/matchers_te...
pkg/unittest/test/matchers_test.dart:653: });
I agree with you that it could be weird, but having a consistent format lends
itself better to automated tools that might post-process the test output.

OTOH I guess you could argue that the "Actual:" part is an optional extension of
the non-optional "but:" part, and that it is still easy enough to post-process,
so my feelings are mixed about this. I'll leave it as your call.

On 2013/05/17 19:50:56, nweiz wrote:
> On 2013/05/17 01:00:30, gram wrote:
> > You seem to have established a new pattern for output of the form:
> > 
> > Expected: <something>
> > But: <reason>
> > Actual: <value>
> > 
> > This and some other matchers are not following that pattern but I think they
> > should; i.e. in this case something like:
> > 
> > Expected: contains value 'ba'
> > But: value 'ba' not found
> > Actual: {'foo': 'bar'}.
> > 
> > There are more cases above and below.
> 
> It seems weird to me when the "But" clause would just be "the thing we
expected
> to be true wasn't." If that doesn't provide any additional information, I
think
> it makes sense just to display the actual value.
> 
> For example, in this case, saying "But: value 'ba' not found" doesn't provide
> any information that "Expected: contains value 'ba'" doesn't.

https://codereview.chromium.org/14600029/diff/4001/pkg/unittest/lib/src/prett...
File pkg/unittest/lib/src/pretty_print.dart (right):

https://codereview.chromium.org/14600029/diff/4001/pkg/unittest/lib/src/prett...
pkg/unittest/lib/src/pretty_print.dart:94: return "${typeName(object)}:$value";
The latest version has a lot of "Unknown:<value>" cases. Can we skip the type if
it is going to print as Unknown? 'Unknown" doesn't seem to add much value.

[nweiz, 2013-05-17 21:07:21]

https://codereview.chromium.org/14600029/diff/1/pkg/unittest/test/matchers_te...
File pkg/unittest/test/matchers_test.dart (right):

https://codereview.chromium.org/14600029/diff/1/pkg/unittest/test/matchers_te...
pkg/unittest/test/matchers_test.dart:653: });
On 2013/05/17 20:38:32, gram wrote:
> I agree with you that it could be weird, but having a consistent format lends
> itself better to automated tools that might post-process the test output.
> 
> OTOH I guess you could argue that the "Actual:" part is an optional extension
of
> the non-optional "but:" part, and that it is still easy enough to
post-process,
> so my feelings are mixed about this. I'll leave it as your call.

I think there's room to improve and normalize these, but I think it's out of
scope for this change, so I'm going to leave it for now.

https://codereview.chromium.org/14600029/diff/4001/pkg/unittest/lib/src/prett...
File pkg/unittest/lib/src/pretty_print.dart (right):

https://codereview.chromium.org/14600029/diff/4001/pkg/unittest/lib/src/prett...
pkg/unittest/lib/src/pretty_print.dart:94: return "${typeName(object)}:$value";
On 2013/05/17 20:38:33, gram wrote:
> The latest version has a lot of "Unknown:<value>" cases. Can we skip the type
if
> it is going to print as Unknown? 'Unknown" doesn't seem to add much value.

Changed to "?" as per offline discussion.

[nweiz, 2013-05-17 21:10:24]

Committed patchset #2 manually as r22873 (presubmit successful).