Refactor compiler and runtime configurations (command line tools command building).

dart/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,
  *   and creating [TestCase]s for those files that meet the relevant criteria.
  * - Preparing tests, including copying files and frameworks to temporary
  *   directories, and computing the command line and arguments to be run.
  */
 library test_suite;
 
 import "dart:async";
 import "dart:io";
 import "drt_updater.dart";
 import "multitest.dart";
 import "status_file_parser.dart";
 import "test_runner.dart";
 import "utils.dart";
 import "http_server.dart" show PREFIX_BUILDDIR, PREFIX_DARTDIR;
 
+import "compiler_configuration.dart" show
+    CommandArtifact,
+    CompilerConfiguration;
+
+import "runtime_configuration.dart" show
+    RuntimeConfiguration;
+
 part "browser_test.dart";
 
 
 /**
  * A simple function that tests [arg] and returns `true` or `false`.
  */
 typedef bool Predicate<T>(T arg);
 
 typedef void CreateTest(Path filePath,
                         bool hasCompileError,
@@ skipping 79 matching lines @@
       'DART_CONFIGURATION' : TestUtils.configurationDir(configuration),
     };
   }
 
   /**
    * Whether or not binaries should be found in the root build directory or
    * in the built SDK.
    */
   bool get useSdk {
     // The pub suite always uses the SDK.
     // TODO(rnystrom): Eventually, all test suites should run out of the SDK

[ricow1, 2014/03/03 07:47:08]

where and why was this introduced - as stated below this seems very broken

[Bob Nystrom, 2014/03/03 21:45:51]

This was added a long time ago when I first started adding support for testing
code that contained "package:" imports. I don't remember much about it, so feel
free to delete or change it as you or Peter see fit.

[ahe, 2014/03/04 07:46:39]

Thank you, Bob. It's awesome that you noticed this. I should have cc'ed you, so
you could respond directly.

     // and this check should go away.
+    // TODO(ahe): This check is broken for several reasons:
+    // First, it is not true that all tests should be running out of the
+    // SDK. It is absolutely critical to VM development that you can test the
+    // VM without building the SDK.
+    // Second, it is convenient for dart2js developers to run tests without
+    // rebuilding the SDK, and similarly, it should be convenient for pub
+    // developers.

[Bob Nystrom, 2014/03/03 21:45:51]

Agreed. I'm not sure what I meant by that comment.

+    // Third, even if pub can only run from the SDK directory, this is the

[Bob Nystrom, 2014/03/03 21:45:51]

The pub binary runs fine from <repo>/sdk/bin/pub, and it will correctly run the
latest pub source code directly from the repo.

However, it does need the SDK to have been built because it uses the dart VM
executable from the built SDK. Pub can't run using xcodebuild/ReleaseIA32/dart
(or whatever) because it needs access to the "version" file that gets built in
the SDK.

[ahe, 2014/03/04 07:46:39]

OK. So I wonder if we should fix this. It would seem to me that if we built a
version file along with .../ReleaseIA32/dart, pub could read that.

+    // wrong place to work around that problem. Instead, test_options.dart
+    // should have been modified so that configuration['use_sdk'] is always
+    // true when testing pub. Attempting to override the value here is brittle
+    // because we read configuration['use_sdk'] directly in many places without
+    // using this getter.
     if (suiteName == 'pub') return true;
 
     return configuration['use_sdk'];
   }
 
   /**
    * The output directory for this suite's configuration.
    */
   String get buildDir => TestUtils.buildDir(configuration);
 
   /**
    * The path to the compiler for this suite's configuration. Returns `null` if
    * no compiler should be used.
    */
   String get compilerPath {
-    if (configuration['compiler'] == 'none') {
-      return null;  // No separate compiler for dartium tests.
-    }
-    var name;
-    switch (configuration['compiler']) {
-      case 'dartanalyzer':
-      case 'dart2analyzer':
-        name = executablePath;
-        break;
-      case 'dart2js':
-      case 'dart2dart':
-        var prefix = 'sdk/bin/';
-        String suffix = getExecutableSuffix(configuration['compiler']);
-        if (configuration['host_checked']) {
-          // The script dart2js_developer is not included in the
-          // shipped SDK, that is the script is not installed in
-          // "$buildDir/dart-sdk/bin/"
-          name = '$prefix/dart2js_developer$suffix';
-        } else {
-          if (configuration['use_sdk']) {
-            prefix = '$buildDir/dart-sdk/bin/';
-          }
-          name = '${prefix}dart2js$suffix';
-        }
-        break;
-      default:
-        throw "Unknown compiler for: ${configuration['compiler']}";
-    }
-    if (!(new File(name)).existsSync() && !configuration['list']) {
-      throw "Executable '$name' does not exist";
-    }
+    var compilerConfiguration = new CompilerConfiguration(configuration);
+    if (!compilerConfiguration.hasCompiler) return null;
+    String name = compilerConfiguration.computeCompilerPath(buildDir);
+    // TODO(ahe): Only validate this once, in test_options.dart.
+    TestUtils.ensureExists(name, configuration);
     return name;
   }
 
   String get pubPath {
     var prefix = 'sdk/bin/';
     if (configuration['use_sdk']) {
       prefix = '$buildDir/dart-sdk/bin/';
     }
     String suffix = getExecutableSuffix('pub');
     var name = '${prefix}pub$suffix';
-    if (!(new File(name)).existsSync() && !configuration['list']) {
-      throw "Executable '$name' does not exist";
-    }
-    return name;
-  }
-
-  String get dartPath {
-    var prefix = 'sdk/bin/';
-    if (configuration['use_sdk']) {
-      prefix = '$buildDir/dart-sdk/bin/';
-    }
-    String suffix = getExecutableSuffix('vm');
-    var name = '${prefix}dart$suffix';
-    if (!(new File(name)).existsSync() && !configuration['list']) {
-      throw "Executable '$name' does not exist";
-    }
-    return name;
-  }
-
-  /**
-   * The path to the executable used to run this suite's tests.
-   */
-  String get executablePath {
-    var suffix = getExecutableSuffix(configuration['compiler']);
-    switch (configuration['compiler']) {
-      case 'none':
-        if (useSdk) {
-          return '$buildDir/dart-sdk/bin/dart$suffix';
-        }
-        return '$buildDir/dart$suffix';
-      case 'dartanalyzer':
-        return 'sdk/bin/dartanalyzer_developer$suffix';
-      case 'dart2analyzer':
-        return 'editor/tools/analyzer';
-      default:
-        throw "Unknown executable for: ${configuration['compiler']}";
-    }
-  }
-
-  String get dartShellFileName {
-    var name = configuration['dart'];
-    if (name == '') {
-      name = executablePath;
-    }
-
     TestUtils.ensureExists(name, configuration);
     return name;
   }
 
