Add DirectoryTestSuite.

Add DirectoryTestSuite.

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

[Bob Nystrom, 2012-03-28 23:00:35]

This patch is a request for comments.

I want to make it easier to add new test suites. Following the existing suites
(in utils and lib), the process is (as far as I know):

1. Create a directory for your tests.
2. Create a status file in there.
3. Create a test_config.dart file. Edit it to point to the files you're creating
in here.
4. Create a src/ directory.
5. Create your tests.
6. Add #import(..test_config.dart) to test.dart.
7. Add an if (selectors.containsKey()) section to test.dart.

That makes me sad and angry, like an emo hulk. With this patch, it is:

1. Create a directory for your tests.
2. Create a status file in there.
3. Create your tests.
4. Add the path to your test directory to TEST_SUITE_DIRECTORIES.

This patch does not convert any existing tests to follow this convention, but I
am up for doing that if anyone wants.

Thoughts?

[Emily Fortuna, 2012-03-28 23:10:10]

lgtm with comments. I'm totally down for moving to this different format for
specifying tests. As an FYI when you're converting stuff: the VM team IIRC has a
slightly different way of denoting tests (must end in "Test.dart" instead of the
underscore _test).

https://chromiumcodereview.appspot.com/9863057/diff/1001/utils/tests/pub/pub....
File utils/tests/pub/pub.status (right):

https://chromiumcodereview.appspot.com/9863057/diff/1001/utils/tests/pub/pub....
utils/tests/pub/pub.status:5: [ $runtime == drt || $runtime == ie || $runtime ==
chrome || $runtime == safari || $runtime == ff || $runtime == opera ]
also $runtime == d8 (we also want to skip)

https://chromiumcodereview.appspot.com/9863057/diff/1001/utils/tests/pub/pub_...
File utils/tests/pub/pub_tests.dart (right):

https://chromiumcodereview.appspot.com/9863057/diff/1001/utils/tests/pub/pub_...
utils/tests/pub/pub_tests.dart:7: //#import('../../../pub/pub.dart');
why is this line here but commented out?

[Emily Fortuna, 2012-03-28 23:18:13]

https://chromiumcodereview.appspot.com/9863057/diff/1001/utils/tests/pub/pub....
File utils/tests/pub/pub.status (right):

https://chromiumcodereview.appspot.com/9863057/diff/1001/utils/tests/pub/pub....
utils/tests/pub/pub.status:5: [ $runtime == drt || $runtime == ie || $runtime ==
chrome || $runtime == safari || $runtime == ff || $runtime == opera ]
On 2012/03/28 23:10:11, Emily Fortuna wrote:
> also $runtime == d8 (we also want to skip)

Here's a more concise way of saying (effectively) the same thing:
[$compiler == frog || $compiler == dart2js || $compiler == dartc || $compiler ==
frogsh ]
*: Skip

[Siggi Cherem (dart-lang), 2012-03-28 23:20:34]

On 2012/03/28 23:10:10, Emily Fortuna wrote:
> lgtm with comments. I'm totally down for moving to this different format for
> specifying tests. As an FYI when you're converting stuff: the VM team IIRC has
a
> slightly different way of denoting tests (must end in "Test.dart" instead of
the
> underscore _test).

There are actually both uses of Test and _tests already, and I personally prefer
the latter :-). I think the naming is a bit historical: we used to have .app and
.lib files and used camelCase names for .dart files. When we removed .app/.lib,
we also switched to start using lower_case names, but we didn't fix the tests
and now we have both kind of files everywhere. 

I'll send an email about this to dart-team, but I strongly prefer we convert
things to use lower_case everywhere.

> 
>
https://chromiumcodereview.appspot.com/9863057/diff/1001/utils/tests/pub/pub....
> File utils/tests/pub/pub.status (right):
> 
>
https://chromiumcodereview.appspot.com/9863057/diff/1001/utils/tests/pub/pub....
> utils/tests/pub/pub.status:5: [ $runtime == drt || $runtime == ie || $runtime
==
> chrome || $runtime == safari || $runtime == ff || $runtime == opera ]
> also $runtime == d8 (we also want to skip)
> 
>
https://chromiumcodereview.appspot.com/9863057/diff/1001/utils/tests/pub/pub_...
> File utils/tests/pub/pub_tests.dart (right):
> 
>
https://chromiumcodereview.appspot.com/9863057/diff/1001/utils/tests/pub/pub_...
> utils/tests/pub/pub_tests.dart:7: //#import('../../../pub/pub.dart');
> why is this line here but commented out?

[Bill Hesse, 2012-03-28 23:37:30]

https://chromiumcodereview.appspot.com/9863057/diff/1001/tools/test.dart
File tools/test.dart (right):

