Refactor testing:TestConfiguration to use smith:Configuration by inclusion.

Change-Id: I5c116ad082a24c25a07b9ceb6aaf8c9cbe3f11e3
Reviewed-on: https://dart-review.googlesource.com/68361
Reviewed-by: Bob Nystrom <rnystrom@google.com>

Author: whesse

pkg/smith/lib/configuration.dart

@@ -201,6 +201,18 @@ class Configuration {
       return value as String;
     }
 
+    List<String> stringListOption(String option) {
+      if (!optionsCopy.containsKey(option)) return null;
+
+      var value = optionsCopy.remove(option);
+      if (value == null) throw FormatException('Option "$option" was null.');
+      if (value is! List) {
+        throw FormatException('Option "$option" had value "$value", which is '
+            'not a List.');
+      }
+      return new List<String>.from(value);
+    }
+
     // Extract options from the name and map.
     var architecture =
         enumOption("architecture", Architecture.names, Architecture.find);
@@ -236,14 +248,13 @@ class Configuration {
     var configuration = Configuration(
         name, architecture, compiler, mode, runtime, system,
         builderTag: stringOption("builder-tag"),
-        vmOptions: stringOption("vm-options"),
+        vmOptions: stringListOption("vm-options"),
         timeout: intOption("timeout"),
         enableAsserts: boolOption("enable-asserts"),
         isChecked: boolOption("checked"),
         isCsp: boolOption("csp"),
         isHostChecked: boolOption("host-checked"),
         isMinified: boolOption("minified"),
-        isStrong: boolOption("strong"),
         previewDart2: boolOption("preview-dart-2"),
         useBlobs: boolOption("use-blobs"),
         useDart2JSWithKernel: boolOption("dart2js-with-kernel"),
@@ -273,12 +284,11 @@ class Configuration {
 
   final System system;
 
-  // TODO(rnystrom): Is this still needed?
   final String builderTag;
 
-  final String vmOptions;
+  final List<String> vmOptions;
 
-  final int timeout;
+  int timeout;
 
   final bool enableAsserts;
 
@@ -292,9 +302,6 @@ class Configuration {
 
   final bool isMinified;
 
-  // TODO(rnystrom): Remove this when Dart 1.0 is no longer supported.
-  final bool isStrong;
-
   // TODO(rnystrom): Remove this when Dart 1.0 is no longer supported.
   final bool previewDart2;
 
@@ -315,14 +322,13 @@ class Configuration {
   Configuration(this.name, this.architecture, this.compiler, this.mode,
       this.runtime, this.system,
       {String builderTag,
-      String vmOptions,
+      List<String> vmOptions,
       int timeout,
       bool enableAsserts,
       bool isChecked,
       bool isCsp,
       bool isHostChecked,
       bool isMinified,
-      bool isStrong,
       bool previewDart2,
       bool useBlobs,
       bool useDart2JSWithKernel,
@@ -332,14 +338,13 @@ class Configuration {
       bool useHotReloadRollback,
       bool useSdk})
       : builderTag = builderTag ?? "",
-        vmOptions = vmOptions ?? "",
-        timeout = timeout ?? 0,
+        vmOptions = vmOptions ?? <String>[],
+        timeout = timeout,
         enableAsserts = enableAsserts ?? false,
         isChecked = isChecked ?? false,
         isCsp = isCsp ?? false,
         isHostChecked = isHostChecked ?? false,
         isMinified = isMinified ?? false,
-        isStrong = isStrong ?? false,
         previewDart2 = previewDart2 ?? true,
         useBlobs = useBlobs ?? false,
         useDart2JSWithKernel = useDart2JSWithKernel ?? false,
@@ -365,7 +370,6 @@ class Configuration {
       isCsp == other.isCsp &&
       isHostChecked == other.isHostChecked &&
       isMinified == other.isMinified &&
-      isStrong == other.isStrong &&
       previewDart2 == other.previewDart2 &&
       useBlobs == other.useBlobs &&
       useDart2JSWithKernel == other.useDart2JSWithKernel &&
@@ -393,15 +397,14 @@ class Configuration {
       (isCsp ? 4 : 0) ^
       (isHostChecked ? 8 : 0) ^
       (isMinified ? 16 : 0) ^
-      (isStrong ? 32 : 0) ^
-      (previewDart2 ? 64 : 0) ^
-      (useBlobs ? 128 : 0) ^
-      (useDart2JSWithKernel ? 256 : 0) ^
-      (useDart2JSOldFrontEnd ? 512 : 0) ^
-      (useFastStartup ? 1024 : 0) ^
-      (useHotReload ? 2048 : 0) ^
-      (useHotReloadRollback ? 4096 : 0) ^
-      (useSdk ? 8192 : 0);
+      (previewDart2 ? 32 : 0) ^
+      (useBlobs ? 64 : 0) ^
+      (useDart2JSWithKernel ? 128 : 0) ^
+      (useDart2JSOldFrontEnd ? 256 : 0) ^
+      (useFastStartup ? 512 : 0) ^
+      (useHotReload ? 1024 : 0) ^
+      (useHotReloadRollback ? 2048 : 0) ^
+      (useSdk ? 4096 : 0);
 
   String toString() {
     var buffer = new StringBuffer();
@@ -423,7 +426,6 @@ class Configuration {
     if (isCsp) fields.add("csp");
     if (isHostChecked) fields.add("host-checked");
     if (isMinified) fields.add("minified");
-    if (isStrong) fields.add("strong");
     if (previewDart2) fields.add("preview-dart-2");
     if (useBlobs) fields.add("use-blobs");
     if (useDart2JSWithKernel) fields.add("dart2js-with-kernel");

pkg/smith/test/configuration_test.dart

@@ -221,7 +221,7 @@ void main() {
               Runtime.d8,
               System.host,
               builderTag: "the tag",
-              vmOptions: "vm stuff",
+              vmOptions: ["vm stuff", "more vm stuff"],
               enableAsserts: true,
               isChecked: true,
               isCsp: true,

tools/bots/test_matrix.json

@@ -167,7 +167,7 @@
         "runtime": "vm",
         "mode": "release",
         "use-sdk": true,
-        "vm-options": "-DuserFastParser=true",
+        "vm-options": ["-DuserFastParser=true"],
         "builder-tag": "analyzer_use_fasta"
       }},
     "vm-legacy-(linux|mac|win)-(debug|release)-(ia32|x64)": {
@@ -227,14 +227,14 @@
       "options": {
         "preview-dart-2": false,
         "builder-tag": "optimization_counter_threshold",
-        "vm-options": "--optimization-counter-threshold=5"
+        "vm-options": ["--optimization-counter-threshold=5"]
       }},
     "vm-legacy-optcounter-checked-linux-release-(ia32|x64)": {
       "options": {
         "preview-dart-2": false,
         "checked": true,
         "builder-tag": "optimization_counter_threshold",
-        "vm-options": "--optimization-counter-threshold=5"
+        "vm-options": ["--optimization-counter-threshold=5"]
       }},
     "vm-legacy-reload-linux-(debug|release)-x64": {
       "options": {
@@ -346,7 +346,7 @@
     "dartkp-linux-release-x64": { },
     "dartkp-linux-debug-x64": {
       "options": {
-        "vm-options": "no-enable-malloc-hooks"
+        "vm-options": ["no-enable-malloc-hooks"]
     }},
     "dartk-(linux|mac)-(debug|release)-x64": { },
     "dartk-win-release-x64": { },
@@ -355,7 +355,7 @@
     "dartk-optcounter-linux-release-x64": {
       "options": {
         "builder-tag": "optimization_counter_threshold",
-        "vm-options": "--optimization-counter-threshold=5",
+        "vm-options": ["--optimization-counter-threshold=5"],
         "preview-dart-2": false
       }},
     "dartk-legacy-linux-release-x64": {

tools/testing/dart/configuration.dart

@@ -22,45 +22,28 @@ import 'runtime_configuration.dart';
 /// executed on, etc.
 class TestConfiguration {
   TestConfiguration(
-      {this.architecture,
-      this.compiler,
-      this.mode,
+      {this.configuration,
+      this.namedConfiguration,
       this.progress,
-      this.runtime,
-      this.system,
       this.selectors,
       this.appendLogs,
       this.batch,
       this.batchDart2JS,
       this.copyCoreDumps,
-      this.hotReload,
-      this.hotReloadRollback,
-      this.isChecked,
-      this.isHostChecked,
-      this.isCsp,
-      this.isMinified,
       this.isVerbose,
       this.listTests,
       this.listStatusFiles,
-      this.noPreviewDart2,
       this.printTiming,
       this.printReport,
       this.reportInJson,
       this.resetBrowser,
       this.skipCompilation,
       this.useAnalyzerCfe,
       this.useAnalyzerFastaParser,
-      this.useBlobs,
-      this.useSdk,
-      this.useFastStartup,
-      this.useEnableAsserts,
-      this.useDart2JSWithKernel,
-      this.useDart2JSOldFrontend,
       this.useKernelBytecode,
       this.writeDebugLog,
       this.writeTestOutcomeLog,
       this.writeResultLog,
-      this.namedConfiguration,
       this.drtPath,
       this.chromePath,
       this.safariPath,
@@ -69,7 +52,6 @@ class TestConfiguration {
       this.dartPrecompiledPath,
       this.flutterPath,
       this.taskCount,
-      int timeout,
       this.shardCount,
       this.shard,
       this.stepName,
@@ -78,26 +60,23 @@ class TestConfiguration {
       this.testDriverErrorPort,
       this.localIP,
       this.dart2jsOptions,
-      this.vmOptions,
       String packages,
       this.packageRoot,
       this.suiteDirectory,
-      this.builderTag,
       this.outputDirectory,
       this.reproducingArguments,
       this.fastTestsOnly,
       this.printPassingStdout})
-      : _packages = packages,
-        _timeout = timeout;
-
-  final Architecture architecture;
-  final Compiler compiler;
-  final Mode mode;
-  final Progress progress;
-  final Runtime runtime;
-  final System system;
+      : _packages = packages;
 
   final Map<String, RegExp> selectors;
+  final Progress progress;
+  // The test configuration computed from the test options.
+  final Configuration configuration;
+  // The test configuration coming from the -n option.  Merging
+  // these two configurations into one will be the focus of some
+  // usability work.
+  final Configuration namedConfiguration;
 
   // Boolean flags.
 
@@ -106,37 +85,42 @@ class TestConfiguration {
   final bool batchDart2JS;
   final bool copyCoreDumps;
   final bool fastTestsOnly;
-  final bool hotReload;
-  final bool hotReloadRollback;
-  final bool isChecked;
-  final bool isHostChecked;
-  final bool isCsp;
-  final bool isMinified;
   final bool isVerbose;
   final bool listTests;
   final bool listStatusFiles;
-  final bool noPreviewDart2;
   final bool printTiming;
   final bool printReport;
   final bool reportInJson;
   final bool resetBrowser;
   final bool skipCompilation;
   final bool useAnalyzerCfe;
   final bool useAnalyzerFastaParser;
-  final bool useBlobs;
-  final bool useSdk;
-  final bool useFastStartup;
-  final bool useEnableAsserts;
-  final bool useDart2JSWithKernel;
-  final bool useDart2JSOldFrontend;
   final bool useKernelBytecode;
   final bool writeDebugLog;
   final bool writeTestOutcomeLog;
   final bool writeResultLog;
   final bool printPassingStdout;
 
-  final Configuration
-      namedConfiguration; // The test configuration containing all test options.
+  Architecture get architecture => configuration.architecture;
+  Compiler get compiler => configuration.compiler;
+  Mode get mode => configuration.mode;
+  Runtime get runtime => configuration.runtime;
+  System get system => configuration.system;
+
+  // Boolean getters
+  bool get hotReload => configuration.useHotReload;
+  bool get hotReloadRollback => configuration.useHotReloadRollback;
+  bool get isChecked => configuration.isChecked;
+  bool get isHostChecked => configuration.isHostChecked;
+  bool get isCsp => configuration.isCsp;
+  bool get isMinified => configuration.isMinified;
+  bool get noPreviewDart2 => !configuration.previewDart2;
+  bool get useBlobs => configuration.useBlobs;
+  bool get useSdk => configuration.useSdk;
+  bool get useFastStartup => configuration.useFastStartup;
+  bool get useEnableAsserts => configuration.enableAsserts;
+  bool get useDart2JSWithKernel => configuration.useDart2JSWithKernel;
+  bool get useDart2JSOldFrontend => configuration.useDart2JSOldFrontEnd;
 
   // Various file paths.
 
@@ -162,7 +146,7 @@ class TestConfiguration {
   final List<String> dart2jsOptions;
 
   /// Extra VM options passed to the testing script.
-  final List<String> vmOptions;
+  List<String> get vmOptions => configuration.vmOptions;
 
   String _packages;
 
@@ -178,7 +162,7 @@ class TestConfiguration {
   final String outputDirectory;
   final String packageRoot;
   final String suiteDirectory;
-  final String builderTag;
+  String get builderTag => configuration.builderTag;
   final List<String> reproducingArguments;
 
   TestingServers _servers;
@@ -226,10 +210,11 @@ class TestConfiguration {
   ///     build/none_vm_release_x64
   String get buildDirectory => system.outputDirectory + configurationDirectory;
 
-  int _timeout;
-
+  // TODO(whesse): Put non-default timeouts explicitly in configs, not this.
+  /// Calculates a default timeout based on the compiler and runtime used,
+  /// and the mode, architecture, etc.
   int get timeout {
-    if (_timeout == null) {
+    if (configuration.timeout == null) {
       var isReload = hotReload || hotReloadRollback;
 
       var compilerMulitiplier = compilerConfiguration.timeoutMultiplier;
@@ -239,10 +224,10 @@ class TestConfiguration {
           isReload: isReload,
           arch: architecture);
 
-      _timeout = 60 * compilerMulitiplier * runtimeMultiplier;
+      configuration.timeout = 60 * compilerMulitiplier * runtimeMultiplier;
     }
 
-    return _timeout;
+    return configuration.timeout;
   }
 
   List<String> get standardOptions {

tools/testing/dart/launch_browser.dart

@@ -34,7 +34,9 @@ void main(List<String> arguments) {
   }
 
   var runtime = Runtime.find(name);
-  var configuration = new TestConfiguration(runtime: runtime);
+  var configuration = new TestConfiguration(
+      configuration: new Configuration(
+          "dummy configuration", null, null, null, runtime, null));
   var executable = configuration.browserLocation;
   var browser = new Browser.byRuntime(runtime, executable);
   browser.start(arguments[1]);