unittest step 3 and 4: remove TestFramework.dart, make test.dart use

unittest step 3 and 4: remove TestFramework.dart, make test.dart use
unittest.dart, so tests written with it will work both in the vm and browsers.

This involved a few changes:
- removing uses of TestFramework, use unittest instead (in test.dart and some
  isolate tests)
- making test.dart use unittest and html_config to wrap standalone tests as
  browser tests. This is done for tests written with or without unittest
- unittest: trap errors at the top-level, so we can detect errors during setup,
  but also to detect normal failures in tests that are wrapped by test.dart.

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

[Siggi Cherem (dart-lang), 2012-04-20 00:58:23]

https://chromiumcodereview.appspot.com/10153005/diff/2001/samples/tests/sampl...
File samples/tests/samples/src/proxy/MintMakerRpcTest.dart (right):

https://chromiumcodereview.appspot.com/10153005/diff/2001/samples/tests/sampl...
samples/tests/samples/src/proxy/MintMakerRpcTest.dart:177: // top-level port,
and that port is resolved first.
... not sure why this wasn't failing before.

https://chromiumcodereview.appspot.com/10153005/diff/2001/samples/tests/sampl...
File samples/tests/samples/src/proxy/PromiseBasedTest.dart (right):

https://chromiumcodereview.appspot.com/10153005/diff/2001/samples/tests/sampl...
samples/tests/samples/src/proxy/PromiseBasedTest.dart:40: void main() {
this code is very obsolete and I'd like to remove it. We should deal with that
separately though.

https://chromiumcodereview.appspot.com/10153005/diff/2001/tests/isolate/src/A...
File tests/isolate/src/APIv2_spawnUriTest.dart (left):

https://chromiumcodereview.appspot.com/10153005/diff/2001/tests/isolate/src/A...
tests/isolate/src/APIv2_spawnUriTest.dart:12: #import('dart:dom'); // import
added so test.dart can treat this as a webtest.
this is one of my favorites parts of this CL :)

This test became platform independent, woo hoo!

https://chromiumcodereview.appspot.com/10153005/diff/2001/tests/isolate/src/C...
File tests/isolate/src/CountTest.dart (left):

