Make the NNBD command-line options less, uh, half-baked.

- Better name than "optedIn".
- Actually hook up the command-line option to the configuration.
- Add tests, which would have caught the previous mistake.
- Don't allow comma-separated values for "--progress" and "--nnbd".
- Remove unused dead "--strong" option.

Change-Id: I57d7cb0d81af50d662dcf3f7f4c9ca1f2b102f2f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/116544
Commit-Queue: Bob Nystrom <rnystrom@google.com>
Auto-Submit: Bob Nystrom <rnystrom@google.com>
Reviewed-by: Mayank Patke <fishythefish@google.com>

pkg/smith/lib/configuration.dart

@@ -908,7 +908,7 @@ class NnbdMode extends NamedEnum {
 
   /// Opted in to NNBD features, but only static checking and weak runtime
   /// checks.
-  static const optedIn = NnbdMode._('opted-in');
+  static const weak = NnbdMode._('weak');
 
   /// Opted in to NNBD features and with full sound runtime checks.
   static const strong = NnbdMode._('strong');
@@ -916,7 +916,7 @@ class NnbdMode extends NamedEnum {
   static final List<String> names = _all.keys.toList();
 
   static final _all = {
-    for (var mode in [legacy, optedIn, strong]) mode.name: mode
+    for (var mode in [legacy, weak, strong]) mode.name: mode
   };
 
   static NnbdMode find(String name) {

pkg/smith/test/configuration_test.dart

@@ -198,7 +198,7 @@ void main() {
       test("other options from map", () {
         expect(
             Configuration.parse("dart2js", {
-              "nnbd": "opted-in",
+              "nnbd": "weak",
               "builder-tag": "the tag",
               "vm-options": ["vm stuff", "more vm stuff"],
               "dart2js-options": ["dart2js stuff", "more dart2js stuff"],
@@ -214,7 +214,7 @@ void main() {
             }),
             equals(Configuration("dart2js", Architecture.x64, Compiler.dart2js,
                 Mode.release, Runtime.d8, System.host,
-                nnbdMode: NnbdMode.optedIn,
+                nnbdMode: NnbdMode.weak,
                 builderTag: "the tag",
                 vmOptions: ["vm stuff", "more vm stuff"],
                 dart2jsOptions: ["dart2js stuff", "more dart2js stuff"],

pkg/test_runner/bin/test_runner.dart

@@ -1,6 +1,7 @@
 // Copyright (c) 2017, 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.
+import 'dart:io';
 
 /// This file is the entrypoint of the Dart repository's custom test system.
 /// It is used to test:
@@ -28,11 +29,16 @@ import "package:test_runner/src/test_configurations.dart";
 void main(List<String> arguments) {
   // Parse the command line arguments to a configuration.
   var parser = OptionsParser();
-  var configurations = parser.parse(arguments);
-  if (configurations == null || configurations.isEmpty) return;
+  try {
+    var configurations = parser.parse(arguments);
+    if (configurations == null || configurations.isEmpty) return;
 
-  // Run all of the configured tests.
-  // TODO(26372): Ensure that all tasks complete and return a future from this
-  // function.
-  testConfigurations(configurations);
+    // Run all of the configured tests.
+    // TODO(26372): Ensure that all tasks complete and return a future from this
+    // function.
+    testConfigurations(configurations);
+  } on OptionParseException catch (exception) {
+    print(exception.message);
+    exit(1);
+  }
 }

pkg/test_runner/lib/src/options.dart

@@ -37,11 +37,16 @@ class _Option {
   // TODO(rnystrom): Some string options use "" to mean "no value" and others
   // use null. Clean that up.
   _Option(this.name, this.description,
-      {String abbr, List<String> values, String defaultsTo, bool hide})
+      {String abbr,
+      List<String> values,
+      String defaultsTo,
+      bool allowMultiple,
+      bool hide})
       : abbreviation = abbr,
         values = values ?? [],
         defaultValue = defaultsTo,
         type = _OptionValueType.string,
+        allowMultiple = allowMultiple ?? true,
         verboseOnly = hide ?? false;
 
   _Option.bool(this.name, this.description, {String abbr, bool hide})
@@ -49,6 +54,7 @@ class _Option {
         values = [],
         defaultValue = false,
         type = _OptionValueType.bool,
+        allowMultiple = false,
         verboseOnly = hide ?? false;
 
   _Option.int(this.name, this.description,
@@ -57,6 +63,7 @@ class _Option {
         values = [],
         defaultValue = defaultsTo,
         type = _OptionValueType.int,
+        allowMultiple = false,
         verboseOnly = hide ?? false;
 
   final String name;
@@ -66,14 +73,16 @@ class _Option {
   final Object defaultValue;
   final _OptionValueType type;
 
+  /// Whether a comma-separated list of values is permitted.
+  final bool allowMultiple;
+
   /// Only show this option in the verbose help.
   final bool verboseOnly;
 
-  /// Gets the shortest command line argument used to refer to this option.
+  /// The shortest command line argument used to refer to this option.
   String get shortCommand => abbreviation != null ? "-$abbreviation" : command;
 
-  /// Gets the canonical long command line argument used to refer to this
-  /// option.
+  /// The canonical long command line argument used to refer to this option.
   String get command => "--${name.replaceAll('_', '-')}";
 }
 
@@ -151,7 +160,6 @@ simdbc, simdbc64''',
 test options, specifying how tests should be run.''',
         abbr: 'n',
         hide: true),
-    _Option.bool('strong', 'Deprecated, no-op.', hide: true),
     // TODO(sigmund): rename flag once we migrate all dart2js bots to the test
     // matrix.
     _Option.bool('host_checked', 'Run compiler with assertions enabled.',
@@ -189,7 +197,8 @@ Allowed values are:
 compact, color, line, verbose, silent, status, buildbot, diff''',
         abbr: 'p',
         values: Progress.names,
-        defaultsTo: Progress.compact.name),
+        defaultsTo: Progress.compact.name,
+        allowMultiple: false),
     _Option('step_name', 'Step name for use by -pbuildbot.', hide: true),
     _Option.bool('report',
         'Print a summary report of the number of tests, by expectation.',
@@ -230,9 +239,14 @@ compact, color, line, verbose, silent, status, buildbot, diff''',
     _Option('chrome', 'Path to chrome browser executable.', hide: true),
     _Option('safari', 'Path to safari browser executable.', hide: true),
     _Option.bool('use_sdk', '''Use compiler or runtime from the SDK.'''),
-    _Option.bool('nnbd', '''Opt tests into non-nullable types.'''),
-    _Option.bool('nnbd_strong_checking',
-        '''Enable strong runtime checks of non-nullable types.'''),
+    _Option(
+        'nnbd',
+        '''Which set of non-nullable type features to use.
+
+Allowed values are: legacy, weak, strong''',
+        values: NnbdMode.names,
+        defaultsTo: NnbdMode.legacy.name,
+        allowMultiple: false),
     // TODO(rnystrom): This does not appear to be used. Remove?
     _Option('build_directory',
         'The name of the build directory, where products are placed.',
@@ -380,6 +394,7 @@ compiler.''',
           verbose: arguments.contains("--verbose") || arguments.contains("-v"));
       return null;
     }
+
     if (arguments.contains("--list-configurations")) {
       var testMatrixFile = "tools/bots/test_matrix.json";
       var testMatrix = TestMatrix.fromPath(testMatrixFile);
@@ -476,11 +491,20 @@ compiler.''',
         case _OptionValueType.string:
           // Validate against the allowed values.
           if (!option.values.isEmpty) {
-            for (var v in value.split(",")) {
-              if (!option.values.contains(v)) {
-                _fail('Unknown value "$v" for command line option "$command".');
+            validate(String value) {
+              if (!option.values.contains(value)) {
+                _fail('Unknown value "$value" for option "$command".');
               }
             }
+
+            if (option.allowMultiple) {
+              value.split(",").forEach(validate);
+            } else {
+              if (value.contains(",")) {
+                _fail('Only a single value is allowed for option "$command".');
+              }
+              validate(value);
+            }
           }
 
           // TODO(rnystrom): Store as a list instead of a comma-delimited
@@ -493,9 +517,9 @@ compiler.''',
     // If a named configuration was specified ensure no other options, which are
     // implied by the named configuration, were specified.
     if (configuration['named_configuration'] is String) {
-      for (final optionName in _namedConfigurationOptions) {
+      for (var optionName in _namedConfigurationOptions) {
         if (configuration.containsKey(optionName)) {
-          final namedConfig = configuration['named_configuration'];
+          var namedConfig = configuration['named_configuration'];
           _fail("The named configuration '$namedConfig' implies "
               "'$optionName'. Try removing '$optionName'.");
         }
@@ -654,6 +678,9 @@ compiler.''',
       compilers.addAll(runtimes.map((runtime) => runtime.defaultCompiler));
     }
 
+    var progress = Progress.find(data["progress"] as String);
+    var nnbdMode = NnbdMode.find(data["nnbd"] as String);
+
     // Expand runtimes.
     for (var runtime in runtimes) {
       // Start installing the runtime if needed.
@@ -676,10 +703,11 @@ compiler.''',
             var mode = Mode.find(modeName);
             var system = System.find(data["system"] as String);
             var namedConfiguration =
-                getNamedConfiguration(data["named_configuration"] as String);
+                _namedConfiguration(data["named_configuration"] as String);
             var innerConfiguration = namedConfiguration ??
                 Configuration("custom configuration", architecture, compiler,
                     mode, runtime, system,
+                    nnbdMode: nnbdMode,
                     timeout: data["timeout"] as int,
                     enableAsserts: data["enable_asserts"] as bool,
                     useAnalyzerCfe: data["use_cfe"] as bool,
@@ -700,7 +728,7 @@ compiler.''',
                     builderTag: data["builder_tag"] as String);
             var configuration = TestConfiguration(
                 configuration: innerConfiguration,
-                progress: Progress.find(data["progress"] as String),
+                progress: progress,
                 selectors: selectors,
                 testList: data["test_list_contents"] as List<String>,
                 repeat: data["repeat"] as int,
@@ -903,14 +931,22 @@ Options:''');
   }
 }
 
-Configuration getNamedConfiguration(String template) {
+/// Exception thrown when the arguments could not be parsed.
+class OptionParseException implements Exception {
+  final String message;
+
+  OptionParseException(this.message);
+}
+
+Configuration _namedConfiguration(String template) {
   if (template == null) return null;
-  final testMatrixFile = "tools/bots/test_matrix.json";
-  TestMatrix testMatrix = TestMatrix.fromPath(testMatrixFile);
-  final configuration = testMatrix.configurations
+
+  var testMatrixFile = "tools/bots/test_matrix.json";
+  var testMatrix = TestMatrix.fromPath(testMatrixFile);
+  var configuration = testMatrix.configurations
       .singleWhere((c) => c.name == template, orElse: () => null);
   if (configuration == null) {
-    final names = testMatrix.configurations
+    var names = testMatrix.configurations
         .map((configuration) => configuration.name)
         .toList();
     names.sort();
@@ -921,8 +957,7 @@ Configuration getNamedConfiguration(String template) {
   return configuration;
 }
 
-/// Prints [message] and exits with a non-zero exit code.
+/// Throws an [OptionParseException] with [message].
 void _fail(String message) {
-  print(message);
-  exit(1);
+  throw OptionParseException(message);
 }

pkg/test_runner/lib/src/static_error.dart

@@ -2,6 +2,9 @@
 // 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.
 
+// Only needed so that [TestFile] can be referenced in doc comments.
+import 'test_file.dart';
+
 /// Describes a static error.
 ///
 /// These can be parsed from comments in [TestFile]s, in which case they

pkg/test_runner/test/options_test.dart

@@ -0,0 +1,80 @@
+// Copyright (c) 2019, 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.
+import 'package:expect/expect.dart';
+
+import 'package:test_runner/src/configuration.dart';
+import 'package:test_runner/src/options.dart';
+
+void main() {
+  testDefaults();
+  testOptions();
+  testValidation();
+}
+
+void testDefaults() {
+  // TODO(rnystrom): Test other options.
+  var configuration = parseConfiguration([]);
+  Expect.equals(Progress.compact, configuration.progress);
+  Expect.equals(NnbdMode.legacy, configuration.nnbdMode);
+}
+
+void testOptions() {
+  // TODO(rnystrom): Test other options.
+  var configurations = parseConfigurations(['--mode=debug,release']);
+  Expect.equals(2, configurations.length);
+  Expect.equals(Mode.debug, configurations[0].mode);
+  Expect.equals(Mode.release, configurations[1].mode);
+
+  var configuration = parseConfiguration(['--nnbd=weak']);
+  Expect.equals(NnbdMode.weak, configuration.nnbdMode);
+}
+
+void testValidation() {
+  // TODO(rnystrom): Test other options.
+  expectValidationError(
+      ['--timeout=notint'], 'Integer value expected for option "--timeout".');
+  expectValidationError(
+      ['--timeout=1,2'], 'Integer value expected for option "--timeout".');
+
+  expectValidationError(['--progress=unknown'],
+      'Unknown value "unknown" for option "--progress".');
+  // Don't allow multiple.
+  expectValidationError(['--progress=compact,silent'],
+      'Only a single value is allowed for option "--progress".');
+
+  expectValidationError(
+      ['--nnbd=unknown'], 'Unknown value "unknown" for option "--nnbd".');
+  // Don't allow multiple.
+  expectValidationError(['--nnbd=weak,strong'],
+      'Only a single value is allowed for option "--nnbd".');
+}
+
+TestConfiguration parseConfiguration(List<String> arguments) {
+  var configurations = parseConfigurations(arguments);
+  Expect.equals(1, configurations.length);
+  return configurations.first;
+}
+
+List<TestConfiguration> parseConfigurations(List<String> arguments) {
+  var parser = OptionsParser();
+  var configurations = parser.parse(arguments);
+
+  // By default, without an explicit selector, you get two configurations, one
+  // for observatory_ui, and one for all the other selectors. Discard the
+  // observatory one to keep things simpler.
+  configurations
+      .removeWhere((config) => config.selectors.containsKey('observatory_ui'));
+  return configurations;
+}
+
+void expectValidationError(List<String> arguments, String error) {
+  try {
+    OptionsParser().parse(arguments);
+    Expect.fail('Should have thrown an exception, but did not.');
+  } on OptionParseException catch (exception) {
+    Expect.equals(error, exception.message);
+  } catch (exception) {
+    Expect.fail('Wrong exception: $exception');
+  }
+}