From 71b3efd34b618d6e20fe7082ddc0a4b785f78a1d Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Mon, 9 Oct 2017 17:18:34 -0700 Subject: [PATCH 1/2] Validate platform selectors against string sets This makes it easier to add platforms that extend existing platforms without running into serialization headaches. It also cleans up the Declarer API and means we're sending less redundant data between isolates. --- lib/src/backend/declarer.dart | 42 +++-- lib/src/backend/metadata.dart | 13 +- lib/src/backend/platform_selector.dart | 18 +-- lib/src/runner/configuration/suite.dart | 6 +- lib/src/runner/loader.dart | 21 +-- lib/src/runner/parse_metadata.dart | 19 +-- lib/src/runner/plugin/platform_helpers.dart | 4 +- lib/src/runner/remote_listener.dart | 17 +- lib/test.dart | 2 +- test/backend/metadata_test.dart | 8 +- test/runner/parse_metadata_test.dart | 169 +++++++------------- test/utils.dart | 5 +- 12 files changed, 143 insertions(+), 181 deletions(-) diff --git a/lib/src/backend/declarer.dart b/lib/src/backend/declarer.dart index 1d40c5524..4e8851d4f 100644 --- a/lib/src/backend/declarer.dart +++ b/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 _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 _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 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 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. diff --git a/lib/src/backend/metadata.dart b/lib/src/backend/metadata.dart index d8441cfca..7ff1a8624 100644 --- a/lib/src/backend/metadata.dart +++ b/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 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 validVariables) { + testOn.validate(validVariables); onPlatform.forEach((selector, metadata) { - selector.validate(allPlatforms); - metadata.validatePlatformSelectors(allPlatforms); + selector.validate(validVariables); + metadata.validatePlatformSelectors(validVariables); }); } diff --git a/lib/src/backend/platform_selector.dart b/lib/src/backend/platform_selector.dart index ff964ad5a..0e3c55465 100644 --- a/lib/src/backend/platform_selector.dart +++ b/lib/src/backend/platform_selector.dart @@ -11,6 +11,7 @@ import 'test_platform.dart'; /// The set of statically-known valid variable names. final _validVariables = new Set.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,16 @@ class PlatformSelector { } } - /// Throws a [FormatException] if any variables are undefined in - /// [allPlatforms] or other sources of valid variables. - void validate(Iterable 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 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) => + _validVariables.contains(name) || validVariables.contains(name)), + _span); } /// Returns whether the selector matches the given [platform] and [os]. diff --git a/lib/src/runner/configuration/suite.dart b/lib/src/runner/configuration/suite.dart index ac360bd10..6ed34586c 100644 --- a/lib/src/runner/configuration/suite.dart +++ b/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 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); }); } diff --git a/lib/src/runner/loader.dart b/lib/src/runner/loader.dart index f74308421..84b3149d0 100644 --- a/lib/src/runner/loader.dart +++ b/lib/src/runner/loader.dart @@ -51,19 +51,10 @@ class Loader { List get allPlatforms => new List.unmodifiable(_platformCallbacks.keys); - List> get _allPlatformsSerialized { - if (__allPlatformsSerialized != null && - __allPlatformsSerialized.length == _platformCallbacks.length) { - return __allPlatformsSerialized; - } - - __allPlatformsSerialized = _platformCallbacks.keys - .map((platform) => platform.serialize()) - .toList(); - return __allPlatformsSerialized; - } - - List> __allPlatformsSerialized; + /// The platform variables supported by this loader, in addition the default + /// variables that are always supported. + Iterable 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) { diff --git a/lib/src/runner/parse_metadata.dart b/lib/src/runner/parse_metadata.dart index 299e861a7..e8aa1ae22 100644 --- a/lib/src/runner/parse_metadata.dart +++ b/lib/src/runner/parse_metadata.dart @@ -12,28 +12,29 @@ 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 allTestPlatforms) => - new _Parser(path, allTestPlatforms).parse(); +Metadata parseMetadata(String path, Set 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 _allTestPlatforms; + /// The set of variables that are valid for platform selectors, in addition to + /// the built-in variables that are allowed everywhere. + final Set _platformVariables; /// All annotations at the top of the file. List _annotations; @@ -41,7 +42,7 @@ class _Parser { /// All prefixes defined by imports in this file. Set _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. diff --git a/lib/src/runner/plugin/platform_helpers.dart b/lib/src/runner/plugin/platform_helpers.dart index c1d2d74de..b41e5ba84 100644 --- a/lib/src/runner/plugin/platform_helpers.dart +++ b/lib/src/runner/plugin/platform_helpers.dart @@ -51,7 +51,7 @@ Future 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 deserializeSuite( // notify the user of the error. loadSuiteZone.handleUncaughtError(error, stackTrace); } else { - completer.completeError(error); + completer.completeError(error, stackTrace); } } diff --git a/lib/src/runner/remote_listener.dart b/lib/src/runner/remote_listener.dart index 5271f6ed5..883101fa4 100644 --- a/lib/src/runner/remote_listener.dart +++ b/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); diff --git a/lib/test.dart b/lib/test.dart index 1ce02c26b..2550bef07 100644 --- a/lib/test.dart +++ b/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(), diff --git a/test/backend/metadata_test.dart b/test/backend/metadata_test.dart index 60ace938e..35fc9ef18 100644 --- a/test/backend/metadata_test.dart +++ b/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); }); }); diff --git a/test/runner/parse_metadata_test.dart b/test/runner/parse_metadata_test.dart index e04b3c14e..343307ec9 100644 --- a/test/runner/parse_metadata_test.dart +++ b/test/runner/parse_metadata_test.dart @@ -27,21 +27,21 @@ void main() { test("returns empty metadata for an empty file", () { new File(_path).writeAsStringSync(""); - var metadata = parseMetadata(_path, TestPlatform.builtIn); + var metadata = parseMetadata(_path, new Set()); expect(metadata.testOn, equals(PlatformSelector.all)); expect(metadata.timeout.scaleFactor, equals(1)); }); test("ignores irrelevant annotations", () { new File(_path).writeAsStringSync("@Fblthp\n@Fblthp.foo\nlibrary foo;"); - var metadata = parseMetadata(_path, TestPlatform.builtIn); + var metadata = parseMetadata(_path, new Set()); expect(metadata.testOn, equals(PlatformSelector.all)); }); test("parses a prefixed annotation", () { new File(_path).writeAsStringSync("@foo.TestOn('vm')\n" "import 'package:test/test.dart' as foo;"); - var metadata = parseMetadata(_path, TestPlatform.builtIn); + var metadata = parseMetadata(_path, new Set()); expect(metadata.testOn.evaluate(TestPlatform.vm), isTrue); expect(metadata.testOn.evaluate(TestPlatform.chrome), isFalse); }); @@ -49,61 +49,54 @@ void main() { group("@TestOn:", () { test("parses a valid annotation", () { new File(_path).writeAsStringSync("@TestOn('vm')\nlibrary foo;"); - var metadata = parseMetadata(_path, TestPlatform.builtIn); + var metadata = parseMetadata(_path, new Set()); expect(metadata.testOn.evaluate(TestPlatform.vm), isTrue); expect(metadata.testOn.evaluate(TestPlatform.chrome), isFalse); }); test("ignores a constructor named TestOn", () { new File(_path).writeAsStringSync("@foo.TestOn('foo')\nlibrary foo;"); - var metadata = parseMetadata(_path, TestPlatform.builtIn); + var metadata = parseMetadata(_path, new Set()); expect(metadata.testOn, equals(PlatformSelector.all)); }); group("throws an error for", () { test("a named constructor", () { new File(_path).writeAsStringSync("@TestOn.name('foo')\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("no argument list", () { new File(_path).writeAsStringSync("@TestOn\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("an empty argument list", () { new File(_path).writeAsStringSync("@TestOn()\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("a named argument", () { new File(_path) .writeAsStringSync("@TestOn(expression: 'foo')\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("multiple arguments", () { new File(_path) .writeAsStringSync("@TestOn('foo', 'bar')\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("a non-string argument", () { new File(_path).writeAsStringSync("@TestOn(123)\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("multiple @TestOns", () { new File(_path) .writeAsStringSync("@TestOn('foo')\n@TestOn('bar')\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); }); }); @@ -120,7 +113,7 @@ void main() { library foo; """); - var metadata = parseMetadata(_path, TestPlatform.builtIn); + var metadata = parseMetadata(_path, new Set()); expect( metadata.timeout.duration, equals(new Duration( @@ -137,7 +130,7 @@ library foo; library foo; """); - var metadata = parseMetadata(_path, TestPlatform.builtIn); + var metadata = parseMetadata(_path, new Set()); expect(metadata.timeout.scaleFactor, equals(1)); }); @@ -147,7 +140,7 @@ library foo; library foo; """); - var metadata = parseMetadata(_path, TestPlatform.builtIn); + var metadata = parseMetadata(_path, new Set()); expect(metadata.timeout.scaleFactor, equals(0.5)); }); @@ -157,109 +150,95 @@ library foo; library foo; """); - var metadata = parseMetadata(_path, TestPlatform.builtIn); + var metadata = parseMetadata(_path, new Set()); expect(metadata.timeout, same(Timeout.none)); }); test("ignores a constructor named Timeout", () { new File(_path).writeAsStringSync("@foo.Timeout('foo')\nlibrary foo;"); - var metadata = parseMetadata(_path, TestPlatform.builtIn); + var metadata = parseMetadata(_path, new Set()); expect(metadata.timeout.scaleFactor, equals(1)); }); group("throws an error for", () { test("an unknown named constructor", () { new File(_path).writeAsStringSync("@Timeout.name('foo')\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("no argument list", () { new File(_path).writeAsStringSync("@Timeout\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("an empty argument list", () { new File(_path).writeAsStringSync("@Timeout()\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("an argument list for Timeout.none", () { new File(_path).writeAsStringSync("@Timeout.none()\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("a named argument", () { new File(_path).writeAsStringSync( "@Timeout(duration: const Duration(seconds: 1))\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("multiple arguments", () { new File(_path) .writeAsStringSync("@Timeout.factor(1, 2)\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("a non-Duration argument", () { new File(_path).writeAsStringSync("@Timeout(10)\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("a non-num argument", () { new File(_path) .writeAsStringSync("@Timeout.factor('foo')\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("multiple @Timeouts", () { new File(_path).writeAsStringSync( "@Timeout.factor(1)\n@Timeout.factor(2)\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); group("a Duration with", () { test("a non-const constructor", () { new File(_path) .writeAsStringSync("@Timeout(new Duration(1))\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("a named constructor", () { new File(_path).writeAsStringSync( "@Timeout(const Duration.name(seconds: 1))\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("a positional argument", () { new File(_path) .writeAsStringSync("@Timeout(const Duration(1))\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("an unknown named argument", () { new File(_path).writeAsStringSync( "@Timeout(const Duration(name: 1))\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("a duplicate named argument", () { new File(_path).writeAsStringSync( "@Timeout(const Duration(seconds: 1, seconds: 1))\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); }); }); @@ -268,60 +247,54 @@ library foo; group("@Skip:", () { test("parses a valid annotation", () { new File(_path).writeAsStringSync("@Skip()\nlibrary foo;"); - var metadata = parseMetadata(_path, TestPlatform.builtIn); + var metadata = parseMetadata(_path, new Set()); expect(metadata.skip, isTrue); expect(metadata.skipReason, isNull); }); test("parses a valid annotation with a reason", () { new File(_path).writeAsStringSync("@Skip('reason')\nlibrary foo;"); - var metadata = parseMetadata(_path, TestPlatform.builtIn); + var metadata = parseMetadata(_path, new Set()); expect(metadata.skip, isTrue); expect(metadata.skipReason, equals('reason')); }); test("ignores a constructor named Skip", () { new File(_path).writeAsStringSync("@foo.Skip('foo')\nlibrary foo;"); - var metadata = parseMetadata(_path, TestPlatform.builtIn); + var metadata = parseMetadata(_path, new Set()); expect(metadata.skip, isFalse); }); group("throws an error for", () { test("a named constructor", () { new File(_path).writeAsStringSync("@Skip.name('foo')\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("no argument list", () { new File(_path).writeAsStringSync("@Skip\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("a named argument", () { new File(_path).writeAsStringSync("@Skip(reason: 'foo')\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("multiple arguments", () { new File(_path).writeAsStringSync("@Skip('foo', 'bar')\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("a non-string argument", () { new File(_path).writeAsStringSync("@Skip(123)\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("multiple @Skips", () { new File(_path) .writeAsStringSync("@Skip('foo')\n@Skip('bar')\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); }); }); @@ -329,13 +302,13 @@ library foo; group("@Tags:", () { test("parses a valid annotation", () { new File(_path).writeAsStringSync("@Tags(const ['a'])\nlibrary foo;"); - var metadata = parseMetadata(_path, TestPlatform.builtIn); + var metadata = parseMetadata(_path, new Set()); expect(metadata.tags, equals(["a"])); }); test("ignores a constructor named Tags", () { new File(_path).writeAsStringSync("@foo.Tags(const ['a'])\nlibrary foo;"); - var metadata = parseMetadata(_path, TestPlatform.builtIn); + var metadata = parseMetadata(_path, new Set()); expect(metadata.tags, isEmpty); }); @@ -343,40 +316,34 @@ library foo; test("a named constructor", () { new File(_path) .writeAsStringSync("@Tags.name(const ['a'])\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("no argument list", () { new File(_path).writeAsStringSync("@Tags\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("a named argument", () { new File(_path).writeAsStringSync("@Tags(tags: ['a'])\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("multiple arguments", () { new File(_path) .writeAsStringSync("@Tags(const ['a'], ['b'])\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("a non-list argument", () { new File(_path).writeAsStringSync("@Tags('a')\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("multiple @Tags", () { new File(_path).writeAsStringSync( "@Tags(const ['a'])\n@Tags(const ['b'])\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); }); }); @@ -389,7 +356,7 @@ library foo; 'vm': const [const Skip(), const Timeout.factor(3)] }) library foo;"""); - var metadata = parseMetadata(_path, TestPlatform.builtIn); + var metadata = parseMetadata(_path, new Set()); var key = metadata.onPlatform.keys.first; expect(key.evaluate(TestPlatform.chrome), isTrue); @@ -407,7 +374,7 @@ library foo;"""); test("ignores a constructor named OnPlatform", () { new File(_path).writeAsStringSync("@foo.OnPlatform('foo')\nlibrary foo;"); - var metadata = parseMetadata(_path, TestPlatform.builtIn); + var metadata = parseMetadata(_path, new Set()); expect(metadata.testOn, equals(PlatformSelector.all)); }); @@ -415,82 +382,70 @@ library foo;"""); test("a named constructor", () { new File(_path) .writeAsStringSync("@OnPlatform.name(const {})\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("no argument list", () { new File(_path).writeAsStringSync("@OnPlatform\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("an empty argument list", () { new File(_path).writeAsStringSync("@OnPlatform()\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("a named argument", () { new File(_path) .writeAsStringSync("@OnPlatform(map: const {})\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("multiple arguments", () { new File(_path) .writeAsStringSync("@OnPlatform(const {}, const {})\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("a non-map argument", () { new File(_path) .writeAsStringSync("@OnPlatform(const Skip())\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("a non-const map", () { new File(_path).writeAsStringSync("@OnPlatform({})\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("a map with a non-String key", () { new File(_path).writeAsStringSync( "@OnPlatform(const {1: const Skip()})\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("a map with a unparseable key", () { new File(_path).writeAsStringSync( "@OnPlatform(const {'invalid': const Skip()})\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("a map with an invalid value", () { new File(_path).writeAsStringSync( "@OnPlatform(const {'vm': const TestOn('vm')})\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("a map with an invalid value in a list", () { new File(_path).writeAsStringSync( "@OnPlatform(const {'vm': [const TestOn('vm')]})\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); test("multiple @OnPlatforms", () { new File(_path).writeAsStringSync( "@OnPlatform(const {})\n@OnPlatform(const {})\nlibrary foo;"); - expect(() => parseMetadata(_path, TestPlatform.builtIn), - throwsFormatException); + expect(() => parseMetadata(_path, new Set()), throwsFormatException); }); }); }); diff --git a/test/utils.dart b/test/utils.dart index c33b5e86b..c2b062a3e 100644 --- a/test/utils.dart +++ b/test/utils.dart @@ -13,7 +13,6 @@ import 'package:test/src/backend/live_test.dart'; import 'package:test/src/backend/metadata.dart'; import 'package:test/src/backend/state.dart'; import 'package:test/src/backend/suite.dart'; -import 'package:test/src/backend/test_platform.dart'; import 'package:test/src/runner/application_exception.dart'; import 'package:test/src/runner/configuration/suite.dart'; import 'package:test/src/runner/engine.dart'; @@ -320,13 +319,13 @@ Future expectTestsPass(void body()) async { /// Runs [body] with a declarer and returns the declared entries. List declare(void body()) { - var declarer = new Declarer(TestPlatform.builtIn)..declare(body); + var declarer = new Declarer()..declare(body); return declarer.build().entries; } /// Runs [body] with a declarer and returns an engine that runs those tests. Engine declareEngine(void body(), {bool runSkipped: false}) { - var declarer = new Declarer(TestPlatform.builtIn)..declare(body); + var declarer = new Declarer()..declare(body); return new Engine.withSuites([ new RunnerSuite(const PluginEnvironment(), new SuiteConfiguration(runSkipped: runSkipped), declarer.build()) From 60d1676753efc962d328e326dd4cf433cb2c0428 Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Tue, 10 Oct 2017 12:43:03 -0700 Subject: [PATCH 2/2] Code review --- lib/src/backend/platform_selector.dart | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/src/backend/platform_selector.dart b/lib/src/backend/platform_selector.dart index 0e3c55465..be644d39a 100644 --- a/lib/src/backend/platform_selector.dart +++ b/lib/src/backend/platform_selector.dart @@ -8,8 +8,8 @@ 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.from(["posix", "dart-vm", "browser", "js", "blink"]) ..addAll(TestPlatform.builtIn.map((platform) => platform.identifier)) ..addAll(OperatingSystem.all.map((os) => os.identifier)); @@ -63,7 +63,8 @@ class PlatformSelector { _wrapFormatException( () => _inner.validate((name) => - _validVariables.contains(name) || validVariables.contains(name)), + _universalValidVariables.contains(name) || + validVariables.contains(name)), _span); }