https://chromiumcodereview.appspot.com/10153005/diff/2001/tests/isolate/src/C...
tests/isolate/src/CountTest.dart:36: local.receive(expect.runs2((int message,
SendPort replyTo) {
this is called more than once, so 'later2' is not a good fit.

[Bill Hesse, 2012-04-20 08:49:46]

The test.dart changes look fine to me.

http://codereview.chromium.org/10153005/diff/1/tools/testing/dart/test_suite....
File tools/testing/dart/test_suite.dart (right):

http://codereview.chromium.org/10153005/diff/1/tools/testing/dart/test_suite....
tools/testing/dart/test_suite.dart:542: DartTestWrapper('$dartDir',
dartLibraryFilename));
We know dartDir is a string, so '$dartDir' is just dartDir.

[Emily Fortuna, 2012-04-20 20:06:19]

lgtm

https://chromiumcodereview.appspot.com/10153005/diff/2001/lib/unittest/unitte...
File lib/unittest/unittest.dart (right):

https://chromiumcodereview.appspot.com/10153005/diff/2001/lib/unittest/unitte...
lib/unittest/unittest.dart:214: // TODO(sigmund): expose this functionality
Comment obsolete now?

https://chromiumcodereview.appspot.com/10153005/diff/2001/lib/unittest/unitte...
lib/unittest/unittest.dart:216: Function later1(Function callback) {
I'm not thrilled with the names of these functions, but I don't have any better
ideas outside of later1Arg, etc so we can keep them as is.

https://chromiumcodereview.appspot.com/10153005/diff/2001/tests/isolate/src/A...
File tests/isolate/src/APIv2_spawnUriTest.dart (left):

https://chromiumcodereview.appspot.com/10153005/diff/2001/tests/isolate/src/A...
tests/isolate/src/APIv2_spawnUriTest.dart:12: #import('dart:dom'); // import
added so test.dart can treat this as a webtest.
On 2012/04/20 00:58:23, sigmund wrote:
> this is one of my favorites parts of this CL :)
> 
> This test became platform independent, woo hoo!

+1!

https://chromiumcodereview.appspot.com/10153005/diff/2001/tests/isolate/src/C...
File tests/isolate/src/ConstructorTest.dart (right):

https://chromiumcodereview.appspot.com/10153005/diff/2001/tests/isolate/src/C...
tests/isolate/src/ConstructorTest.dart:21: 
Extra whitespace?

https://chromiumcodereview.appspot.com/10153005/diff/2001/tests/isolate/src/T...
File tests/isolate/src/TestFramework.dart (left):

https://chromiumcodereview.appspot.com/10153005/diff/2001/tests/isolate/src/T...
tests/isolate/src/TestFramework.dart:4: 
whoohoo!

[Bob Nystrom, 2012-04-20 22:31:36]

I'm not crazy about later0, et. al. and I'd like to talk about that some more
either before you check in or in another patch before we start using it widely.
Otherwise, LGTM.

https://chromiumcodereview.appspot.com/10153005/diff/2001/lib/unittest/html_p...
File lib/unittest/html_print.dart (right):

https://chromiumcodereview.appspot.com/10153005/diff/2001/lib/unittest/html_p...
lib/unittest/html_print.dart:8: void _showResultsInPage(int passed, int failed,
int errors,
If we keep adding arguments here, eventually we may want to consider lumping
them all into a TestResults object.

https://chromiumcodereview.appspot.com/10153005/diff/2001/lib/unittest/unitte...
File lib/unittest/unittest.dart (right):

https://chromiumcodereview.appspot.com/10153005/diff/2001/lib/unittest/unitte...
lib/unittest/unittest.dart:205: Function later0(Function callback) {
I remember discussing it, but I don't remember the details. Is there a way we
can unify these into a single method using named optional arguments? The numbers
really bug me.

I'm also not sure about "later". It doesn't convey much of anything to me. Maybe
"testCallback"?

https://chromiumcodereview.appspot.com/10153005/diff/2001/lib/unittest/unitte...
lib/unittest/unittest.dart:279: /** Menchanism to notify that an error was
caught outside of this library. */
Should this even be public?

https://chromiumcodereview.appspot.com/10153005/diff/2001/tests/isolate/src/C...
File tests/isolate/src/ConstructorTest.dart (right):

https://chromiumcodereview.appspot.com/10153005/diff/2001/tests/isolate/src/C...
tests/isolate/src/ConstructorTest.dart:23: test("constructor initialized
correctly child isolate", () {
"constructor correctly initialized child isolate"

https://chromiumcodereview.appspot.com/10153005/diff/2001/tests/isolate/src/C...
File tests/isolate/src/CountTest.dart (right):

https://chromiumcodereview.appspot.com/10153005/diff/2001/tests/isolate/src/C...
tests/isolate/src/CountTest.dart:37: Function done = later0(() {});
This is hackish. Either our API should support this pattern more gracefully or
there should be a comment here explaining what's going on.

https://chromiumcodereview.appspot.com/10153005/diff/2001/tests/isolate/src/M...
File tests/isolate/src/MandelIsolateTest.dart (right):

https://chromiumcodereview.appspot.com/10153005/diff/2001/tests/isolate/src/M...
tests/isolate/src/MandelIsolateTest.dart:14: test("Render mandelbroth in
parallel", () {
"Mandelbrot"

https://chromiumcodereview.appspot.com/10153005/diff/2001/tests/isolate/src/M...
tests/isolate/src/MandelIsolateTest.dart:17: Expect.isTrue(result);
expect(result).isTrue();

https://chromiumcodereview.appspot.com/10153005/diff/2001/tests/isolate/src/M...
File tests/isolate/src/MessageTest.dart (right):

https://chromiumcodereview.appspot.com/10153005/diff/2001/tests/isolate/src/M...
tests/isolate/src/MessageTest.dart:116: Expect.equals(true, sendObject is List);
This should only be indented +2, not +4.

https://chromiumcodereview.appspot.com/10153005/diff/2001/tests/isolate/src/M...
File tests/isolate/src/MintMakerTest.dart (right):

https://chromiumcodereview.appspot.com/10153005/diff/2001/tests/isolate/src/M...
tests/isolate/src/MintMakerTest.dart:194: Expect.equals(expected, balance);
expect(balance).equals(expected);

?

https://chromiumcodereview.appspot.com/10153005/diff/2001/tools/testing/dart/...
File tools/testing/dart/browser_test.dart (right):

https://chromiumcodereview.appspot.com/10153005/diff/2001/tools/testing/dart/...
tools/testing/dart/browser_test.dart:5: String GetHtmlContents(String title,
These methods are named strangely. Any reason why?

[Siggi Cherem (dart-lang), 2012-04-21 00:03:43]

thanks for the comments

https://chromiumcodereview.appspot.com/10153005/diff/2001/lib/unittest/html_p...
File lib/unittest/html_print.dart (right):

https://chromiumcodereview.appspot.com/10153005/diff/2001/lib/unittest/html_p...
lib/unittest/html_print.dart:8: void _showResultsInPage(int passed, int failed,
int errors,
On 2012/04/20 22:31:36, Bob Nystrom wrote:
> If we keep adding arguments here, eventually we may want to consider lumping
> them all into a TestResults object.

agreed - I was thinking of getting rid of the first 3, since they are redundant
with what the list contains.

https://chromiumcodereview.appspot.com/10153005/diff/2001/lib/unittest/unitte...
File lib/unittest/unittest.dart (right):

https://chromiumcodereview.appspot.com/10153005/diff/2001/lib/unittest/unitte...
lib/unittest/unittest.dart:205: Function later0(Function callback) {
On 2012/04/20 22:31:36, Bob Nystrom wrote:
> I remember discussing it, but I don't remember the details. Is there a way we
> can unify these into a single method using named optional arguments? The
numbers
> really bug me.

I tried the approach that conflates them in a single method (simulating spread
args), but that trick doesn't works in frog, that's why I removed it. 

This trick:
later(f) {
 ...
 return ([a1=_def, a2=_def, ...]) { 
    ... // invoke f with correct num of args
 };
}

breaks because frog can't handle named arguments in closures (bug 282).

This trick breaks because frog doesn't keep optional argument information on
bound functions (bug 854). class _Helper {
  Function f;
  _Helper(this.f);
  invoke ([a1=_def, a2=_def, ...]) { 
    ... // invoke f with correct num of args
  }
}

later(f) {
 ...
 return new _Helper(f).invoke;
}

I added a TODO here to improve the API once they are fixed (or when we move to
use dart2js). I will take a closer look at 854. It might have an easy fix.

> 
> I'm also not sure about "later". It doesn't convey much of anything to me.
Maybe
> "testCallback"?


I want something that reads nicely, in the same way as 'expect' reads nice. What
about 'runsLater'? (or thinking of the later tests maybe 'runsOnce' and
'runsAtLeastOnce'?

test("timeout is called", () {
  window.setTimeout(runsLater(() { ... }), 0);
}

https://chromiumcodereview.appspot.com/10153005/diff/2001/lib/unittest/unitte...
lib/unittest/unittest.dart:214: // TODO(sigmund): expose this functionality
On 2012/04/20 20:06:19, Emily Fortuna wrote:
> Comment obsolete now?

Done.

https://chromiumcodereview.appspot.com/10153005/diff/2001/lib/unittest/unitte...
lib/unittest/unittest.dart:216: Function later1(Function callback) {
On 2012/04/20 20:06:19, Emily Fortuna wrote:
> I'm not thrilled with the names of these functions, but I don't have any
better
> ideas outside of later1Arg, etc so we can keep them as is.

I'm not thrilled either. There is a way to eliminate the 0/1/2, but it depends
on a feature that is not yet implemented in frog/dart2js

https://chromiumcodereview.appspot.com/10153005/diff/2001/lib/unittest/unitte...
lib/unittest/unittest.dart:279: /** Menchanism to notify that an error was
caught outside of this library. */
On 2012/04/20 22:31:36, Bob Nystrom wrote:
> Should this even be public?

Yes - we need it to be public so that unittest can be used by html_config (to
call it on window.onerror) and by test.dart (to call it when errors occur at the
top-level main method).

https://chromiumcodereview.appspot.com/10153005/diff/2001/tests/isolate/src/C...
File tests/isolate/src/ConstructorTest.dart (right):

https://chromiumcodereview.appspot.com/10153005/diff/2001/tests/isolate/src/C...
tests/isolate/src/ConstructorTest.dart:21: 
On 2012/04/20 20:06:19, Emily Fortuna wrote:
> Extra whitespace?

empty line. Removed.

https://chromiumcodereview.appspot.com/10153005/diff/2001/tests/isolate/src/C...
tests/isolate/src/ConstructorTest.dart:23: test("constructor initialized
correctly child isolate", () {
On 2012/04/20 22:31:36, Bob Nystrom wrote:
> "constructor correctly initialized child isolate"

Done.

https://chromiumcodereview.appspot.com/10153005/diff/2001/tests/isolate/src/C...
File tests/isolate/src/CountTest.dart (right):

https://chromiumcodereview.appspot.com/10153005/diff/2001/tests/isolate/src/C...
tests/isolate/src/CountTest.dart:37: Function done = later0(() {});
On 2012/04/20 22:31:36, Bob Nystrom wrote:
> This is hackish. Either our API should support this pattern more gracefully or
> there should be a comment here explaining what's going on.

How about these names?
- calledOnce(f): replacing later(f), returns a function that we verify that will
be called only once
- callAtLeastOnce(f): returns a function that we will verify that is called at
least once.

In this scenario we'd need to do this:

Function done = calledOnce(() {}); // called when the test is done.
local.receive(calledAtLeastOnce2((int msg, SendPort reply) {
  ...
  if (/* last time */) {
     done();
  }
}));

https://chromiumcodereview.appspot.com/10153005/diff/2001/tests/isolate/src/M...
File tests/isolate/src/MandelIsolateTest.dart (right):

https://chromiumcodereview.appspot.com/10153005/diff/2001/tests/isolate/src/M...
tests/isolate/src/MandelIsolateTest.dart:14: test("Render mandelbroth in
parallel", () {
On 2012/04/20 22:31:36, Bob Nystrom wrote:
> "Mandelbrot"

this one had to make you laugh :)

https://chromiumcodereview.appspot.com/10153005/diff/2001/tests/isolate/src/M...
tests/isolate/src/MandelIsolateTest.dart:17: Expect.isTrue(result);
On 2012/04/20 22:31:36, Bob Nystrom wrote:
> expect(result).isTrue();

Done. In general I was trying to do the minimal changes to get it working with
unittest, so there will be many changes like this that we'd have to do later on.

https://chromiumcodereview.appspot.com/10153005/diff/2001/tests/isolate/src/M...
File tests/isolate/src/MessageTest.dart (right):

https://chromiumcodereview.appspot.com/10153005/diff/2001/tests/isolate/src/M...
tests/isolate/src/MessageTest.dart:116: Expect.equals(true, sendObject is List);
On 2012/04/20 22:31:36, Bob Nystrom wrote:
> This should only be indented +2, not +4.

Done.

https://chromiumcodereview.appspot.com/10153005/diff/2001/tests/isolate/src/M...
File tests/isolate/src/MintMakerTest.dart (right):

https://chromiumcodereview.appspot.com/10153005/diff/2001/tests/isolate/src/M...
tests/isolate/src/MintMakerTest.dart:194: Expect.equals(expected, balance);
On 2012/04/20 22:31:36, Bob Nystrom wrote:
> expect(balance).equals(expected);
> 
> ?

Done.

https://chromiumcodereview.appspot.com/10153005/diff/2001/tools/testing/dart/...
File tools/testing/dart/browser_test.dart (right):

https://chromiumcodereview.appspot.com/10153005/diff/2001/tools/testing/dart/...
tools/testing/dart/browser_test.dart:5: String GetHtmlContents(String title,
On 2012/04/20 22:31:36, Bob Nystrom wrote:
> These methods are named strangely. Any reason why?

I agree, I don't like the capitalization. I'll keep that separate from this CL
though.

[Siggi Cherem (dart-lang), 2012-04-21 04:44:51]

I updated a new version, assuming that my fix for bug 854 gets in (this CL has a
copy of it temporarily).

Let's use this to discuss on Monday to narrow down the API. I rewrote the
examples in the dartdoc to illustrate the new API.

[Bob Nystrom, 2012-04-23 17:34:07]

https://chromiumcodereview.appspot.com/10153005/diff/2001/lib/unittest/html_p...
File lib/unittest/html_print.dart (right):

https://chromiumcodereview.appspot.com/10153005/diff/2001/lib/unittest/html_p...
lib/unittest/html_print.dart:8: void _showResultsInPage(int passed, int failed,
int errors,
On 2012/04/21 00:03:43, sigmund wrote:
> On 2012/04/20 22:31:36, Bob Nystrom wrote:
> > If we keep adding arguments here, eventually we may want to consider lumping
> > them all into a TestResults object.
> 
> agreed - I was thinking of getting rid of the first 3, since they are
redundant
> with what the list contains.

I do like that this code doesn't have to loop and count the successes because
that seems like boilerplate that every result handler will do. I would just
consider lumping this all into a single class where delivering these results is
its core concern.

https://chromiumcodereview.appspot.com/10153005/diff/2001/lib/unittest/unitte...
File lib/unittest/unittest.dart (right):

https://chromiumcodereview.appspot.com/10153005/diff/2001/lib/unittest/unitte...
lib/unittest/unittest.dart:279: /** Menchanism to notify that an error was
caught outside of this library. */
On 2012/04/21 00:03:43, sigmund wrote:
> On 2012/04/20 22:31:36, Bob Nystrom wrote:
> > Should this even be public?
> 
> Yes - we need it to be public so that unittest can be used by html_config (to
> call it on window.onerror) and by test.dart (to call it when errors occur at
the
> top-level main method). 

In that case, can we rename it something a little clearer? "notify" reads
strangely to me. How about "reportTestError"?

https://chromiumcodereview.appspot.com/10153005/diff/2001/tests/isolate/src/M...
File tests/isolate/src/MandelIsolateTest.dart (right):

https://chromiumcodereview.appspot.com/10153005/diff/2001/tests/isolate/src/M...
tests/isolate/src/MandelIsolateTest.dart:17: Expect.isTrue(result);
On 2012/04/21 00:03:43, sigmund wrote:
> On 2012/04/20 22:31:36, Bob Nystrom wrote:
> > expect(result).isTrue();
> 
> Done. In general I was trying to do the minimal changes to get it working with
> unittest, so there will be many changes like this that we'd have to do later
on.

Totally fine. I'm OK with gradually changing this stuff over in later changes.
Actually it would be good to hold off since we've talked about changing the API
for expect() too. :)

[Siggi Cherem (dart-lang), 2012-04-25 01:03:52]

PTAL

Once I fixed the bug in frog, I got things working, but they fail again in
checked mode because of a clause in the spec that I hope we can change. I added
a bug for it (http://dartbug.com/2706).

Meanwhile I added back the multiple versions of expectAsync0, expectAsync1 and
expectAsync2.

I made this change last, so if you want to see what the change would look like
without this workaround, see Patch #10

[Bob Nystrom, 2012-04-25 18:21:17]

Two nits but otherwise LGTM.

https://chromiumcodereview.appspot.com/10153005/diff/29003/lib/unittest/unitt...
File lib/unittest/unittest.dart (right):

https://chromiumcodereview.appspot.com/10153005/diff/29003/lib/unittest/unitt...
lib/unittest/unittest.dart:95: * features or fixes to the current spec (e.g. see
dartbug.com/2706).
This comment is excellent.

https://chromiumcodereview.appspot.com/10153005/diff/29003/lib/unittest/unitt...
lib/unittest/unittest.dart:378: } else {
Remove the empty else clause.

https://chromiumcodereview.appspot.com/10153005/diff/29003/lib/unittest/unitt...
lib/unittest/unittest.dart:383: void notifyError(String msg, String trace) {
Rename? See my previous comment.

[Siggi Cherem (dart-lang), 2012-04-25 18:35:37]

https://chromiumcodereview.appspot.com/10153005/diff/29003/lib/unittest/unitt...
File lib/unittest/unittest.dart (right):

https://chromiumcodereview.appspot.com/10153005/diff/29003/lib/unittest/unitt...
lib/unittest/unittest.dart:378: } else {
On 2012/04/25 18:21:17, Bob Nystrom wrote:
> Remove the empty else clause.

Oops, thanks

https://chromiumcodereview.appspot.com/10153005/diff/29003/lib/unittest/unitt...
lib/unittest/unittest.dart:383: void notifyError(String msg, String trace) {
On 2012/04/25 18:21:17, Bob Nystrom wrote:
> Rename? See my previous comment.

sorry I forgot this one. Done.