When we have excess callbacks, throw instead of calling error() so that we

When we have excess callbacks, throw instead of calling error() so that we
get a stack trace.
Clean up SpreadArgsHelper.
We can now specify a min and max count for callbacks.
Split onTestResult into two - onTestResult and onTestResultChanged. The
first one is called initially, and the second if further callbacks occur 
after the test was considered complete, or if teardown fails.

See https://code.google.com/p/dart/issues/detail?id=8660
Committed: https://code.google.com/p/dart/source/detail?r=19269

[Jennifer Messerly, 2013-02-27 22:42:55]

lgtm!

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

https://codereview.chromium.org/12366004/diff/1/pkg/unittest/lib/unittest.dar...
pkg/unittest/lib/unittest.dart:440: return false;
remove?

[Siggi Cherem (dart-lang), 2013-02-27 23:02:21]

same here: lgtm, but would be great to make the check a void function now.

[gram, 2013-02-28 01:56:29]

Siggy, PTAL. I have reworked SpreadArgsHelper to hopefully be cleaner and more
flexible.

[gram, 2013-02-28 17:49:25]

Adding Kevin

[kevmoo-old, 2013-02-28 18:02:19]

I'm seeing a failure after applying your patch to r19251 on a mac

FAIL: late exception test
  Expected: '1:0:1:2:0:::null:testOne:Callback called after test case testOne
has already been marked as done.:testTwo:'
       but: was '0:1:1:2:0:::null:testOne:Caught
'package:unittest/src/config.dart': Failed assertion: line 66 pos 12:
'_currentTestCase == null || _currentTestCase == testCase' is not
true.:testTwo:Caught 'package:unittest/src/config.dart': Failed assertion: line
66 pos 12: '_currentTestCase == null || _currentTestCase == testCase' is not
true.'.
  
  #0      DefaultFailureHandler.fail (package:unittest/src/expect.dart:105:5)
  #1      DefaultFailureHandler.failMatch
(package:unittest/src/expect.dart:109:9)
  #2      expect (package:unittest/src/expect.dart:67:29)
  #3      nextTest.<anonymous closure>.<anonymous closure>
