Don't use TestPlatform.all in PlatformSelector (#693)

We want users to be able to dynamically define new platforms, which
means we need infrastructure in place for piping those platforms to
places that previously assumed TestPlatform.all was a full list of
available platforms. PlatformSelector is the trickiest example, since
it's parsed in a number of different places and needs to provide useful
feedback to users when they use an undefined platform.

This splits parsing and platform validation into two separate steps.
Validation will be done immediately after parsing when the selectors
come from top-level annotations or parameters passed to test() or
group(), but selectors defined in configuration files are now parsed
only after all configuration is parsed. This will allow new platforms to
be defined *and* referenced in configuration files.

See flutter#99
See flutter#391

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;
+}