Modified unittest to use new argument syntax.

Modified unittest to use new argument syntax.
Fix affected unit tests.
Clean up unit tests that are using Expect instead of expect.
Get rid of warnings about duplicated library names.
Committed: https://code.google.com/p/dart/source/detail?r=14158

[gram, 2012-10-26 00:28:20]

This is a monster, and so I don't drive Siggy to the edge of madness, I need
your help.

If you are the primary or a major contributor to one of these files, please
review it, and add a comment at the top if you lgtm it so that others can see.
Once we've done a reasonable pass I'll divvy up the stragglers and try to get
every one done. Thanks and I'm sorry :-|

[Siggi Cherem (dart-lang), 2012-10-26 00:44:51]

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

https://codereview.chromium.org/11275054/diff/1/pkg/unittest/expect.dart#newc...
pkg/unittest/expect.dart:33: void expect(actual, matcher, { String reason :
null,
nits:
- remove space between { and String
- remove space before ':' (e.g. {String reason: null, ...})

https://codereview.chromium.org/11275054/diff/1/tests/html/cross_frame_test.dart
File tests/html/cross_frame_test.dart (right):

https://codereview.chromium.org/11275054/diff/1/tests/html/cross_frame_test.d...
tests/html/cross_frame_test.dart:9: var isWindow = new
isInstanceOf<Window>('Window');
is this now fixed in dart2js? it used to only work in the vm/dartium

https://codereview.chromium.org/11275054/diff/1/tests/html/instance_of_test.dart
File tests/html/instance_of_test.dart (right):

https://codereview.chromium.org/11275054/diff/1/tests/html/instance_of_test.d...
tests/html/instance_of_test.dart:27: // uses the matchers above. As dart2js
returns true for is/is! regardless
that's what I was worried about. then below: wont
  expect(_, isElement)
always pass? even if I pass something of the wrong type?

For instance,
   expect(new Foo(), isElement) // would pass
when:
   expect(new Foo() is Element, isTrue) // fails?

https://codereview.chromium.org/11275054/diff/1/tests/html/window_open_test.dart
File tests/html/window_open_test.dart (right):

https://codereview.chromium.org/11275054/diff/1/tests/html/window_open_test.d...
tests/html/window_open_test.dart:13: //  evaluateJavaScript('(window.testRunner
|| window.layoutTestController) ? (window.testRunner ||
window.layoutTestController).setCanOpenWindows() : null');
strange - uncomment?

https://codereview.chromium.org/11275054/diff/1/utils/tests/pub/version_test....
File utils/tests/pub/version_test.dart (right):

https://codereview.chromium.org/11275054/diff/1/utils/tests/pub/version_test....
utils/tests/pub/version_test.dart:200: expect(range.allows(new
Version.parse('1.2.2')), isFalse);
not for this CL, but this looks like you almost want a rangeAllows matcher...

[gram, 2012-10-26 00:50:34]

A few updates

https://codereview.chromium.org/11275054/diff/1/tests/html/window_open_test.dart
File tests/html/window_open_test.dart (right):

https://codereview.chromium.org/11275054/diff/1/tests/html/window_open_test.d...
tests/html/window_open_test.dart:14: 
Good catch - this file should not be in the CL. It's a throwback to when Peter
and I were making the changes to better support exceptions thrown outside of the
Dart code under test. I've reverted it.

https://codereview.chromium.org/11275054/diff/1/tests/html/xmldocument_test.dart
File tests/html/xmldocument_test.dart (right):

https://codereview.chromium.org/11275054/diff/1/tests/html/xmldocument_test.d...
tests/html/xmldocument_test.dart:65: expect(extractClasses(doc), ["foo", "bar",
"baz", "qux"]);
This is wrong; it should use unorderedEquals

https://codereview.chromium.org/11275054/diff/1/tests/html/xmldocument_test.d...
tests/html/xmldocument_test.dart:71: expect(doc.classes, ["foo", "qux"]);
Ditto

https://codereview.chromium.org/11275054/diff/1/tests/html/xmldocument_test.d...
tests/html/xmldocument_test.dart:78: expect(extractClasses(doc), ["foo",
"qux"]);
Ditto

[Jennifer Messerly, 2012-10-26 02:57:05]

this is cool, I like the new syntax

https://chromiumcodereview.appspot.com/11275054/diff/1/pkg/unittest/expect.dart
File pkg/unittest/expect.dart (right):

https://chromiumcodereview.appspot.com/11275054/diff/1/pkg/unittest/expect.da...
pkg/unittest/expect.dart:33: void expect(actual, matcher, { String reason :
null,
On 2012/10/26 00:44:51, Siggi Cherem (dart-lang) wrote:
> nits:
> - remove space between { and String
> - remove space before ':' (e.g. {String reason: null, ...})


also, remove "null" if you can. AFAIK, it's implied.

https://chromiumcodereview.appspot.com/11275054/diff/1/pkg/unittest/html_conf...
File pkg/unittest/html_config.dart (right):

https://chromiumcodereview.appspot.com/11275054/diff/1/pkg/unittest/html_conf...
pkg/unittest/html_config.dart:8: #library('unittest_html_config');
new library syntax?

https://chromiumcodereview.appspot.com/11275054/diff/1/pkg/unittest/mock.dart
File pkg/unittest/mock.dart (right):

https://chromiumcodereview.appspot.com/11275054/diff/1/pkg/unittest/mock.dart...
pkg/unittest/mock.dart:435: expect(logs, matcher, reason:filter, failureHandler:
_mockFailureHandler);
I think this would read better if it was called "failure" or "onFailure"

https://chromiumcodereview.appspot.com/11275054/diff/1/tests/html/audiocontex...
File tests/html/audiocontext_test.dart (right):

https://chromiumcodereview.appspot.com/11275054/diff/1/tests/html/audiocontex...
tests/html/audiocontext_test.dart:13: expect(ctx is AudioContext, isTrue);
use isInstanceOf? I saw it used in a few other places.

https://chromiumcodereview.appspot.com/11275054/diff/1/tests/html/client_rect...
File tests/html/client_rect_test.dart (right):

https://chromiumcodereview.appspot.com/11275054/diff/1/tests/html/client_rect...
tests/html/client_rect_test.dart:24: List<ClientRect> rects =
range.getClientRects();
fyi, this was a pre-existing problem but: I would make this "var", otherwise it
will blow up in checked mode before it gets to the isInstanceOf test.

https://chromiumcodereview.appspot.com/11275054/diff/1/tests/isolate/multiple...
File tests/isolate/multiple_timer_test.dart (right):

https://chromiumcodereview.appspot.com/11275054/diff/1/tests/isolate/multiple...
tests/isolate/multiple_timer_test.dart:27: expect(_order[_message], 0);
isZero?

https://chromiumcodereview.appspot.com/11275054/diff/1/tests/isolate/multiple...
tests/isolate/multiple_timer_test.dart:34: expect(_order[_message], 1);
equals(1) and below?

https://chromiumcodereview.appspot.com/11275054/diff/1/tests/isolate/timer_ca...
File tests/isolate/timer_cancel_test.dart (right):

https://chromiumcodereview.appspot.com/11275054/diff/1/tests/isolate/timer_ca...
tests/isolate/timer_cancel_test.dart:26: expect(repeatTimer, 1);
equals(1)? or do primitives match themselves?

[nweiz, 2012-10-26 18:36:33]

lgtm

https://codereview.chromium.org/11275054/diff/1/utils/tests/pub/pubspec_test....
File utils/tests/pub/pubspec_test.dart (right):

https://codereview.chromium.org/11275054/diff/1/utils/tests/pub/pubspec_test....
utils/tests/pub/pubspec_test.dart:41: expect(!foo.constraint.allows(new
Version(3, 4, 5)), isTrue);
Can we get rid of the "!" and use isFalse?

https://codereview.chromium.org/11275054/diff/1/utils/tests/pub/version_test....
File utils/tests/pub/version_test.dart (right):

https://codereview.chromium.org/11275054/diff/1/utils/tests/pub/version_test....
utils/tests/pub/version_test.dart:102: isTrue);
Is there no isEmpty matcher? That seems like it might be useful.

[gram, 2012-10-26 22:26:27]

PTAL

Apart from addressing the comments thus far, I:

- updated the library/import syntax to the new form
- replaced all isInstanceOf matchers (which don't currently work with dart2js)
with predicate matchers (which do).

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

https://codereview.chromium.org/11275054/diff/1/pkg/unittest/expect.dart#newc...
pkg/unittest/expect.dart:33: void expect(actual, matcher, { String reason :
null,
On 2012/10/26 00:44:51, Siggi Cherem (dart-lang) wrote:
> nits:
> - remove space between { and String
> - remove space before ':' (e.g. {String reason: null, ...})

Done.

https://codereview.chromium.org/11275054/diff/1/pkg/unittest/expect.dart#newc...
pkg/unittest/expect.dart:33: void expect(actual, matcher, { String reason :
null,
On 2012/10/26 02:57:05, John Messerly wrote:
> On 2012/10/26 00:44:51, Siggi Cherem (dart-lang) wrote:
> > nits:
> > - remove space between { and String
> > - remove space before ':' (e.g. {String reason: null, ...})
> 
> 
> also, remove "null" if you can. AFAIK, it's implied.

Done.

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

https://codereview.chromium.org/11275054/diff/1/pkg/unittest/html_config.dart...
pkg/unittest/html_config.dart:8: #library('unittest_html_config');
On 2012/10/26 02:57:05, John Messerly wrote:
> new library syntax?

Done.

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

https://codereview.chromium.org/11275054/diff/1/pkg/unittest/mock.dart#newcod...
pkg/unittest/mock.dart:435: expect(logs, matcher, reason:filter, failureHandler:
_mockFailureHandler);
On 2012/10/26 02:57:05, John Messerly wrote:
> I think this would read better if it was called "failure" or "onFailure"

It's a class, not a method, so it should be a noun, not a verb.

https://codereview.chromium.org/11275054/diff/1/tests/html/audiocontext_test....
File tests/html/audiocontext_test.dart (right):

https://codereview.chromium.org/11275054/diff/1/tests/html/audiocontext_test....
tests/html/audiocontext_test.dart:13: expect(ctx is AudioContext, isTrue);
On 2012/10/26 02:57:05, John Messerly wrote:
> use isInstanceOf? I saw it used in a few other places.

Done.

https://codereview.chromium.org/11275054/diff/1/tests/html/client_rect_test.dart
File tests/html/client_rect_test.dart (right):

https://codereview.chromium.org/11275054/diff/1/tests/html/client_rect_test.d...
tests/html/client_rect_test.dart:24: List<ClientRect> rects =
range.getClientRects();
On 2012/10/26 02:57:05, John Messerly wrote:
> fyi, this was a pre-existing problem but: I would make this "var", otherwise
it
> will blow up in checked mode before it gets to the isInstanceOf test.

Done.

https://codereview.chromium.org/11275054/diff/1/tests/html/cross_frame_test.dart
File tests/html/cross_frame_test.dart (right):

https://codereview.chromium.org/11275054/diff/1/tests/html/cross_frame_test.d...
tests/html/cross_frame_test.dart:9: var isWindow = new
isInstanceOf<Window>('Window');
On 2012/10/26 00:44:51, Siggi Cherem (dart-lang) wrote:
> is this now fixed in dart2js? it used to only work in the vm/dartium

No - as you can see from my comments below about not being able to write tests
with isNot(is) yet. Such tests will always fail as is will always pass. I have
changed them to use predicate().

https://codereview.chromium.org/11275054/diff/1/tests/html/instance_of_test.dart
File tests/html/instance_of_test.dart (right):

https://codereview.chromium.org/11275054/diff/1/tests/html/instance_of_test.d...
tests/html/instance_of_test.dart:27: // uses the matchers above. As dart2js
returns true for is/is! regardless
On 2012/10/26 00:44:51, Siggi Cherem (dart-lang) wrote:
> that's what I was worried about. then below: wont
>   expect(_, isElement)
> always pass? even if I pass something of the wrong type?
> 
> For instance,
>    expect(new Foo(), isElement) // would pass
> when:
>    expect(new Foo() is Element, isTrue) // fails?

Yes, for now, these will always pass in dart2js. I didn't realize that these
would work correctly in the non-templated version, but it seems they do. So I
will have to undo all the type tests. I am going to leave the new versions in
but commented out so that when the time comes we can easily clean this up.

https://codereview.chromium.org/11275054/diff/1/tests/isolate/multiple_timer_...
File tests/isolate/multiple_timer_test.dart (right):

https://codereview.chromium.org/11275054/diff/1/tests/isolate/multiple_timer_...
tests/isolate/multiple_timer_test.dart:27: expect(_order[_message], 0);
On 2012/10/26 02:57:05, John Messerly wrote:
> isZero?

I chose not to use isZero as we have 1, 2, 3 below. isZero should really be
confined to binary cases where we have isZero vs isNonZero.

https://codereview.chromium.org/11275054/diff/1/tests/isolate/multiple_timer_...
tests/isolate/multiple_timer_test.dart:34: expect(_order[_message], 1);
On 2012/10/26 02:57:05, John Messerly wrote:
> equals(1) and below?

Automagic.

https://codereview.chromium.org/11275054/diff/1/tests/isolate/timer_cancel_te...
File tests/isolate/timer_cancel_test.dart (right):

https://codereview.chromium.org/11275054/diff/1/tests/isolate/timer_cancel_te...
tests/isolate/timer_cancel_test.dart:26: expect(repeatTimer, 1);
On 2012/10/26 02:57:05, John Messerly wrote:
> equals(1)? or do primitives match themselves?

Yes, if the second argument is not a matcher, it will get wrapped in an equals()
matcher.

https://codereview.chromium.org/11275054/diff/1/utils/tests/pub/pubspec_test....
File utils/tests/pub/pubspec_test.dart (right):

https://codereview.chromium.org/11275054/diff/1/utils/tests/pub/pubspec_test....
utils/tests/pub/pubspec_test.dart:41: expect(!foo.constraint.allows(new
Version(3, 4, 5)), isTrue);
On 2012/10/26 18:36:33, nweiz wrote:
> Can we get rid of the "!" and use isFalse?

Done.

https://codereview.chromium.org/11275054/diff/1/utils/tests/pub/version_test....
File utils/tests/pub/version_test.dart (right):

https://codereview.chromium.org/11275054/diff/1/utils/tests/pub/version_test....
utils/tests/pub/version_test.dart:102: isTrue);
On 2012/10/26 18:36:33, nweiz wrote:
> Is there no isEmpty matcher? That seems like it might be useful.

There is for collections; doesn't help here.

https://codereview.chromium.org/11275054/diff/1/utils/tests/pub/version_test....
utils/tests/pub/version_test.dart:200: expect(range.allows(new
Version.parse('1.2.2')), isFalse);
On 2012/10/26 00:44:51, Siggi Cherem (dart-lang) wrote:
> not for this CL, but this looks like you almost want a rangeAllows matcher... 

Noted

[Jennifer Messerly, 2012-10-26 22:50:16]

lgtm

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

https://codereview.chromium.org/11275054/diff/1/pkg/unittest/mock.dart#newcod...
pkg/unittest/mock.dart:435: expect(logs, matcher, reason:filter, failureHandler:
_mockFailureHandler);
On 2012/10/26 22:26:27, gram wrote:
> On 2012/10/26 02:57:05, John Messerly wrote:
> > I think this would read better if it was called "failure" or "onFailure"
> 
> It's a class, not a method, so it should be a noun, not a verb.

Makes sense. I was assuming it was a callback :)

btw, small nit, +1 space after colon: "reason: filter"

https://codereview.chromium.org/11275054/diff/11001/pkg/unittest/interactive_...
File pkg/unittest/interactive_html_config.dart (right):

https://codereview.chromium.org/11275054/diff/11001/pkg/unittest/interactive_...
pkg/unittest/interactive_html_config.dart:12:
#library('unittest_interactive_html_config');
this could use the new syntax too.
but totally fine to get this in another CL!

[Siggi Cherem (dart-lang), 2012-10-29 16:58:34]

lgtm