Change test scripts to use Path library in most places, instead of strings.

Add Path library and change test scripts, combined change.



Use Path for the filename and TestInformation.filename throughout test scripts.


Change test_suite.dart to use Path class.


Fix bug in join


Address comments.


Address comments


Address comments on Path library.


Add path library to dart:io.


BUG=
TEST=

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

[Bill Hesse, 2012-06-26 12:58:40]

https://chromiumcodereview.appspot.com/10584014/diff/12001/tools/testing/dart...
File tools/testing/dart/test_suite.dart (left):

https://chromiumcodereview.appspot.com/10584014/diff/12001/tools/testing/dart...
tools/testing/dart/test_suite.dart:362: if (start > testsStart) {
This branch is only hit by co19 tests, and it produces the same value as the
else branch does, so drop it.

https://chromiumcodereview.appspot.com/10584014/diff/12001/tools/testing/dart...
File tools/testing/dart/test_suite.dart (right):

https://chromiumcodereview.appspot.com/10584014/diff/12001/tools/testing/dart...
tools/testing/dart/test_suite.dart:365: if
(TestUtils.isBrowserRuntime(configuration['runtime'])) return;
The skip of multitest on browser is moved here from 2 later places.

https://chromiumcodereview.appspot.com/10584014/diff/12001/tools/testing/dart...
tools/testing/dart/test_suite.dart:704: // Create '[build<
dir]/generated_tests/$compiler-$runtime/$testUniqueName',
Stray typo.

