step 1 in making unittest platform independent.

step 1 in making unittest platform independent.

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

[Siggi Cherem (dart-lang), 2012-04-10 00:20:47]

FYI

step 2 would be to do this:
* shared.dart => unittest.dart with a defalt config that works everywhere (the
one commented out in config.dart)
* all files will import unittest.dart (not a platform specific one)
* add dom_config and html_config, which are used in test.dart to change how we
run the code/generate output.

[Bob Nystrom, 2012-04-10 00:30:31]

Nice!

https://chromiumcodereview.appspot.com/10031022/diff/1/lib/unittest/config.dart
File lib/unittest/config.dart (right):

https://chromiumcodereview.appspot.com/10031022/diff/1/lib/unittest/config.da...
lib/unittest/config.dart:13: class Configuration {
A class full of functions is a bit strange. How about just making those
functions methods (and make their implementations be the _defaultConfig() stuff
below). Then the other implementations can subclass it and override the methods
they care about.

https://chromiumcodereview.appspot.com/10031022/diff/1/lib/unittest/html_prin...
File lib/unittest/html_print.dart (right):

https://chromiumcodereview.appspot.com/10031022/diff/1/lib/unittest/html_prin...
lib/unittest/html_print.dart:9: if (isLayoutTest && testsPassed ==
results.length) {
I would parenthesize the == expression.

https://chromiumcodereview.appspot.com/10031022/diff/1/lib/unittest/shared.dart
File lib/unittest/shared.dart (right):

https://chromiumcodereview.appspot.com/10031022/diff/1/lib/unittest/shared.da...
lib/unittest/shared.dart:7: void configure(Configuration c) {
Is anything using this?

https://chromiumcodereview.appspot.com/10031022/diff/1/lib/unittest/unittest_...
File lib/unittest/unittest_dom.dart (right):

https://chromiumcodereview.appspot.com/10031022/diff/1/lib/unittest/unittest_...
lib/unittest/unittest_dom.dart:63: _platformInitialize,
Instead of closures here, I would just subclass Configuration and make those
ugly _platform functions be overridden methods.

https://chromiumcodereview.appspot.com/10031022/diff/1/lib/unittest/unittest_...
File lib/unittest/unittest_node.dart (right):

https://chromiumcodereview.appspot.com/10031022/diff/1/lib/unittest/unittest_...
lib/unittest/unittest_node.dart:1: // Copyright (c) 2011, the Dart project
authors.  Please see the AUTHORS file
This file can be deleted now that we don't support node.

[Siggi Cherem (dart-lang), 2012-04-10 01:05:28]

thanks Bob!

https://chromiumcodereview.appspot.com/10031022/diff/1/lib/unittest/config.dart
File lib/unittest/config.dart (right):

https://chromiumcodereview.appspot.com/10031022/diff/1/lib/unittest/config.da...
lib/unittest/config.dart:13: class Configuration {
On 2012/04/10 00:30:31, Bob Nystrom wrote:
> A class full of functions is a bit strange. How about just making those
> functions methods (and make their implementations be the _defaultConfig()
stuff
> below). Then the other implementations can subclass it and override the
methods
> they care about.

Good point - this was a bit historical (I used to make this configuration one
that you could override by setting a particular listener programatically).

Done.

https://chromiumcodereview.appspot.com/10031022/diff/1/lib/unittest/html_prin...
File lib/unittest/html_print.dart (right):

https://chromiumcodereview.appspot.com/10031022/diff/1/lib/unittest/html_prin...
lib/unittest/html_print.dart:9: if (isLayoutTest && testsPassed ==
results.length) {
On 2012/04/10 00:30:31, Bob Nystrom wrote:
> I would parenthesize the == expression.

Done.

https://chromiumcodereview.appspot.com/10031022/diff/1/lib/unittest/shared.dart
File lib/unittest/shared.dart (right):

https://chromiumcodereview.appspot.com/10031022/diff/1/lib/unittest/shared.da...
lib/unittest/shared.dart:7: void configure(Configuration c) {
On 2012/04/10 00:30:31, Bob Nystrom wrote:
> Is anything using this?

Not yet, step 2 :) - removed.

https://chromiumcodereview.appspot.com/10031022/diff/1/lib/unittest/unittest_...
File lib/unittest/unittest_dom.dart (right):

https://chromiumcodereview.appspot.com/10031022/diff/1/lib/unittest/unittest_...
lib/unittest/unittest_dom.dart:63: _platformInitialize,
On 2012/04/10 00:30:31, Bob Nystrom wrote:
> Instead of closures here, I would just subclass Configuration and make those
> ugly _platform functions be overridden methods.

Done.

https://chromiumcodereview.appspot.com/10031022/diff/1/lib/unittest/unittest_...
File lib/unittest/unittest_node.dart (right):

https://chromiumcodereview.appspot.com/10031022/diff/1/lib/unittest/unittest_...
lib/unittest/unittest_node.dart:1: // Copyright (c) 2011, the Dart project
authors.  Please see the AUTHORS file
On 2012/04/10 00:30:31, Bob Nystrom wrote:
> This file can be deleted now that we don't support node.

Done.

[Bob Nystrom, 2012-04-10 01:17:15]

https://chromiumcodereview.appspot.com/10031022/diff/5001/lib/unittest/unitte...
File lib/unittest/unittest_html.dart (right):

https://chromiumcodereview.appspot.com/10031022/diff/5001/lib/unittest/unitte...
lib/unittest/unittest_html.dart:39: class _DefaultConfiguration() {
This doesn't look like valid Dart code. Ditch the () and add implements
Configuration.

https://chromiumcodereview.appspot.com/10031022/diff/5001/lib/unittest/unitte...
File lib/unittest/unittest_vm.dart (right):

https://chromiumcodereview.appspot.com/10031022/diff/5001/lib/unittest/unitte...
lib/unittest/unittest_vm.dart:45: Configuration _defaultConfig() {
Need to update this to the new class-based pattern.

[Siggi Cherem (dart-lang), 2012-04-10 01:28:44]

https://chromiumcodereview.appspot.com/10031022/diff/5001/lib/unittest/unitte...
File lib/unittest/unittest_html.dart (right):

https://chromiumcodereview.appspot.com/10031022/diff/5001/lib/unittest/unitte...
lib/unittest/unittest_html.dart:39: class _DefaultConfiguration() {
On 2012/04/10 01:17:15, Bob Nystrom wrote:
> This doesn't look like valid Dart code. Ditch the () and add implements
> Configuration.

Doh!... fixed

https://chromiumcodereview.appspot.com/10031022/diff/5001/lib/unittest/unitte...
File lib/unittest/unittest_vm.dart (right):

https://chromiumcodereview.appspot.com/10031022/diff/5001/lib/unittest/unitte...
lib/unittest/unittest_vm.dart:45: Configuration _defaultConfig() {
On 2012/04/10 01:17:15, Bob Nystrom wrote:
> Need to update this to the new class-based pattern.

Done.

[Siggi Cherem (dart-lang), 2012-04-10 02:03:14]

done, also applied the changes we discussed in person.

[eub, 2012-04-10 18:06:02]

Thanks, Siggi!

http://codereview.chromium.org/10031022/diff/2003/lib/unittest/config.dart
File lib/unittest/config.dart (right):

http://codereview.chromium.org/10031022/diff/2003/lib/unittest/config.dart#ne...
lib/unittest/config.dart:45: }
This implementation makes me scratch my head -- it looks incomplete (a one-sided
test?), who will use it?

http://codereview.chromium.org/10031022/diff/2003/lib/unittest/shared.dart
File lib/unittest/shared.dart (right):

http://codereview.chromium.org/10031022/diff/2003/lib/unittest/shared.dart#ne...
lib/unittest/shared.dart:4: 
I know we don't have a convention, but I'd like to encourage any file that
expects to be #sourced to have a file comment saying so, and by whom.

http://codereview.chromium.org/10031022/diff/2003/lib/unittest/shared.dart#ne...
lib/unittest/shared.dart:5: Configuration _config = null;
(And what definitions the file expects to rely on from outside.)

http://codereview.chromium.org/10031022/diff/2003/lib/unittest/unittest_html....
File lib/unittest/unittest_html.dart (right):

http://codereview.chromium.org/10031022/diff/2003/lib/unittest/unittest_html....
lib/unittest/unittest_html.dart:69: onDone(int testsPassed, int testsFailed, int
testsErrors,
Let's match the arg names between the parent class and here (and other
children).

http://codereview.chromium.org/10031022/diff/2003/lib/unittest/unittest_vm.dart
File lib/unittest/unittest_vm.dart (right):

http://codereview.chromium.org/10031022/diff/2003/lib/unittest/unittest_vm.da...
lib/unittest/unittest_vm.dart:46: // A non-zero exit code is used by the test
infrastructure to detect failure.
80 cols

[Emily Fortuna, 2012-04-10 18:14:12]

with the other comments, looks fine to me, but I'm going to defer to rnystrom
and eub who know a bit more about the isolate testing innards than I do.

https://chromiumcodereview.appspot.com/10031022/diff/2003/lib/unittest/config...
File lib/unittest/config.dart (right):

https://chromiumcodereview.appspot.com/10031022/diff/2003/lib/unittest/config...
lib/unittest/config.dart:5: /**
should we be switching to the /// style comments?

I know there's /** elsewhere around... so it's fine if we keep it for
consistency

https://chromiumcodereview.appspot.com/10031022/diff/2003/lib/unittest/shared...
File lib/unittest/shared.dart (right):

https://chromiumcodereview.appspot.com/10031022/diff/2003/lib/unittest/shared...
lib/unittest/shared.dart:4: 
On 2012/04/10 18:06:02, eub wrote:
> I know we don't have a convention, but I'd like to encourage any file that
> expects to be #sourced to have a file comment saying so, and by whom.

+1

[Bob Nystrom, 2012-04-10 20:04:25]

A couple of minor changes. After those are done, LGTM.

https://chromiumcodereview.appspot.com/10031022/diff/2003/lib/unittest/config...
File lib/unittest/config.dart (right):

https://chromiumcodereview.appspot.com/10031022/diff/2003/lib/unittest/config...
lib/unittest/config.dart:33: Expect.fail("FAIL: ${testCase.description}");
Will this prevent subsequent tests in the suite from running? Is that desired?

https://chromiumcodereview.appspot.com/10031022/diff/2003/lib/unittest/config...
lib/unittest/config.dart:43: if (failed == 0 && errors == 0) {
I think we should also report an error if there were no tests at all (zero
passes, fails, and errors). That would catch common bugs where tests aren't
being run.

https://chromiumcodereview.appspot.com/10031022/diff/2003/lib/unittest/unitte...
File lib/unittest/unittest_dom.dart (right):

https://chromiumcodereview.appspot.com/10031022/diff/2003/lib/unittest/unitte...
lib/unittest/unittest_dom.dart:24: class PlatformConfiguration extends
Configuration {
"DomConfiguration".

https://chromiumcodereview.appspot.com/10031022/diff/2003/lib/unittest/unitte...
File lib/unittest/unittest_html.dart (right):

https://chromiumcodereview.appspot.com/10031022/diff/2003/lib/unittest/unitte...
lib/unittest/unittest_html.dart:24: void serialInvokeAsync(List closures) {
What is this and what is it used for? Needs docs.

https://chromiumcodereview.appspot.com/10031022/diff/2003/lib/unittest/unitte...
lib/unittest/unittest_html.dart:39: class PlatformConfiguration extends
Configuration {
HtmlConfiguration

https://chromiumcodereview.appspot.com/10031022/diff/2003/lib/unittest/unitte...
File lib/unittest/unittest_vm.dart (right):

https://chromiumcodereview.appspot.com/10031022/diff/2003/lib/unittest/unitte...
lib/unittest/unittest_vm.dart:16: class PlatformConfiguration implements
Configuration {
StandaloneConfiguration

https://chromiumcodereview.appspot.com/10031022/diff/2003/lib/unittest/unitte...
lib/unittest/unittest_vm.dart:17: void onInit() {}
Remove this.

https://chromiumcodereview.appspot.com/10031022/diff/2003/lib/unittest/unitte...
lib/unittest/unittest_vm.dart:18: void onStart() {}
And this.

[Siggi Cherem (dart-lang), 2012-04-10 21:40:18]

thanks for all the comments!

https://chromiumcodereview.appspot.com/10031022/diff/2003/lib/unittest/config...
File lib/unittest/config.dart (right):

https://chromiumcodereview.appspot.com/10031022/diff/2003/lib/unittest/config...
lib/unittest/config.dart:5: /**
On 2012/04/10 18:14:12, Emily Fortuna wrote:
> should we be switching to the /// style comments?
> 
> I know there's /** elsewhere around... so it's fine if we keep it for
> consistency

we should talk more about this. For now I want to keep it for consistency

https://chromiumcodereview.appspot.com/10031022/diff/2003/lib/unittest/config...
lib/unittest/config.dart:33: Expect.fail("FAIL: ${testCase.description}");
On 2012/04/10 20:04:25, Bob Nystrom wrote:
> Will this prevent subsequent tests in the suite from running? Is that desired?

I changed this just to print the result. It was on purpose, but not well thought
out.

https://chromiumcodereview.appspot.com/10031022/diff/2003/lib/unittest/config...
lib/unittest/config.dart:43: if (failed == 0 && errors == 0) {
On 2012/04/10 20:04:25, Bob Nystrom wrote:
> I think we should also report an error if there were no tests at all (zero
> passes, fails, and errors). That would catch common bugs where tests aren't
> being run.

Done. Basically I moved the VM logic here and changed exit(0) to throw an
exception. I'm open to suggestions on other ways to fix it.

https://chromiumcodereview.appspot.com/10031022/diff/2003/lib/unittest/config...
lib/unittest/config.dart:45: }
On 2012/04/10 18:06:02, eub wrote:
> This implementation makes me scratch my head -- it looks incomplete (a
one-sided
> test?), who will use it?

This was related to the way I was failing on onTestResult. Fixed.

https://chromiumcodereview.appspot.com/10031022/diff/2003/lib/unittest/shared...
File lib/unittest/shared.dart (right):

https://chromiumcodereview.appspot.com/10031022/diff/2003/lib/unittest/shared...
lib/unittest/shared.dart:4: 
On 2012/04/10 18:06:02, eub wrote:
> I know we don't have a convention, but I'd like to encourage any file that
> expects to be #sourced to have a file comment saying so, and by whom.

Sounds like a good idea. In this particular case, I added the comment, but note
I plan to get rid of this file and graduate it into a library...

https://chromiumcodereview.appspot.com/10031022/diff/2003/lib/unittest/shared...
lib/unittest/shared.dart:5: Configuration _config = null;
On 2012/04/10 18:06:02, eub wrote:
> (And what definitions the file expects to rely on from outside.)

Done.

https://chromiumcodereview.appspot.com/10031022/diff/2003/lib/unittest/unitte...
File lib/unittest/unittest_dom.dart (right):

https://chromiumcodereview.appspot.com/10031022/diff/2003/lib/unittest/unitte...
lib/unittest/unittest_dom.dart:24: class PlatformConfiguration extends
Configuration {
On 2012/04/10 20:04:25, Bob Nystrom wrote:
> "DomConfiguration".

Unfortunately for now it has to be [PlatformConfiguration]  because I'm still
using 'shared.dart'. Moving forward I plan to have better names for these as we
get rid of the multiple entry points.

https://chromiumcodereview.appspot.com/10031022/diff/2003/lib/unittest/unitte...
File lib/unittest/unittest_html.dart (right):

https://chromiumcodereview.appspot.com/10031022/diff/2003/lib/unittest/unitte...
lib/unittest/unittest_html.dart:24: void serialInvokeAsync(List closures) {
On 2012/04/10 20:04:25, Bob Nystrom wrote:
> What is this and what is it used for? Needs docs.

It was added by Jacob for some html tests. It was in shared.dart, and I moved it
here. I don't think this belongs in this library, any suggestions where it
should go?

So far I'm taking it out and moving it to the tests that use it (there are only
2).

https://chromiumcodereview.appspot.com/10031022/diff/2003/lib/unittest/unitte...
lib/unittest/unittest_html.dart:39: class PlatformConfiguration extends
Configuration {
On 2012/04/10 20:04:25, Bob Nystrom wrote:
> HtmlConfiguration

See comment on DomConfiguration

https://chromiumcodereview.appspot.com/10031022/diff/2003/lib/unittest/unitte...
lib/unittest/unittest_html.dart:69: onDone(int testsPassed, int testsFailed, int
testsErrors,
On 2012/04/10 18:06:02, eub wrote:
> Let's match the arg names between the parent class and here (and other
> children).

Done.

https://chromiumcodereview.appspot.com/10031022/diff/2003/lib/unittest/unitte...
File lib/unittest/unittest_vm.dart (right):

https://chromiumcodereview.appspot.com/10031022/diff/2003/lib/unittest/unitte...
lib/unittest/unittest_vm.dart:16: class PlatformConfiguration implements
Configuration {
On 2012/04/10 20:04:25, Bob Nystrom wrote:
> StandaloneConfiguration

see other comments.

https://chromiumcodereview.appspot.com/10031022/diff/2003/lib/unittest/unitte...
lib/unittest/unittest_vm.dart:17: void onInit() {}
On 2012/04/10 20:04:25, Bob Nystrom wrote:
> Remove this.

Done - :(, forgot to do a git cl upload

https://chromiumcodereview.appspot.com/10031022/diff/2003/lib/unittest/unitte...
lib/unittest/unittest_vm.dart:18: void onStart() {}
On 2012/04/10 20:04:25, Bob Nystrom wrote:
> And this.

Done.

https://chromiumcodereview.appspot.com/10031022/diff/2003/lib/unittest/unitte...
lib/unittest/unittest_vm.dart:46: // A non-zero exit code is used by the test
infrastructure to detect failure.
On 2012/04/10 18:06:02, eub wrote:
> 80 cols

Done.

[eub, 2012-04-10 21:52:24]

LGTM!  Thanks, Siggi.

http://codereview.chromium.org/10031022/diff/2004/lib/unittest/config.dart
File lib/unittest/config.dart (right):

http://codereview.chromium.org/10031022/diff/2004/lib/unittest/config.dart#ne...
lib/unittest/config.dart:62: // A non-zero exit code is used by the test
infrastructure to detect
"An exception is used ... to indicate failure"?

[Siggi Cherem (dart-lang), 2012-04-10 22:15:17]

done. THanks!

http://codereview.chromium.org/10031022/diff/2004/lib/unittest/config.dart
File lib/unittest/config.dart (right):

http://codereview.chromium.org/10031022/diff/2004/lib/unittest/config.dart#ne...
lib/unittest/config.dart:62: // A non-zero exit code is used by the test
infrastructure to detect
On 2012/04/10 21:52:24, eub wrote:
> "An exception is used ... to indicate failure"?

Done.

[Ivan Posva, 2012-04-13 07:16:16]

DBC

https://chromiumcodereview.appspot.com/10031022/diff/14001/lib/unittest/unitt...
File lib/unittest/unittest_html.dart (right):

https://chromiumcodereview.appspot.com/10031022/diff/14001/lib/unittest/unitt...
lib/unittest/unittest_html.dart:15: #source('html_print.dart');
Please add files in alphabetical order.