Validate platform selectors against string sets

lib/src/backend/declarer.dart

@@ -4,6 +4,7 @@
 
 import 'dart:async';
 
+import 'package:collection/collection.dart';
 import 'package:stack_trace/stack_trace.dart';
 
 import '../frontend/timeout.dart';
@@ -13,7 +14,6 @@ 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.
 ///
@@ -23,9 +23,6 @@ import 'test_platform.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;
 
@@ -39,6 +36,10 @@ class Declarer {
   /// and of the test suite.
   final Metadata _metadata;
 
+  /// The set of variables that are valid for platform selectors, in addition to
+  /// the built-in variables that are allowed everywhere.
+  final Set<String> _platformVariables;
+
   /// The stack trace for this group.
   final Trace _trace;
 
@@ -88,17 +89,30 @@ class Declarer {
   /// If [metadata] is passed, it's used as the metadata for the implicit root
   /// group.
   ///
+  /// The [platformVariables] are the set of variables that are valid for
+  /// platform selectors in test and group metadata, in addition to the built-in
+  /// variables that are allowed everywhere.
+  ///
   /// If [collectTraces] is `true`, this will set [GroupEntry.trace] for all
   /// entries built by the declarer. Note that this can be noticeably slow when
   /// thousands of tests are being declared (see #457).
   ///
   /// If [noRetry] is `true` tests will be run at most once.
-  Declarer(List<TestPlatform> allPlatforms,
-      {Metadata metadata, bool collectTraces: false, bool noRetry: false})
-      : this._(allPlatforms, null, null, metadata ?? new Metadata(),
-            collectTraces, null, noRetry);
-
-  Declarer._(this._allPlatforms, this._parent, this._name, this._metadata,
+  Declarer(
+      {Metadata metadata,
+      Set<String> platformVariables,
+      bool collectTraces: false,
+      bool noRetry: false})
+      : this._(
+            null,
+            null,
+            metadata ?? new Metadata(),
+            platformVariables ?? const UnmodifiableSetView.empty(),
+            collectTraces,
+            null,
+            noRetry);
+
+  Declarer._(this._parent, this._name, this._metadata, this._platformVariables,
       this._collectTraces, this._trace, this._noRetry);
 
   /// Runs [body] with this declarer as [Declarer.current].
@@ -123,7 +137,7 @@ class Declarer {
         onPlatform: onPlatform,
         tags: tags,
         retry: _noRetry ? 0 : retry);
-    newMetadata.validatePlatformSelectors(_allPlatforms);
+    newMetadata.validatePlatformSelectors(_platformVariables);
     var metadata = _metadata.merge(newMetadata);
 
     _entries.add(new LocalTest(_prefix(name), metadata, () async {
@@ -165,12 +179,12 @@ class Declarer {
         onPlatform: onPlatform,
         tags: tags,
         retry: _noRetry ? 0 : retry);
-    newMetadata.validatePlatformSelectors(_allPlatforms);
+    newMetadata.validatePlatformSelectors(_platformVariables);
     var metadata = _metadata.merge(newMetadata);
     var trace = _collectTraces ? new Trace.current(2) : null;
 
-    var declarer = new Declarer._(_allPlatforms, this, _prefix(name), metadata,
-        _collectTraces, trace, _noRetry);
+    var declarer = new Declarer._(this, _prefix(name), metadata,
+        _platformVariables, _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,13 +286,14 @@ 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);
+  /// Throws a [FormatException] if any [PlatformSelector]s use any variables
+  /// that don't appear either in [validVariables] or in the set of variables
+  /// that are known to be valid for all selectors.
+  void validatePlatformSelectors(Set<String> validVariables) {
+    testOn.validate(validVariables);
     onPlatform.forEach((selector, metadata) {
-      selector.validate(allPlatforms);
-      metadata.validatePlatformSelectors(allPlatforms);
+      selector.validate(validVariables);
+      metadata.validatePlatformSelectors(validVariables);
     });
   }
 

lib/src/backend/platform_selector.dart

@@ -8,9 +8,10 @@ import 'package:source_span/source_span.dart';
 import 'operating_system.dart';
 import 'test_platform.dart';
 
-/// The set of statically-known valid variable names.
-final _validVariables =
+/// The set of variable names that are valid for all platform selectors.
+final _universalValidVariables =
     new Set<String>.from(["posix", "dart-vm", "browser", "js", "blink"])
+      ..addAll(TestPlatform.builtIn.map((platform) => platform.identifier))
       ..addAll(OperatingSystem.all.map((os) => os.identifier));
 
 /// An expression for selecting certain platforms, including operating systems
@@ -54,17 +55,17 @@ class PlatformSelector {
     }
   }
 
-  /// Throws a [FormatException] if any variables are undefined in
-  /// [allPlatforms] or other sources of valid variables.
-  void validate(Iterable<TestPlatform> allPlatforms) {
+  /// Throws a [FormatException] if this selector uses any variables that don't
+  /// appear either in [validVariables] or in the set of variables that are
+  /// known to be valid for all selectors.
+  void validate(Set<String> validVariables) {
     if (identical(this, all)) return;
 
-    _wrapFormatException(() {
-      _inner.validate((name) {
-        if (_validVariables.contains(name)) return true;
-        return allPlatforms.any((platform) => name == platform.identifier);
-      });
-    }, _span);
+    _wrapFormatException(
+        () => _inner.validate((name) =>
+            _universalValidVariables.contains(name) ||
+            validVariables.contains(name)),
+        _span);
   }
 
   /// Returns whether the selector matches the given [platform] and [os].

lib/src/runner/configuration/suite.dart

@@ -293,7 +293,9 @@ class SuiteConfiguration {
 
   /// Throws a [FormatException] if [this] refers to any undefined platforms.
   void validatePlatforms(List<TestPlatform> allPlatforms) {
-    _metadata.validatePlatformSelectors(allPlatforms);
+    var validVariables =
+        allPlatforms.map((platform) => platform.identifier).toSet();
+    _metadata.validatePlatformSelectors(validVariables);
 
     if (_platforms != null) {
       for (var selection in _platforms) {
@@ -310,7 +312,7 @@ class SuiteConfiguration {
     }
 
     onPlatform.forEach((selector, config) {
-      selector.validate(allPlatforms);
+      selector.validate(validVariables);
       config.validatePlatforms(allPlatforms);
     });
   }

lib/src/runner/loader.dart

@@ -51,19 +51,10 @@ class Loader {
   List<TestPlatform> get allPlatforms =>
       new List.unmodifiable(_platformCallbacks.keys);
 
-  List<Map<String, Object>> get _allPlatformsSerialized {
-    if (__allPlatformsSerialized != null &&
-        __allPlatformsSerialized.length == _platformCallbacks.length) {
-      return __allPlatformsSerialized;
-    }
-
-    __allPlatformsSerialized = _platformCallbacks.keys
-        .map((platform) => platform.serialize())
-        .toList();
-    return __allPlatformsSerialized;
-  }
-
-  List<Map<String, Object>> __allPlatformsSerialized;
+  /// The platform variables supported by this loader, in addition the default
+  /// variables that are always supported.
+  Iterable<String> get _platformVariables =>
+      _platformCallbacks.keys.map((platform) => platform.identifier);
 
   /// Creates a new loader that loads tests on platforms defined in
   /// [Configuration.current].
@@ -146,7 +137,7 @@ class Loader {
       String path, SuiteConfiguration suiteConfig) async* {
     try {
       suiteConfig = suiteConfig.merge(new SuiteConfiguration.fromMetadata(
-          parseMetadata(path, allPlatforms)));
+          parseMetadata(path, _platformVariables.toSet())));
     } on AnalyzerErrorGroup catch (_) {
       // Ignore the analyzer's error, since its formatting is much worse than
       // the VM's or dart2js's.
@@ -195,7 +186,7 @@ class Loader {
         try {
           var plugin = await memo.runOnce(_platformCallbacks[platform]);
           var suite = await plugin.load(path, platform, platformConfig,
-              {"testPlatforms": _allPlatformsSerialized});
+              {"platformVariables": _platformVariables.toList()});
           if (suite != null) _suites.add(suite);
           return suite;
         } catch (error, stackTrace) {

lib/src/runner/parse_metadata.dart

@@ -12,36 +12,37 @@ import 'package:source_span/source_span.dart';
 
 import '../backend/metadata.dart';
 import '../backend/platform_selector.dart';
-import '../backend/test_platform.dart';
 import '../frontend/timeout.dart';
 import '../util/dart.dart';
 import '../utils.dart';
 
 /// Parse the test metadata for the test file at [path].
 ///
-/// The [allTestPlatforms] argument should list all test plaforms that are
-/// defined for the current test run.
+/// The [platformVariables] are the set of variables that are valid for platform
+/// selectors in suite metadata, in addition to the built-in variables that are
+/// allowed everywhere.
 ///
 /// Throws an [AnalysisError] if parsing fails or a [FormatException] if the
 /// test annotations are incorrect.
-Metadata parseMetadata(String path, List<TestPlatform> allTestPlatforms) =>
-    new _Parser(path, allTestPlatforms).parse();
+Metadata parseMetadata(String path, Set<String> platformVariables) =>
+    new _Parser(path, platformVariables).parse();
 
 /// A parser for test suite metadata.
 class _Parser {
   /// The path to the test suite.
   final String _path;
 
-  /// All test plaforms that are defined for the current test run.
-  final List<TestPlatform> _allTestPlatforms;
+  /// The set of variables that are valid for platform selectors, in addition to
+  /// the built-in variables that are allowed everywhere.
+  final Set<String> _platformVariables;
 
   /// All annotations at the top of the file.
   List<Annotation> _annotations;
 
   /// All prefixes defined by imports in this file.
   Set<String> _prefixes;
 
-  _Parser(String path, this._allTestPlatforms) : _path = path {
+  _Parser(String path, this._platformVariables) : _path = path {
     var contents = new File(path).readAsStringSync();
     var directives = parseDirectives(contents, name: path).directives;
     _annotations = directives.isEmpty ? [] : directives.first.metadata;
@@ -123,7 +124,7 @@ class _Parser {
     return _contextualize(
         literal,
         () => new PlatformSelector.parse(literal.stringValue)
-          ..validate(_allTestPlatforms));
+          ..validate(_platformVariables));
   }
 
   /// Parses a `@Retry` annotation.

lib/src/runner/plugin/platform_helpers.dart

@@ -51,7 +51,7 @@ Future<RunnerSuiteController> deserializeSuite(
   var suiteChannel = new MultiChannel(channel.transform(disconnector));
 
   suiteChannel.sink.add({
-    'platform': platform.identifier,
+    'platform': platform.serialize(),
     'metadata': suiteConfig.metadata.serialize(),
     'os': platform == TestPlatform.vm ? currentOS.identifier : null,
     'asciiGlyphs': Platform.isWindows,
@@ -75,7 +75,7 @@ Future<RunnerSuiteController> deserializeSuite(
       // notify the user of the error.
       loadSuiteZone.handleUncaughtError(error, stackTrace);
     } else {
-      completer.completeError(error);
+      completer.completeError(error, stackTrace);
     }
   }
 

lib/src/runner/remote_listener.dart

@@ -73,15 +73,13 @@ class RemoteListener {
       }
 
       var message = await channel.stream.first;
-      var platforms = message['testPlatforms']
-          .map((platform) => new TestPlatform.deserialize(platform))
-          .toList();
 
       if (message['asciiGlyphs'] ?? false) glyph.ascii = true;
       var metadata = new Metadata.deserialize(message['metadata']);
       verboseChain = metadata.verboseTrace;
-      var declarer = new Declarer(platforms,
+      var declarer = new Declarer(
           metadata: metadata,
+          platformVariables: message['platformVariables'].toSet(),
           collectTraces: message['collectTraces'],
           noRetry: message['noRetry']);
 
@@ -92,12 +90,13 @@ class RemoteListener {
 
       await declarer.declare(main);
 
-      var os =
-          message['os'] == null ? null : OperatingSystem.find(message['os']);
-      var platform = platforms
-          .firstWhere((platform) => platform.identifier == message['platform']);
       var suite = new Suite(declarer.build(),
-          platform: platform, os: os, path: message['path']);
+          platform: new TestPlatform.deserialize(message['platform']),
+          os: message['os'] == null
+              ? null
+              : OperatingSystem.find(message['os']),
+          path: message['path']);
+
       new RemoteListener._(suite, printZone)._listen(channel);
     }, onError: (error, stackTrace) {
       _sendError(channel, error, stackTrace, verboseChain);

lib/test.dart

@@ -53,7 +53,7 @@ Declarer get _declarer {
   // In order to run the tests, we set up our own Declarer via
   // [_globalDeclarer], and schedule a microtask to run the tests once they're
   // finished being defined.
-  _globalDeclarer = new Declarer(TestPlatform.builtIn);
+  _globalDeclarer = new Declarer();
   scheduleMicrotask(() async {
     var suite = new RunnerSuite(const PluginEnvironment(),
         SuiteConfiguration.empty, _globalDeclarer.build(),

test/backend/metadata_test.dart

@@ -197,25 +197,25 @@ void main() {
   group("validatePlatformSelectors", () {
     test("succeeds if onPlatform uses valid platforms", () {
       new Metadata.parse(onPlatform: {"vm || browser": new Skip()})
-          .validatePlatformSelectors([TestPlatform.vm]);
+          .validatePlatformSelectors(new Set.from(["vm"]));
     });
 
     test("succeeds if testOn uses valid platforms", () {
       new Metadata.parse(testOn: "vm || browser")
-          .validatePlatformSelectors([TestPlatform.vm]);
+          .validatePlatformSelectors(new Set.from(["vm"]));
     });
 
     test("fails if onPlatform uses an invalid platform", () {
       expect(() {
         new Metadata.parse(onPlatform: {"unknown": new Skip()})
-            .validatePlatformSelectors([TestPlatform.vm]);
+            .validatePlatformSelectors(new Set.from(["vm"]));
       }, throwsFormatException);
     });
 
     test("fails if testOn uses an invalid platform", () {
       expect(() {
         new Metadata.parse(testOn: "unknown")
-            .validatePlatformSelectors([TestPlatform.vm]);
+            .validatePlatformSelectors(new Set.from(["vm"]));
       }, throwsFormatException);
     });
   });