https://chromiumcodereview.appspot.com/10584014/diff/12001/tools/testing/dart...
tools/testing/dart/test_suite.dart:1125: for (String segment in
relativePath.segments()) {
This loop is really inefficient, because segments is recalculated each time.

https://chromiumcodereview.appspot.com/10584014/diff/12001/tools/testing/dart...
tools/testing/dart/test_suite.dart:1251: File scriptF = new File(new
Options().script);
Fix tabx.

[Anton Muhin, 2012-06-26 13:22:18]

that's huge

https://chromiumcodereview.appspot.com/10584014/diff/12001/tools/testing/dart...
File tools/testing/dart/test_suite.dart (right):

https://chromiumcodereview.appspot.com/10584014/diff/12001/tools/testing/dart...
tools/testing/dart/test_suite.dart:196: // String get filename() => '$filePath';
commented out code

https://chromiumcodereview.appspot.com/10584014/diff/15001/samples/tests/dart...
File samples/tests/dartc/test_config.dart (right):

https://chromiumcodereview.appspot.com/10584014/diff/15001/samples/tests/dart...
samples/tests/dartc/test_config.dart:41: bool isTestFile(String filename) {
shouldn't this method access Path too?

https://chromiumcodereview.appspot.com/10584014/diff/15001/tools/test-runtime...
File tools/test-runtime.dart (right):

https://chromiumcodereview.appspot.com/10584014/diff/15001/tools/test-runtime...
tools/test-runtime.dart:84: for (final testSuiteDir in TEST_SUITE_DIRECTORIES) {
should TEST_SUITE_DIRECTORIES be a list of Paths now?

https://chromiumcodereview.appspot.com/10584014/diff/15001/tools/test-runtime...
tools/test-runtime.dart:86: final name = testSuitePath.filename;
nit: that reads slightly weird: it's rather dir name than file name, but that's
API

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

https://chromiumcodereview.appspot.com/10584014/diff/15001/tools/testing/dart...
tools/testing/dart/browser_test.dart:36: String DartTestWrapper(Path dartHome,
Path library) {
nit: switch to => syntax here too?

https://chromiumcodereview.appspot.com/10584014/diff/15001/tools/testing/dart...
File tools/testing/dart/multitest.dart (right):

https://chromiumcodereview.appspot.com/10584014/diff/15001/tools/testing/dart...
tools/testing/dart/multitest.dart:176: if
(relativePath.toString().contains('..')) {
looks like if you swap ..-check and foundImports, you won't have to call
toString on path.  Or just keep both and Path and String version.

https://chromiumcodereview.appspot.com/10584014/diff/15001/tools/testing/dart...
File tools/testing/dart/test_suite.dart (right):

https://chromiumcodereview.appspot.com/10584014/diff/15001/tools/testing/dart...
tools/testing/dart/test_suite.dart:114: : dartDir =
TestUtils.dartDir().toNativePath() {
should dartDir be a String?

https://chromiumcodereview.appspot.com/10584014/diff/15001/tools/testing/dart...
tools/testing/dart/test_suite.dart:196: // String get filename() => '$filePath';
commented out code

https://chromiumcodereview.appspot.com/10584014/diff/15001/tools/testing/dart...
tools/testing/dart/test_suite.dart:334:
'${dartDir.toNativePath()}/$statusFilePath',
join?

[Emily Fortuna, 2012-06-26 18:26:24]

lgtm, with comments.

https://chromiumcodereview.appspot.com/10584014/diff/13004/tools/test-runtime...
File tools/test-runtime.dart (right):

https://chromiumcodereview.appspot.com/10584014/diff/13004/tools/test-runtime...
tools/test-runtime.dart:85: final name = testSuiteDir.filename;
nice! :-D

https://chromiumcodereview.appspot.com/10584014/diff/13004/tools/testing/dart...
File tools/testing/dart/test_suite.dart (right):

https://chromiumcodereview.appspot.com/10584014/diff/13004/tools/testing/dart...
tools/testing/dart/test_suite.dart:372:
Expect.isTrue(filePath.toNativePath().startsWith(
Just FYI: Isn't we planning on removing Expect as a built in feature of the
language?

https://chromiumcodereview.appspot.com/10584014/diff/13004/tools/testing/dart...
tools/testing/dart/test_suite.dart:1281: 'ff': 1}.containsKey(runtime);
Why make this a map here? How about:

static bool isBrowserRuntime(String runtime) =>
     const <String>['drt', 'dartium', 'ie', 'safari', 'opera', 'chrome',
'ff'].indexOf(runtime) > -1;

or similar

[Bill Hesse, 2012-06-27 09:35:03]

This change uses the new Path class in most places in the core test scripts
code.  I didn't use it in places where it just made things more complex, and I
didn't refactor all the custom implementations of TestSuite, because I didn't
think it was worth the time to refactor code that isn't touched much.

https://chromiumcodereview.appspot.com/10584014/diff/12001/tools/testing/dart...
File tools/testing/dart/test_suite.dart (right):

https://chromiumcodereview.appspot.com/10584014/diff/12001/tools/testing/dart...
tools/testing/dart/test_suite.dart:196: // String get filename() => '$filePath';
On 2012/06/26 13:22:18, antonmuhin wrote:
> commented out code

Done.

https://chromiumcodereview.appspot.com/10584014/diff/12001/tools/testing/dart...
tools/testing/dart/test_suite.dart:704: // Create '[build<
dir]/generated_tests/$compiler-$runtime/$testUniqueName',
On 2012/06/26 12:58:40, Bill Hesse wrote:
> Stray typo.

Done.

https://chromiumcodereview.appspot.com/10584014/diff/12001/tools/testing/dart...
tools/testing/dart/test_suite.dart:1125: for (String segment in
relativePath.segments()) {
On 2012/06/26 12:58:40, Bill Hesse wrote:
> This loop is really inefficient, because segments is recalculated each time.

Done.

https://chromiumcodereview.appspot.com/10584014/diff/12001/tools/testing/dart...
tools/testing/dart/test_suite.dart:1251: File scriptF = new File(new
Options().script);
On 2012/06/26 12:58:40, Bill Hesse wrote:
> Fix tabx.

Done.

https://chromiumcodereview.appspot.com/10584014/diff/15001/samples/tests/dart...
File samples/tests/dartc/test_config.dart (right):

https://chromiumcodereview.appspot.com/10584014/diff/15001/samples/tests/dart...
samples/tests/dartc/test_config.dart:41: bool isTestFile(String filename) {
On 2012/06/26 13:22:18, antonmuhin wrote:
> shouldn't this method access Path too?

All the existing tests are against strings, and this happens immediately on
getting a filename from the directory listing, as a string, so I saw no
advantage, and only disadvantages, to making the change.  I didn't change all of
the classes, only the code likely to be extended and maintained in the future.

https://chromiumcodereview.appspot.com/10584014/diff/15001/tools/test-runtime...
File tools/test-runtime.dart (right):

https://chromiumcodereview.appspot.com/10584014/diff/15001/tools/test-runtime...
tools/test-runtime.dart:84: for (final testSuiteDir in TEST_SUITE_DIRECTORIES) {
On 2012/06/26 13:22:18, antonmuhin wrote:
> should TEST_SUITE_DIRECTORIES be a list of Paths now?

It is just a pain to write a list of paths instead of a list of strings, but you
are probably right - changed.

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

https://chromiumcodereview.appspot.com/10584014/diff/15001/tools/testing/dart...
tools/testing/dart/browser_test.dart:36: String DartTestWrapper(Path dartHome,
Path library) {
On 2012/06/26 13:22:18, antonmuhin wrote:
> nit: switch to => syntax here too?

Done.

https://chromiumcodereview.appspot.com/10584014/diff/15001/tools/testing/dart...
File tools/testing/dart/multitest.dart (right):

https://chromiumcodereview.appspot.com/10584014/diff/15001/tools/testing/dart...
tools/testing/dart/multitest.dart:176: if
(relativePath.toString().contains('..')) {
I like the principle of creating the path immediately.  And toString is cheap -
Path contains a single final string instance variable.  Path is a string.

I think the better choice is to add .contains to Path.

On 2012/06/26 13:22:18, antonmuhin wrote:
> looks like if you swap ..-check and foundImports, you won't have to call
> toString on path.  Or just keep both and Path and String version.

https://chromiumcodereview.appspot.com/10584014/diff/15001/tools/testing/dart...
File tools/testing/dart/test_suite.dart (right):

https://chromiumcodereview.appspot.com/10584014/diff/15001/tools/testing/dart...
tools/testing/dart/test_suite.dart:114: : dartDir =
TestUtils.dartDir().toNativePath() {
On 2012/06/26 13:22:18, antonmuhin wrote:
> should dartDir be a String?

I'm not changing the class CCTestSuite, except to make it use the new interface
of the helper functions.

https://chromiumcodereview.appspot.com/10584014/diff/15001/tools/testing/dart...
tools/testing/dart/test_suite.dart:196: // String get filename() => '$filePath';
On 2012/06/26 13:22:18, antonmuhin wrote:
> commented out code

Done.

https://chromiumcodereview.appspot.com/10584014/diff/15001/tools/testing/dart...
tools/testing/dart/test_suite.dart:334:
'${dartDir.toNativePath()}/$statusFilePath',
Actually, append, since statusFilePaths is a list of strings.
On 2012/06/26 13:22:18, antonmuhin wrote:
> join?

Done.

https://chromiumcodereview.appspot.com/10584014/diff/13004/tools/testing/dart...
File tools/testing/dart/test_suite.dart (right):

https://chromiumcodereview.appspot.com/10584014/diff/13004/tools/testing/dart...
tools/testing/dart/test_suite.dart:372:
Expect.isTrue(filePath.toNativePath().startsWith(
On 2012/06/26 18:26:24, Emily Fortuna wrote:
> Just FYI: Isn't we planning on removing Expect as a built in feature of the
> language?

Yes, but it fits such a need here, in a script that just wants to bail out with
an informative message on tests, that if it really goes away, I would replace it
with utility methods here.

I would really miss Expect - I like writing quick tests with error messages, and
the unittest library seems like it won't replace it in general use.  Assert only
runs in debug mode, and throwing exceptions manually is more lines of
boilerplate everywhere.

https://chromiumcodereview.appspot.com/10584014/diff/13004/tools/testing/dart...
tools/testing/dart/test_suite.dart:1281: 'ff': 1}.containsKey(runtime);
It was a list, using contains, but that doesn't take advantage of hashing.  I
just think there should be an example of how to make a constant lookup table,
since there is no such thing as a constant Set or HashSet.  This illustrates a
missing feature of the language, and how to solve it.



On 2012/06/26 18:26:24, Emily Fortuna wrote:
> Why make this a map here? How about:
> 
> static bool isBrowserRuntime(String runtime) =>
>      const <String>['drt', 'dartium', 'ie', 'safari', 'opera', 'chrome',
> 'ff'].indexOf(runtime) > -1;
> 
> or similar

[Anton Muhin, 2012-06-27 13:11:24]

lgtm w/ comments addressed

https://chromiumcodereview.appspot.com/10584014/diff/13004/tools/testing/dart...
File tools/testing/dart/test_suite.dart (right):

https://chromiumcodereview.appspot.com/10584014/diff/13004/tools/testing/dart...
tools/testing/dart/test_suite.dart:375: filePath.relativeTo(suiteDir);
nit: won't it fit a single line?

https://chromiumcodereview.appspot.com/10584014/diff/13004/tools/testing/dart...
tools/testing/dart/test_suite.dart:696: testUniqueName =
testUniqueName.substring(0, testUniqueName.length - 5);
isn't that to fetch the path w/o extension?  if yes, do not we have better
methods in Path class?

[Mads Ager (google), 2012-06-27 16:08:18]

DBC

Overall this is looking a lot better than the old string manipulations! :)

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

https://chromiumcodereview.appspot.com/10584014/diff/13004/tools/testing/dart...
tools/testing/dart/browser_test.dart:41:
#import('${dartHome.append('lib/unittest/html_config.dart')}', prefix:
'config');
Long line.

https://chromiumcodereview.appspot.com/10584014/diff/13004/tools/testing/dart...
File tools/testing/dart/multitest.dart (right):

https://chromiumcodereview.appspot.com/10584014/diff/13004/tools/testing/dart...
tools/testing/dart/multitest.dart:104: "Invalid test directive '$nextOutcome' on
line ${lineCount}:\n"
Is the addition of a newline character on purpose here?

https://chromiumcodereview.appspot.com/10584014/diff/13004/tools/testing/dart...
tools/testing/dart/multitest.dart:118: Expect.fail("Test ${outcomeKey} has no
valid annotated outcomes.\n"
Ditto.

https://chromiumcodereview.appspot.com/10584014/diff/13004/tools/testing/dart...
File tools/testing/dart/test_suite.dart (right):

https://chromiumcodereview.appspot.com/10584014/diff/13004/tools/testing/dart...
tools/testing/dart/test_suite.dart:696: testUniqueName =
testUniqueName.substring(0, testUniqueName.length - 5);
On 2012/06/27 13:11:24, antonmuhin wrote:
> isn't that to fetch the path w/o extension?  if yes, do not we have better
> methods in Path class?

Something like:

path = path.directoryPath.append(path.filenameWithoutExtension);

should do it and be easier to read. :)

https://chromiumcodereview.appspot.com/10584014/diff/13004/tools/testing/dart...
tools/testing/dart/test_suite.dart:749: String get dumpRenderTreeFilename() {
Should we make these return Path objects and wait with the toNativePath call
until we actually need to have the native path?

https://chromiumcodereview.appspot.com/10584014/diff/13004/tools/testing/dart...
tools/testing/dart/test_suite.dart:1250: File scriptF = new File(new
Options().script);
Spell out File and Path?

This is so much nicer than the old version! :)

https://chromiumcodereview.appspot.com/10584014/diff/13004/tools/testing/dart...
tools/testing/dart/test_suite.dart:1281: 'ff': 1}.containsKey(runtime);
On 2012/06/27 09:35:03, Bill Hesse wrote:
> It was a list, using contains, but that doesn't take advantage of hashing.  I
> just think there should be an example of how to make a constant lookup table,
> since there is no such thing as a constant Set or HashSet.  This illustrates a
> missing feature of the language, and how to solve it.

I agree with Emily. I think the code in test.dart should be as simple as
possible. Using a map here does not seem like the simplest thing and performance
is definitely not an issue here.

> On 2012/06/26 18:26:24, Emily Fortuna wrote:
> > Why make this a map here? How about:
> > 
> > static bool isBrowserRuntime(String runtime) =>
> >      const <String>['drt', 'dartium', 'ie', 'safari', 'opera', 'chrome',
> > 'ff'].indexOf(runtime) > -1;
> > 
> > or similar
>

[Emily Fortuna, 2012-06-27 16:56:49]

https://chromiumcodereview.appspot.com/10584014/diff/13004/tools/testing/dart...
File tools/testing/dart/test_suite.dart (right):

https://chromiumcodereview.appspot.com/10584014/diff/13004/tools/testing/dart...
tools/testing/dart/test_suite.dart:1281: 'ff': 1}.containsKey(runtime);
On 2012/06/27 16:08:18, Mads Ager wrote:
> On 2012/06/27 09:35:03, Bill Hesse wrote:
> > It was a list, using contains, but that doesn't take advantage of hashing. 
I
> > just think there should be an example of how to make a constant lookup
table,
> > since there is no such thing as a constant Set or HashSet.  This illustrates
a
> > missing feature of the language, and how to solve it.
> 
> I agree with Emily. I think the code in test.dart should be as simple as
> possible. Using a map here does not seem like the simplest thing and
performance
> is definitely not an issue here.
> 
> > On 2012/06/26 18:26:24, Emily Fortuna wrote:
> > > Why make this a map here? How about:
> > > 
> > > static bool isBrowserRuntime(String runtime) =>
> > >      const <String>['drt', 'dartium', 'ie', 'safari', 'opera', 'chrome',
> > > 'ff'].indexOf(runtime) > -1;
> > > 
> > > or similar
> > 
> 

Yeah, for the sake of readability and clarity, I prefer a list, even if it is
slightly less performant. I doubt any bottlenecks that develop would be here.

[Siggi Cherem (dart-lang), 2012-06-27 17:19:03]

very nice to see Path used here :)

https://chromiumcodereview.appspot.com/10584014/diff/13004/tools/testing/dart...
File tools/testing/dart/test_suite.dart (right):

https://chromiumcodereview.appspot.com/10584014/diff/13004/tools/testing/dart...
tools/testing/dart/test_suite.dart:602: String compiledName =
namePath.filenameWithoutExtension;
missing .js extension:
=> '${namePath.filenameWithoutExtension}.js'

https://chromiumcodereview.appspot.com/10584014/diff/13004/tools/testing/dart...
tools/testing/dart/test_suite.dart:810: args.add('$filePath');
Not sure, but should this be toNativePath?

[Bill Hesse, 2012-06-28 15:31:22]

Strangely, I can't get a failure to happen with the broken version of this line:

tools/testing/dart/test_suite.dart:602: String compiledName =
namePath.filenameWithoutExtension;
On 2012/06/27 17:19:03, sigmund wrote:
> missing .js extension:
> => '${namePath.filenameWithoutExtension}.js'
, which is only on browser tests with // OtherScripts lines.

http://codereview.chromium.org/10584014/diff/13004/tools/testing/dart/browser...
File tools/testing/dart/browser_test.dart (right):

http://codereview.chromium.org/10584014/diff/13004/tools/testing/dart/browser...
tools/testing/dart/browser_test.dart:41:
#import('${dartHome.append('lib/unittest/html_config.dart')}', prefix:
'config');
Okay, broke the #import statement to span multiple lines - the spec says that is
OK.

On 2012/06/27 16:08:18, Mads Ager wrote:
> Long line.

http://codereview.chromium.org/10584014/diff/13004/tools/testing/dart/multite...
File tools/testing/dart/multitest.dart (right):

http://codereview.chromium.org/10584014/diff/13004/tools/testing/dart/multite...
tools/testing/dart/multitest.dart:104: "Invalid test directive '$nextOutcome' on
line ${lineCount}:\n"
On 2012/06/27 16:08:18, Mads Ager wrote:
> Is the addition of a newline character on purpose here?

Yes.  If it can't fit on a line in the source, why not make sure it doesn't wrap
confusingly when printed?  I think there is no need to make this error message a
single line.

http://codereview.chromium.org/10584014/diff/13004/tools/testing/dart/test_su...
File tools/testing/dart/test_suite.dart (right):

http://codereview.chromium.org/10584014/diff/13004/tools/testing/dart/test_su...
tools/testing/dart/test_suite.dart:375: filePath.relativeTo(suiteDir);
On 2012/06/27 13:11:24, antonmuhin wrote:
> nit: won't it fit a single line?

Done.

http://codereview.chromium.org/10584014/diff/13004/tools/testing/dart/test_su...
tools/testing/dart/test_suite.dart:602: String compiledName =
namePath.filenameWithoutExtension;
On 2012/06/27 17:19:03, sigmund wrote:
> missing .js extension:
> => '${namePath.filenameWithoutExtension}.js'

Done.

http://codereview.chromium.org/10584014/diff/13004/tools/testing/dart/test_su...
tools/testing/dart/test_suite.dart:696: testUniqueName =
testUniqueName.substring(0, testUniqueName.length - 5);
On 2012/06/27 16:08:18, Mads Ager wrote:
> On 2012/06/27 13:11:24, antonmuhin wrote:
> > isn't that to fetch the path w/o extension?  if yes, do not we have better
> > methods in Path class?
> 
> Something like:
> 
> path = path.directoryPath.append(path.filenameWithoutExtension);
> 
> should do it and be easier to read. :)

Done.

http://codereview.chromium.org/10584014/diff/13004/tools/testing/dart/test_su...
tools/testing/dart/test_suite.dart:749: String get dumpRenderTreeFilename() {
On 2012/06/27 16:08:18, Mads Ager wrote:
> Should we make these return Path objects and wait with the toNativePath call
> until we actually need to have the native path?

Considering that one of the sources of the value is a String already, and that
this change would just put the toNativePath in multiple places, instead of one,
I didn't do it.  I didn't think we needed to refactor and use Path in every
possible place, just the places it helped, or would clean up code for future
changes.

http://codereview.chromium.org/10584014/diff/13004/tools/testing/dart/test_su...
tools/testing/dart/test_suite.dart:810: args.add('$filePath');
Yes.
On 2012/06/27 17:19:03, sigmund wrote:
> Not sure, but should this be toNativePath?

http://codereview.chromium.org/10584014/diff/13004/tools/testing/dart/test_su...
tools/testing/dart/test_suite.dart:1250: File scriptF = new File(new
Options().script);
On 2012/06/27 16:08:18, Mads Ager wrote:
> Spell out File and Path?
> 
> This is so much nicer than the old version! :)

Done.

http://codereview.chromium.org/10584014/diff/13004/tools/testing/dart/test_su...
tools/testing/dart/test_suite.dart:1281: 'ff': 1}.containsKey(runtime);
OK, changed back, but using indexOf, instead of .some().

On 2012/06/27 16:56:49, Emily Fortuna wrote:
> On 2012/06/27 16:08:18, Mads Ager wrote:
> > On 2012/06/27 09:35:03, Bill Hesse wrote:
> > > It was a list, using contains, but that doesn't take advantage of hashing.

> I
> > > just think there should be an example of how to make a constant lookup
> table,
> > > since there is no such thing as a constant Set or HashSet.  This
illustrates
> a
> > > missing feature of the language, and how to solve it.
> > 
> > I agree with Emily. I think the code in test.dart should be as simple as
> > possible. Using a map here does not seem like the simplest thing and
> performance
> > is definitely not an issue here.
> > 
> > > On 2012/06/26 18:26:24, Emily Fortuna wrote:
> > > > Why make this a map here? How about:
> > > > 
> > > > static bool isBrowserRuntime(String runtime) =>
> > > >      const <String>['drt', 'dartium', 'ie', 'safari', 'opera', 'chrome',
> > > > 'ff'].indexOf(runtime) > -1;
> > > > 
> > > > or similar
> > > 
> > 
> 
> Yeah, for the sake of readability and clarity, I prefer a list, even if it is
> slightly less performant. I doubt any bottlenecks that develop would be here.