+  /// Returns the name of the Dart VM executable.
+  String get dartVmBinaryFileName {
+    // Controlled by user with the option "--dart".
+    String dartExecutable = configuration['dart'];
+
+    if (dartExecutable == '') {
+      String suffix = executableBinarySuffix;
+      dartExecutable = useSdk
+          ? '$buildDir/dart-sdk/bin/dart$suffix'
+          : '$buildDir/dart$suffix';
+    }
+
+    TestUtils.ensureExists(dartExecutable, configuration);
+    return dartExecutable;
+  }
+
   String get d8FileName {
     var suffix = getExecutableSuffix('d8');
     var d8Dir = TestUtils.dartDir().append('third_party/d8');
     var d8Path = d8Dir.append('${Platform.operatingSystem}/d8$suffix');
     var d8 = d8Path.toNativePath();
     TestUtils.ensureExists(d8, configuration);
     return d8;
   }
 
   String get jsShellFileName {
     var executableSuffix = getExecutableSuffix('jsshell');
     var executable = 'jsshell$executableSuffix';
     var jsshellDir = '${TestUtils.dartDir()}/tools/testing/bin';
     return '$jsshellDir/$executable';
   }
 
   /**
-   * The file name of the Dart VM executable.
-   */
-  String get vmFileName {
-    var suffix = getExecutableSuffix('vm');
-    var vm = '$buildDir/dart$suffix';
-    TestUtils.ensureExists(vm, configuration);
-    return vm;
-  }
-
-  /**
    * The file extension (if any) that should be added to the given executable
    * name for the current platform.
    */
+  // TODO(ahe): Get rid of this. Use executableBinarySuffix instead.
   String getExecutableSuffix(String executable) {
     if (Platform.operatingSystem == 'windows') {
       if (executable == 'd8' || executable == 'vm' || executable == 'none') {
         return '.exe';
       } else {
         return '.bat';
       }
     }
     return '';
   }
 
