From 4a4174e43a30fc3fe6769ce01ebd5d1734d64629 Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Mon, 2 Oct 2017 15:54:09 -0700 Subject: [PATCH] Don't use TestPlatform.all in PlatformSelector 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 #99 See #391 --- lib/src/backend/declarer.dart | 31 ++-- lib/src/backend/metadata.dart | 10 ++ lib/src/backend/platform_selector.dart | 45 ++++- lib/src/executable.dart | 6 + lib/src/runner.dart | 2 + lib/src/runner/configuration.dart | 14 +- lib/src/runner/configuration/args.dart | 13 +- lib/src/runner/configuration/load.dart | 28 +-- .../configuration/platform_selection.dart | 22 +++ lib/src/runner/configuration/suite.dart | 38 +++- lib/src/runner/loader.dart | 8 +- lib/src/runner/parse_metadata.dart | 28 ++- lib/src/runner/remote_listener.dart | 2 +- lib/test.dart | 2 +- test/backend/metadata_test.dart | 28 ++- test/runner/browser/loader_test.dart | 9 +- test/runner/configuration/platform_test.dart | 7 +- test/runner/configuration/suite_test.dart | 22 ++- test/runner/parse_metadata_test.dart | 169 +++++++++++------- test/utils.dart | 5 +- 20 files changed, 354 insertions(+), 135 deletions(-) create mode 100644 lib/src/runner/configuration/platform_selection.dart diff --git a/lib/src/backend/declarer.dart b/lib/src/backend/declarer.dart index 5432e9f12..1d40c5524 100644 --- a/lib/src/backend/declarer.dart +++ b/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 _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 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 = []; @@ -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. diff --git a/lib/src/backend/metadata.dart b/lib/src/backend/metadata.dart index 92a8ac811..d8441cfca 100644 --- a/lib/src/backend/metadata.dart +++ b/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 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 diff --git a/lib/src/backend/platform_selector.dart b/lib/src/backend/platform_selector.dart index 590fc4e6a..ff964ad5a 100644 --- a/lib/src/backend/platform_selector.dart +++ b/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.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 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 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]. /// diff --git a/lib/src/executable.dart b/lib/src/executable.dart index 1b2b1d772..697fb066d 100644 --- a/lib/src/executable.dart +++ b/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); diff --git a/lib/src/runner.dart b/lib/src/runner.dart index 166d6d05b..e038e0152 100644 --- a/lib/src/runner.dart +++ b/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 run() => _config.asCurrent(() async { + _config.validatePlatforms(_loader.allPlatforms); + if (_closed) { throw new StateError("run() may not be called on a closed Runner."); } diff --git a/lib/src/runner/configuration.dart b/lib/src/runner/configuration.dart index b9f19add0..39f2aa354 100644 --- a/lib/src/runner/configuration.dart +++ b/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 dart2jsArgs, String precompiledPath, Iterable patterns, - Iterable platforms, + Iterable platforms, BooleanSelector includeTags, BooleanSelector excludeTags, Map tags, @@ -383,6 +385,14 @@ class Configuration { /// asynchronous callbacks transitively created by [body]. T asCurrent(T body()) => runZoned(body, zoneValues: {_currentKey: this}); + /// Throws a [FormatException] if [this] refers to any undefined platforms. + void validatePlatforms(List 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 dart2jsArgs, String precompiledPath, Iterable patterns, - Iterable platforms, + Iterable platforms, BooleanSelector includeTags, BooleanSelector excludeTags, Map tags, diff --git a/lib/src/runner/configuration/args.dart b/lib/src/runner/configuration/args.dart index fabcabc03..74f52ea0f 100644 --- a/lib/src/runner/configuration/args.dart +++ b/lib/src/runner/configuration/args.dart @@ -10,6 +10,7 @@ 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'; @@ -17,7 +18,7 @@ import 'values.dart'; 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, + platforms: (_ifParsed('platform') as List) + ?.map((platform) => new PlatformSelection(platform)) + ?.toList(), runSkipped: _ifParsed('run-skipped'), chosenPresets: _ifParsed('preset') as List, paths: _options.rest.isEmpty ? null : _options.rest, diff --git a/lib/src/runner/configuration/load.dart b/lib/src/runner/configuration/load.dart index 175ebf2fd..cc66ca759 100644 --- a/lib/src/runner/configuration/load.dart +++ b/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. /// diff --git a/lib/src/runner/configuration/platform_selection.dart b/lib/src/runner/configuration/platform_selection.dart new file mode 100644 index 000000000..386ecfdd9 --- /dev/null +++ b/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; +} diff --git a/lib/src/runner/configuration/suite.dart b/lib/src/runner/configuration/suite.dart index 96a8630af..ac360bd10 100644 --- a/lib/src/runner/configuration/suite.dart +++ b/lib/src/runner/configuration/suite.dart @@ -4,12 +4,14 @@ import 'package:boolean_selector/boolean_selector.dart'; import 'package:collection/collection.dart'; +import 'package:source_span/source_span.dart'; import '../../backend/metadata.dart'; import '../../backend/operating_system.dart'; import '../../backend/platform_selector.dart'; import '../../backend/test_platform.dart'; import '../../frontend/timeout.dart'; +import 'platform_selection.dart'; /// Suite-level configuration. /// @@ -51,8 +53,10 @@ class SuiteConfiguration { final Set patterns; /// The set of platforms on which to run tests. - List get platforms => _platforms ?? const ["vm"]; - final List _platforms; + List get platforms => _platforms == null + ? const ["vm"] + : new List.unmodifiable(_platforms.map((platform) => platform.name)); + final List _platforms; /// Only run tests whose tags match this selector. /// @@ -124,7 +128,7 @@ class SuiteConfiguration { Iterable dart2jsArgs, String precompiledPath, Iterable patterns, - Iterable platforms, + Iterable platforms, BooleanSelector includeTags, BooleanSelector excludeTags, Map tags, @@ -172,7 +176,7 @@ class SuiteConfiguration { Iterable dart2jsArgs, this.precompiledPath, Iterable patterns, - Iterable platforms, + Iterable platforms, BooleanSelector includeTags, BooleanSelector excludeTags, Map tags, @@ -249,7 +253,7 @@ class SuiteConfiguration { Iterable dart2jsArgs, String precompiledPath, Iterable patterns, - Iterable platforms, + Iterable platforms, BooleanSelector includeTags, BooleanSelector excludeTags, Map tags, @@ -287,6 +291,30 @@ class SuiteConfiguration { return config._resolveTags(); } + /// Throws a [FormatException] if [this] refers to any undefined platforms. + void validatePlatforms(List allPlatforms) { + _metadata.validatePlatformSelectors(allPlatforms); + + if (_platforms != null) { + for (var selection in _platforms) { + if (!allPlatforms + .any((platform) => platform.identifier == selection.name)) { + if (selection.span != null) { + throw new SourceSpanFormatException( + 'Unknown platform "${selection.name}".', selection.span); + } else { + throw new FormatException('Unknown platform "${selection.name}".'); + } + } + } + } + + onPlatform.forEach((selector, config) { + selector.validate(allPlatforms); + config.validatePlatforms(allPlatforms); + }); + } + /// Returns a copy of [this] with all platform-specific configuration from /// [onPlatform] resolved. SuiteConfiguration forPlatform(TestPlatform platform, {OperatingSystem os}) { diff --git a/lib/src/runner/loader.dart b/lib/src/runner/loader.dart index 80301c554..b57121d65 100644 --- a/lib/src/runner/loader.dart +++ b/lib/src/runner/loader.dart @@ -43,6 +43,10 @@ class Loader { /// These are passed to the plugins' async memoizers when a plugin is needed. final _platformCallbacks = {}; + /// All plaforms supported by this [Loader]. + List get allPlatforms => + new List.unmodifiable(_platformCallbacks.keys); + /// Creates a new loader that loads tests on platforms defined in /// [Configuration.current]. /// @@ -117,8 +121,8 @@ class Loader { Stream loadFile( String path, SuiteConfiguration suiteConfig) async* { try { - suiteConfig = suiteConfig - .merge(new SuiteConfiguration.fromMetadata(parseMetadata(path))); + suiteConfig = suiteConfig.merge(new SuiteConfiguration.fromMetadata( + parseMetadata(path, allPlatforms))); } on AnalyzerErrorGroup catch (_) { // Ignore the analyzer's error, since its formatting is much worse than // the VM's or dart2js's. diff --git a/lib/src/runner/parse_metadata.dart b/lib/src/runner/parse_metadata.dart index 1ab4c30b8..299e861a7 100644 --- a/lib/src/runner/parse_metadata.dart +++ b/lib/src/runner/parse_metadata.dart @@ -12,28 +12,36 @@ 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. +/// /// Throws an [AnalysisError] if parsing fails or a [FormatException] if the /// test annotations are incorrect. -Metadata parseMetadata(String path) => new _Parser(path).parse(); +Metadata parseMetadata(String path, List allTestPlatforms) => + new _Parser(path, allTestPlatforms).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; + /// All annotations at the top of the file. List _annotations; /// All prefixes defined by imports in this file. Set _prefixes; - _Parser(String path) : _path = path { + _Parser(String path, this._allTestPlatforms) : _path = path { var contents = new File(path).readAsStringSync(); var directives = parseDirectives(contents, name: path).directives; _annotations = directives.isEmpty ? [] : directives.first.metadata; @@ -105,9 +113,17 @@ class _Parser { PlatformSelector _parseTestOn(Annotation annotation, String constructorName) { _assertConstructorName(constructorName, 'TestOn', annotation); _assertArguments(annotation.arguments, 'TestOn', annotation, positional: 1); - var literal = _parseString(annotation.arguments.arguments.first); + return _parsePlatformSelector(annotation.arguments.arguments.first); + } + + /// Parses an [expression] that should contain a string representing a + /// [PlatformSelector]. + PlatformSelector _parsePlatformSelector(Expression expression) { + var literal = _parseString(expression); return _contextualize( - literal, () => new PlatformSelector.parse(literal.stringValue)); + literal, + () => new PlatformSelector.parse(literal.stringValue) + ..validate(_allTestPlatforms)); } /// Parses a `@Retry` annotation. @@ -217,9 +233,7 @@ class _Parser { positional: 1); return _parseMap(annotation.arguments.arguments.first, key: (key) { - var selector = _parseString(key); - return _contextualize( - selector, () => new PlatformSelector.parse(selector.stringValue)); + return _parsePlatformSelector(key); }, value: (value) { var expressions = []; if (value is ListLiteral) { diff --git a/lib/src/runner/remote_listener.dart b/lib/src/runner/remote_listener.dart index a61fd0edf..e0fe0fefa 100644 --- a/lib/src/runner/remote_listener.dart +++ b/lib/src/runner/remote_listener.dart @@ -77,7 +77,7 @@ class RemoteListener { if (message['asciiGlyphs'] ?? false) glyph.ascii = true; var metadata = new Metadata.deserialize(message['metadata']); verboseChain = metadata.verboseTrace; - var declarer = new Declarer( + var declarer = new Declarer(TestPlatform.all, metadata: metadata, collectTraces: message['collectTraces'], noRetry: message['noRetry']); diff --git a/lib/test.dart b/lib/test.dart index 2550bef07..99a4606c5 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(); + _globalDeclarer = new Declarer(TestPlatform.all); 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 b32dfaf98..60ace938e 100644 --- a/test/backend/metadata_test.dart +++ b/test/backend/metadata_test.dart @@ -173,7 +173,7 @@ void main() { test("refuses an invalid platform selector", () { expect(() { - new Metadata.parse(onPlatform: {"invalid": new Skip()}); + new Metadata.parse(onPlatform: {"vm &&": new Skip()}); }, throwsFormatException); }); @@ -194,6 +194,32 @@ void main() { }); }); + group("validatePlatformSelectors", () { + test("succeeds if onPlatform uses valid platforms", () { + new Metadata.parse(onPlatform: {"vm || browser": new Skip()}) + .validatePlatformSelectors([TestPlatform.vm]); + }); + + test("succeeds if testOn uses valid platforms", () { + new Metadata.parse(testOn: "vm || browser") + .validatePlatformSelectors([TestPlatform.vm]); + }); + + test("fails if onPlatform uses an invalid platform", () { + expect(() { + new Metadata.parse(onPlatform: {"unknown": new Skip()}) + .validatePlatformSelectors([TestPlatform.vm]); + }, throwsFormatException); + }); + + test("fails if testOn uses an invalid platform", () { + expect(() { + new Metadata.parse(testOn: "unknown") + .validatePlatformSelectors([TestPlatform.vm]); + }, throwsFormatException); + }); + }); + group("change", () { test("preserves all fields if no parameters are passed", () { var metadata = new Metadata( diff --git a/test/runner/browser/loader_test.dart b/test/runner/browser/loader_test.dart index 2ff697853..a2739c83d 100644 --- a/test/runner/browser/loader_test.dart +++ b/test/runner/browser/loader_test.dart @@ -12,6 +12,7 @@ import 'package:test_descriptor/test_descriptor.dart' as d; import 'package:test/src/backend/state.dart'; import 'package:test/src/backend/test.dart'; import 'package:test/src/backend/test_platform.dart'; +import 'package:test/src/runner/configuration/platform_selection.dart'; import 'package:test/src/runner/configuration/suite.dart'; import 'package:test/src/runner/loader.dart'; import 'package:test/test.dart'; @@ -21,8 +22,8 @@ import '../../utils.dart'; Loader _loader; /// A configuration that loads suites on Chrome. -final _chrome = - new SuiteConfiguration(platforms: [TestPlatform.chrome.identifier]); +final _chrome = new SuiteConfiguration( + platforms: [new PlatformSelection(TestPlatform.chrome.identifier)]); void main() { setUp(() async { @@ -126,8 +127,8 @@ Future main() { .loadFile( path, new SuiteConfiguration(platforms: [ - TestPlatform.vm.identifier, - TestPlatform.chrome.identifier + new PlatformSelection(TestPlatform.vm.identifier), + new PlatformSelection(TestPlatform.chrome.identifier) ])) .asyncMap((loadSuite) => loadSuite.getSuite()) .toList(); diff --git a/test/runner/configuration/platform_test.dart b/test/runner/configuration/platform_test.dart index 2bf1d6989..2d5c59dde 100644 --- a/test/runner/configuration/platform_test.dart +++ b/test/runner/configuration/platform_test.dart @@ -116,11 +116,10 @@ void main() { })) .create(); + await d.dir("test").create(); + var test = await runTest([]); - expect( - test.stderr, - containsInOrder( - ["Invalid on_platform key: Undefined variable.", "^^^^^"])); + expect(test.stderr, containsInOrder(["Undefined variable.", "^^^^^"])); await test.shouldExit(exit_codes.data); }); diff --git a/test/runner/configuration/suite_test.dart b/test/runner/configuration/suite_test.dart index 8e9f1291e..a6ec33f9d 100644 --- a/test/runner/configuration/suite_test.dart +++ b/test/runner/configuration/suite_test.dart @@ -8,6 +8,7 @@ import 'package:test/test.dart'; import 'package:test/src/backend/platform_selector.dart'; import 'package:test/src/backend/test_platform.dart'; +import 'package:test/src/runner/configuration/platform_selection.dart'; import 'package:test/src/runner/configuration/suite.dart'; void main() { @@ -23,11 +24,12 @@ void main() { test("if only the old configuration's is defined, uses it", () { var merged = new SuiteConfiguration( - jsTrace: true, - runSkipped: true, - precompiledPath: "/tmp/js", - platforms: [TestPlatform.chrome.identifier]) - .merge(new SuiteConfiguration()); + jsTrace: true, + runSkipped: true, + precompiledPath: "/tmp/js", + platforms: [ + new PlatformSelection(TestPlatform.chrome.identifier) + ]).merge(new SuiteConfiguration()); expect(merged.jsTrace, isTrue); expect(merged.runSkipped, isTrue); @@ -40,7 +42,9 @@ void main() { jsTrace: true, runSkipped: true, precompiledPath: "/tmp/js", - platforms: [TestPlatform.chrome.identifier])); + platforms: [ + new PlatformSelection(TestPlatform.chrome.identifier) + ])); expect(merged.jsTrace, isTrue); expect(merged.runSkipped, isTrue); @@ -55,12 +59,14 @@ void main() { jsTrace: false, runSkipped: true, precompiledPath: "/tmp/js", - platforms: [TestPlatform.chrome.identifier]); + platforms: [new PlatformSelection(TestPlatform.chrome.identifier)]); var newer = new SuiteConfiguration( jsTrace: true, runSkipped: false, precompiledPath: "../js", - platforms: [TestPlatform.dartium.identifier]); + platforms: [ + new PlatformSelection(TestPlatform.dartium.identifier) + ]); var merged = older.merge(newer); expect(merged.jsTrace, isTrue); diff --git a/test/runner/parse_metadata_test.dart b/test/runner/parse_metadata_test.dart index 4da4ddcd9..30d20bc43 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); + var metadata = parseMetadata(_path, TestPlatform.all); 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); + var metadata = parseMetadata(_path, TestPlatform.all); 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); + var metadata = parseMetadata(_path, TestPlatform.all); expect(metadata.testOn.evaluate(TestPlatform.vm), isTrue); expect(metadata.testOn.evaluate(TestPlatform.chrome), isFalse); }); @@ -49,54 +49,61 @@ void main() { group("@TestOn:", () { test("parses a valid annotation", () { new File(_path).writeAsStringSync("@TestOn('vm')\nlibrary foo;"); - var metadata = parseMetadata(_path); + var metadata = parseMetadata(_path, TestPlatform.all); 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); + var metadata = parseMetadata(_path, TestPlatform.all); 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), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("no argument list", () { new File(_path).writeAsStringSync("@TestOn\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("an empty argument list", () { new File(_path).writeAsStringSync("@TestOn()\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("a named argument", () { new File(_path) .writeAsStringSync("@TestOn(expression: 'foo')\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("multiple arguments", () { new File(_path) .writeAsStringSync("@TestOn('foo', 'bar')\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("a non-string argument", () { new File(_path).writeAsStringSync("@TestOn(123)\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("multiple @TestOns", () { new File(_path) .writeAsStringSync("@TestOn('foo')\n@TestOn('bar')\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); }); }); @@ -113,7 +120,7 @@ void main() { library foo; """); - var metadata = parseMetadata(_path); + var metadata = parseMetadata(_path, TestPlatform.all); expect( metadata.timeout.duration, equals(new Duration( @@ -130,7 +137,7 @@ library foo; library foo; """); - var metadata = parseMetadata(_path); + var metadata = parseMetadata(_path, TestPlatform.all); expect(metadata.timeout.scaleFactor, equals(1)); }); @@ -140,7 +147,7 @@ library foo; library foo; """); - var metadata = parseMetadata(_path); + var metadata = parseMetadata(_path, TestPlatform.all); expect(metadata.timeout.scaleFactor, equals(0.5)); }); @@ -150,95 +157,109 @@ library foo; library foo; """); - var metadata = parseMetadata(_path); + var metadata = parseMetadata(_path, TestPlatform.all); 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); + var metadata = parseMetadata(_path, TestPlatform.all); 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), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("no argument list", () { new File(_path).writeAsStringSync("@Timeout\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("an empty argument list", () { new File(_path).writeAsStringSync("@Timeout()\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("an argument list for Timeout.none", () { new File(_path).writeAsStringSync("@Timeout.none()\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("a named argument", () { new File(_path).writeAsStringSync( "@Timeout(duration: const Duration(seconds: 1))\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("multiple arguments", () { new File(_path) .writeAsStringSync("@Timeout.factor(1, 2)\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("a non-Duration argument", () { new File(_path).writeAsStringSync("@Timeout(10)\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("a non-num argument", () { new File(_path) .writeAsStringSync("@Timeout.factor('foo')\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("multiple @Timeouts", () { new File(_path).writeAsStringSync( "@Timeout.factor(1)\n@Timeout.factor(2)\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); group("a Duration with", () { test("a non-const constructor", () { new File(_path) .writeAsStringSync("@Timeout(new Duration(1))\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("a named constructor", () { new File(_path).writeAsStringSync( "@Timeout(const Duration.name(seconds: 1))\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("a positional argument", () { new File(_path) .writeAsStringSync("@Timeout(const Duration(1))\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("an unknown named argument", () { new File(_path).writeAsStringSync( "@Timeout(const Duration(name: 1))\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("a duplicate named argument", () { new File(_path).writeAsStringSync( "@Timeout(const Duration(seconds: 1, seconds: 1))\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); }); }); @@ -247,54 +268,60 @@ library foo; group("@Skip:", () { test("parses a valid annotation", () { new File(_path).writeAsStringSync("@Skip()\nlibrary foo;"); - var metadata = parseMetadata(_path); + var metadata = parseMetadata(_path, TestPlatform.all); 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); + var metadata = parseMetadata(_path, TestPlatform.all); 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); + var metadata = parseMetadata(_path, TestPlatform.all); 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), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("no argument list", () { new File(_path).writeAsStringSync("@Skip\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("a named argument", () { new File(_path).writeAsStringSync("@Skip(reason: 'foo')\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("multiple arguments", () { new File(_path).writeAsStringSync("@Skip('foo', 'bar')\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("a non-string argument", () { new File(_path).writeAsStringSync("@Skip(123)\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("multiple @Skips", () { new File(_path) .writeAsStringSync("@Skip('foo')\n@Skip('bar')\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); }); }); @@ -302,13 +329,13 @@ library foo; group("@Tags:", () { test("parses a valid annotation", () { new File(_path).writeAsStringSync("@Tags(const ['a'])\nlibrary foo;"); - var metadata = parseMetadata(_path); + var metadata = parseMetadata(_path, TestPlatform.all); 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); + var metadata = parseMetadata(_path, TestPlatform.all); expect(metadata.tags, isEmpty); }); @@ -316,34 +343,40 @@ library foo; test("a named constructor", () { new File(_path) .writeAsStringSync("@Tags.name(const ['a'])\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("no argument list", () { new File(_path).writeAsStringSync("@Tags\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("a named argument", () { new File(_path).writeAsStringSync("@Tags(tags: ['a'])\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("multiple arguments", () { new File(_path) .writeAsStringSync("@Tags(const ['a'], ['b'])\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("a non-list argument", () { new File(_path).writeAsStringSync("@Tags('a')\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("multiple @Tags", () { new File(_path).writeAsStringSync( "@Tags(const ['a'])\n@Tags(const ['b'])\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); }); }); @@ -356,7 +389,7 @@ library foo; 'vm': const [const Skip(), const Timeout.factor(3)] }) library foo;"""); - var metadata = parseMetadata(_path); + var metadata = parseMetadata(_path, TestPlatform.all); var key = metadata.onPlatform.keys.first; expect(key.evaluate(TestPlatform.chrome), isTrue); @@ -374,7 +407,7 @@ library foo;"""); test("ignores a constructor named OnPlatform", () { new File(_path).writeAsStringSync("@foo.OnPlatform('foo')\nlibrary foo;"); - var metadata = parseMetadata(_path); + var metadata = parseMetadata(_path, TestPlatform.all); expect(metadata.testOn, equals(PlatformSelector.all)); }); @@ -382,70 +415,82 @@ library foo;"""); test("a named constructor", () { new File(_path) .writeAsStringSync("@OnPlatform.name(const {})\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("no argument list", () { new File(_path).writeAsStringSync("@OnPlatform\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("an empty argument list", () { new File(_path).writeAsStringSync("@OnPlatform()\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("a named argument", () { new File(_path) .writeAsStringSync("@OnPlatform(map: const {})\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("multiple arguments", () { new File(_path) .writeAsStringSync("@OnPlatform(const {}, const {})\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("a non-map argument", () { new File(_path) .writeAsStringSync("@OnPlatform(const Skip())\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("a non-const map", () { new File(_path).writeAsStringSync("@OnPlatform({})\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("a map with a non-String key", () { new File(_path).writeAsStringSync( "@OnPlatform(const {1: const Skip()})\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("a map with a unparseable key", () { new File(_path).writeAsStringSync( "@OnPlatform(const {'invalid': const Skip()})\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("a map with an invalid value", () { new File(_path).writeAsStringSync( "@OnPlatform(const {'vm': const TestOn('vm')})\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + 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), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); test("multiple @OnPlatforms", () { new File(_path).writeAsStringSync( "@OnPlatform(const {})\n@OnPlatform(const {})\nlibrary foo;"); - expect(() => parseMetadata(_path), throwsFormatException); + expect(() => parseMetadata(_path, TestPlatform.all), + throwsFormatException); }); }); }); diff --git a/test/utils.dart b/test/utils.dart index c2b062a3e..099ba4675 100644 --- a/test/utils.dart +++ b/test/utils.dart @@ -13,6 +13,7 @@ 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'; @@ -319,13 +320,13 @@ Future expectTestsPass(void body()) async { /// Runs [body] with a declarer and returns the declared entries. List declare(void body()) { - var declarer = new Declarer()..declare(body); + var declarer = new Declarer(TestPlatform.all)..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()..declare(body); + var declarer = new Declarer(TestPlatform.all)..declare(body); return new Engine.withSuites([ new RunnerSuite(const PluginEnvironment(), new SuiteConfiguration(runSkipped: runSkipped), declarer.build())