From 37df6ea6171ebfb9f9e65d76d0622a907740591c Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Mon, 16 Oct 2017 17:50:43 -0700 Subject: [PATCH] Add an override_platforms config field See #391 --- CHANGELOG.md | 9 + doc/configuration.md | 23 +- lib/src/runner/browser/platform.dart | 3 + lib/src/runner/configuration.dart | 24 ++ lib/src/runner/configuration/load.dart | 34 +- .../configuration/platform_settings.dart | 26 ++ lib/src/runner/loader.dart | 28 +- .../runner/plugin/customizable_platform.dart | 9 + lib/src/utils.dart | 5 +- pubspec.yaml | 2 +- .../configuration/custom_platform_test.dart | 341 +++++++++++++++++- 11 files changed, 477 insertions(+), 27 deletions(-) create mode 100644 lib/src/runner/configuration/platform_settings.dart diff --git a/CHANGELOG.md b/CHANGELOG.md index a3427cf93..0f9a687d4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,12 @@ +## 0.12.25 + +* Add a `override_platforms` configuration field which allows test platforms' + settings (such as browsers' executables) to be overridden by the user. + +* Add a `define_platforms` configuration field which makes it possible to define + new platforms that use the same logic as existing ones but have different + settings. + ## 0.12.24+8 * `spawnHybridUri()` now interprets relative URIs correctly in browser tests. diff --git a/doc/configuration.md b/doc/configuration.md index d3d78d505..c95fa004e 100644 --- a/doc/configuration.md +++ b/doc/configuration.md @@ -52,7 +52,8 @@ tags: * [Configuring Platforms](#configuring-platforms) * [`on_os`](#on_os) * [`on_platform`](#on_platform) - * [`define_platform`](#define_platform) + * [`override_platforms`](#override_platforms) + * [`define_platforms`](#define_platforms) * [Browser Settings](#browser-settings) * [`executable`](#executable) * [Configuration Presets](#configuration-presets) @@ -570,6 +571,26 @@ when running on a particular operating system, use [`on_os`](#on_os) instead. This field counts as [test configuration](#test-configuration). +### `override_platforms` + +This field allows you to customize the settings for built-in test platforms. It +takes a map from platform identifiers to settings for those platforms. For example: + +```yaml +override_platforms: + chrome: + # The settings to override for this platform. + settings: + executable: chromium +``` + +This tells the test runner to use the `chromium` executable for Chrome tests. It +calls that executable with the same logic and flags it normally uses for Chrome. + +Each platform can define exactly which settings it supports. All browsers +support [the same settings](#browser-settings), but the VM doesn't support any +settings and so can't be overridden. + ### `define_platforms` You can define new platforms in terms of old ones using the `define_platforms` diff --git a/lib/src/runner/browser/platform.dart b/lib/src/runner/browser/platform.dart index 776f1be91..02c1984de 100644 --- a/lib/src/runner/browser/platform.dart +++ b/lib/src/runner/browser/platform.dart @@ -202,6 +202,9 @@ class BrowserPlatform extends PlatformPlugin ExecutableSettings parsePlatformSettings(YamlMap settings) => new ExecutableSettings.parse(settings); + ExecutableSettings mergePlatformSettings(ExecutableSettings settings1, ExecutableSettings settings2) => + settings1.merge(settings2); + void customizePlatform(TestPlatform platform, ExecutableSettings settings) { var oldSettings = _browserSettings[platform] ?? _browserSettings[platform.root]; diff --git a/lib/src/runner/configuration.dart b/lib/src/runner/configuration.dart index acd8c6567..bba0fa78f 100644 --- a/lib/src/runner/configuration.dart +++ b/lib/src/runner/configuration.dart @@ -8,6 +8,7 @@ import 'package:boolean_selector/boolean_selector.dart'; import 'package:collection/collection.dart'; import 'package:glob/glob.dart'; import 'package:path/path.dart' as p; +import 'package:source_span/source_span.dart'; import '../backend/platform_selector.dart'; import '../backend/test_platform.dart'; @@ -18,6 +19,7 @@ import 'configuration/args.dart' as args; import 'configuration/custom_platform.dart'; import 'configuration/load.dart'; import 'configuration/platform_selection.dart'; +import 'configuration/platform_settings.dart'; import 'configuration/reporters.dart'; import 'configuration/suite.dart'; import 'configuration/values.dart'; @@ -172,6 +174,9 @@ class Configuration { Set _knownPresets; + /// Built-in platforms whose settings are overridden by the user. + final Map overridePlatforms; + /// Platforms defined by the user in terms of existing platforms. final Map definePlatforms; @@ -218,6 +223,7 @@ class Configuration { Glob filename, Iterable chosenPresets, Map presets, + Map overridePlatforms, Map definePlatforms, bool noRetry, @@ -261,6 +267,7 @@ class Configuration { filename: filename, chosenPresets: chosenPresetSet, presets: _withChosenPresets(presets, chosenPresetSet), + overridePlatforms: overridePlatforms, definePlatforms: definePlatforms, noRetry: noRetry, suiteDefaults: new SuiteConfiguration( @@ -316,6 +323,7 @@ class Configuration { Glob filename, Iterable chosenPresets, Map presets, + Map overridePlatforms, Map definePlatforms, bool noRetry, SuiteConfiguration suiteDefaults}) @@ -337,6 +345,7 @@ class Configuration { chosenPresets = new UnmodifiableSetView(chosenPresets?.toSet() ?? new Set()), presets = _map(presets), + overridePlatforms = _map(overridePlatforms), definePlatforms = _map(definePlatforms), _noRetry = noRetry, suiteDefaults = pauseAfterLoad == true @@ -399,6 +408,13 @@ class Configuration { // We don't need to verify [customPlatforms] here because those platforms // already need to be verified and resolved to create [allPlatforms]. + for (var settings in overridePlatforms.values) { + if (!allPlatforms.any((platform) => platform.identifier == settings.identifier)) { + throw new SourceSpanFormatException( + 'Unknown platform "${settings.identifier}".', settings.identifierSpan); + } + } + suiteDefaults.validatePlatforms(allPlatforms); for (var config in presets.values) { config.validatePlatforms(allPlatforms); @@ -448,6 +464,12 @@ class Configuration { filename: other._filename ?? _filename, chosenPresets: chosenPresets.union(other.chosenPresets), presets: _mergeConfigMaps(presets, other.presets), + overridePlatforms: mergeUnmodifiableMaps( + overridePlatforms, other.overridePlatforms, + value: (settings1, settings2) => new PlatformSettings( + settings1.identifier, + settings1.identifierSpan, + settings1.settings.toList()..addAll(settings2.settings))), definePlatforms: mergeUnmodifiableMaps(definePlatforms, other.definePlatforms), noRetry: other._noRetry ?? _noRetry, @@ -482,6 +504,7 @@ class Configuration { Glob filename, Iterable chosenPresets, Map presets, + Map overridePlatforms, Map definePlatforms, bool noRetry, @@ -523,6 +546,7 @@ class Configuration { filename: filename ?? _filename, chosenPresets: chosenPresets ?? this.chosenPresets, presets: presets ?? this.presets, + overridePlatforms: overridePlatforms ?? this.overridePlatforms, definePlatforms: definePlatforms ?? this.definePlatforms, noRetry: noRetry ?? _noRetry, suiteDefaults: suiteDefaults.change( diff --git a/lib/src/runner/configuration/load.dart b/lib/src/runner/configuration/load.dart index 705e16d0e..68695e679 100644 --- a/lib/src/runner/configuration/load.dart +++ b/lib/src/runner/configuration/load.dart @@ -20,6 +20,7 @@ import '../configuration.dart'; import '../configuration/suite.dart'; import 'custom_platform.dart'; import 'platform_selection.dart'; +import 'platform_settings.dart'; import 'reporters.dart'; /// A regular expression matching a Dart identifier. @@ -193,6 +194,7 @@ class _ConfigurationLoader { _disallow("plain_names"); _disallow("platforms"); _disallow("add_presets"); + _disallow("override_platforms"); return Configuration.empty; } @@ -215,13 +217,42 @@ class _ConfigurationLoader { var chosenPresets = _getList("add_presets", (presetNode) => _parseIdentifierLike(presetNode, "Preset name")); + var overridePlatforms = _loadOverridePlatforms(); + return new Configuration( pauseAfterLoad: pauseAfterLoad, runSkipped: runSkipped, reporter: reporter, concurrency: concurrency, platforms: platforms, - chosenPresets: chosenPresets); + chosenPresets: chosenPresets, + overridePlatforms: overridePlatforms); + } + + /// Loads the `override_platforms` field. + Map _loadOverridePlatforms() { + var platformsNode = + _getNode("override_platforms", "map", (value) => value is Map) as YamlMap; + if (platformsNode == null) return const {}; + + var platforms = {}; + platformsNode.nodes.forEach((identifierNode, valueNode) { + var identifier = + _parseIdentifierLike(identifierNode, "Platform identifier"); + + _validate(valueNode, "Platform definition must be a map.", + (value) => value is Map); + var map = valueNode as YamlMap; + + var settings = _expect(map, "settings"); + _validate(settings, "Must be a map.", (value) => value is Map); + + platforms[identifier] = new PlatformSettings( + identifier, + identifierNode.span, + [settings as YamlMap]); + }); + return platforms; } /// Loads runner configuration that's not allowed in the global configuration @@ -316,6 +347,7 @@ class _ConfigurationLoader { }); } + /// Loads the `define_platforms` field. Map _loadDefinePlatforms() { var platformsNode = _getNode("define_platforms", "map", (value) => value is Map) as YamlMap; diff --git a/lib/src/runner/configuration/platform_settings.dart b/lib/src/runner/configuration/platform_settings.dart new file mode 100644 index 000000000..203901564 --- /dev/null +++ b/lib/src/runner/configuration/platform_settings.dart @@ -0,0 +1,26 @@ +// 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'; +import 'package:yaml/yaml.dart'; + +import '../plugin/customizable_platform.dart'; + +/// User-defined settings for a built-in test platform. +class PlatformSettings { + /// The identifier used to look up the platform being overridden. + final String identifier; + + /// The location that [identifier] was defined in the configuration file. + final SourceSpan identifierSpan; + + /// The user's settings for this platform. + /// + /// This is a list of settings, from most global to most specific, that will + /// eventually be merged using [CustomizablePlatform.mergePlatformSettings]. + final List settings; + + PlatformSettings(this.identifier, this.identifierSpan, List settings) + : settings = new List.unmodifiable(settings); +} diff --git a/lib/src/runner/loader.dart b/lib/src/runner/loader.dart index 4c71321b3..16c146964 100644 --- a/lib/src/runner/loader.dart +++ b/lib/src/runner/loader.dart @@ -53,8 +53,9 @@ class Loader { /// their string identifiers. final _platformsByIdentifier = {}; - /// The user-provided settings for platforms. - final _platformSettings = {}; + /// The user-provided settings for platforms, as a list of settings that will + /// be merged together using [CustomizablePlatform.mergePlatformSettings]. + final _platformSettings = >{}; /// The user-provided settings for platforms. final _parsedPlatformSettings = {}; @@ -104,6 +105,8 @@ class Loader { _registerCustomPlatforms(); _config.validatePlatforms(allPlatforms); + + _registerPlatformOverrides(); } /// Registers a [PlatformPlugin] for [platforms]. @@ -140,7 +143,19 @@ class Loader { _platformCallbacks[platform] = _platformCallbacks[parent]; _platformsByIdentifier[platform.identifier] = platform; - _platformSettings[platform] = customPlatform.settings; + _platformSettings[platform] = [customPlatform.settings]; + } + } + + /// Registers users' platform settings from [Configuration.overridePlatforms]. + void _registerPlatformOverrides() { + for (var settings in _config.overridePlatforms.values) { + var platform = _platformsByIdentifier[settings.identifier]; + + // This is officially validated in [Configuration.validatePlatforms]. + assert(platform != null); + + _platformSettings.putIfAbsent(platform, () => []).addAll(settings.settings); } } @@ -258,7 +273,7 @@ class Loader { if (settings == null) return; if (plugin is CustomizablePlatform) { - parsed = plugin.parsePlatformSettings(settings); + parsed = settings.map(plugin.parsePlatformSettings).reduce(plugin.mergePlatformSettings); plugin.customizePlatform(platform, parsed); _parsedPlatformSettings[platform] = parsed; } else { @@ -268,9 +283,8 @@ class Loader { identifier = platform.parent.identifier; span = _config.definePlatforms[platform.identifier].parentSpan; } else { - // TODO(nweiz): Get information from [_config.overridePlatforms] when - // that exists. - throw new UnimplementedError(); + identifier = platform.identifier; + span = _config.overridePlatforms[platform.identifier].identifierSpan; } throw new SourceSpanFormatException( diff --git a/lib/src/runner/plugin/customizable_platform.dart b/lib/src/runner/plugin/customizable_platform.dart index d8a2ce582..e1ebd57e2 100644 --- a/lib/src/runner/plugin/customizable_platform.dart +++ b/lib/src/runner/plugin/customizable_platform.dart @@ -34,6 +34,15 @@ abstract class CustomizablePlatform extends PlatformPlugin { /// possible. T parsePlatformSettings(YamlMap settings); + /// Merges [settings1] with [settings2] and returns a new settings object that + /// includes the configuration of both. + /// + /// When the settings conflict, [settings2] should take priority. + /// + /// This is used to merge global settings with local settings, or a custom + /// platform's settings with its parent's. + T mergePlatformSettings(T settings1, T settings2); + /// Defines user-provided [settings] for [platform]. /// /// The [platform] is a platform this plugin was declared to accept when diff --git a/lib/src/utils.dart b/lib/src/utils.dart index 687cd1a3e..2e06ef1c1 100644 --- a/lib/src/utils.dart +++ b/lib/src/utils.dart @@ -199,10 +199,11 @@ List flatten(Iterable nested) { /// creating a new map unnecessarily. /// /// The return value *may or may not* be unmodifiable. -Map mergeUnmodifiableMaps(Map map1, Map map2) { +Map mergeUnmodifiableMaps(Map map1, Map map2, + {V value(V value1, V value2)}) { if (map1.isEmpty) return map2; if (map2.isEmpty) return map1; - return mergeMaps(map1, map2); + return mergeMaps(map1, map2, value: value); } /// Truncates [text] to fit within [maxLength]. diff --git a/pubspec.yaml b/pubspec.yaml index 86f72f713..764d2e816 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -1,5 +1,5 @@ name: test -version: 0.12.24+8 +version: 0.12.25-dev author: Dart Team description: A library for writing dart unit tests. homepage: https://github.com/dart-lang/test diff --git a/test/runner/configuration/custom_platform_test.dart b/test/runner/configuration/custom_platform_test.dart index 45bf15c27..18efc25d3 100644 --- a/test/runner/configuration/custom_platform_test.dart +++ b/test/runner/configuration/custom_platform_test.dart @@ -27,17 +27,328 @@ void main() { """).create(); }); - group("define_platforms", () { - group("can define a new browser", () { - group("without any changes", () { - setUp(() async { - await d.file("dart_test.yaml", """ + group("override_platforms", () { + group("can override a browser", () { + test("without any changes", () async { + await d.file("dart_test.yaml", """ + override_platforms: + chrome: + settings: {} + """).create(); + + var test = await runTest(["-p", "chrome", "test.dart"]); + expect(test.stdout, emitsThrough(contains("All tests passed!"))); + await test.shouldExit(0); + }, tags: "chrome"); + + test("that's user-defined", () async { + await d.file("dart_test.yaml", """ define_platforms: chromium: name: Chromium extends: chrome settings: {} + + override_platforms: + chromium: + settings: {} + """).create(); + + var test = await runTest(["-p", "chromium", "test.dart"]); + expect(test.stdout, emitsThrough(contains("All tests passed!"))); + await test.shouldExit(0); + }, tags: "chrome"); + + test("with a basename-only executable", () async { + await d.file("dart_test.yaml", """ + override_platforms: + dartium: + settings: + executable: + linux: dartium + mac_os: dartium + windows: dartium.exe + """).create(); + + var test = await runTest(["-p", "dartium", "test.dart"]); + expect(test.stdout, emitsThrough(contains("All tests passed!"))); + await test.shouldExit(0); + }, tags: "dartium"); + + test("with an absolute-path executable", () async { + String path; + if (Platform.isLinux) { + var process = await TestProcess.start("which", ["google-chrome"]); + path = await process.stdout.next; + await process.shouldExit(0); + } else { + path = defaultSettings[TestPlatform.chrome].executable; + } + + await d.file("dart_test.yaml", """ + override_platforms: + chrome: + settings: + executable: $path + """).create(); + + var test = await runTest(["-p", "chrome", "test.dart"]); + expect(test.stdout, emitsThrough(contains("All tests passed!"))); + await test.shouldExit(0); + }, tags: "chrome"); + }); + + group("errors", () { + test("rejects a non-map value", () async { + await d.file("dart_test.yaml", "override_platforms: 12").create(); + + var test = await runTest([]); + expect(test.stderr, + containsInOrder(["override_platforms must be a map.", "^^"])); + await test.shouldExit(exit_codes.data); + }); + + test("rejects a non-string key", () async { + await d.file("dart_test.yaml", "override_platforms: {12: null}").create(); + + var test = await runTest([]); + expect(test.stderr, + containsInOrder(["Platform identifier must be a string.", "^^"])); + await test.shouldExit(exit_codes.data); + }); + + test("rejects a non-identifier-like key", () async { + await d + .file("dart_test.yaml", "override_platforms: {foo bar: null}") + .create(); + + var test = await runTest([]); + expect( + test.stderr, + containsInOrder([ + "Platform identifier must be an (optionally hyphenated) Dart " + "identifier.", + "^^^^^^^" + ])); + await test.shouldExit(exit_codes.data); + }); + + test("rejects a non-map definition", () async { + await d.file("dart_test.yaml", """ + override_platforms: + chrome: 12 + """).create(); + + var test = await runTest([]); + expect(test.stderr, + containsInOrder(["Platform definition must be a map.", "^^"])); + await test.shouldExit(exit_codes.data); + }); + + test("requires a settings key", () async { + await d.file("dart_test.yaml", """ + override_platforms: + chrome: {} + """).create(); + + var test = await runTest([]); + expect( + test.stderr, + containsInOrder( + ['Missing required field "settings".', "^^"])); + await test.shouldExit(exit_codes.data); + }); + + test("settings must be a map", () async { + await d.file("dart_test.yaml", """ + override_platforms: + chrome: + settings: null + """).create(); + + var test = await runTest([]); + expect(test.stderr, containsInOrder(['Must be a map.', "^^^^"])); + await test.shouldExit(exit_codes.data); + }); + + test("the overridden platform must exist", () async { + await d.file("dart_test.yaml", """ + override_platforms: + chromium: + settings: {} + """).create(); + + var test = await runTest(["test.dart"]); + expect( + test.stderr, + containsInOrder([ + 'Unknown platform "chromium".', + "^^^^^^" + ])); + await test.shouldExit(exit_codes.data); + }); + + test("uncustomizable platforms can't be overridden", + () async { + await d.file("dart_test.yaml", """ + override_platforms: + vm: + settings: {} """).create(); + + var test = await runTest(["-p", "vm", "test.dart"]); + expect(test.stdout, + containsInOrder(['The "vm" platform can\'t be customized.', "^^"])); + await test.shouldExit(1); + }); + + group("when overriding browsers", () { + test("executable must be a string or map", () async { + await d.file("dart_test.yaml", """ + override_platforms: + chrome: + settings: + executable: 12 + """).create(); + + var test = await runTest(["-p", "chrome", "test.dart"]); + expect(test.stdout, + containsInOrder(['Must be a map or a string.', "^^"])); + await test.shouldExit(1); + }); + + test("executable string may not be relative on POSIX", () async { + await d.file("dart_test.yaml", """ + override_platforms: + chrome: + settings: + executable: foo/bar + """).create(); + + var test = await runTest(["-p", "chrome", "test.dart"]); + expect( + test.stdout, + containsInOrder([ + 'Linux and Mac OS executables may not be relative paths.', + "^^^^^^^" + ])); + await test.shouldExit(1); + }, + // We allow relative executables for Windows so that Windows users + // can set a global executable without having to explicitly write + // `windows:`. + testOn: "!windows"); + + test("Linux executable must be a string", () async { + await d.file("dart_test.yaml", """ + override_platforms: + chrome: + settings: + executable: + linux: 12 + """).create(); + + var test = await runTest(["-p", "chrome", "test.dart"]); + expect(test.stdout, containsInOrder(['Must be a string.', "^^"])); + await test.shouldExit(1); + }); + + test("Linux executable may not be relative", () async { + await d.file("dart_test.yaml", """ + override_platforms: + chrome: + settings: + executable: + linux: foo/bar + """).create(); + + var test = await runTest(["-p", "chrome", "test.dart"]); + expect( + test.stdout, + containsInOrder([ + 'Linux and Mac OS executables may not be relative paths.', + "^^^^^^^" + ])); + await test.shouldExit(1); + }); + + test("Mac OS executable must be a string", () async { + await d.file("dart_test.yaml", """ + override_platforms: + chrome: + settings: + executable: + mac_os: 12 + """).create(); + + var test = await runTest(["-p", "chrome", "test.dart"]); + expect(test.stdout, containsInOrder(['Must be a string.', "^^"])); + await test.shouldExit(1); + }); + + test("Mac OS executable may not be relative", () async { + await d.file("dart_test.yaml", """ + override_platforms: + chrome: + settings: + executable: + mac_os: foo/bar + """).create(); + + var test = await runTest(["-p", "chrome", "test.dart"]); + expect( + test.stdout, + containsInOrder([ + 'Linux and Mac OS executables may not be relative paths.', + "^^^^^^^" + ])); + await test.shouldExit(1); + }); + + test("Windows executable must be a string", () async { + await d.file("dart_test.yaml", """ + override_platforms: + chrome: + settings: + executable: + windows: 12 + """).create(); + + var test = await runTest(["-p", "chrome", "test.dart"]); + expect(test.stdout, containsInOrder(['Must be a string.', "^^"])); + await test.shouldExit(1); + }); + + test("executable must exist", () async { + await d.file("dart_test.yaml", """ + override_platforms: + chrome: + settings: + executable: _does_not_exist + """).create(); + + var test = await runTest(["-p", "chrome", "test.dart"]); + expect( + test.stdout, + emitsThrough( + contains("Failed to run Chrome: $noSuchFileMessage"))); + await test.shouldExit(1); + }); + }); + }); + }); + + group("define_platforms", () { + group("can define a new browser", () { + group("without any changes", () { + setUp(() async { + await d.file("dart_test.yaml", """ + define_platforms: + chromium: + name: Chromium + extends: chrome + settings: {} + """).create(); }); test("that can be used to run tests", () async { @@ -48,12 +359,12 @@ void main() { test("that can be used in platform selectors", () async { await d.file("test.dart", """ - import 'package:test/test.dart'; + import 'package:test/test.dart'; - void main() { - test("success", () {}, testOn: "chromium"); - } - """).create(); + void main() { + test("success", () {}, testOn: "chromium"); + } + """).create(); var test = await runTest(["-p", "chromium", "test.dart"]); expect(test.stdout, emitsThrough(contains("All tests passed!"))); @@ -66,12 +377,12 @@ void main() { test("that counts as its parent", () async { await d.file("test.dart", """ - import 'package:test/test.dart'; + import 'package:test/test.dart'; - void main() { - test("success", () {}, testOn: "chrome"); - } - """).create(); + void main() { + test("success", () {}, testOn: "chrome"); + } + """).create(); var test = await runTest(["-p", "chromium", "test.dart"]); expect(test.stdout, emitsThrough(contains("All tests passed!")));