Add the ability to define and override test platforms

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.

doc/configuration.md

@@ -52,6 +52,10 @@ tags:
 * [Configuring Platforms](#configuring-platforms)
   * [`on_os`](#on_os)
   * [`on_platform`](#on_platform)
+  * [`override_platforms`](#override_platforms)
+  * [`define_platforms`](#define_platforms)
+  * [Browser/Node.js Settings](#browser-and-node-js-settings)
+    * [`executable`](#executable)
 * [Configuration Presets](#configuration-presets)
   * [`presets`](#presets)
   * [`add_preset`](#add_preset)
@@ -567,6 +571,126 @@ 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 and
+Node.js support [the same settings](#browser-and-node-js-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`
+field. This lets you define variants of existing platforms without overriding
+the old ones. This field takes a map from the new platform identifiers to
+definitions for those platforms. For example:
+
+```yaml
+define_platforms:
+  # This identifier is used to select the platform with the --platform flag.
+  chromium:
+    # A human-friendly name for the platform.
+    name: Chromium
+
+    # The identifier for the platform that this is based on.
+    extends: chrome
+
+    # Settings for the new child platform.
+    settings:
+      executable: chromium
+```
+
+Once this is defined, you can run `pub run test -p chromium` and it will run
+those tests in the Chromium browser, using the same logic it normally uses for
+Chrome. You can even use `chromium` in platform selectors; for example, you
+might pass `testOn: "chromium"` to declare that a test is Chromium-specific.
+User-defined platforms also count as their parents, so Chromium will run tests
+that say `testOn: "chrome"` as well.
+
+Each platform can define exactly which settings it supports. All browsers and
+Node.js support [the same settings](#browser-and-node-js-settings), but the VM
+doesn't support any settings and so can't be extended.
+
+This field is not supported in the
+[global configuration file](#global-configuration).
+
+### Browser and Node.js Settings
+
+All built-in browser platforms, as well as the built-in Node.js platform,
+provide the same settings that can be set using
+[`define_platforms`](#define_platforms), which control how their executables are
+invoked.
+
+#### `arguments`
+
+The `arguments` field provides extra arguments to the executable. It takes a
+string, and parses it in the same way as the POSIX shell:
+
+```yaml
+override_platforms:
+  firefox:
+    settings:
+      arguments: -headless
+```
+
+#### `executable`
+
+The `executable` field tells the test runner where to look for the executable to
+use to start the subprocess. It has three sub-keys, one for each supported
+operating system, which each take a path or an executable name:
+
+```yaml
+define_platforms:
+  chromium:
+    name: Chromium
+    extends: chrome
+
+    settings:
+      executable:
+        linux: chromium
+        mac_os: /Applications/Chromium.app/Contents/MacOS/Chromium
+        windows: Chromium\Application\chrome.exe
+```
+
+Executables can be defined in three ways:
+
+* As a plain basename, with no path separators. These executables are passed
+  directly to the OS, which looks them up using the `PATH` environment variable.
+
+* As an absolute path, which is used as-is.
+
+* **Only on Windows**, as a relative path. The test runner will look up this
+  path relative to the `LOCALAPPATA`, `PROGRAMFILES`, and `PROGRAMFILES(X86)`
+  environment variables, in that order.
+
+If a platform is omitted, it defaults to using the built-in executable location.
+
+As a shorthand, you can also define the same executable for all operating
+systems:
+
+```yaml
+define_platforms:
+  chromium:
+    name: Chromium
+    extends: chrome
+
+    settings:
+      executable: chromium
+```
+
 ## Configuration Presets
 
 *Presets* are collections of configuration that can be explicitly selected on

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';
@@ -35,6 +36,10 @@ class Declarer {
   /// and of the test suite.
   final Metadata _metadata;
 
+  /// The set of variables that are valid for platform selectors, in addition to
+  /// the built-in variables that are allowed everywhere.
+  final Set<String> _platformVariables;
+
   /// The stack trace for this group.
   final Trace _trace;
 
@@ -84,17 +89,31 @@ 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({Metadata metadata, bool collectTraces: false, bool noRetry: false})
-      : this._(null, null, metadata ?? new Metadata(), collectTraces, null,
+  Declarer(
+      {Metadata metadata,
+      Set<String> platformVariables,
+      bool collectTraces: false,
+      bool noRetry: false})
+      : this._(
+            null,
+            null,
+            metadata ?? new Metadata(),
+            platformVariables ?? const UnmodifiableSetView.empty(),
+            collectTraces,
+            null,
             noRetry);
 
-  Declarer._(this._parent, this._name, this._metadata, this._collectTraces,
-      this._trace, this._noRetry);
+  Declarer._(this._parent, this._name, this._metadata, this._platformVariables,
+      this._collectTraces, this._trace, this._noRetry);
 
   /// Runs [body] with this declarer as [Declarer.current].
   ///
@@ -111,13 +130,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(_platformVariables);
+    var metadata = _metadata.merge(newMetadata);
 
     _entries.add(new LocalTest(_prefix(name), metadata, () async {
       var parents = <Declarer>[];
@@ -151,17 +172,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(_platformVariables);
+    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._(this, _prefix(name), metadata,
+        _platformVariables, _collectTraces, trace, _noRetry);
     declarer.declare(() {
       // Cast to dynamic to avoid the analyzer complaining about us using the
       // result of a void method.

lib/src/backend/metadata.dart

@@ -286,6 +286,17 @@ class Metadata {
         "Dart identifiers.");
   }
 
+  /// Throws a [FormatException] if any [PlatformSelector]s use any variables
+  /// that don't appear either in [validVariables] or in the set of variables
+  /// that are known to be valid for all selectors.
+  void validatePlatformSelectors(Set<String> validVariables) {
+    testOn.validate(validVariables);
+    onPlatform.forEach((selector, metadata) {
+      selector.validate(validVariables);
+      metadata.validatePlatformSelectors(validVariables);
+    });
+  }
+
   /// Return a new [Metadata] that merges [this] with [other].
   ///
   /// If the two [Metadata]s have conflicting properties, [other] wins. If

lib/src/backend/platform_selector.dart

@@ -8,10 +8,10 @@ import 'package:source_span/source_span.dart';
 import 'operating_system.dart';
 import 'test_platform.dart';
 
-/// The set of all valid variable names.
-final _validVariables =
+/// The set of variable names that are valid for all platform selectors.
+final _universalValidVariables =
     new Set<String>.from(["posix", "dart-vm", "browser", "js", "blink"])
-      ..addAll(TestPlatform.all.map((platform) => platform.identifier))
+      ..addAll(TestPlatform.builtIn.map((platform) => platform.identifier))
       ..addAll(OperatingSystem.all.map((os) => os.identifier));
 
 /// An expression for selecting certain platforms, including operating systems
@@ -27,16 +27,46 @@ class PlatformSelector {
   /// The boolean selector used to implement this selector.
   final BooleanSelector _inner;
 
+  /// The source span from which this selector was parsed.
+  final SourceSpan _span;
+
   /// Parses [selector].
   ///
-  /// This will throw a [SourceSpanFormatException] if the selector is
-  /// malformed or if it uses an undefined variable.
-  PlatformSelector.parse(String selector)
-      : _inner = new BooleanSelector.parse(selector) {
-    _inner.validate(_validVariables.contains);
+  /// If [span] is passed, it indicates the location of the text for [selector]
+  /// in a larger document. It's used for error reporting.
+  PlatformSelector.parse(String selector, [SourceSpan span])
+      : _inner = _wrapFormatException(
+            () => new BooleanSelector.parse(selector), span),
+        _span = span;
+
+  const PlatformSelector._(this._inner) : _span = null;
+
+  /// Runs [body] and wraps any [FormatException] it throws in a
+  /// [SourceSpanFormatException] using [span].
+  ///
+  /// If [span] is `null`, runs [body] as-is.
+  static T _wrapFormatException<T>(T body(), SourceSpan span) {
+    if (span == null) return body();
+
+    try {
+      return body();
+    } on FormatException catch (error) {
+      throw new SourceSpanFormatException(error.message, span);
+    }
   }
 
-  const PlatformSelector._(this._inner);
+  /// Throws a [FormatException] if this selector uses any variables that don't
+  /// appear either in [validVariables] or in the set of variables that are
+  /// known to be valid for all selectors.
+  void validate(Set<String> validVariables) {
+    if (identical(this, all)) return;
+
+    _wrapFormatException(
+        () => _inner.validate((name) =>
+            _universalValidVariables.contains(name) ||
+            validVariables.contains(name)),
+        _span);
+  }
 
   /// Returns whether the selector matches the given [platform] and [os].
   ///
@@ -46,6 +76,7 @@ class PlatformSelector {
 
     return _inner.evaluate((variable) {
       if (variable == platform.identifier) return true;
+      if (variable == platform.parent?.identifier) return true;
       if (variable == os.identifier) return true;
       switch (variable) {
         case "dart-vm":