Some unit test fixes:

Some unit test fixes:
Fixed a couple of issues in the ensureInitialized() function.
Added expectAsyncUntil methods to support callbackDone style tests.
Split the matcher tests up so that we can exclude those that use isInstance
from the environments where they don't work.
Eliminated the need for two functions calls in throws/throwsA.
Fixed a couple of tests.
Committed: https://code.google.com/p/dart/source/detail?r=8668

[gram, 2012-06-13 23:30:48]

PTAL at these. For now I have backed out all my tests of the unit test framework
itself as I am having a hard time getting that to work in the different
runtimes. If these can get in then I can focus on that problem in isolation.

[Siggi Cherem (dart-lang), 2012-06-13 23:43:53]

http://codereview.chromium.org/10545167/diff/10/lib/unittest/unittest.dart
File lib/unittest/unittest.dart (right):

http://codereview.chromium.org/10545167/diff/10/lib/unittest/unittest.dart#ne...
lib/unittest/unittest.dart:306: _Init(Function callback, Function
shouldCallBack, Function isDone,
-> lower case.

http://codereview.chromium.org/10545167/diff/10/lib/unittest/unittest.dart#ne...
lib/unittest/unittest.dart:317: _SpreadArgsHelper( callback, shouldCallBack,
isDone) {
remove extra space

http://codereview.chromium.org/10545167/diff/10/lib/unittest/unittest.dart#ne...
lib/unittest/unittest.dart:321: _SpreadArgsHelper.fixedCallCount(callback,
expectedCalls) {
let's make _callback final and change all 3 constructors to say 'this._callback'

http://codereview.chromium.org/10545167/diff/10/lib/unittest/unittest.dart#ne...
lib/unittest/unittest.dart:330: if (_isDone())
style: add { } or merge this and the next line in a single line.

http://codereview.chromium.org/10545167/diff/10/lib/unittest/unittest.dart#ne...
lib/unittest/unittest.dart:453: Function expectAsyncUntil(Function callback,
Function isDone) {
make this private until issue 2706 gets fixed

http://codereview.chromium.org/10545167/diff/10/tests/lib/unittest/instance_t...
File tests/lib/unittest/instance_test.dart (right):

http://codereview.chromium.org/10545167/diff/10/tests/lib/unittest/instance_t...
tests/lib/unittest/instance_test.dart:6: #library('instanceTest');
=> instance_test (we are not consistent in the codebase, but I strongly prefer
this)

http://codereview.chromium.org/10545167/diff/10/tests/lib/unittest/instance_t...
tests/lib/unittest/instance_test.dart:30: 
remove these empty lines at the bottom

http://codereview.chromium.org/10545167/diff/10/tests/lib/unittest/testers.dart
File tests/lib/unittest/testers.dart (right):

http://codereview.chromium.org/10545167/diff/10/tests/lib/unittest/testers.da...
tests/lib/unittest/testers.dart:4: 
let's rename this file, ideas:
- test_utils.dart
- test_helpers.dart

http://codereview.chromium.org/10545167/diff/10/tests/lib/unittest/testers.da...
tests/lib/unittest/testers.dart:16: void initTesters() {
rename according to file rename...

http://codereview.chromium.org/10545167/diff/10/tests/lib/unittest/testers.da...
tests/lib/unittest/testers.dart:43: 
empty line

[Bob Nystrom, 2012-06-13 23:52:29]

Some style nits. The asyncUntil stuff will be nice when it's in.

http://codereview.chromium.org/10545167/diff/10/lib/unittest/unittest.dart
File lib/unittest/unittest.dart (right):

http://codereview.chromium.org/10545167/diff/10/lib/unittest/unittest.dart#ne...
lib/unittest/unittest.dart:157: /** Set the [Configuration] used by the unittest
library.*/
?

http://codereview.chromium.org/10545167/diff/10/lib/unittest/unittest.dart#ne...
lib/unittest/unittest.dart:306: _Init(Function callback, Function
shouldCallBack, Function isDone,
Can you use a delegating constructor for this? Something like:

// turn _Init into:
_SpreadArgsHelper(...) { }

// And then make the constructor calls like:
_SpreadArgsHelper.fixedCallCount(...) : this(...)

I know Dart is supposed to support this, but I'm not sure if it's implemented
yet, or if I have the right syntax.

http://codereview.chromium.org/10545167/diff/10/lib/unittest/unittest.dart#ne...
lib/unittest/unittest.dart:308: Expect.isTrue(_currentTest < _tests.length);
This Expect call is weird. Make it an assert instead, or remove it?

http://codereview.chromium.org/10545167/diff/10/lib/unittest/unittest.dart#ne...
lib/unittest/unittest.dart:317: _SpreadArgsHelper( callback, shouldCallBack,
isDone) {
No space after '('.

http://codereview.chromium.org/10545167/diff/10/lib/unittest/unittest.dart#ne...
lib/unittest/unittest.dart:331: _handleAllCallbacksDone();
Make this one line or use {}.

http://codereview.chromium.org/10545167/diff/10/lib/unittest/unittest.dart#ne...
lib/unittest/unittest.dart:334: _allCallsDone() {
Make this an =>?

http://codereview.chromium.org/10545167/diff/10/lib/unittest/unittest.dart#ne...
lib/unittest/unittest.dart:339: // always run except if the test is done
Full sentence: "Always... done."

http://codereview.chromium.org/10545167/diff/10/lib/unittest/unittest.dart#ne...
lib/unittest/unittest.dart:342: 'Callback called after already being marked as
done ($_calls)',
Indent another 2.

http://codereview.chromium.org/10545167/diff/10/lib/unittest/unittest.dart#ne...
lib/unittest/unittest.dart:344: _state = _UNCAUGHT_ERROR;
Indent +1.

http://codereview.chromium.org/10545167/diff/10/lib/unittest/unittest.dart#ne...
lib/unittest/unittest.dart:399: 'Callback called more times than expected
($_calls > $_expectedCalls)',
Indent another 1.

http://codereview.chromium.org/10545167/diff/10/lib/unittest/unittest.dart#ne...
lib/unittest/unittest.dart:447: * true. The unittest framework check [isDone]
after each callback and only
"check" -> "checks"

http://codereview.chromium.org/10545167/diff/10/tests/lib/unittest/instance_t...
File tests/lib/unittest/instance_test.dart (right):

http://codereview.chromium.org/10545167/diff/10/tests/lib/unittest/instance_t...
tests/lib/unittest/instance_test.dart:6: #library('instanceTest');
On 2012/06/13 23:43:53, sigmund wrote:
> => instance_test (we are not consistent in the codebase, but I strongly prefer
> this)

Right. The style guide says libraries should be named
lowercase_with_underscores. Generally just use the name of the file.

http://codereview.chromium.org/10545167/diff/10/tests/lib/unittest/testers.dart
File tests/lib/unittest/testers.dart (right):

http://codereview.chromium.org/10545167/diff/10/tests/lib/unittest/testers.da...
tests/lib/unittest/testers.dart:22: void shouldFail(var value, Matcher matcher,
expected) {
Don't need "var" here.

http://codereview.chromium.org/10545167/diff/10/tests/lib/unittest/testers.da...
tests/lib/unittest/testers.dart:29: if (expected is String)
Multi-line ifs should use curlies.

[gram, 2012-06-14 00:50:58]

PTAL.

http://codereview.chromium.org/10545167/diff/10/lib/unittest/unittest.dart
File lib/unittest/unittest.dart (right):

http://codereview.chromium.org/10545167/diff/10/lib/unittest/unittest.dart#ne...
lib/unittest/unittest.dart:157: /** Set the [Configuration] used by the unittest
library.*/
On 2012/06/13 23:52:29, Bob Nystrom wrote:
> ?

Done.

http://codereview.chromium.org/10545167/diff/10/lib/unittest/unittest.dart#ne...
lib/unittest/unittest.dart:306: _Init(Function callback, Function
shouldCallBack, Function isDone,
On 2012/06/13 23:43:53, sigmund wrote:
> -> lower case.

Done.

http://codereview.chromium.org/10545167/diff/10/lib/unittest/unittest.dart#ne...
lib/unittest/unittest.dart:306: _Init(Function callback, Function
shouldCallBack, Function isDone,
On 2012/06/13 23:52:29, Bob Nystrom wrote:
> Can you use a delegating constructor for this? Something like:
> 
> // turn _Init into:
> _SpreadArgsHelper(...) { }
> 
> // And then make the constructor calls like:
> _SpreadArgsHelper.fixedCallCount(...) : this(...)
> 
> I know Dart is supposed to support this, but I'm not sure if it's implemented
> yet, or if I have the right syntax.

I tried this originally. It didn't work because seemingly you can't reference
other methods like _allCallsDone from within the referenced constructor call. I
had to move that into the constructor body, hence _init.

http://codereview.chromium.org/10545167/diff/10/lib/unittest/unittest.dart#ne...
lib/unittest/unittest.dart:308: Expect.isTrue(_currentTest < _tests.length);
On 2012/06/13 23:52:29, Bob Nystrom wrote:
> This Expect call is weird. Make it an assert instead, or remove it?

Made assert.

http://codereview.chromium.org/10545167/diff/10/lib/unittest/unittest.dart#ne...
lib/unittest/unittest.dart:317: _SpreadArgsHelper( callback, shouldCallBack,
isDone) {
On 2012/06/13 23:43:53, sigmund wrote:
> remove extra space

Done.

http://codereview.chromium.org/10545167/diff/10/lib/unittest/unittest.dart#ne...
lib/unittest/unittest.dart:317: _SpreadArgsHelper( callback, shouldCallBack,
isDone) {
On 2012/06/13 23:52:29, Bob Nystrom wrote:
> No space after '('.

Done.

http://codereview.chromium.org/10545167/diff/10/lib/unittest/unittest.dart#ne...
lib/unittest/unittest.dart:321: _SpreadArgsHelper.fixedCallCount(callback,
expectedCalls) {
On 2012/06/13 23:43:53, sigmund wrote:
> let's make _callback final and change all 3 constructors to say
'this._callback'

Tried this; it doesn't work for the same reason delegating constructors don't
work. Well, it could be done for callback, but not the other members (which can
be initialized to point to methods within _SpreadArgsHelper), and that gets
weird, so I think it is more consistent to leave it as is.

http://codereview.chromium.org/10545167/diff/10/lib/unittest/unittest.dart#ne...
lib/unittest/unittest.dart:330: if (_isDone())
On 2012/06/13 23:43:53, sigmund wrote:
> style: add { } or merge this and the next line in a single line.

Done.

http://codereview.chromium.org/10545167/diff/10/lib/unittest/unittest.dart#ne...
lib/unittest/unittest.dart:331: _handleAllCallbacksDone();
On 2012/06/13 23:52:29, Bob Nystrom wrote:
> Make this one line or use {}.

Done.

http://codereview.chromium.org/10545167/diff/10/lib/unittest/unittest.dart#ne...
lib/unittest/unittest.dart:334: _allCallsDone() {
On 2012/06/13 23:52:29, Bob Nystrom wrote:
> Make this an =>?

Done.

http://codereview.chromium.org/10545167/diff/10/lib/unittest/unittest.dart#ne...
lib/unittest/unittest.dart:339: // always run except if the test is done
On 2012/06/13 23:52:29, Bob Nystrom wrote:
> Full sentence: "Always... done."

Done.

http://codereview.chromium.org/10545167/diff/10/lib/unittest/unittest.dart#ne...
lib/unittest/unittest.dart:342: 'Callback called after already being marked as
done ($_calls)',
On 2012/06/13 23:52:29, Bob Nystrom wrote:
> Indent another 2.

Done.

http://codereview.chromium.org/10545167/diff/10/lib/unittest/unittest.dart#ne...
lib/unittest/unittest.dart:344: _state = _UNCAUGHT_ERROR;
On 2012/06/13 23:52:29, Bob Nystrom wrote:
> Indent +1.

Done.

http://codereview.chromium.org/10545167/diff/10/lib/unittest/unittest.dart#ne...
lib/unittest/unittest.dart:399: 'Callback called more times than expected
($_calls > $_expectedCalls)',
On 2012/06/13 23:52:29, Bob Nystrom wrote:
> Indent another 1.

Done.

http://codereview.chromium.org/10545167/diff/10/lib/unittest/unittest.dart#ne...
lib/unittest/unittest.dart:447: * true. The unittest framework check [isDone]
after each callback and only
On 2012/06/13 23:52:29, Bob Nystrom wrote:
> "check" -> "checks"

Done.

http://codereview.chromium.org/10545167/diff/10/lib/unittest/unittest.dart#ne...
lib/unittest/unittest.dart:453: Function expectAsyncUntil(Function callback,
Function isDone) {
On 2012/06/13 23:43:53, sigmund wrote:
> make this private until issue 2706 gets fixed

Done.

http://codereview.chromium.org/10545167/diff/10/tests/lib/unittest/instance_t...
File tests/lib/unittest/instance_test.dart (right):

http://codereview.chromium.org/10545167/diff/10/tests/lib/unittest/instance_t...
tests/lib/unittest/instance_test.dart:6: #library('instanceTest');
On 2012/06/13 23:52:29, Bob Nystrom wrote:
> On 2012/06/13 23:43:53, sigmund wrote:
> > => instance_test (we are not consistent in the codebase, but I strongly
prefer
> > this)
> 
> Right. The style guide says libraries should be named
> lowercase_with_underscores. Generally just use the name of the file.

Done.

http://codereview.chromium.org/10545167/diff/10/tests/lib/unittest/instance_t...
tests/lib/unittest/instance_test.dart:30: 
On 2012/06/13 23:43:53, sigmund wrote:
> remove these empty lines at the bottom

Done.

http://codereview.chromium.org/10545167/diff/10/tests/lib/unittest/testers.dart
File tests/lib/unittest/testers.dart (right):

http://codereview.chromium.org/10545167/diff/10/tests/lib/unittest/testers.da...
tests/lib/unittest/testers.dart:4: 
On 2012/06/13 23:43:53, sigmund wrote:
> let's rename this file, ideas:
> - test_utils.dart
> - test_helpers.dart

Done.

http://codereview.chromium.org/10545167/diff/10/tests/lib/unittest/testers.da...
tests/lib/unittest/testers.dart:16: void initTesters() {
On 2012/06/13 23:43:53, sigmund wrote:
> rename according to file rename... 

Done.

http://codereview.chromium.org/10545167/diff/10/tests/lib/unittest/testers.da...
tests/lib/unittest/testers.dart:22: void shouldFail(var value, Matcher matcher,
expected) {
On 2012/06/13 23:52:29, Bob Nystrom wrote:
> Don't need "var" here.

Done.

http://codereview.chromium.org/10545167/diff/10/tests/lib/unittest/testers.da...
tests/lib/unittest/testers.dart:29: if (expected is String)
On 2012/06/13 23:52:29, Bob Nystrom wrote:
> Multi-line ifs should use curlies.

Done.

[Bob Nystrom, 2012-06-14 01:00:49]

One nit. Otherwise LGTM.

http://codereview.chromium.org/10545167/diff/13/lib/unittest/unittest.dart
File lib/unittest/unittest.dart (right):

http://codereview.chromium.org/10545167/diff/13/lib/unittest/unittest.dart#ne...
lib/unittest/unittest.dart:306: _Init(Function callback, Function
shouldCallBack, Function isDone,
_init