(file:///Users/kevin/source/google/dart-repo/dart/pkg/unittest/test/unittest_test.dart:344:36)
  #4      TestCase._runTest (package:unittest/src/test_case.dart:81:17)
  #5      TestCase.run.run (package:unittest/src/test_case.dart:123:23)
  #6      guardAsync (package:unittest/unittest.dart:724:19)
  #0      _nextBatch._nextBatch (package:unittest/unittest.dart:764:23)
  #1      runTests.runTests.<anonymous closure>
(package:unittest/unittest.dart:713:16)
  #2      _defer.<anonymous closure> (package:unittest/unittest.dart:673:52)
  #3      _ThenFuture._sendValue (dart:async:422:24)
  #4      _FutureImpl._handleValue.<anonymous closure> (dart:async:296:28)
  #5      Timer.run.<anonymous closure> (dart:async:2176:21)
  #6      Timer.Timer.<anonymous closure> (dart:async-patch:9:15)

[kevmoo-old, 2013-02-28 18:03:37]

Hopefully this is my asserts adding more value than headache...

[Siggi Cherem (dart-lang), 2013-02-28 19:29:23]

some minor comments, but ow looks nice!

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

https://codereview.chromium.org/12366004/diff/8001/pkg/unittest/lib/unittest....
pkg/unittest/lib/unittest.dart:291: int _minExpectedCalls;
Now that we don't have _init, can we make some of these final?

I think we can do many of them

https://codereview.chromium.org/12366004/diff/8001/pkg/unittest/lib/unittest....
pkg/unittest/lib/unittest.dart:298: bool _complete;
since this class is private and we don't return it, we can actually remove the _
from all these fields

https://codereview.chromium.org/12366004/diff/8001/pkg/unittest/lib/unittest....
pkg/unittest/lib/unittest.dart:301: _SpreadArgsHelper(Function callback, int
minExpected, int maxExpected,
more precisely:
_SpreadArgsHelper(Function callback, this.minExpected, int maxExpected,
this._isDone, String id)
    : this.callback = callback,
      this.maxExpected = (maxExpected == 0 && minExpected > 0) ? minExpected :
maxExpected,
      this.id = _computeId(id, callback) {
    ...
}

https://codereview.chromium.org/12366004/diff/8001/pkg/unittest/lib/unittest....
pkg/unittest/lib/unittest.dart:505: return new _SpreadArgsHelper(callback, 0,
-1, isDone, id).invoke;
strange - I was expecting that max was always >= 0.

[gram, 2013-02-28 22:49:50]

Added onTestResultChanged handler to config.dart.
Made members of SpreadArgsHandler not start with '_' and made them final where
applicable.
Removed currentTestCase from config.dart; added it to unittest.dart

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

https://codereview.chromium.org/12366004/diff/8001/pkg/unittest/lib/unittest....
pkg/unittest/lib/unittest.dart:291: int _minExpectedCalls;
On 2013/02/28 19:29:23, Siggi Cherem (dart-lang) wrote:
> Now that we don't have _init, can we make some of these final?
> 
> I think we can do many of them

Done.

https://codereview.chromium.org/12366004/diff/8001/pkg/unittest/lib/unittest....
pkg/unittest/lib/unittest.dart:298: bool _complete;
On 2013/02/28 19:29:23, Siggi Cherem (dart-lang) wrote:
> since this class is private and we don't return it, we can actually remove the
_
> from all these fields

Done.

https://codereview.chromium.org/12366004/diff/8001/pkg/unittest/lib/unittest....
pkg/unittest/lib/unittest.dart:301: _SpreadArgsHelper(Function callback, int
minExpected, int maxExpected,
On 2013/02/28 19:29:23, Siggi Cherem (dart-lang) wrote:
> more precisely:
> _SpreadArgsHelper(Function callback, this.minExpected, int maxExpected,
> this._isDone, String id)
>     : this.callback = callback,
>       this.maxExpected = (maxExpected == 0 && minExpected > 0) ? minExpected :
> maxExpected,
>       this.id = _computeId(id, callback) {
>     ...
> }

Done.

https://codereview.chromium.org/12366004/diff/8001/pkg/unittest/lib/unittest....
pkg/unittest/lib/unittest.dart:505: return new _SpreadArgsHelper(callback, 0,
-1, isDone, id).invoke;
On 2013/02/28 19:29:23, Siggi Cherem (dart-lang) wrote:
> strange - I was expecting that max was always >= 0.

-1 means unlimited

[kevmoo-old, 2013-02-28 22:54:21]

lgtm

love the API change
pkg/unittest tests passed locally

[Siggi Cherem (dart-lang), 2013-02-28 23:31:53]

https://codereview.chromium.org/12366004/diff/9003/pkg/unittest/lib/src/confi...
File pkg/unittest/lib/src/config.dart (right):

https://codereview.chromium.org/12366004/diff/9003/pkg/unittest/lib/src/confi...
pkg/unittest/lib/src/config.dart:69: assert(testCase != null);
remove trailing space/tab

https://codereview.chromium.org/12366004/diff/9003/pkg/unittest/lib/src/confi...
pkg/unittest/lib/src/config.dart:78: if (currentTestCase == null) {
but currentTestCase doesn't exist anymore?

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

https://codereview.chromium.org/12366004/diff/9003/pkg/unittest/lib/unittest....
pkg/unittest/lib/unittest.dart:303: int testNum;
can this be final too?

https://codereview.chromium.org/12366004/diff/9003/pkg/unittest/lib/unittest....
pkg/unittest/lib/unittest.dart:308: Function isDone, String id) :
constructor style nits:
* indent this line only + 4
* move ':' to the next line, and indent it also +4
* indent + 2 the line after it. 

For example:

  _SpreadArgsHelper(Function callback, int minExpected, int maxExpected,
      Function isDone, String id)
      : this.callback = callback,
        this.minExpectCalls = ...

https://codereview.chromium.org/12366004/diff/9003/pkg/unittest/lib/unittest....
pkg/unittest/lib/unittest.dart:310: this.minExpectedCalls = minExpected,
remove 'this'
(it is only necessary if the name is different than the argument name.)

https://codereview.chromium.org/12366004/diff/9003/pkg/unittest/lib/unittest....
pkg/unittest/lib/unittest.dart:311: this.maxExpectedCalls = (maxExpected == 0 &&
minExpected > 0) ? minExpected : maxExpected,
long line: wrap after before '?'

https://codereview.chromium.org/12366004/diff/9003/pkg/unittest/lib/unittest....
pkg/unittest/lib/unittest.dart:332: // Try to create a reasonable id.
by moving this code to a static private function, you can use it in an
initializer and make 'id' final

https://codereview.chromium.org/12366004/diff/9003/pkg/unittest/lib/unittest....
pkg/unittest/lib/unittest.dart:362: '');
nit: move this to the previous line.

https://codereview.chromium.org/12366004/diff/9003/pkg/unittest/lib/unittest....
pkg/unittest/lib/unittest.dart:374: if (minExpectedCalls > 0 && actualCalls <
minExpectedCalls) {
nit: if it fits in one line, do the fast-exit pattern here:

if (minExpectedCalls > 0 && actualCalls < minExpectedCalls) return;
if (isDone != null && !isDone()) return;

https://codereview.chromium.org/12366004/diff/9003/pkg/unittest/lib/unittest....
pkg/unittest/lib/unittest.dart:473: * called exactly [count] times. A value of
-1 for [max] will mean no upper
thanks =)

[kevmoo-old, 2013-02-28 23:33:47]

https://codereview.chromium.org/12366004/diff/9003/pkg/unittest/lib/src/confi...
File pkg/unittest/lib/src/config.dart (right):

https://codereview.chromium.org/12366004/diff/9003/pkg/unittest/lib/src/confi...
pkg/unittest/lib/src/config.dart:78: if (currentTestCase == null) {
It's now top-level

[Siggi Cherem (dart-lang), 2013-02-28 23:34:59]

On 2013/02/28 23:33:47, kevmoo wrote:
>
https://codereview.chromium.org/12366004/diff/9003/pkg/unittest/lib/src/confi...
> File pkg/unittest/lib/src/config.dart (right):
> 
>
https://codereview.chromium.org/12366004/diff/9003/pkg/unittest/lib/src/confi...
> pkg/unittest/lib/src/config.dart:78: if (currentTestCase == null) {
> It's now top-level

oops, thanks! I saw it later and forgot to remove the comment after I finished
the review =)

lgtm!

[gram, 2013-03-01 00:27:54]

Committed patchset #7 manually as r19269 (presubmit successful).

[gram, 2013-03-04 19:51:25]

Publishing old draft comments

https://codereview.chromium.org/12366004/diff/9003/pkg/unittest/lib/src/confi...
File pkg/unittest/lib/src/config.dart (right):

https://codereview.chromium.org/12366004/diff/9003/pkg/unittest/lib/src/confi...
pkg/unittest/lib/src/config.dart:69: assert(testCase != null);
On 2013/02/28 23:31:53, Siggi Cherem (dart-lang) wrote:
> remove trailing space/tab

Done.

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

https://codereview.chromium.org/12366004/diff/9003/pkg/unittest/lib/unittest....
pkg/unittest/lib/unittest.dart:303: int testNum;
On 2013/02/28 23:31:53, Siggi Cherem (dart-lang) wrote:
> can this be final too?

Done.

https://codereview.chromium.org/12366004/diff/9003/pkg/unittest/lib/unittest....
pkg/unittest/lib/unittest.dart:308: Function isDone, String id) :
On 2013/02/28 23:31:53, Siggi Cherem (dart-lang) wrote:
> constructor style nits:
> * indent this line only + 4
> * move ':' to the next line, and indent it also +4
> * indent + 2 the line after it. 
> 
> For example:
> 
>   _SpreadArgsHelper(Function callback, int minExpected, int maxExpected,
>       Function isDone, String id)
>       : this.callback = callback,
>         this.minExpectCalls = ... 

Done.

https://codereview.chromium.org/12366004/diff/9003/pkg/unittest/lib/unittest....
pkg/unittest/lib/unittest.dart:310: this.minExpectedCalls = minExpected,
On 2013/02/28 23:31:53, Siggi Cherem (dart-lang) wrote:
> remove 'this'
> (it is only necessary if the name is different than the argument name.)

Done.

https://codereview.chromium.org/12366004/diff/9003/pkg/unittest/lib/unittest....
pkg/unittest/lib/unittest.dart:311: this.maxExpectedCalls = (maxExpected == 0 &&
minExpected > 0) ? minExpected : maxExpected,
On 2013/02/28 23:31:53, Siggi Cherem (dart-lang) wrote:
> long line: wrap after before '?'

Done.

https://codereview.chromium.org/12366004/diff/9003/pkg/unittest/lib/unittest....
pkg/unittest/lib/unittest.dart:332: // Try to create a reasonable id.
On 2013/02/28 23:31:53, Siggi Cherem (dart-lang) wrote:
> by moving this code to a static private function, you can use it in an
> initializer and make 'id' final

Done.

https://codereview.chromium.org/12366004/diff/9003/pkg/unittest/lib/unittest....
pkg/unittest/lib/unittest.dart:362: '');
On 2013/02/28 23:31:53, Siggi Cherem (dart-lang) wrote:
> nit: move this to the previous line.

Done.

https://codereview.chromium.org/12366004/diff/9003/pkg/unittest/lib/unittest....
pkg/unittest/lib/unittest.dart:374: if (minExpectedCalls > 0 && actualCalls <
minExpectedCalls) {
On 2013/02/28 23:31:53, Siggi Cherem (dart-lang) wrote:
> nit: if it fits in one line, do the fast-exit pattern here:
> 
> if (minExpectedCalls > 0 && actualCalls < minExpectedCalls) return;
> if (isDone != null && !isDone()) return;

Done.

https://codereview.chromium.org/12366004/diff/9003/pkg/unittest/lib/unittest....
pkg/unittest/lib/unittest.dart:473: * called exactly [count] times. A value of
-1 for [max] will mean no upper
On 2013/02/28 23:31:53, Siggi Cherem (dart-lang) wrote:
> thanks =)

Done.