https://chromiumcodereview.appspot.com/9863057/diff/1001/tools/test.dart#newc...
tools/test.dart:161: queue.addTestSuite(new DirectoryTestSuite(conf,
testSuiteDir));
I would rather call this StandardTestSuite, and either move StandardTestSuite to
TestSuiteBaseImpl, or just add a new constructor to StandardTestSuite (like
new StandardTestSuite.fromDirectory(conf, testSuiteDir).

https://chromiumcodereview.appspot.com/9863057/diff/1001/tools/testing/dart/t...
File tools/testing/dart/test_suite.dart (right):

https://chromiumcodereview.appspot.com/9863057/diff/1001/tools/testing/dart/t...
tools/testing/dart/test_suite.dart:956: class DirectoryTestSuite extends
StandardTestSuite {
I really think we can merge the classes:
Make isTestFile call a closure passed to the constructor.
Default closure for StandardTestSuite is (name)=>name.endsWith("Test.dart");

Then all of DirectoryTestSuite can just be a new constructor for
StandardTestSuite.

This may also allow other subclasses, which only override isTestFile, to be
removed, and to be replaced with instances of StandardTestSuite.

https://chromiumcodereview.appspot.com/9863057/diff/1001/tools/testing/dart/t...
tools/testing/dart/test_suite.dart:959: print(name);
Remove the print statement.

https://chromiumcodereview.appspot.com/9863057/diff/1001/utils/tests/pub/pub....
File utils/tests/pub/pub.status (right):

https://chromiumcodereview.appspot.com/9863057/diff/1001/utils/tests/pub/pub....
utils/tests/pub/pub.status:5: [ $runtime == drt || $runtime == ie || $runtime ==
chrome || $runtime == safari || $runtime == ff || $runtime == opera ]
Is this an indication that we really need to add negation to the status
expression logic?  I left it out, because I thought that this case would not
happen.


On 2012/03/28 23:18:13, Emily Fortuna wrote:
> On 2012/03/28 23:10:11, Emily Fortuna wrote:
> > also $runtime == d8 (we also want to skip)
> 
> Here's a more concise way of saying (effectively) the same thing:
> [$compiler == frog || $compiler == dart2js || $compiler == dartc || $compiler
==
> frogsh ]
> *: Skip

[Bob Nystrom, 2012-03-29 00:10:16]

Thanks!

https://chromiumcodereview.appspot.com/9863057/diff/1001/tools/test.dart
File tools/test.dart (right):

https://chromiumcodereview.appspot.com/9863057/diff/1001/tools/test.dart#newc...
tools/test.dart:161: queue.addTestSuite(new DirectoryTestSuite(conf,
testSuiteDir));
On 2012/03/28 23:37:30, Bill Hesse wrote:
> I would rather call this StandardTestSuite, and either move StandardTestSuite
to
> TestSuiteBaseImpl, or just add a new constructor to StandardTestSuite (like
> new StandardTestSuite.fromDirectory(conf, testSuiteDir).

Great idea. Done.

https://chromiumcodereview.appspot.com/9863057/diff/1001/tools/testing/dart/t...
File tools/testing/dart/test_suite.dart (right):

https://chromiumcodereview.appspot.com/9863057/diff/1001/tools/testing/dart/t...
tools/testing/dart/test_suite.dart:956: class DirectoryTestSuite extends
StandardTestSuite {
On 2012/03/28 23:37:30, Bill Hesse wrote:
> I really think we can merge the classes:
> Make isTestFile call a closure passed to the constructor.
> Default closure for StandardTestSuite is (name)=>name.endsWith("Test.dart");
> 
> Then all of DirectoryTestSuite can just be a new constructor for
> StandardTestSuite.
> 
> This may also allow other subclasses, which only override isTestFile, to be
> removed, and to be replaced with instances of StandardTestSuite.

Excellent idea. Done!

https://chromiumcodereview.appspot.com/9863057/diff/1001/tools/testing/dart/t...
tools/testing/dart/test_suite.dart:959: print(name);
On 2012/03/28 23:37:30, Bill Hesse wrote:
> Remove the print statement.

Done.

https://chromiumcodereview.appspot.com/9863057/diff/1001/utils/tests/pub/pub....
File utils/tests/pub/pub.status (right):

https://chromiumcodereview.appspot.com/9863057/diff/1001/utils/tests/pub/pub....
utils/tests/pub/pub.status:5: [ $runtime == drt || $runtime == ie || $runtime ==
chrome || $runtime == safari || $runtime == ff || $runtime == opera ]
On 2012/03/28 23:18:13, Emily Fortuna wrote:
> On 2012/03/28 23:10:11, Emily Fortuna wrote:
> > also $runtime == d8 (we also want to skip)
> 
> Here's a more concise way of saying (effectively) the same thing:
> [$compiler == frog || $compiler == dart2js || $compiler == dartc || $compiler
==
> frogsh ]
> *: Skip

Done.

https://chromiumcodereview.appspot.com/9863057/diff/1001/utils/tests/pub/pub....
utils/tests/pub/pub.status:5: [ $runtime == drt || $runtime == ie || $runtime ==
chrome || $runtime == safari || $runtime == ff || $runtime == opera ]
On 2012/03/28 23:37:30, Bill Hesse wrote:
> Is this an indication that we really need to add negation to the status
> expression logic?  I left it out, because I thought that this case would not
> happen.
> 
> 
> On 2012/03/28 23:18:13, Emily Fortuna wrote:
> > On 2012/03/28 23:10:11, Emily Fortuna wrote:
> > > also $runtime == d8 (we also want to skip)
> > 
> > Here's a more concise way of saying (effectively) the same thing:
> > [$compiler == frog || $compiler == dart2js || $compiler == dartc ||
$compiler
> ==
> > frogsh ]
> > *: Skip
> 

I thought about just adding "!=" support to test.dart for this, but I figured I
was making enough changes in this patch as it is. I can do that in another patch
if you'd like.

https://chromiumcodereview.appspot.com/9863057/diff/1001/utils/tests/pub/pub_...
File utils/tests/pub/pub_tests.dart (right):

https://chromiumcodereview.appspot.com/9863057/diff/1001/utils/tests/pub/pub_...
utils/tests/pub/pub_tests.dart:7: //#import('../../../pub/pub.dart');
On 2012/03/28 23:10:11, Emily Fortuna wrote:
> why is this line here but commented out?

Pub doesn't exist yet! This is a work-in-progress. It's basically a shell so
that I can use it to validate that the test.dart changes work. Removed it.