Unit test improvements.

Unit test improvements.

The base config now sets things up so failed expects are routed through it.
A flag controls whether or not an expect failure stops the test (the default)
or if the test keeps pressing on.

Another flag has been added to disable the exception that gets thrown at the 
end of there were errors. The default remains to throw.

Finally, some cleaning up of test failure stacks is done. In particular we 
remove internal unittest frames from the stack.

R=sigmund@google.com

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

[Siggi Cherem (dart-lang), 2013-04-27 01:36:08]

+bob 

Is there any chance that the stack_trace package could be converted to work in a
platform independent way so that it can be used here too?

https://codereview.chromium.org/14021021/diff/1/pkg/unittest/lib/src/config.dart
File pkg/unittest/lib/src/config.dart (right):

https://codereview.chromium.org/14021021/diff/1/pkg/unittest/lib/src/config.d...
pkg/unittest/lib/src/config.dart:101: StringBuffer reason = new StringBuffer();
var

https://codereview.chromium.org/14021021/diff/1/pkg/unittest/lib/src/config.d...
pkg/unittest/lib/src/config.dart:102: for (var i = 0; i < _testLogBuffer.length
- 2; i+=2) {
fix spaces around binary operators (here and in the rest of this method):
i += 2
i + 1
_testLogBuffer.length - 1
_testLogBuffer.length - 2

https://codereview.chromium.org/14021021/diff/1/pkg/unittest/lib/unittest.dart
File pkg/unittest/lib/unittest.dart (right):

https://codereview.chromium.org/14021021/diff/1/pkg/unittest/lib/unittest.dar...
pkg/unittest/lib/unittest.dart:187: * a default configuration. */
multiline comment format fix: move */ to it's own line.

https://codereview.chromium.org/14021021/diff/1/pkg/unittest/lib/unittest.dar...
pkg/unittest/lib/unittest.dart:190: _config = new Configuration();
this is already done just before onInit, why do we need to set it here? 

Is it so people can adjust the default values? If so, then we can either:
 - make them create a new configuration object, or
 - remove the allocation from below and read unittestConfiguration instead.

https://codereview.chromium.org/14021021/diff/1/pkg/unittest/lib/unittest.dar...
pkg/unittest/lib/unittest.dart:876: String _formatStack(stack) {
as we were discussing offline. why can't we use the stack_trace package here?

Where do these stack traces work? in dart, dartium, dart2js? Is the stack trace
format the same in all of them?

https://codereview.chromium.org/14021021/diff/1/pkg/unittest/lib/unittest.dar...
pkg/unittest/lib/unittest.dart:890: for (var pass = 0; pass < 2; pass++) {
I'd prefer if we split this in 2 loops, so that each loop is just concerned with
one task. You might want to also first filter the lines you want to print out,
so the processing is only concerned with formatting at that point:

var relevantLines = lines.where((l) =>
  var match = firtMatch(line);
  return (match != null && match.indexOf('unittest') != 0))
});

int padding = relevantLines.fold(0, (n, line) => math.max(n,
line_member_thing.length));

for (line in relevantLines) { append to buffer }

https://codereview.chromium.org/14021021/diff/1/pkg/unittest/lib/unittest.dar...
pkg/unittest/lib/unittest.dart:896: "Couldn't parse stack trace line
'${lines[i]}' from $stack.");
I don't think we should throw if we can't parse a frame.

Unittest users want to know about the error in their code and would probably
prefer we show the original error rather than say that we couldn't parse the
stack trace.

https://codereview.chromium.org/14021021/diff/1/pkg/unittest/lib/unittest.dar...
pkg/unittest/lib/unittest.dart:900: if (location.indexOf('unittest/lib/') >= 0)
{
when running things outside of the repo, the stacktraces don't show 'lib'. Here
is an example stack trace in web_ui:

  #0      main.<anonymous closure>.<anonymous closure>
(file:///usr/local/google/home/sigmund/dart/web-ui-mirror/test/analyzer_test.dart:41:7)
  #1      TestCase._runTest (package:unittest/src/test_case.dart:82:25)
  #2      TestCase._run._run (package:unittest/src/test_case.dart:124:23)
  #3      _guardAsync (package:unittest/unittest.dart:727:19)
  #4      _nextBatch (package:unittest/unittest.dart:767:24)
  #5      runTests.runTests.<anonymous closure>
(package:unittest/unittest.dart:710:15)
  #6      _defer.<anonymous closure> (package:unittest/unittest.dart:669:44)
  #7      _ThenFuture._sendValue (dart:async:397:24)
  #8      _FutureImpl._handleValue.<anonymous closure> (dart:async:270:28)
  #9      Timer.run.<anonymous closure> (dart:async:2251:21)
  #10     Timer.Timer.<anonymous closure> (dart:async-patch:15:15)

[gram, 2013-04-29 21:24:18]

PTAL

https://codereview.chromium.org/14021021/diff/1/pkg/unittest/lib/src/config.dart
File pkg/unittest/lib/src/config.dart (right):

https://codereview.chromium.org/14021021/diff/1/pkg/unittest/lib/src/config.d...
pkg/unittest/lib/src/config.dart:101: StringBuffer reason = new StringBuffer();
On 2013/04/27 01:36:08, Siggi Cherem (dart-lang) wrote:
> var

Done.

https://codereview.chromium.org/14021021/diff/1/pkg/unittest/lib/src/config.d...
pkg/unittest/lib/src/config.dart:102: for (var i = 0; i < _testLogBuffer.length
- 2; i+=2) {
On 2013/04/27 01:36:08, Siggi Cherem (dart-lang) wrote:
> fix spaces around binary operators (here and in the rest of this method):
> i += 2
> i + 1
> _testLogBuffer.length - 1
> _testLogBuffer.length - 2

Done.

https://codereview.chromium.org/14021021/diff/1/pkg/unittest/lib/unittest.dart
File pkg/unittest/lib/unittest.dart (right):

https://codereview.chromium.org/14021021/diff/1/pkg/unittest/lib/unittest.dar...
pkg/unittest/lib/unittest.dart:187: * a default configuration. */
On 2013/04/27 01:36:08, Siggi Cherem (dart-lang) wrote:
> multiline comment format fix: move */ to it's own line.

Done.

https://codereview.chromium.org/14021021/diff/1/pkg/unittest/lib/unittest.dar...
pkg/unittest/lib/unittest.dart:190: _config = new Configuration();
On 2013/04/27 01:36:08, Siggi Cherem (dart-lang) wrote:
> this is already done just before onInit, why do we need to set it here? 
> 
> Is it so people can adjust the default values? If so, then we can either:
>  - make them create a new configuration object, or
>  - remove the allocation from below and read unittestConfiguration instead.

Yes, it is so they can set values. I don't want them to have to do an explicit
allocation if we can do a default one for them, which is why I avoided the first
option. I'm not sure I understand your second option; can you elaborate?

https://codereview.chromium.org/14021021/diff/1/pkg/unittest/lib/unittest.dar...
pkg/unittest/lib/unittest.dart:876: String _formatStack(stack) {
On 2013/04/27 01:36:08, Siggi Cherem (dart-lang) wrote:
> as we were discussing offline. why can't we use the stack_trace package here?
> 
> Where do these stack traces work? in dart, dartium, dart2js? Is the stack
trace
> format the same in all of them?

In the latest version they work in Firefox, Dartium, Chrome and VM (I haven't
tested IE or Opera). As we discussed, I don't want to take a dependency on a
package (yet), and most of the processing I am doing is specific to unit test.

https://codereview.chromium.org/14021021/diff/1/pkg/unittest/lib/unittest.dar...
pkg/unittest/lib/unittest.dart:890: for (var pass = 0; pass < 2; pass++) {
On 2013/04/27 01:36:08, Siggi Cherem (dart-lang) wrote:
> I'd prefer if we split this in 2 loops, so that each loop is just concerned
with
> one task. You might want to also first filter the lines you want to print out,
> so the processing is only concerned with formatting at that point:
> 
> var relevantLines = lines.where((l) =>
>   var match = firtMatch(line);
>   return (match != null && match.indexOf('unittest') != 0))
> });
> 
> int padding = relevantLines.fold(0, (n, line) => math.max(n,
> line_member_thing.length));
> 
> for (line in relevantLines) { append to buffer }

The filtering is not that conducive to this now that I handle multiple stack
formats but I have eliminated the loop.

https://codereview.chromium.org/14021021/diff/1/pkg/unittest/lib/unittest.dar...
pkg/unittest/lib/unittest.dart:896: "Couldn't parse stack trace line
'${lines[i]}' from $stack.");
On 2013/04/27 01:36:08, Siggi Cherem (dart-lang) wrote:
> I don't think we should throw if we can't parse a frame.
> 
> Unittest users want to know about the error in their code and would probably
> prefer we show the original error rather than say that we couldn't parse the
> stack trace.

Done.

https://codereview.chromium.org/14021021/diff/1/pkg/unittest/lib/unittest.dar...
pkg/unittest/lib/unittest.dart:900: if (location.indexOf('unittest/lib/') >= 0)
{
On 2013/04/27 01:36:08, Siggi Cherem (dart-lang) wrote:
> when running things outside of the repo, the stacktraces don't show 'lib'.
Here
> is an example stack trace in web_ui:
> 
>   #0      main.<anonymous closure>.<anonymous closure>
>
(file:///usr/local/google/home/sigmund/dart/web-ui-mirror/test/analyzer_test.dart:41:7)
>   #1      TestCase._runTest (package:unittest/src/test_case.dart:82:25)
>   #2      TestCase._run._run (package:unittest/src/test_case.dart:124:23)
>   #3      _guardAsync (package:unittest/unittest.dart:727:19)
>   #4      _nextBatch (package:unittest/unittest.dart:767:24)
>   #5      runTests.runTests.<anonymous closure>
> (package:unittest/unittest.dart:710:15)
>   #6      _defer.<anonymous closure> (package:unittest/unittest.dart:669:44)
>   #7      _ThenFuture._sendValue (dart:async:397:24)
>   #8      _FutureImpl._handleValue.<anonymous closure> (dart:async:270:28)
>   #9      Timer.run.<anonymous closure> (dart:async:2251:21)
>   #10     Timer.Timer.<anonymous closure> (dart:async-patch:15:15)

I've addressed this.

[Siggi Cherem (dart-lang), 2013-04-30 00:42:32]

Thanks Graham

LGTM, modulo the 'max' comment below.

https://codereview.chromium.org/14021021/diff/1/pkg/unittest/lib/unittest.dart
File pkg/unittest/lib/unittest.dart (right):

https://codereview.chromium.org/14021021/diff/1/pkg/unittest/lib/unittest.dar...
pkg/unittest/lib/unittest.dart:190: _config = new Configuration();
On 2013/04/29 21:24:18, gram wrote:
> On 2013/04/27 01:36:08, Siggi Cherem (dart-lang) wrote:
> > this is already done just before onInit, why do we need to set it here? 
> > 
> > Is it so people can adjust the default values? If so, then we can either:
> >  - make them create a new configuration object, or
> >  - remove the allocation from below and read unittestConfiguration instead.
> 
> Yes, it is so they can set values. I don't want them to have to do an explicit
> allocation if we can do a default one for them, which is why I avoided the
first
> option. I'm not sure I understand your second option; can you elaborate?

by option 2 I meant to consider replacing lines 832-835:

 if (_config == null) {
   unittestConfiguration = new Configuration();
 }
 _config.onInit();

by:
  unittestConfiguration.onInit();

https://codereview.chromium.org/14021021/diff/10001/pkg/unittest/lib/unittest...
File pkg/unittest/lib/unittest.dart (right):

https://codereview.chromium.org/14021021/diff/10001/pkg/unittest/lib/unittest...
pkg/unittest/lib/unittest.dart:873: // Stack fram lines are parsed with a
regexp, which has been tested
fram => frame

https://codereview.chromium.org/14021021/diff/10001/pkg/unittest/lib/unittest...
pkg/unittest/lib/unittest.dart:881: r'^\s*(?:(?:#\d+\s*)|(?:at
)|(?:))(.+)\s*[@\(]((?:.+:\/\/.+\/[^:]*)|(?:dart:[^:]*)|(?:package:[^:]*)):([:\d]+)[\)]?$');
is there a way to split this string in pieces to make it easier to understand?
new RegExp(
  r'^\s*'  // spaces
  r'(?:(?:#\d+\s*)| ....' // read the vm stack frame number (optional)
  ...

https://codereview.chromium.org/14021021/diff/10001/pkg/unittest/lib/unittest...
pkg/unittest/lib/unittest.dart:898: return match[1].length + 1;
missing call to max(n, *) here?

[gram, 2013-04-30 01:00:09]

PTAL

https://codereview.chromium.org/14021021/diff/1/pkg/unittest/lib/unittest.dart
File pkg/unittest/lib/unittest.dart (right):

https://codereview.chromium.org/14021021/diff/1/pkg/unittest/lib/unittest.dar...
pkg/unittest/lib/unittest.dart:190: _config = new Configuration();
On 2013/04/30 00:42:33, Siggi Cherem (dart-lang) wrote:
> On 2013/04/29 21:24:18, gram wrote:
> > On 2013/04/27 01:36:08, Siggi Cherem (dart-lang) wrote:
> > > this is already done just before onInit, why do we need to set it here? 
> > > 
> > > Is it so people can adjust the default values? If so, then we can either:
> > >  - make them create a new configuration object, or
> > >  - remove the allocation from below and read unittestConfiguration
instead.
> > 
> > Yes, it is so they can set values. I don't want them to have to do an
explicit
> > allocation if we can do a default one for them, which is why I avoided the
> first
> > option. I'm not sure I understand your second option; can you elaborate?
> 
> by option 2 I meant to consider replacing lines 832-835:
> 
>  if (_config == null) {
>    unittestConfiguration = new Configuration();
>  }
>  _config.onInit();
> 
> by:
>   unittestConfiguration.onInit();

Done.

https://codereview.chromium.org/14021021/diff/10001/pkg/unittest/lib/unittest...
File pkg/unittest/lib/unittest.dart (right):

https://codereview.chromium.org/14021021/diff/10001/pkg/unittest/lib/unittest...
pkg/unittest/lib/unittest.dart:873: // Stack fram lines are parsed with a
regexp, which has been tested
On 2013/04/30 00:42:33, Siggi Cherem (dart-lang) wrote:
> fram => frame

Done.

https://codereview.chromium.org/14021021/diff/10001/pkg/unittest/lib/unittest...
pkg/unittest/lib/unittest.dart:881: r'^\s*(?:(?:#\d+\s*)|(?:at
)|(?:))(.+)\s*[@\(]((?:.+:\/\/.+\/[^:]*)|(?:dart:[^:]*)|(?:package:[^:]*)):([:\d]+)[\)]?$');
On 2013/04/30 00:42:33, Siggi Cherem (dart-lang) wrote:
> is there a way to split this string in pieces to make it easier to understand?
> new RegExp(
>   r'^\s*'  // spaces
>   r'(?:(?:#\d+\s*)| ....' // read the vm stack frame number (optional)
>   ... 

Done.

https://codereview.chromium.org/14021021/diff/10001/pkg/unittest/lib/unittest...
pkg/unittest/lib/unittest.dart:898: return match[1].length + 1;
On 2013/04/30 00:42:33, Siggi Cherem (dart-lang) wrote:
> missing call to max(n, *) here?

Done.

[Siggi Cherem (dart-lang), 2013-04-30 02:26:57]

thanks! LGTM

https://codereview.chromium.org/14021021/diff/15001/pkg/unittest/lib/unittest...
File pkg/unittest/lib/unittest.dart (right):

https://codereview.chromium.org/14021021/diff/15001/pkg/unittest/lib/unittest...
pkg/unittest/lib/unittest.dart:172: import 'dart:math';
+ 'show max'
(it defines a lot of top-level names)

[gram, 2013-04-30 17:34:40]

Committed patchset #5 manually as r22186 (presubmit successful).