unittest step2: bye bye to multiple entrypoints for unittest

unittest step2: bye bye to multiple entrypoints for unittest

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

[Siggi Cherem (dart-lang), 2012-04-12 01:13:53]

this step focuses on merging the libraries and using an external configuration.

It looks more verbose at the client side (import *_config.dart, then call
use*Configuration), but the idea is that this is only going to be used if you
need a specific configuration.

In step 4 I plan to take any test that is independent of the platform and remove
it's uses of *_config. That will require changing test.dart first, so that it
will use the html_config whenever it wraps a test with no configuration to run
in a browser.

I said step 4, because step 3 will focus in getting ready for step 4 :-/.
Basically. we need to remove the current wrapping logic in test.dart, which
forces us to delete the TestFramework.dart (that lives in tests/isolates/src/),
which in turns requires from that we convert those tests to use unittest.dart.

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

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/config...
lib/unittest/config.dart:41: for (final t in _tests) {
this is because we are getting errors if names collide: test is also a top-level
function in this scope.

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/expect...
File lib/unittest/expectation.dart (right):

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/expect...
lib/unittest/expectation.dart:11: class Expectation {
this code is unchanged - just moved to its own file.

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/testca...
File lib/unittest/testcase.dart (right):

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/testca...
lib/unittest/testcase.dart:11: class TestCase {
ditto (unchanged, moved to its own file)

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

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/unitte...
lib/unittest/unittest.dart:256: Function _later2(Function callback) {
this is something I'll need for "step 3" (see email message), but somehow I got
started on it. I can remove this from this CL if you like.

https://chromiumcodereview.appspot.com/10037027/diff/1001/samples/tests/sampl...
File samples/tests/samples/src/lib/touch/TouchTest.dart (right):

https://chromiumcodereview.appspot.com/10037027/diff/1001/samples/tests/sampl...
samples/tests/samples/src/lib/touch/TouchTest.dart:7: #import('dart:html'); //
TODO(rnystrom): Only needed to tell architecture.py
this will hopefully be gone in step4 of this process ...

[Bob Nystrom, 2012-04-12 16:45:49]

Lots of questions, but this is coming along great.

https://chromiumcodereview.appspot.com/10037027/diff/1001/client/tests/client...
File client/tests/client/json/json_tests.dart (right):

https://chromiumcodereview.appspot.com/10037027/diff/1001/client/tests/client...
client/tests/client/json/json_tests.dart:13: useHtmlConfiguration();
Is this needed? Can we run JSON tests on all platforms?

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

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/config...
lib/unittest/config.dart:41: for (final t in _tests) {
On 2012/04/12 01:13:53, sigmund wrote:
> this is because we are getting errors if names collide: test is also a
top-level
> function in this scope.

Shadowing is an error? Can you leave a TODO explaining this?

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/dom_co...
File lib/unittest/dom_config.dart (right):

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/dom_co...
lib/unittest/dom_config.dart:16: DomConfiguration(this._isLayoutTest);
Style nit, newline before this to separate from field declaration?

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/dom_co...
lib/unittest/dom_config.dart:25: notifyError('(DOM callback has errors) Caught
${e}', '');
$e

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/expect...
File lib/unittest/expectation.dart (right):

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/expect...
lib/unittest/expectation.dart:11: class Expectation {
On 2012/04/12 01:13:53, sigmund wrote:
> this code is unchanged - just moved to its own file.

+1!

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/html_c...
File lib/unittest/html_config.dart (right):

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/html_c...
lib/unittest/html_config.dart:15: class HtmlConfiguration extends Configuration
{
Can this be unified with DomConfiguration? They seem very similar.

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/testca...
File lib/unittest/testcase.dart (right):

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/testca...
lib/unittest/testcase.dart:13: final id;
Type?

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/testca...
lib/unittest/testcase.dart:47: void fail(String message_, String stackTrace_) {
Are the trailing underscores needed here?

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/testca...
lib/unittest/testcase.dart:49: this.message = message_;
If they are, then get rid of "this." here.

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

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/unitte...
lib/unittest/unittest.dart:8: * **Concepts**
Use Markdown, one of:

## Concepts
## Concepts ##

Concepts
========

Concepts
--------

Pick whatever you like.

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/unitte...
lib/unittest/unittest.dart:22: *    
#import('path-to-dart/lib/unittest/unitest.dart');
Need blank line above this to separate out code block.

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/unitte...
lib/unittest/unittest.dart:90: *     }
This documentation is excellent!

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/unitte...
lib/unittest/unittest.dart:98: #source('testcase.dart');
"test_case"

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/unitte...
lib/unittest/unittest.dart:104: void configure(Configuration c) {
"c" -> "config".

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/unitte...
lib/unittest/unittest.dart:190: class _SentinelType {
I'd just do "_Sentinel".

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/unitte...
lib/unittest/unittest.dart:194: final _SentinelType _sentinel = const
_SentinelType();
Move this into the class and rename, like:

class _Sentinel {
  static final value = const _Sentinel();
  const _Sentinel();
}

Then usage is _Sentinel.value

WDYT?

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/unitte...
lib/unittest/unittest.dart:256: Function _later2(Function callback) {
On 2012/04/12 01:13:53, sigmund wrote:
> this is something I'll need for "step 3" (see email message), but somehow I
got
> started on it. I can remove this from this CL if you like.

Nope, it's fine. Thanks for clarifying.

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/unitte...
lib/unittest/unittest.dart:344: _runTest(TestCase testCase) {
I don't see this being called anywhere, can it be removed?

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/unitte...
lib/unittest/unittest.dart:359: _guard(TestCase testCase, tryBody,
[finallyBody]) {
Nice. In every callsite I see, testCase is always the current test. Instead of
passing it in, make it just assume that.

https://chromiumcodereview.appspot.com/10037027/diff/1001/samples/markdown/te...
File samples/markdown/test/markdown_tests.dart (right):

https://chromiumcodereview.appspot.com/10037027/diff/1001/samples/markdown/te...
samples/markdown/test/markdown_tests.dart:11:
#import('../../../lib/unittest/unittest.dart');
\o/

Does this mean we can change the status file and run these tests on the browser
and dart2js->d8 too?

[Siggi Cherem (dart-lang), 2012-04-12 17:36:28]

https://chromiumcodereview.appspot.com/10037027/diff/1001/client/tests/client...
File client/tests/client/json/json_tests.dart (right):

https://chromiumcodereview.appspot.com/10037027/diff/1001/client/tests/client...
client/tests/client/json/json_tests.dart:13: useHtmlConfiguration();
On 2012/04/12 16:45:49, Bob Nystrom wrote:
> Is this needed? Can we run JSON tests on all platforms?

It wont be needed in the near future (step 4) :)

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

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/config...
lib/unittest/config.dart:41: for (final t in _tests) {
On 2012/04/12 16:45:49, Bob Nystrom wrote:
> On 2012/04/12 01:13:53, sigmund wrote:
> > this is because we are getting errors if names collide: test is also a
> top-level
> > function in this scope.
> 
> Shadowing is an error? Can you leave a TODO explaining this?

Not sure, I thought this was intended (spec #3.1)?

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/dom_co...
File lib/unittest/dom_config.dart (right):

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/dom_co...
lib/unittest/dom_config.dart:16: DomConfiguration(this._isLayoutTest);
On 2012/04/12 16:45:49, Bob Nystrom wrote:
> Style nit, newline before this to separate from field declaration?

Done.

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/dom_co...
lib/unittest/dom_config.dart:25: notifyError('(DOM callback has errors) Caught
${e}', '');
On 2012/04/12 16:45:49, Bob Nystrom wrote:
> $e

Done.

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/html_c...
File lib/unittest/html_config.dart (right):

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/html_c...
lib/unittest/html_config.dart:15: class HtmlConfiguration extends Configuration
{
On 2012/04/12 16:45:49, Bob Nystrom wrote:
> Can this be unified with DomConfiguration? They seem very similar.

I don't recall the current status, but I was under the understanding that we
couldn't import html and dom together. Would you like me to create a common base
class for both of them?

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/testca...
File lib/unittest/testcase.dart (right):

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/testca...
lib/unittest/testcase.dart:13: final id;
On 2012/04/12 16:45:49, Bob Nystrom wrote:
> Type?

Done.

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/testca...
lib/unittest/testcase.dart:47: void fail(String message_, String stackTrace_) {
On 2012/04/12 16:45:49, Bob Nystrom wrote:
> Are the trailing underscores needed here?

Done.

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/testca...
lib/unittest/testcase.dart:49: this.message = message_;
On 2012/04/12 16:45:49, Bob Nystrom wrote:
> If they are, then get rid of "this." here.

Done.

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

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/unitte...
lib/unittest/unittest.dart:8: * **Concepts**
On 2012/04/12 16:45:49, Bob Nystrom wrote:
> Use Markdown, one of:
> 
> ## Concepts
> ## Concepts ##
> 
> Concepts
> ========
> 
> Concepts
> --------
> 
> Pick whatever you like.

Done.

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/unitte...
lib/unittest/unittest.dart:22: *    
#import('path-to-dart/lib/unittest/unitest.dart');
On 2012/04/12 16:45:49, Bob Nystrom wrote:
> Need blank line above this to separate out code block.

Ok, it didn't seem to need it when I created the page manually...

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/unitte...
lib/unittest/unittest.dart:90: *     }
On 2012/04/12 16:45:49, Bob Nystrom wrote:
> This documentation is excellent!

Thanks!

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/unitte...
lib/unittest/unittest.dart:98: #source('testcase.dart');
On 2012/04/12 16:45:49, Bob Nystrom wrote:
> "test_case"

Done.

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/unitte...
lib/unittest/unittest.dart:104: void configure(Configuration c) {
On 2012/04/12 16:45:49, Bob Nystrom wrote:
> "c" -> "config".

Done.

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/unitte...
lib/unittest/unittest.dart:190: class _SentinelType {
On 2012/04/12 16:45:49, Bob Nystrom wrote:
> I'd just do "_Sentinel".

Done.

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/unitte...
lib/unittest/unittest.dart:194: final _SentinelType _sentinel = const
_SentinelType();
On 2012/04/12 16:45:49, Bob Nystrom wrote:
> Move this into the class and rename, like:
> 
> class _Sentinel {
>   static final value = const _Sentinel();
>   const _Sentinel();
> }
> 
> Then usage is _Sentinel.value
> 
> WDYT?

cool, done

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/unitte...
lib/unittest/unittest.dart:344: _runTest(TestCase testCase) {
On 2012/04/12 16:45:49, Bob Nystrom wrote:
> I don't see this being called anywhere, can it be removed?

It's actually called by [nextBatch] below. I decided to inline it below.

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/unitte...
lib/unittest/unittest.dart:359: _guard(TestCase testCase, tryBody,
[finallyBody]) {
On 2012/04/12 16:45:49, Bob Nystrom wrote:
> Nice. In every callsite I see, testCase is always the current test. Instead of
> passing it in, make it just assume that.

Done.

https://chromiumcodereview.appspot.com/10037027/diff/1001/samples/markdown/te...
File samples/markdown/test/markdown_tests.dart (right):

https://chromiumcodereview.appspot.com/10037027/diff/1001/samples/markdown/te...
samples/markdown/test/markdown_tests.dart:11:
#import('../../../lib/unittest/unittest.dart');
On 2012/04/12 16:45:49, Bob Nystrom wrote:
> \o/
> 
> Does this mean we can change the status file and run these tests on the
browser
> and dart2js->d8 too?

probably - after step 4 for the browser. frog-d8 seems to run ok now, dart2js-d8
not yet (I haven't looked at the details)

[vsm, 2012-04-12 17:41:45]

A couple quick comments.

https://chromiumcodereview.appspot.com/10037027/diff/1001/client/tests/client...
File client/tests/client/dom/AsyncWindowTest.dart (right):

https://chromiumcodereview.appspot.com/10037027/diff/1001/client/tests/client...
client/tests/client/dom/AsyncWindowTest.dart:3:
#import('../../../../lib/unittest/dom_config.dart');
It's unfortunate that this requires an extra line in our tests.

https://chromiumcodereview.appspot.com/10037027/diff/1001/client/tests/client...
client/tests/client/dom/AsyncWindowTest.dart:7:
useDomConfiguration(isLayoutTest: true);
I think these can now go for all tests in the main dart repo (the isLayoutTest:
true part at least).  The dartium bot only runs these as via test.dart.

There are a handful of layout tests in the Dartium repo that use the framework. 
They'll need to be updated and probably need isLayoutTest set as well.

[Emily Fortuna, 2012-04-12 17:48:42]

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

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/html_p...
lib/unittest/html_print.dart:5: /** This file is sourced by both dom_config.dart
and html_config.dart. */
+1 helpful comments about source!

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

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/unitte...
lib/unittest/unittest.dart:90: *     }
On 2012/04/12 16:45:49, Bob Nystrom wrote:
> This documentation is excellent!

+1

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/unitte...
lib/unittest/unittest.dart:194: final _SentinelType _sentinel = const
_SentinelType();
On 2012/04/12 16:45:49, Bob Nystrom wrote:
> Move this into the class and rename, like:
> 
> class _Sentinel {
>   static final value = const _Sentinel();
>   const _Sentinel();
> }
> 
> Then usage is _Sentinel.value
> 
> WDYT?

+1

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/unitte...
lib/unittest/unittest.dart:206: Function _later(Function callback) {
Perhaps I missed it, but where is this function called?

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/unitte...
lib/unittest/unittest.dart:219: } else if (arg3 == _sentinel) {
arg2

[Bob Nystrom, 2012-04-12 18:01:37]

LGTM!

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

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/config...
lib/unittest/config.dart:41: for (final t in _tests) {
On 2012/04/12 17:36:28, sigmund wrote:
> On 2012/04/12 16:45:49, Bob Nystrom wrote:
> > On 2012/04/12 01:13:53, sigmund wrote:
> > > this is because we are getting errors if names collide: test is also a
> > top-level
> > > function in this scope.
> > 
> > Shadowing is an error? Can you leave a TODO explaining this?
> 
> Not sure, I thought this was intended (spec #3.1)?

Argh, the spec it's allowed but a static warning, which in practice means "not
allowed". OK, then.

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/html_c...
File lib/unittest/html_config.dart (right):

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/html_c...
lib/unittest/html_config.dart:15: class HtmlConfiguration extends Configuration
{
On 2012/04/12 17:36:28, sigmund wrote:
> On 2012/04/12 16:45:49, Bob Nystrom wrote:
> > Can this be unified with DomConfiguration? They seem very similar.
> 
> I don't recall the current status, but I was under the understanding that we
> couldn't import html and dom together. Would you like me to create a common
base
> class for both of them?

No, I wouldn't bother. DomConfiguration will just go away when dart:dom does.

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

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/unitte...
lib/unittest/unittest.dart:22: *    
#import('path-to-dart/lib/unittest/unitest.dart');
On 2012/04/12 17:36:28, sigmund wrote:
> On 2012/04/12 16:45:49, Bob Nystrom wrote:
> > Need blank line above this to separate out code block.
> 
> Ok, it didn't seem to need it when I created the page manually...

Hmm, yeah I think dartdoc will allow it but most markdown parsers won't. It
doesn't help to add a blank line just in case dartdoc gets closer to "spec".

[Emily Fortuna, 2012-04-12 19:24:40]

Don't commit yet, this still needs to be fixed! (see below)

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

https://chromiumcodereview.appspot.com/10037027/diff/10001/lib/unittest/unitt...
lib/unittest/unittest.dart:226: } else if (arg3 == _Sentinel.value) {
arg2!!!

[Siggi Cherem (dart-lang), 2012-04-12 21:13:36]

Thanks for all the comments. 

All set - I'm running one last round of local tests and submitting.

https://chromiumcodereview.appspot.com/10037027/diff/1001/client/tests/client...
File client/tests/client/dom/AsyncWindowTest.dart (right):

https://chromiumcodereview.appspot.com/10037027/diff/1001/client/tests/client...
client/tests/client/dom/AsyncWindowTest.dart:3:
#import('../../../../lib/unittest/dom_config.dart');
On 2012/04/12 17:41:45, vsm wrote:
> It's unfortunate that this requires an extra line in our tests.

yeah. For many tests the extra line will go away later on (at least for test
that are not specific for a particular configuration).

https://chromiumcodereview.appspot.com/10037027/diff/1001/client/tests/client...
client/tests/client/dom/AsyncWindowTest.dart:7:
useDomConfiguration(isLayoutTest: true);
On 2012/04/12 17:41:45, vsm wrote:
> I think these can now go for all tests in the main dart repo (the
isLayoutTest:
> true part at least).  The dartium bot only runs these as via test.dart.
> 
> There are a handful of layout tests in the Dartium repo that use the
framework. 
> They'll need to be updated and probably need isLayoutTest set as well.

Thanks, fixed.

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

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/html_p...
lib/unittest/html_print.dart:5: /** This file is sourced by both dom_config.dart
and html_config.dart. */
On 2012/04/12 17:48:42, Emily Fortuna wrote:
> +1 helpful comments about source!

I really liked this suggestion from Eli :)

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

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/unitte...
lib/unittest/unittest.dart:206: Function _later(Function callback) {
On 2012/04/12 17:48:42, Emily Fortuna wrote:
> Perhaps I missed it, but where is this function called?

it is not yet (see note below about step 3 :-))

https://chromiumcodereview.appspot.com/10037027/diff/1001/lib/unittest/unitte...
lib/unittest/unittest.dart:219: } else if (arg3 == _sentinel) {
On 2012/04/12 17:48:42, Emily Fortuna wrote:
> arg2

nice catch

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

https://chromiumcodereview.appspot.com/10037027/diff/10001/lib/unittest/unitt...
lib/unittest/unittest.dart:226: } else if (arg3 == _Sentinel.value) {
On 2012/04/12 19:24:41, Emily Fortuna wrote:
> arg2!!!

Done.