+  String get executableBinarySuffix => Platform.isWindows ? '.exe' : '';
+
   /**
    * Call the callback function onTest with a [TestCase] argument for each
    * test in the suite.  When all tests have been processed, call [onDone].
    *
    * The [testCache] argument provides a persistent store that can be used to
    * cache information about the test suite, so that directories do not need
    * to be listed each time.
    */
   void forEachTest(TestCaseEvent onTest, Map testCache, [VoidFunction onDone]);
 
@@ skipping 27 matching lines @@
     if (!pattern.hasMatch(testCase.displayName)) {
       return;
     }
 
     // Update Summary report
     if (configuration['report']) {
       SummaryReport.add(expectations);
       if (testCase.info != null &&
           testCase.info.hasCompileError &&
           TestUtils.isBrowserRuntime(configuration['runtime']) &&
-          configuration['compiler'] != 'none') {
+          new CompilerConfiguration(configuration).hasCompiler) {
         SummaryReport.addCompileErrorSkipTest();
         return;
       }
     }
 
     // Handle skipped tests
     if (expectations.contains(Expectation.SKIP) ||
         expectations.contains(Expectation.SKIP_BY_DESIGN)) {
       return;
     }
@@ skipping 592 matching lines @@
           if (optionsFromFile['packageRoot'] == null ||
               optionsFromFile['packageRoot'] == "") {
             optionsFromFile['packageRoot'] = packageRoot.toNativePath();
           }
       }
     }
     String testName = buildTestCaseDisplayName(suiteDir, info.originTestPath,
         multitestName: optionsFromFile['isMultitest'] ? info.multitestKey : "");
 
     Set<Expectation> expectations = testExpectations.expectations(testName);
