Don't use TestPlatform.all in PlatformSelector

lib/src/backend/declarer.dart

@@ -13,6 +13,7 @@ import 'group_entry.dart';
 import 'invoker.dart';
 import 'metadata.dart';
 import 'test.dart';
+import 'test_platform.dart';
 
 /// A class that manages the state of tests as they're declared.
 ///
@@ -22,6 +23,9 @@ import 'test.dart';
 /// for a block using [Declarer.declare], and it can be accessed using
 /// [Declarer.current].
 class Declarer {
+  /// All test plaforms that are defined for the current test run.
+  final List<TestPlatform> _allPlatforms;
+
   /// The parent declarer, or `null` if this corresponds to the root group.
   final Declarer _parent;
 
@@ -89,12 +93,13 @@ class Declarer {
   /// thousands of tests are being declared (see #457).
   ///
   /// If [noRetry] is `true` tests will be run at most once.
-  Declarer({Metadata metadata, bool collectTraces: false, bool noRetry: false})
-      : this._(null, null, metadata ?? new Metadata(), collectTraces, null,
-            noRetry);
+  Declarer(List<TestPlatform> allPlatforms,
+      {Metadata metadata, bool collectTraces: false, bool noRetry: false})
+      : this._(allPlatforms, null, null, metadata ?? new Metadata(),
+            collectTraces, null, noRetry);
 
-  Declarer._(this._parent, this._name, this._metadata, this._collectTraces,
-      this._trace, this._noRetry);
+  Declarer._(this._allPlatforms, this._parent, this._name, this._metadata,
+      this._collectTraces, this._trace, this._noRetry);
 
   /// Runs [body] with this declarer as [Declarer.current].
   ///
@@ -111,13 +116,15 @@ class Declarer {
       int retry}) {
     _checkNotBuilt("test");
 
-    var metadata = _metadata.merge(new Metadata.parse(
+    var newMetadata = new Metadata.parse(
         testOn: testOn,
         timeout: timeout,
         skip: skip,
         onPlatform: onPlatform,
         tags: tags,
-        retry: _noRetry ? 0 : retry));
+        retry: _noRetry ? 0 : retry);
+    newMetadata.validatePlatformSelectors(_allPlatforms);
+    var metadata = _metadata.merge(newMetadata);
 
     _entries.add(new LocalTest(_prefix(name), metadata, () async {
       var parents = <Declarer>[];
@@ -151,17 +158,19 @@ class Declarer {
       int retry}) {
     _checkNotBuilt("group");
 
-    var metadata = _metadata.merge(new Metadata.parse(
+    var newMetadata = new Metadata.parse(
         testOn: testOn,
         timeout: timeout,
         skip: skip,
         onPlatform: onPlatform,
         tags: tags,
-        retry: retry));
+        retry: _noRetry ? 0 : retry);
+    newMetadata.validatePlatformSelectors(_allPlatforms);
+    var metadata = _metadata.merge(newMetadata);
     var trace = _collectTraces ? new Trace.current(2) : null;
 
-    var declarer = new Declarer._(
-        this, _prefix(name), metadata, _collectTraces, trace, _noRetry);
+    var declarer = new Declarer._(_allPlatforms, this, _prefix(name), metadata,
+        _collectTraces, trace, _noRetry);
     declarer.declare(() {
       // Cast to dynamic to avoid the analyzer complaining about us using the
       // result of a void method.

lib/src/backend/metadata.dart

@@ -286,6 +286,16 @@ class Metadata {
         "Dart identifiers.");
   }
 
+  /// Throws a [FormatException] if any [PlatformSelector]s use variables that
+  /// are undefined in [allPlatforms] or other sources of valid variables.
+  void validatePlatformSelectors(List<TestPlatform> allPlatforms) {
+    testOn.validate(allPlatforms);
+    onPlatform.forEach((selector, metadata) {
+      selector.validate(allPlatforms);
+      metadata.validatePlatformSelectors(allPlatforms);
+    });
+  }
+
   /// Return a new [Metadata] that merges [this] with [other].
   ///
   /// If the two [Metadata]s have conflicting properties, [other] wins. If

lib/src/backend/platform_selector.dart

@@ -8,10 +8,9 @@ import 'package:source_span/source_span.dart';
 import 'operating_system.dart';
 import 'test_platform.dart';
 
-/// The set of all valid variable names.
+/// The set of statically-known valid variable names.
 final _validVariables =
     new Set<String>.from(["posix", "dart-vm", "browser", "js", "blink"])
-      ..addAll(TestPlatform.all.map((platform) => platform.identifier))
       ..addAll(OperatingSystem.all.map((os) => os.identifier));
 
 /// An expression for selecting certain platforms, including operating systems
@@ -27,16 +26,46 @@ class PlatformSelector {
   /// The boolean selector used to implement this selector.
   final BooleanSelector _inner;
 
+  /// The source span from which this selector was parsed.
+  final SourceSpan _span;
+
   /// Parses [selector].
   ///
-  /// This will throw a [SourceSpanFormatException] if the selector is
-  /// malformed or if it uses an undefined variable.
-  PlatformSelector.parse(String selector)
-      : _inner = new BooleanSelector.parse(selector) {
-    _inner.validate(_validVariables.contains);
+  /// If [span] is passed, it indicates the location of the text for [selector]
+  /// in a larger document. It's used for error reporting.
+  PlatformSelector.parse(String selector, [SourceSpan span])
+      : _inner = _wrapFormatException(
+            () => new BooleanSelector.parse(selector), span),
+        _span = span;
+
+  const PlatformSelector._(this._inner) : _span = null;
+
+  /// Runs [body] and wraps any [FormatException] it throws in a
+  /// [SourceSpanFormatException] using [span].
+  ///
+  /// If [span] is `null`, runs [body] as-is.
+  static T _wrapFormatException<T>(T body(), SourceSpan span) {
+    if (span == null) return body();
+
+    try {
+      return body();
+    } on FormatException catch (error) {
+      throw new SourceSpanFormatException(error.message, span);
+    }
   }
 
-  const PlatformSelector._(this._inner);
+  /// Throws a [FormatException] if any variables are undefined in
+  /// [allPlatforms] or other sources of valid variables.
+  void validate(Iterable<TestPlatform> allPlatforms) {
+    if (identical(this, all)) return;
+
+    _wrapFormatException(() {
+      _inner.validate((name) {
+        if (_validVariables.contains(name)) return true;
+        return allPlatforms.any((platform) => name == platform.identifier);
+      });
+    }, _span);
+  }
 
   /// Returns whether the selector matches the given [platform] and [os].
   ///

lib/src/executable.dart

@@ -172,6 +172,12 @@ transformers:
   } on ApplicationException catch (error) {
     stderr.writeln(error.message);
     exitCode = exit_codes.data;
+  } on SourceSpanFormatException catch (error) {
+    stderr.writeln(error.toString(color: configuration.color));
+    exitCode = exit_codes.data;
+  } on FormatException catch (error) {
+    stderr.writeln(error.message);
+    exitCode = exit_codes.data;
   } catch (error, stackTrace) {
     stderr.writeln(getErrorMessage(error));
     stderr.writeln(new Trace.from(stackTrace).terse);

lib/src/runner.dart

@@ -80,6 +80,8 @@ class Runner {
   /// This starts running tests and printing their progress. It returns whether
   /// or not they ran successfully.
   Future<bool> run() => _config.asCurrent(() async {
+        _config.validatePlatforms(_loader.allPlatforms);
+
         if (_closed) {
           throw new StateError("run() may not be called on a closed Runner.");
         }

lib/src/runner/configuration.dart

@@ -10,10 +10,12 @@ import 'package:glob/glob.dart';
 import 'package:path/path.dart' as p;
 
 import '../backend/platform_selector.dart';
+import '../backend/test_platform.dart';
 import '../frontend/timeout.dart';
 import '../util/io.dart';
 import 'configuration/args.dart' as args;
 import 'configuration/load.dart';
+import 'configuration/platform_selection.dart';
 import 'configuration/reporters.dart';
 import 'configuration/suite.dart';
 import 'configuration/values.dart';
@@ -219,7 +221,7 @@ class Configuration {
       Iterable<String> dart2jsArgs,
       String precompiledPath,
       Iterable<Pattern> patterns,
-      Iterable<String> platforms,
+      Iterable<PlatformSelection> platforms,
       BooleanSelector includeTags,
       BooleanSelector excludeTags,
       Map<BooleanSelector, SuiteConfiguration> tags,
@@ -383,6 +385,14 @@ class Configuration {
   /// asynchronous callbacks transitively created by [body].
   T asCurrent<T>(T body()) => runZoned(body, zoneValues: {_currentKey: this});
 
+  /// Throws a [FormatException] if [this] refers to any undefined platforms.
+  void validatePlatforms(List<TestPlatform> allPlatforms) {
+    suiteDefaults.validatePlatforms(allPlatforms);
+    for (var config in presets.values) {
+      config.validatePlatforms(allPlatforms);
+    }
+  }
+
   /// Merges this with [other].
   ///
   /// For most fields, if both configurations have values set, [other]'s value
@@ -466,7 +476,7 @@ class Configuration {
       Iterable<String> dart2jsArgs,
       String precompiledPath,
       Iterable<Pattern> patterns,
-      Iterable<String> platforms,
+      Iterable<PlatformSelection> platforms,
       BooleanSelector includeTags,
       BooleanSelector excludeTags,
       Map<BooleanSelector, SuiteConfiguration> tags,

lib/src/runner/configuration/args.dart

@@ -10,14 +10,15 @@ import 'package:boolean_selector/boolean_selector.dart';
 import '../../backend/test_platform.dart';
 import '../../frontend/timeout.dart';
 import '../configuration.dart';
+import 'platform_selection.dart';
 import 'reporters.dart';
 import 'values.dart';
 
 /// The parser used to parse the command-line arguments.
 final ArgParser _parser = (() {
   var parser = new ArgParser(allowTrailingOptions: true);
 
-  var allPlatforms = TestPlatform.all.toList();
+  var allPlatforms = TestPlatform.all.toList()..remove(TestPlatform.vm);
   if (!Platform.isMacOS) allPlatforms.remove(TestPlatform.safari);
   if (!Platform.isWindows) allPlatforms.remove(TestPlatform.internetExplorer);
 
@@ -62,9 +63,9 @@ final ArgParser _parser = (() {
   parser.addSeparator("======== Running Tests");
   parser.addOption("platform",
       abbr: 'p',
-      help: 'The platform(s) on which to run the tests.',
-      defaultsTo: 'vm',
-      allowed: allPlatforms.map((platform) => platform.identifier).toList(),
+      help: 'The platform(s) on which to run the tests.\n'
+          '[vm (default), '
+          '${allPlatforms.map((platform) => platform.identifier).join(", ")}]',
       allowMultiple: true);
   parser.addOption("preset",
       abbr: 'P',
@@ -220,7 +221,9 @@ class _Parser {
         totalShards: totalShards,
         timeout: _parseOption('timeout', (value) => new Timeout.parse(value)),
         patterns: patterns,
-        platforms: _ifParsed('platform') as List<String>,
+        platforms: (_ifParsed('platform') as List<String>)
+            ?.map((platform) => new PlatformSelection(platform))
+            ?.toList(),
         runSkipped: _ifParsed('run-skipped'),
         chosenPresets: _ifParsed('preset') as List<String>,
         paths: _options.rest.isEmpty ? null : _options.rest,

lib/src/runner/configuration/load.dart

@@ -13,12 +13,12 @@ import 'package:yaml/yaml.dart';
 
 import '../../backend/operating_system.dart';
 import '../../backend/platform_selector.dart';
-import '../../backend/test_platform.dart';
 import '../../frontend/timeout.dart';
 import '../../util/io.dart';
 import '../../utils.dart';
 import '../configuration.dart';
 import '../configuration/suite.dart';
+import 'platform_selection.dart';
 import 'reporters.dart';
 
 /// A regular expression matching a Dart identifier.
@@ -94,7 +94,7 @@ class _ConfigurationLoader {
 
     var onPlatform = _getMap("on_platform",
         key: (keyNode) => _parseNode(keyNode, "on_platform key",
-            (value) => new PlatformSelector.parse(value)),
+            (value) => new PlatformSelector.parse(value, keyNode.span)),
         value: (valueNode) =>
             _nestedConfig(valueNode, "on_platform value", runnerConfig: false));
 
@@ -155,8 +155,7 @@ class _ConfigurationLoader {
       skip = true;
     }
 
-    var testOn =
-        _parseValue("test_on", (value) => new PlatformSelector.parse(value));
+    var testOn = _parsePlatformSelector("test_on");
 
     var addTags = _getList(
         "add_tags", (tagNode) => _parseIdentifierLike(tagNode, "Tag name"));
@@ -206,14 +205,11 @@ class _ConfigurationLoader {
 
     var concurrency = _getInt("concurrency");
 
-    var platforms = _getList("platforms", (platformNode) {
-      var name = _parseIdentifierLike(platformNode, "Platform name");
-      if (!TestPlatform.all.any((platform) => platform.identifier == name)) {
-        throw new SourceSpanFormatException(
-            'Unknown platform "$name"', platformNode.span, _source);
-      }
-      return name;
-    });
+    var platforms = _getList(
+        "platforms",
+        (platformNode) => new PlatformSelection(
+            _parseIdentifierLike(platformNode, "Platform name"),
+            platformNode.span));
 
     var chosenPresets = _getList("add_presets",
         (presetNode) => _parseIdentifierLike(presetNode, "Preset name"));
@@ -409,6 +405,14 @@ class _ConfigurationLoader {
   BooleanSelector _parseBooleanSelector(String name) =>
       _parseValue(name, (value) => new BooleanSelector.parse(value));
 
+  /// Parses [node]'s value as a platform selector.
+  PlatformSelector _parsePlatformSelector(String field) {
+    var node = _document.nodes[field];
+    if (node == null) return null;
+    return _parseNode(
+        node, field, (value) => new PlatformSelector.parse(value, node.span));
+  }
+
   /// Asserts that [node] is a string, passes its value to [parse], and returns
   /// the result.
   ///

lib/src/runner/configuration/platform_selection.dart

@@ -0,0 +1,22 @@
+// 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 'package:source_span/source_span.dart';
+
+/// A platform on which the user has chosen to run tests.
+class PlatformSelection {
+  /// The name of the platform.
+  final String name;
+
+  /// The location in the configuration file of this platform string, or `null`
+  /// if it was defined outside a configuration file (for example, on the
+  /// command line).
+  final SourceSpan span;
+
+  PlatformSelection(this.name, [this.span]);
+
+  bool operator ==(other) => other is PlatformSelection && other.name == name;
+
+  int get hashCode => name.hashCode;
+}