From 3f7805a90ea9e4ecf9ae7bff003edfa655467d4c Mon Sep 17 00:00:00 2001 From: Robert Nystrom Date: Tue, 10 Sep 2019 22:39:19 +0000 Subject: [PATCH] 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 Auto-Submit: Bob Nystrom Reviewed-by: Mayank Patke --- pkg/smith/lib/configuration.dart | 4 +- pkg/smith/test/configuration_test.dart | 4 +- pkg/test_runner/bin/test_runner.dart | 18 +++-- pkg/test_runner/lib/src/options.dart | 83 ++++++++++++++++------- pkg/test_runner/lib/src/static_error.dart | 3 + pkg/test_runner/test/options_test.dart | 80 ++++++++++++++++++++++ 6 files changed, 158 insertions(+), 34 deletions(-) create mode 100644 pkg/test_runner/test/options_test.dart diff --git a/pkg/smith/lib/configuration.dart b/pkg/smith/lib/configuration.dart index fc7883aaf8f..07653eb3b87 100644 --- a/pkg/smith/lib/configuration.dart +++ b/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 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) { diff --git a/pkg/smith/test/configuration_test.dart b/pkg/smith/test/configuration_test.dart index fcbbebfe000..4f5736ed208 100644 --- a/pkg/smith/test/configuration_test.dart +++ b/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"], diff --git a/pkg/test_runner/bin/test_runner.dart b/pkg/test_runner/bin/test_runner.dart index 491e10b76b0..3f5e0486d31 100644 --- a/pkg/test_runner/bin/test_runner.dart +++ b/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 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); + } } diff --git a/pkg/test_runner/lib/src/options.dart b/pkg/test_runner/lib/src/options.dart index 9de4b810ee9..ed972e750ae 100644 --- a/pkg/test_runner/lib/src/options.dart +++ b/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 values, String defaultsTo, bool hide}) + {String abbr, + List 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, 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); } diff --git a/pkg/test_runner/lib/src/static_error.dart b/pkg/test_runner/lib/src/static_error.dart index 518d91b3a4f..e339a5e4b8b 100644 --- a/pkg/test_runner/lib/src/static_error.dart +++ b/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 diff --git a/pkg/test_runner/test/options_test.dart b/pkg/test_runner/test/options_test.dart new file mode 100644 index 00000000000..cf1b98ee639 --- /dev/null +++ b/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 arguments) { + var configurations = parseConfigurations(arguments); + Expect.equals(1, configurations.length); + return configurations.first; +} + +List parseConfigurations(List 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 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'); + } +}