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

tools/testing/dart/test_suite.dart

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 /**
  * Classes and methods for enumerating and preparing tests.
  *
  * This library includes:
  *
  * - Creating tests by listing all the Dart files in certain directories,
@@ skipping 93 matching lines @@
   Function doTest;
   Function doDone;
   ReceivePort receiveTestName;
   TestExpectations testExpectations;
 
   CCTestSuite(Map this.configuration,
               String this.suiteName,
               String runnerName,
               List<String> this.statusFilePaths,
               [this.testPrefix = ''])
-      : dartDir = TestUtils.dartDir() {
+      : dartDir = TestUtils.dartDir().toNativePath() {
     runnerPath = '${TestUtils.buildDir(configuration)}/$runnerName';
   }
 
   void testNameHandler(String testName, ignore) {
     if (testName == "") {
       receiveTestName.close();
       doDone(true);
     } else {
       // Only run the tests that match the pattern. Use the name
       // "suiteName/testName" for cc tests.
@@ skipping 47 matching lines @@
                                statusFileRead);
     }
   }
 
   void completeHandler(TestCase testCase) {
   }
 }
 
 
 class TestInformation {
-  String filename;
+  Path filePath;
   Map optionsFromFile;
   bool isNegative;
   bool isNegativeIfChecked;
   bool hasFatalTypeErrors;
   bool hasRuntimeErrors;
   Set<String> multitestOutcome;
 
-  TestInformation(this.filename, this.optionsFromFile, this.isNegative,
+  TestInformation(this.filePath, this.optionsFromFile, this.isNegative,
                   this.isNegativeIfChecked, this.hasFatalTypeErrors,
-                  this.hasRuntimeErrors, this.multitestOutcome);
+                  this.hasRuntimeErrors, this.multitestOutcome) {
+    Expect.isTrue(filePath.isAbsolute);
+  }
 }
 
 
 /**
  * A standard [TestSuite] implementation that searches for tests in a
  * directory, and creates [TestCase]s that compile and/or run them.
  */
 class StandardTestSuite implements TestSuite {
   Map configuration;
   String suiteName;
-  String directoryPath;
+  Path suiteDir;
   List<String> statusFilePaths;
   Function doTest;
   Function doDone;
   int activeTestGenerators = 0;
   bool listingDone = false;
   TestExpectations testExpectations;
   List<TestInformation> cachedTests;
-  final String dartDir;
+  final Path dartDir;
   Predicate<String> isTestFilePredicate;
   bool _listRecursive;
 
-  StandardTestSuite(Map this.configuration,
-                    String this.suiteName,
-                    String this.directoryPath,
-                    List<String> this.statusFilePaths,
-                    [Predicate<String> this.isTestFilePredicate,
+  StandardTestSuite(this.configuration,
+                    this.suiteName,
+                    Path suiteDirectory,
+                    this.statusFilePaths,
+                    [this.isTestFilePredicate,
                     bool recursive = false])
-    : dartDir = TestUtils.dartDir(), _listRecursive = recursive;
+  : dartDir = TestUtils.dartDir(), _listRecursive = recursive,
+    suiteDir = TestUtils.dartDir().join(suiteDirectory);
 
   /**
    * Creates a test suite whose file organization matches an expected structure.
    * To use this, your suite should look like:
    *
    *     dart/
    *       path/
    *         to/
    *           mytestsuite/
    *             mytestsuite.status
    *             example1_test.dart
    *             example2_test.dart
    *             example3_test.dart
    *
    * The important parts:
    *
    * * The leaf directory name is the name of your test suite.
    * * The status file uses the same name.
    * * Test files are directly in that directory and end in "_test.dart".
    *
    * If you follow that convention, then you can construct one of these like:
    *
    * new StandardTestSuite.forDirectory(configuration, 'path/to/mytestsuite');
    *
    * instead of having to create a custom [StandardTestSuite] subclass. In
    * particular, if you add 'path/to/mytestsuite' to [TEST_SUITE_DIRECTORIES]
    * in test.dart, this will all be set up for you.
    */
   factory StandardTestSuite.forDirectory(
-      Map configuration, String directory) {
-    final name = directory.substring(directory.lastIndexOf('/') + 1);
+      Map configuration, Path directory) {
+    final name = directory.filename;
 
     return new StandardTestSuite(configuration,
         name, directory,
         ['$directory/$name.status', '$directory/${name}_dart2js.status'],
         (filename) => filename.endsWith('_test.dart'),
         recursive: true);
   }
 
   /**
    * The default implementation assumes a file is a test if
    * it ends in "Test.dart".
    */
   bool isTestFile(String filename) {
     // Use the specified predicate, if provided.
     if (isTestFilePredicate != null) return isTestFilePredicate(filename);
 
     return filename.endsWith("Test.dart");
   }
 
   bool listRecursively() => _listRecursive;
 
   String shellPath() => TestUtils.dartShellFileName(configuration);
 
-  List<String> additionalOptions(String filename) => [];
+  List<String> additionalOptions(Path filePath) => [];
 
   void forEachTest(Function onTest, Map testCache, [Function onDone = null]) {
     // If DumpRenderTree/Dartium is required, and not yet updated,
     // wait for update.
     var updater = runtimeUpdater(configuration);
     if (updater !== null && !updater.updated) {
       Expect.isTrue(updater.isActive);
       updater.onUpdated.add(() {
         forEachTest(onTest, testCache, onDone);
       });
@@ skipping 25 matching lines @@
         }
       }
     }
 
     // Read test expectations from status files.
     testExpectations = new TestExpectations();
     for (var statusFilePath in statusFilePaths) {
       // [forDirectory] adds name_dart2js.status for all tests suites, use it if
       // it exists, but otherwise skip it and don't fail.
       if (statusFilePath.endsWith('_dart2js.status')) {
-        File file = new File('$dartDir/$statusFilePath');
+        File file = new File.fromPath(dartDir.append(statusFilePath));
         if (!file.existsSync()) {
           filesRead++;
           continue;
         }
       }
       ReadTestExpectationsInto(testExpectations,
-                               '$dartDir/$statusFilePath',
+                               dartDir.append(statusFilePath).toString(),
                                configuration,
                                statusFileRead);
     }
   }
 
   void processDirectory() {
-    directoryPath = '$dartDir/$directoryPath';
-    Directory dir = new Directory(directoryPath);
+    Directory dir = new Directory.fromPath(suiteDir);
     dir.exists().then((exists) {
       if (!exists) {
-        print('Directory containing tests not found: $directoryPath');
+        print('Directory containing tests not found: $suiteDir');
         directoryListingDone(false);
       } else {
         var lister = dir.list(recursive: listRecursively());
         lister.onFile = processFile;
         lister.onDone = directoryListingDone;
       }
     });
   }
 
   void enqueueTestCaseFromTestInformation(TestInformation info) {
-    var filename = info.filename;
+    var filePath = info.filePath;
     var optionsFromFile = info.optionsFromFile;
     var isNegative = info.isNegative;
 
-    // Look up expectations in status files using a modified file path.
+    // Look up expectations in status files using a test name generated
+    // from the test file's path.
     String testName;
-    filename = filename.replaceAll('\\', '/');
 
-    // See if there's a 'src' directory inside the 'tests' one.
-    int testsStart = filename.lastIndexOf('tests/');
-    int start = filename.lastIndexOf('src/');
-    if (start > testsStart) {
-      // Old-style test suites with tests in a 'src' subdirectory.
-      // TODO(sigmund): delete this branch once all tests stop using the src/
-      // directory
-      testName = filename.substring(start + 4, filename.length - 5);
-    } else if (optionsFromFile['isMultitest']) {
-      start = filename.lastIndexOf('/');
-      int middle = filename.lastIndexOf('_');
-      var multitestBase = filename.substring(start + 1, middle);
-      var multitestKey = filename.substring(middle + 1, filename.length - 5);
-      testName = '$multitestBase/$multitestKey';
+    if (optionsFromFile['isMultitest']) {
+      // Multitests do not run on browsers.
+      if (TestUtils.isBrowserRuntime(configuration['runtime'])) return;
+      // Multitests are in [build directory]/generated_tests/... .
+      // The test name will be '[test filename (no extension)]/[multitest key].
+      String name = filePath.filenameWithoutExtension;
+      int middle = name.lastIndexOf('_');
+      testName = '${name.substring(0, middle)}/${name.substring(middle + 1)}';
     } else {
-      // New-style test suites created by StandardTestSuite.forDirectory().
-      start = filename.indexOf(directoryPath);
-      if (start != -1) {
-        testName = filename.substring(start + directoryPath.length + 1);
-      } else {
-        testName = filename;
-      }
-      if (testName.endsWith('.dart')) {
-        testName = testName.substring(0, testName.length - 5);
+      // The test name is the relative path from the test suite directory to
+      // the test, with the .dart extension removed.
+      Expect.isTrue(filePath.toNativePath().startsWith(

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

Just FYI: Isn't we planning on removing Expect as a built in feature of the
language?

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

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.

+                    suiteDir.toNativePath()));
+      var testNamePath =
+          filePath.relativeTo(suiteDir);

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

nit: won't it fit a single line?

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

Done.

+      Expect.isTrue(testNamePath.extension == 'dart');
+      if (testNamePath.extension == 'dart') {
+        testName = testNamePath.directoryPath.append(
+            testNamePath.filenameWithoutExtension).toString();
       }
     }
     int shards = configuration['shards'];
     if (shards > 1) {
       int shard = configuration['shard'];
       if (testName.hashCode() % shards != shard - 1) {
         return;
       }
     }
 
     Set<String> expectations = testExpectations.expectations(testName);
     if (configuration['report']) {
       // Tests with multiple VMOptions are counted more than once.
       for (var dummy in getVmOptions(optionsFromFile)) {
-        if (TestUtils.isBrowserRuntime(configuration['runtime']) &&
-            optionsFromFile['isMultitest']) {
-          break;  // Browser tests skip multitests.
-        }
         SummaryReport.add(expectations);
       }
     }
     if (expectations.contains(SKIP)) return;
 
     if (TestUtils.isBrowserRuntime(configuration['runtime'])) {
       enqueueBrowserTest(info, testName, expectations);
     } else {
       enqueueStandardTest(info, testName, expectations);
     }
   }
 
   void enqueueStandardTest(TestInformation info,
                            String testName,
                            Set<String> expectations) {
     bool isNegative = info.isNegative ||
         (configuration['checked'] && info.isNegativeIfChecked);
 
     if (configuration['compiler'] == 'dartc') {
       // dartc can detect static type warnings by the
       // format of the error line
       if (info.hasFatalTypeErrors) {
         isNegative = true;
       } else if (info.hasRuntimeErrors) {
         isNegative = false;
       }
     }
 
-    var argumentLists = argumentListsFromFile(info.filename,
+    var argumentLists = argumentListsFromFile(info.filePath,
                                               info.optionsFromFile);
 
     for (var args in argumentLists) {
       doTest(new TestCase('$suiteName/$testName',
                           makeCommands(info, args),
                           configuration,
                           completeHandler,
                           expectations,
                           isNegative,
                           info));
     }
   }
 
   List<Command> makeCommands(TestInformation info, var args) {
     if (configuration['compiler'] == 'dart2js') {
       args = new List.from(args);
-      String testPath =
-          new File(info.filename).fullPathSync().replaceAll('\\', '/');
-      String tempDir = createOutputDirectory(testPath, '');
+      String tempDir = createOutputDirectory(info.filePath, '');
       args.add('--out=$tempDir/out.js');
       List<Command> commands = <Command>[new Command(shellPath(), args)];
       if (configuration['runtime'] == 'd8') {
         var d8 = TestUtils.d8FileName(configuration);
         commands.add(new Command(d8, ['$tempDir/out.js']));
       }
       return commands;
     } else {
       return <Command>[new Command(shellPath(), args)];
     }
   }
 
   Function makeTestCaseCreator(Map optionsFromFile) {
-    return (String filename,
+    return (Path filePath,
             bool isNegative,
             [bool isNegativeIfChecked = false,
              bool hasFatalTypeErrors = false,
              bool hasRuntimeErrors = false,
              Set<String> multitestOutcome = null]) {
       // Cache the test information for each test case.
-      var info = new TestInformation(filename,
+      var info = new TestInformation(filePath,
                                      optionsFromFile,
                                      isNegative,
                                      isNegativeIfChecked,
                                      hasFatalTypeErrors,
                                      hasRuntimeErrors,
                                      multitestOutcome);
       cachedTests.add(info);
       enqueueTestCaseFromTestInformation(info);
     };
   }
 
   void processFile(String filename) {
     if (!isTestFile(filename)) return;
+    Path filePath = new Path.fromNative(filename);
 
     // Only run the tests that match the pattern.
     RegExp pattern = configuration['selectors'][suiteName];
-    if (!pattern.hasMatch(filename)) return;
-    if (filename.endsWith('test_config.dart')) return;
+    if (!pattern.hasMatch('$filePath')) return;
+    if (filePath.filename.endsWith('test_config.dart')) return;
 
-    var optionsFromFile = readOptionsFromFile(filename);
+    var optionsFromFile = readOptionsFromFile(filePath);
     Function createTestCase = makeTestCaseCreator(optionsFromFile);
 
     if (optionsFromFile['isMultitest']) {
       testGeneratorStarted();
-      DoMultitest(filename,
+      DoMultitest(filePath,
                   TestUtils.buildDir(configuration),
-                  directoryPath,
+                  suiteDir,
                   createTestCase,
                   testGeneratorDone);
     } else {
-      createTestCase(filename, optionsFromFile['isNegative']);
+      createTestCase(filePath, optionsFromFile['isNegative']);
     }
   }
 
   /**
    * The [StandardTestSuite] has support for tests that
    * compile a test from Dart to Javascript, and then run the resulting
    * Javascript.  This function creates a working directory to hold the
    * Javascript version of the test, and copies the appropriate framework
    * files to that directory.  It creates a [BrowserTestCase], which has
    * two sequential steps to be run by the [ProcessQueue when] the test is
    * executed: a compilation
    * step and an execution step, both with the appropriate executable and
    * arguments.
    */
   void enqueueBrowserTest(TestInformation info,
                           String testName,
                           Set<String> expectations) {
     Map optionsFromFile = info.optionsFromFile;
-    String filename = info.filename;
-    if (optionsFromFile['isMultitest']) return;
+    Path filePath = info.filePath;
+    String filename = filePath.toString();
     bool isWebTest = optionsFromFile['containsDomImport'];
     bool isLibraryDefinition = optionsFromFile['isLibraryDefinition'];
     if (!isLibraryDefinition && optionsFromFile['containsSourceOrImport']) {
       print('Warning for $filename: Browser tests require #library '
             'in any file that uses #import, #source, or #resource');
     }
 
     final String compiler = configuration['compiler'];
     final String runtime = configuration['runtime'];
-    final String testPath =
-        new File(filename).fullPathSync().replaceAll('\\', '/');
 
     for (var vmOptions in getVmOptions(optionsFromFile)) {
       // Create a unique temporary directory for each set of vmOptions.
       // TODO(dart:429): Replace separate replaceAlls with a RegExp when
       // replaceAll(RegExp, String) is implemented.
       String optionsName = '';
       if (getVmOptions(optionsFromFile).length > 1) {
           optionsName = Strings.join(vmOptions, '-').replaceAll('-','')
                                                     .replaceAll('=','')
                                                     .replaceAll('/','');
       }
-      final String tempDir = createOutputDirectory(testPath, optionsName);
+      final String tempDir = createOutputDirectory(info.filePath, optionsName);
 
       String dartWrapperFilename = '$tempDir/test.dart';
       String compiledDartWrapperFilename = '$tempDir/test.js';
 
       String htmlPath = '$tempDir/test.html';
       if (!isWebTest) {
         // test.dart will import the dart test directly, if it is a library,
         // or indirectly through test_as_library.dart, if it is not.
-        String dartLibraryFilename;
-        if (isLibraryDefinition) {
-          dartLibraryFilename = testPath;
-        } else {
-          dartLibraryFilename = 'test_as_library.dart';
+        Path dartLibraryFilename = filePath;
+        if (!isLibraryDefinition) {
+          dartLibraryFilename = new Path('test_as_library.dart');
           File file = new File('$tempDir/$dartLibraryFilename');
           RandomAccessFile dartLibrary = file.openSync(FileMode.WRITE);
-          dartLibrary.writeStringSync(WrapDartTestInLibrary(testPath));
+          dartLibrary.writeStringSync(WrapDartTestInLibrary(filePath));
           dartLibrary.closeSync();
         }
 
         File file = new File(dartWrapperFilename);
         RandomAccessFile dartWrapper = file.openSync(FileMode.WRITE);
         dartWrapper.writeStringSync(
             DartTestWrapper(dartDir, dartLibraryFilename));
         dartWrapper.closeSync();
       } else {
-        dartWrapperFilename = testPath;
+        dartWrapperFilename = filename;
         // TODO(whesse): Once test.py is retired, adjust the relative path in
         // the client/samples/dartcombat test to its css file, remove the
         // "../../" from this path, and move this out of the isWebTest guard.
         // Also remove getHtmlName, and just use test.html.
         // TODO(efortuna): this shortening of htmlFilename is a band-aid until
         // the above TODO gets fixed. Windows cannot have paths that are longer
         // than 260 characters, and without this hack, we were running past the
         // the limit.
         String htmlFilename = getHtmlName(filename);
         while ('$tempDir/../$htmlFilename'.length >= 260) {
           htmlFilename = htmlFilename.substring(htmlFilename.length~/2);
         }
         htmlPath = '$tempDir/../$htmlFilename';
       }
       final String scriptPath = (compiler == 'none') ?
           dartWrapperFilename : compiledDartWrapperFilename;
       // Create the HTML file for the test.
       RandomAccessFile htmlTest = new File(htmlPath).openSync(FileMode.WRITE);
       String filePrefix = '';
       if (Platform.operatingSystem == 'windows') {
         // Firefox on Windows does not like absolute file path names that start
         // with 'C:' adding 'file:///' solves the problem.
         filePrefix = 'file:///';
       }
       htmlTest.writeStringSync(GetHtmlContents(
           filename,
-          '$filePrefix$dartDir/lib/unittest/test_controller.js',
+          '$filePrefix${dartDir.append("lib/unittest/test_controller.js")}',
           scriptType,
           '$filePrefix$scriptPath'));
       htmlTest.closeSync();
 
       // Construct the command(s) that compile all the inputs needed by the
       // browser test. For running Dart in DRT, this will be noop commands.
       List<Command> commands = [];
       if (compiler != 'none') {
         commands.add(_compileCommand(
             dartWrapperFilename, compiledDartWrapperFilename,
             compiler, tempDir, vmOptions));
 
         // some tests require compiling multiple input scripts.
         List<String> otherScripts = optionsFromFile['otherScripts'];
         for (String name in otherScripts) {
-          int end = filename.lastIndexOf('/');
-          if (end == -1) {
-            print('Warning: error processing "OtherScripts" of $filename.');
-            print('Skipping test ($testName).');
-            return;
-          }
-          String dir = filename.substring(0, end);
-          end = name.lastIndexOf('.dart');
-          if (end == -1) {
-            print('Warning: error processing "OtherScripts" in $filename.');
-            print('Skipping test ($testName).');
-            return;
-          }
-          String compiledName = '${name.substring(0, end)}.js';
+          Path namePath = new Path(name);
+          Expect.equals(namePath.extension, 'dart');
+          String compiledName = namePath.filenameWithoutExtension;

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

missing .js extension:
=> '${namePath.filenameWithoutExtension}.js'

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

Done.

+          Path fromPath = filePath.directoryPath.join(namePath);
           commands.add(_compileCommand(
-              '$dir/$name', '$tempDir/$compiledName',
+              fromPath.toNativePath(), '$tempDir/$compiledName',
               compiler, tempDir, vmOptions));
         }
       }
 
       // Construct the command that executes the browser test
       List<String> args;
       if (runtime == 'ie' || runtime == 'ff' || runtime == 'chrome' ||
           runtime == 'safari' || runtime == 'opera' || runtime == 'dartium') {
-        args = ['$dartDir/tools/testing/run_selenium.py',
+        args = [dartDir.append('tools/testing/run_selenium.py').toNativePath(),
             '--browser=$runtime',
             '--timeout=${configuration["timeout"] - 2}',
             '--out=$htmlPath'];
         if (runtime == 'dartium') {
           args.add('--executable=$dartiumFilename');
         }
       } else {
         args = [
-            '$dartDir/tools/testing/drt-trampoline.py',
+            dartDir.append('tools/testing/drt-trampoline.py').toNativePath(),
             dumpRenderTreeFilename,
             '--no-timeout'
         ];
         if (runtime == 'drt' && compiler == 'none') {
           var dartFlags = ['--ignore-unrecognized-flags'];
           if (configuration["checked"]) {
             dartFlags.add('--enable_asserts');
             dartFlags.add("--enable_type_checks");
           }
           dartFlags.addAll(vmOptions);
@@ skipping 13 matching lines @@
 
   /** Helper to create a compilation command for a single input file. */
   Command _compileCommand(String inputFile, String outputFile,
       String compiler, String dir, var vmOptions) {
     String executable = TestUtils.compilerPath(configuration);
     List<String> args = TestUtils.standardOptions(configuration);
     switch (compiler) {
       case 'frog':
         String libdir = configuration['froglib'];
         if (libdir == '') {
-          libdir = '$dartDir/frog/lib';
+          libdir = dartDir.append('frog/lib').toNativePath();
         }
         args.addAll(['--libdir=$libdir',
                      '--compile-only',
                      '--out=$outputFile']);
         args.addAll(vmOptions);
         args.add(inputFile);
         break;
       case 'dart2js':
         args.add('--out=$outputFile');
         args.add(inputFile);
@@ skipping 15 matching lines @@
    * an HTML page, with a testing framework based on scripting and DOM events.
    * These scripts and pages are written to a generated_test directory
    * inside the build directory of the checkout.
    *
    * Those tests which are already HTML web applications (web tests), with
    * resources including CSS files and HTML files, need to be compiled into
    * a work directory where the relative URLS to the resources work.
    * We use a subdirectory of the build directory that is the same number
    * of levels down in the checkout as the original path of the web test.
    */
-  String createOutputDirectory(String testPath, String optionsName) {
-    String testUniqueName =
-        testPath.substring(dartDir.length + 1, testPath.length - 5);
+  String createOutputDirectory(Path testPath, String optionsName) {
+    Path testUniqueNamePath = testPath.relativeTo(TestUtils.dartDir());
+    String testUniqueName = testUniqueNamePath.toString();
+    testUniqueName = testUniqueName.substring(0, testUniqueName.length - 5);

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

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]

Something like:

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

should do it and be easier to read. :)

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

Done.

     testUniqueName = testUniqueName.replaceAll('/', '_');
     if (!optionsName.isEmpty()) {
       testUniqueName = '$testUniqueName-$optionsName';
     }
 
     // Create '[build dir]/generated_tests/$compiler-$runtime/$testUniqueName',
     // including any intermediate directories that don't exist.
     var generatedTestPath = Strings.join(
         [TestUtils.buildDir(configuration),
          'generated_tests',
          "${configuration['compiler']}-${configuration['runtime']}",
          testUniqueName], '/');
 
-    TestUtils.mkdirRecursive('.', generatedTestPath);
+    TestUtils.mkdirRecursive(new Path('.'), new Path(generatedTestPath));
     return new File(generatedTestPath).fullPathSync().replaceAll('\\', '/');
   }
 
   String get scriptType() {
     switch (configuration['compiler']) {
       case 'none':
         return 'application/dart';
       case 'frog':
       case 'dart2js':
       case 'dartc':
@@ skipping 18 matching lines @@
 
   String getHtmlName(String filename) {
     var cleanFilename =  filename.replaceAll('/', '_')
                                  .replaceAll(':', '_')
                                  .replaceAll('\\', '_');
 
     return "$cleanFilename"
         "${configuration['compiler']}-${configuration['runtime']}.html";
   }
 
   String get dumpRenderTreeFilename() {

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

Should we make these return Path objects and wait with the toNativePath call
until we actually need to have the native path?

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

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.

     if (configuration['drt'] != '') {
       return configuration['drt'];
     }
     if (Platform.operatingSystem == 'macos') {
-      return '$dartDir/client/tests/drt/DumpRenderTree.app/Contents/'
-          'MacOS/DumpRenderTree';
+      return dartDir.append('/client/tests/drt/DumpRenderTree.app/Contents/'
+                            'MacOS/DumpRenderTree').toNativePath();
     }
-    return '$dartDir/client/tests/drt/DumpRenderTree';
+    return dartDir.append('client/tests/drt/DumpRenderTree').toNativePath();
   }
 
   String get dartiumFilename() {
     if (configuration['dartium'] != '') {
       return configuration['dartium'];
     }
     if (Platform.operatingSystem == 'macos') {
-      return '$dartDir/client/tests/dartium/Chromium.app/Contents/'
-          'MacOS/Chromium';
+      return dartDir.append('client/tests/dartium/Chromium.app/Contents/'
+                            'MacOS/Chromium').toNativePath();
     }
-    return '$dartDir/client/tests/dartium/chrome';
+    return dartDir.append('client/tests/dartium/chrome').toNativePath();
   }
 
   void testGeneratorStarted() {
     ++activeTestGenerators;
   }
 
   void testGeneratorDone() {
     --activeTestGenerators;
     if (activeTestGenerators == 0 && listingDone) {
       doDone();
     }
   }
 
   void directoryListingDone(ignore) {
     listingDone = true;
     if (activeTestGenerators == 0) {
       doDone();
     }
   }
 
   void completeHandler(TestCase testCase) {
   }
 
-  List<List<String>> argumentListsFromFile(String filename,
+  List<List<String>> argumentListsFromFile(Path filePath,
                                            Map optionsFromFile) {
     List args = TestUtils.standardOptions(configuration);
-    args.addAll(additionalOptions(filename));
+    args.addAll(additionalOptions(filePath));
     if (configuration['compiler'] == 'dartc') {
       args.add('--error_format');
       args.add('machine');
     }
     if ((configuration['compiler'] == 'frog')
         && (configuration['runtime'] == 'none')) {
       args.add('--compile-only');
     }
 
     bool isMultitest = optionsFromFile["isMultitest"];
     List<String> dartOptions = optionsFromFile["dartOptions"];
     List<List<String>> vmOptionsList = getVmOptions(optionsFromFile);
     Expect.isTrue(!isMultitest || dartOptions == null);
     if (dartOptions == null) {
-      args.add(filename);
+      args.add('$filePath');

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

Not sure, but should this be toNativePath?

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

Yes.
On 2012/06/27 17:19:03, sigmund wrote:

     } else {
       var executable_name = dartOptions[0];
       // TODO(ager): Get rid of this hack when the runtime checkout goes away.
       var file = new File(executable_name);
       if (!file.existsSync()) {
         executable_name = '../$executable_name';
         Expect.isTrue(new File(executable_name).existsSync());
         dartOptions[0] = executable_name;
       }
       args.addAll(dartOptions);
     }
 
     var result = new List<List<String>>();
     Expect.isFalse(vmOptionsList.isEmpty(), "empty vmOptionsList");
     for (var vmOptions in vmOptionsList) {
       var options = new List<String>.from(vmOptions);
       options.addAll(args);
       result.add(options);
     }
 
     return result;
   }
 
-  Map readOptionsFromFile(String filename) {
+  Map readOptionsFromFile(Path filePath) {
     RegExp testOptionsRegExp = const RegExp(@"// VMOptions=(.*)");
     RegExp dartOptionsRegExp = const RegExp(@"// DartOptions=(.*)");
     RegExp otherScriptsRegExp = const RegExp(@"// OtherScripts=(.*)");
     RegExp multiTestRegExp = const RegExp(@"/// [0-9][0-9]:(.*)");
     RegExp staticTypeRegExp =
         const RegExp(@"/// ([0-9][0-9]:){0,1}\s*static type warning");
     RegExp compileTimeRegExp =
         const RegExp(@"/// ([0-9][0-9]:){0,1}\s*compile-time error");
     RegExp staticCleanRegExp = const RegExp(@"// @static-clean");
     RegExp leadingHashRegExp = const RegExp(@"^#", multiLine: true);
     RegExp isolateStubsRegExp = const RegExp(@"// IsolateStubs=(.*)");
     RegExp domImportRegExp =
         const RegExp(@"^#import.*(dart:(dom|html)|html\.dart).*\)",
                      multiLine: true);
     RegExp libraryDefinitionRegExp =
         const RegExp(@"^#library\(", multiLine: true);
     RegExp sourceOrImportRegExp =
         const RegExp(@"^#(source|import|resource)\(", multiLine: true);
 
     // Read the entire file into a byte buffer and transform it to a
     // String. This will treat the file as ascii but the only parts
     // we are interested in will be ascii in any case.
-    RandomAccessFile file = new File(filename).openSync(FileMode.READ);
+    RandomAccessFile file = new File.fromPath(filePath).openSync(FileMode.READ);
     List chars = new List(file.lengthSync());
     var offset = 0;
     while (offset != chars.length) {
       offset += file.readListSync(chars, offset, chars.length - offset);
     }
     file.closeSync();
     String contents = new String.fromCharCodes(chars);
     chars = null;
 
     // Find the options in the file.
     List<List> result = new List<List>();
     List<String> dartOptions;
     bool isNegative = false;
     bool isStaticClean = false;
 
     Iterable<Match> matches = testOptionsRegExp.allMatches(contents);
     for (var match in matches) {
       result.add(match[1].split(' ').filter((e) => e != ''));
     }
     if (result.isEmpty()) result.add([]);
 
     matches = dartOptionsRegExp.allMatches(contents);
     for (var match in matches) {
       if (dartOptions != null) {
         throw new Exception(
-            'More than one "// DartOptions=" line in test $filename');
+            'More than one "// DartOptions=" line in test $filePath');
       }
       dartOptions = match[1].split(' ').filter((e) => e != '');
     }
 
     matches = staticCleanRegExp.allMatches(contents);
     for (var match in matches) {
       if (isStaticClean) {
         throw new Exception(
-            'More than one "// @static-clean=" line in test $filename');
+            'More than one "// @static-clean=" line in test $filePath');
       }
       isStaticClean = true;
     }
 
     List<String> otherScripts = new List<String>();
     matches = otherScriptsRegExp.allMatches(contents);
     for (var match in matches) {
       otherScripts.addAll(match[1].split(' ').filter((e) => e != ''));
     }
 
@@ skipping 50 matching lines @@
   List<String> _testDirs;
   int activityCount = 0;
 
   DartcCompilationTestSuite(Map configuration,
                             String suiteName,
                             String directoryPath,
                             List<String> this._testDirs,
                             List<String> expectations)
       : super(configuration,
               suiteName,
-              directoryPath,
+              new Path.fromNative(directoryPath),
               expectations);
 
   void activityStarted() { ++activityCount; }
 
   void activityCompleted() {
     if (--activityCount == 0) {
       directoryListingDone(true);
     }
   }
 
   String shellPath() => TestUtils.compilerPath(configuration);
 
-  List<String> additionalOptions(String filename) {
+  List<String> additionalOptions(Path filePath) {
     return ['--fatal-warnings', '--fatal-type-errors'];
   }
 
   void processDirectory() {
-    directoryPath = '$dartDir/$directoryPath';
     // Enqueueing the directory listers is an activity.
     activityStarted();
     for (String testDir in _testDirs) {
-      Directory dir = new Directory("$directoryPath/$testDir");
+      Directory dir = new Directory.fromPath(suiteDir.append(testDir));
       if (dir.existsSync()) {
         activityStarted();
         var lister = dir.list(recursive: listRecursively());
         lister.onFile = processFile;
         lister.onDone = (ignore) => activityCompleted();
       }
     }
     // Completed the enqueueing of listers.
     activityCompleted();
   }
@@ skipping 10 matching lines @@
   String classPath;
   List<String> testClasses;
   Function doTest;
   Function doDone;
   TestExpectations testExpectations;
 
   JUnitTestSuite(Map this.configuration,
                  String this.suiteName,
                  String this.directoryPath,
                  String this.statusFilePath)
-      : dartDir = TestUtils.dartDir();
+      : dartDir = TestUtils.dartDir().toNativePath();
 
   bool isTestFile(String filename) => filename.endsWith("Tests.java") &&
       !filename.contains('com/google/dart/compiler/vm') &&
       !filename.contains('com/google/dart/corelib/SharedTests.java');
 
   void forEachTest(Function onTest,
                    Map testCacheIgnored,
                    [Function onDone = null]) {
     doTest = onTest;
     doDone = (onDone != null) ? onDone : (() => null);
@@ skipping 83 matching lines @@
         ':');  // Path separator.
   }
 }
 
 
 class TestUtils {
   /**
    * Creates a directory using a [relativePath] to an existing
    * [base] directory if that [relativePath] does not already exist.
    */
-  static Directory mkdirRecursive(String base, String relativePath) {
-    Directory baseDir = new Directory(base);
-    Expect.isTrue(baseDir.existsSync(),
-      "Expected ${base} to already exist");
-    var tempDir = new Directory(base);
-    for (String dir in relativePath.split('/')) {
-      base = "$base/$dir";
-      tempDir = new Directory(base);
-      if (!tempDir.existsSync()) {
-          tempDir.createSync();
+  static Directory mkdirRecursive(Path base, Path relativePath) {
+    Directory dir = new Directory.fromPath(base);
+    Expect.isTrue(dir.existsSync(),
+                  "Expected ${dir} to already exist");
+    var segments = relativePath.segments();
+    for (String segment in segments) {
+      base = base.append(segment);
+      dir = new Directory.fromPath(base);
+      if (!dir.existsSync()) {
+          dir.createSync();
       }
-      Expect.isTrue(tempDir.existsSync(), "Failed to create ${tempDir.path}");
+      Expect.isTrue(dir.existsSync(), "Failed to create ${dir.path}");
     }
-    return tempDir;
+    return dir;
   }
 
   /**
    * Copy a [source] file to a new place.
    * Assumes that the directory for [dest] already exists.
    */
-  static void copyFile(File source, File dest) {
-    List contents = source.readAsBytesSync();
-    RandomAccessFile handle = dest.openSync(FileMode.WRITE);
-    handle.writeListSync(contents, 0, contents.length);
-    handle.closeSync();
+  static Future copyFile(Path source, Path dest) {
+    var output = new File.fromPath(dest).openOutputStream();
+    new File.fromPath(source).openInputStream().pipe(output);
+    var completer = new Completer();
+    output.onClosed = (){ completer.complete(null); };
+    return completer.future;
   }
 
   static String executableSuffix(String executable) {
     if (Platform.operatingSystem == 'windows') {
       if (executable == 'd8' || executable == 'vm' || executable == 'none') {
         return '.exe';
       } else {
         return '.bat';
       }
     }
@@ skipping 84 matching lines @@
     }
     return result;
   }
 
   static String buildDir(Map configuration) {
     String mode = (configuration['mode'] == 'debug') ? 'Debug' : 'Release';
     String arch = configuration['arch'].toUpperCase();
     return "${outputDir(configuration)}$mode$arch";
  }
 
-  static String dartDir() {
-    String scriptPath = new Options().script.replaceAll('\\', '/');
-    String toolsDir = scriptPath.substring(0, scriptPath.lastIndexOf('/'));
-    return new File('$toolsDir/..').fullPathSync().replaceAll('\\', '/');
+  static Path dartDir() {
+    File scriptF = new File(new Options().script);

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

Spell out File and Path?

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

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

Done.

+    Path scriptP = new Path.fromNative(scriptF.fullPathSync());
+    return scriptP.directoryPath.directoryPath;
   }
 
   static List<String> standardOptions(Map configuration) {
     List args = ["--ignore-unrecognized-flags"];
     if (configuration["checked"]) {
       args.add('--enable_asserts');
       args.add("--enable_type_checks");
     }
     if (configuration["compiler"] == "dart2js") {
       args = [];
       if (configuration["checked"]) {
         args.add('--enable-checked-mode');
       }
       args.add("--verbose");
       if (!isBrowserRuntime(configuration['runtime'])) {
         args.add("--allow-mock-compilation");
       }
     }
     return args;
   }
 
   static bool isBrowserRuntime(String runtime) =>
-      const <String>['drt',
-                     'dartium',
-                     'ie',
-                     'safari',
-                     'opera',
-                     'chrome',
-                     'ff'].some((x) => x == runtime);
+      const {'drt': 1,
+             'dartium': 1,
+             'ie': 1,
+             'safari': 1,
+             'opera': 1,
+             'chrome': 1,
+             'ff': 1}.containsKey(runtime);

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

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]

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:

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

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.

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

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.

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

OK, changed back, but using indexOf, instead of .some().

On 2012/06/27 16:56:49, Emily Fortuna wrote:

 }
 
 class SummaryReport {
   static int total = 0;
   static int skipped = 0;
   static int noCrash = 0;
   static int pass = 0;
   static int failOk = 0;
   static int fail = 0;
   static int crash = 0;
@@ skipping 33 matching lines @@
  * $noCrash tests are expected to be flaky but not crash
  * $pass tests are expected to pass
  * $failOk tests are expected to fail that we won't fix
  * $fail tests are expected to fail that we should fix
  * $crash tests are expected to crash that we should fix
  * $timeout tests are allowed to timeout
 """;
     print(report);
    }
 }