-    if (configuration['compiler'] != 'none' && info.hasCompileError) {
+    if (new CompilerConfiguration(configuration).hasCompiler &&
+        info.hasCompileError) {
       // If a compile-time error is expected, and we're testing a
       // compiler, we never need to attempt to run the program (in a
       // browser or otherwise).
       enqueueStandardTest(baseCommands, info, testName, expectations);
     } else if (TestUtils.isBrowserRuntime(configuration['runtime'])) {
       if (info.optionsFromFile['isMultiHtmlTest']) {
         // A browser multi-test has multiple expectations for one test file.
         // Find all the different sub-test expecations for one entire test file.
         List<String> subtestNames = info.optionsFromFile['subtestNames'];
         Map<String, Set<Expectation>> multiHtmlTestExpectations = {};
@@ skipping 45 matching lines @@
   bool isNegative(TestInformation info) {
     bool negative = info.hasCompileError ||
         (configuration['checked'] && info.isNegativeIfChecked);
     if (info.hasRuntimeError && hasRuntime) {
       negative = true;
     }
     return negative;
   }
 
   List<Command> makeCommands(TestInformation info, var vmOptions, var args) {
-    var compiler = configuration['compiler'];
+    List<Command> commands = <Command>[];
+    CompilerConfiguration compilerConfiguration =
+        new CompilerConfiguration(configuration);
     List<String> sharedOptions = info.optionsFromFile['sharedOptions'];
-    switch (compiler) {
-    case 'dart2js':
-      args = new List.from(args);
-      String tempDir = createCompilationOutputDirectory(info.filePath);
-      args.addAll(sharedOptions);
-      args.add('--out=$tempDir/out.js');
 
-      var command = CommandBuilder.instance.getCompilationCommand(
-          compiler, "$tempDir/out.js", !useSdk,
-          dart2JsBootstrapDependencies, compilerPath, args,
-          environmentOverrides);
+    List<String> compileTimeArguments = <String>[];
+    String tempDir;

[ricow1, 2014/03/03 07:47:08]

wouldn't a better name be outputDir here?

+    if (compilerConfiguration.hasCompiler) {
+      compileTimeArguments
+          ..addAll(args)
+          ..addAll(sharedOptions);
+      // Avoid doing this for analyzer.
+      tempDir = createCompilationOutputDirectory(info.filePath);
+    }
 
-      var javascriptFile = '$tempDir/out.js';
-      if (configuration['csp']) {
-        javascriptFile = '$tempDir/out.precompiled.js';
-      }
+    CommandArtifact compilationArtifact =
+        compilerConfiguration.computeCompilationArtifact(
+            buildDir,
+            tempDir,
+            CommandBuilder.instance,
+            compileTimeArguments,
+            environmentOverrides);
+    commands.addAll(compilationArtifact.commands);
 
-      List<Command> commands = <Command>[command];
-      if (info.hasCompileError) {
-        // Do not attempt to run the compiled result. A compilation
-        // error should be reported by the compilation command.
-      } else if (configuration['runtime'] == 'd8') {
-        commands.add(CommandBuilder.instance.getJSCommandlineCommand(
-            "d8", d8FileName, [javascriptFile], environmentOverrides));
-      } else if (configuration['runtime'] == 'jsshell') {
-        commands.add(CommandBuilder.instance.getJSCommandlineCommand(
-            "jsshell", jsShellFileName, [javascriptFile],
-            environmentOverrides));
-      }
+    if (info.hasCompileError && compilerConfiguration.hasCompiler) {
+      // Do not attempt to run the compiled result. A compilation
+      // error should be reported by the compilation command.
       return commands;
-    case 'dart2dart':
-      args = new List.from(args);
-      args.addAll(sharedOptions);
-      args.add('--output-type=dart');
-      String tempDir = createCompilationOutputDirectory(info.filePath);
-      args.add('--out=$tempDir/out.dart');
+    }
 
-      List<Command> commands =
-          <Command>[CommandBuilder.instance.getCompilationCommand(
-              compiler, "$tempDir/out.dart", !useSdk,
-              dart2JsBootstrapDependencies, compilerPath, args,
-              environmentOverrides)];
-      if (info.hasCompileError) {
-        // Do not attempt to run the compiled result. A compilation
-        // error should be reported by the compilation command.
-      } else if (configuration['runtime'] == 'vm') {
-        // TODO(antonm): support checked.
-        var vmArguments = new List.from(vmOptions);
-        vmArguments.addAll([
-            '--ignore-unrecognized-flags', '$tempDir/out.dart']);
-        commands.add(CommandBuilder.instance.getVmCommand(
-            vmFileName, vmArguments, environmentOverrides));
-      } else {
-        throw 'Unsupported runtime ${configuration["runtime"]} for dart2dart';
-      }
-      return commands;
+    RuntimeConfiguration runtimeConfiguration =
+        new RuntimeConfiguration(configuration);
+    List<String> runtimeArguments =
+        compilerConfiguration.computeRuntimeArguments(
+            runtimeConfiguration,
+            info,
+            vmOptions, sharedOptions, args,
+            compilationArtifact);
 
-    case 'none':
-      var arguments = new List.from(vmOptions);
-      arguments.addAll(sharedOptions);
-      arguments.addAll(args);
-      return <Command>[CommandBuilder.instance.getVmCommand(
-          dartShellFileName, arguments, environmentOverrides)];
-
-    case 'dartanalyzer':
-    case 'dart2analyzer':
-      return <Command>[makeAnalysisCommand(info, args)];
-
-    default:
-      throw 'Unknown compiler ${configuration["compiler"]}';
-    }
-  }
-
-  AnalysisCommand makeAnalysisCommand(TestInformation info,
-                                      List<String> arguments) {
-    return CommandBuilder.instance.getAnalysisCommand(
-        configuration['compiler'], dartShellFileName, arguments,
-        environmentOverrides,
-        flavor: configuration['compiler']);
+    return commands

[ricow1, 2014/03/03 07:47:08]

I would not do a method cascade here, but call the method on commands above
instead, this seems to complicate (as in decreased readability) more than it
beautifies - but that could just be me, 

+        ..addAll(
+            runtimeConfiguration.computeRuntimeCommands(
+                this,
+                CommandBuilder.instance,
+                compilationArtifact,
+                runtimeArguments,
+                environmentOverrides));
   }
 
   CreateTest makeTestCaseCreator(Map optionsFromFile) {
     return (Path filePath,
             bool hasCompileError,
             bool hasRuntimeError,
             {bool isNegativeIfChecked: false,
              bool hasStaticWarning: false,
              Set<String> multitestOutcome: null,
              String multitestKey,
@@ skipping 300 matching lines @@
       packageRootArgument(optionsFromFile['packageRoot']);
     if (packageRoot != null) {
       args.add(packageRoot);
     }
     args.add('--out=$outputFile');
     args.add(inputFile);
     args.addAll(optionsFromFile['sharedOptions']);
     if (executable.endsWith('.dart')) {
       // Run the compiler script via the Dart VM.
       args.insert(0, executable);
-      executable = dartShellFileName;
+      executable = dartVmBinaryFileName;
     }
     return CommandBuilder.instance.getCompilationCommand(
         compiler, outputFile, !useSdk,
         dart2JsBootstrapDependencies, compilerPath, args, environmentOverrides);
   }
 
   /** Helper to create a Polymer deploy command for a single HTML file. */
   Command _polymerDeployCommand(String inputFile, String outputDir,
       optionsFromFile) {
     List<String> args = [];
     String packageRoot = packageRootArgument(optionsFromFile['packageRoot']);
     if (packageRoot != null) args.add(packageRoot);
     args..add('package:polymer/deploy.dart')
         ..add('--test')..add(inputFile)
         ..add('--out')..add(outputDir);
     if (configuration['csp']) args.add('--csp');
 
     return CommandBuilder.instance.getProcessCommand(
-        'polymer_deploy', vmFileName, args, environmentOverrides);
+        'polymer_deploy', dartVmBinaryFileName, args, environmentOverrides);
   }
 
   String createGeneratedTestDirectoryHelper(
       String name, String dirname, Path testPath, String optionsName) {
     Path relative = testPath.relativeTo(TestUtils.dartDir());
     relative = relative.directoryPath.append(relative.filenameWithoutExtension);
     String testUniqueName = relative.toString().replaceAll('/', '_');
     if (!optionsName.isEmpty) {
       testUniqueName = '$testUniqueName-$optionsName';
     }
@@ skipping 377 matching lines @@
   }
 
   bool isTestFile(String filename) {
     var sep = Platform.pathSeparator;
     // NOTE: We exclude tests and patch files for now.
     return filename.endsWith(".dart") &&
         !filename.endsWith("_test.dart") &&
         !filename.contains("_internal/lib");
   }
 
-  AnalysisCommand makeAnalysisCommand(TestInformation info,
-                                      List<String> arguments) {
-    return CommandBuilder.instance.getAnalysisCommand(
-        configuration['compiler'], dartShellFileName, arguments,
-        environmentOverrides, flavor: configuration['compiler']);
-  }
-
   bool get listRecursively => true;
 }
 
 class PkgBuildTestSuite extends TestSuite {
   final String statusFilePath;
 
   PkgBuildTestSuite(Map configuration, String suiteName, this.statusFilePath)
       : super(configuration, suiteName) {
     assert(configuration['use_sdk']);;
   }
@@ skipping 44 matching lines @@
             CommandBuilder.instance.getCopyCommand(directory, checkoutDir));
         commands.add(CommandBuilder.instance.getModifyPubspecCommand(
             pubspecYamlFile, dependencyOverrides));
         commands.add(CommandBuilder.instance.getPubCommand(
             "get", pubPath, checkoutDir, cacheDir));
 
         bool containsWebDirectory = dirExists(directoryPath.append('web'));
         bool containsBuildDartFile =
             fileExists(directoryPath.append('build.dart'));
         if (containsBuildDartFile) {
-          var dartBinary = new File(dartPath).absolute.path;
+          var dartBinary = new File(dartVmBinaryFileName).absolute.path;
 
           commands.add(CommandBuilder.instance.getProcessCommand(
               "custom_build", dartBinary, ['build.dart'], null,
               checkoutDir));
 
           // We only try to deploy the application if it's a webapp.
           if (containsWebDirectory) {
             commands.add(CommandBuilder.instance.getProcessCommand(
                  "custom_deploy", dartBinary, ['build.dart', '--deploy'], null,
                  checkoutDir));
@@ skipping 154 matching lines @@
     return ".flaky.log";
   }
 
   static String testOutcomeFileName() {
     // If test.py was invoked with '--write-test-outcome-log it will write
     // test outcomes to this file.
     return ".test-outcome.log";
   }
 
   static void ensureExists(String filename, Map configuration) {
-    if (!configuration['list'] && !(new File(filename).existsSync())) {
-      throw "Executable '$filename' does not exist";
+    if (!configuration['list'] && !existsCache.doesFileExist(filename)) {
+      throw "'$filename' does not exist";
     }
   }
 
   static Path absolutePath(Path path) {
     if (!path.isAbsolute) {
       return currentWorkingDirectory.join(path);
     }
     return path;
   }
 
@@ skipping 189 matching lines @@
  * $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
  * $compileErrorSkip tests are skipped on browsers due to compile-time error
 """;
     print(report);
    }
 }