Enable Dartium tests in tools/test.dart.

Enable Dartium tests in tools/test.dart.

BUG=
TEST=tools/test.dart

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

[Bill Hesse, 2011-12-08 17:08:01]

This is not polished yet, but you can get a feel for the design before you make
your changes.

StandardTestSuite is subclassed by suite, so it cannot also be subclassed by
component.  ProcessFile is pretty much the only thing that needs to be
dispatched by component.

[Mads Ager (google), 2011-12-09 09:21:11]

Looks pretty simple. Let's get this finished ASAP!

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

http://codereview.chromium.org/8889016/diff/1/tools/testing/dart/browser_test...
tools/testing/dart/browser_test.dart:8: String sourceScript) => """
Nit: I think I would put the """ on a new line by itself.

http://codereview.chromium.org/8889016/diff/1/tools/testing/dart/browser_test...
tools/testing/dart/browser_test.dart:48: String WrapDartTestInLibrary(String
test) => """
Ditto.

http://codereview.chromium.org/8889016/diff/1/tools/testing/dart/browser_test...
tools/testing/dart/browser_test.dart:57: String library) => """
And here. I'll shut up now. ;-)

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

http://codereview.chromium.org/8889016/diff/1/tools/testing/dart/test_runner....
tools/testing/dart/test_runner.dart:71: if (!e.isEmpty()) newArguments.add(e);
Please undo. Two-space indent plese.

http://codereview.chromium.org/8889016/diff/1/tools/testing/dart/test_runner....
tools/testing/dart/test_runner.dart:83: class CompilingTestCase extends TestCase
{
Should we call this a BrowserTestCase instead? When you talk about it in
comments you write "browser tests fails ..."

http://codereview.chromium.org/8889016/diff/1/tools/testing/dart/test_runner....
tools/testing/dart/test_runner.dart:102: // RunningProcess.start() handles
CompilingTestCase specially, executing
Could you put this comment on the class level instead of here?

http://codereview.chromium.org/8889016/diff/1/tools/testing/dart/test_runner....
tools/testing/dart/test_runner.dart:153: // Browser tests fail unless stdout
contains
I would add a blank line before this comment.

http://codereview.chromium.org/8889016/diff/1/tools/testing/dart/test_runner....
tools/testing/dart/test_runner.dart:206: 
?

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

http://codereview.chromium.org/8889016/diff/1/tools/testing/dart/test_suite.d...
tools/testing/dart/test_suite.dart:7: //#source("libraries.dart");
?

http://codereview.chromium.org/8889016/diff/1/tools/testing/dart/test_suite.d...
tools/testing/dart/test_suite.dart:141: String this.suiteName,
Indentation

http://codereview.chromium.org/8889016/diff/1/tools/testing/dart/test_suite.d...
tools/testing/dart/test_suite.dart:235: if (configuration['component'] ==
'dartium') {
Everything up to and including the parsing of options from the file is done in
both. Extract into method? You should do the split here at the level of the
directory lister. Just register the processDartiumFile as the file callback
instead of processFile if the configuration is 'dartium'?

http://codereview.chromium.org/8889016/diff/1/tools/testing/dart/test_suite.d...
tools/testing/dart/test_suite.dart:273: String pathSeparator = new
Platform().pathSeparator();
This block of stuff looks familiar. That looks like duplication. Extract into
helper?

http://codereview.chromium.org/8889016/diff/1/tools/testing/dart/test_suite.d...
tools/testing/dart/test_suite.dart:291: 
Remove extra blank line.

http://codereview.chromium.org/8889016/diff/1/tools/testing/dart/test_suite.d...
tools/testing/dart/test_suite.dart:372: return filename.replaceAll('/', '_') +
'dartium.html';
path separator instead of '/'

[Bill Hesse, 2011-12-09 12:33:28]

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

http://codereview.chromium.org/8889016/diff/1/tools/testing/dart/browser_test...
tools/testing/dart/browser_test.dart:57: String library) => """
On 2011/12/09 09:21:12, Mads Ager wrote:
> And here. I'll shut up now. ;-)

Done.

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

http://codereview.chromium.org/8889016/diff/1/tools/testing/dart/test_runner....
tools/testing/dart/test_runner.dart:153: // Browser tests fail unless stdout
contains
On 2011/12/09 09:21:12, Mads Ager wrote:
> I would add a blank line before this comment.

Done.

http://codereview.chromium.org/8889016/diff/1/tools/testing/dart/test_runner....
tools/testing/dart/test_runner.dart:206: 
Implementation added.

On 2011/12/09 09:21:12, Mads Ager wrote:
> ?

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

http://codereview.chromium.org/8889016/diff/1/tools/testing/dart/test_suite.d...
tools/testing/dart/test_suite.dart:235: if (configuration['component'] ==
'dartium') {
Split is done at the directory lister.
These initial lines are hard to put in a method, because they just do early
returns from the function, and set a local variable of the function.
On 2011/12/09 09:21:12, Mads Ager wrote:
> Everything up to and including the parsing of options from the file is done in
> both. Extract into method? You should do the split here at the level of the
> directory lister. Just register the processDartiumFile as the file callback
> instead of processFile if the configuration is 'dartium'?

http://codereview.chromium.org/8889016/diff/1/tools/testing/dart/test_suite.d...
tools/testing/dart/test_suite.dart:273: String pathSeparator = new
Platform().pathSeparator();
Moved to final class member.

On 2011/12/09 09:21:12, Mads Ager wrote:
> This block of stuff looks familiar. That looks like duplication. Extract into
> helper?

http://codereview.chromium.org/8889016/diff/1/tools/testing/dart/test_suite.d...
tools/testing/dart/test_suite.dart:372: return filename.replaceAll('/', '_') +
'dartium.html';
On 2011/12/09 09:21:12, Mads Ager wrote:
> path separator instead of '/'

Done.

[Mads Ager (google), 2011-12-09 13:19:37]

LGTM with a couple of comments.

http://codereview.chromium.org/8889016/diff/3002/tools/testing/dart/test_runn...
File tools/testing/dart/test_runner.dart (right):

http://codereview.chromium.org/8889016/diff/3002/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:83: /** RunningProcess.start() handles
CompilingTestCase specially, executing
BrowserTestCase

I would reformulate to say something about the class instead of start:

/**
  * BrowserTestCase that is handled specially by...
  */
?

http://codereview.chromium.org/8889016/diff/3002/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:240: 
Remove the extra new lines.

http://codereview.chromium.org/8889016/diff/3002/tools/testing/dart/test_suit...
File tools/testing/dart/test_suite.dart (right):

http://codereview.chromium.org/8889016/diff/3002/tools/testing/dart/test_suit...
tools/testing/dart/test_suite.dart:265: // Only run the tests that match the
pattern.
I still think these line should be extracted into a helper that returns a
boolean indicating whether the test should be skipped?

http://codereview.chromium.org/8889016/diff/3002/tools/testing/dart/test_suit...
tools/testing/dart/test_suite.dart:300: // TODO(whesse): Create directory in the
client case, if it doesn't exist.
I'm not sure I understand this TODO. If client doesn't exist should you get
here? If you should then you should deal with this TODO before commit.