Add configurable platform support

[stevenroose]

Chromium and Chrome should have similar run properties. However Chromium is open-source, while Chrome is branded. This makes Chromium more widely available on Linux systems and the preferred choice of many Linux users.

Since they would probably only differ in the name of the command, it's easy to add Chromium support as a separate platform, but alternatively, the chrome platform could just use google-chrome or chromium, whichever is available. (I assume something similar should already happen in order to support google-chrome-beta.)

[nex3]

I'm going to retarget this issue to refer to general support for configuring platforms. This is blocked on #46, since that's where the configuration will live.

I envision the ability to define new platforms that are small configurations of existing ones, like so:

platforms:
  chromium:
    type: chrome
    name: Chromium
    executable: chromium

The "type" is an existing platform whose spawning logic can be re-used. The name is an optional human-readable name for that platform, and the executable is the executable name.

In practice, finding the executable will likely be a little more complicated than that, because different operating systems have different locations and executable path conventions. We'd probably want to support something like this:

platforms:
  chromium:
    type: chrome
    name: Chromium
    executable:
      linux: chromium
      mac_os: /Applications/Chromium.app/Contents/MacOS/Chromium
      windows: Chromium\Application\chrome.exe

Absolute paths would be resolved absolutely, executable names would use the executable path, and for Windows in particular relative paths would check against LOCALAPPDATA, PROGRAMFILES, and PROGRAMFILES(X64).

You'd also be able to use this to override the executable locations of existing platforms:

platforms:
  dartium:
    executable:
      linux: dartium
      mac_os: /Applications/Dartium.app/Contents/MacOS/Chromium
      windows: Dartium\Application\chrome.exe

Although it would probably be better style to put that in the global config file (#63).

[stevenroose]

That sounds awesome!

[nex3]

Another thing I should mention: derived platforms would inherit the platform selectors of their parents, so @TestOn("chrome") would also allow a test to run on Chromium. They would also introduce their own platform selector variable, so @TestOn("chromium") would work as well, and would not allow plain Chrome.

[nex3]

Initial configuration file support has landed, so this is no longer blocked.

[StepFischer]

Is the topic of configurable platforms still in the work or is it abandoned?

[nex3]

This is definitely still something we plan to do (which is why this issue is still open 😉).

[nex3]

I've started work on this, and I think the config format should be adjusted somewhat from the examples I wrote up before. For one thing, the field name platforms is already in use, but there are a couple other goals I want to meet:

• The distinction between configuring an existing platform and defining a new platform via extension should be explicit. The former should be allowed in the global config, but the latter should not; we only want people to write testOn: "chromium" if Chromium is a defined platform within their package.

• Configuration should be forwards-compatible with platform plugins (Support pluggable launchers #99), which probably means having a mapping that we can pass as-is to the plugin along with the new TestPlatform.

With that in mind, I think we should define configuration like this:

# Overrides existing platforms. Runner configuration, allowed in the global or
# local file.
override_platforms:
  # Platform names must match existing platforms. This is validated after
  # plugins are loaded, so that plugin-defined platforms can be overridden.
  chrome:
    # "settings" is the only key here, to match define_platforms and to give us
    # the flexibility to add new fields without backward-compatibiliy issues.
    settings:
      # The contents of "settings" is passed as a YamlMap to the PlatformPlugin.
      executable: chromium

# Defines a new platform in terms of an existing one. Runner configuration,
# allowed only in the local file.
define_platforms:
  # Any (optionally hyphenated) Dart identifier is allowed.
  chromium:
    # "name", "extends", and "settings" are all required.
    name: Chromium

    # Only built-in and plugin platforms may be extended. That is, you can't
    # create another platform that extends chromium. This isn't strictly
    # necessary, but I don't see a good reason to allow multiple levels of
    # nesting.
    extends: chrome

    settings:
      # As above, this is passed directly to the PlatformPlugin.
      executable: chromium

Platform plugins that want to allow per-platform customization can then implement a CustomizablePlatform interface:

abstract class CustomizablePlatform {
  /// Defines user-provided [settings] for [platform].
  ///
  /// The [platform] is a platform this plugin was declared to accept when
  /// registered with [Loader.registerPlatformPlugin], or a platform whose
  /// [TestPlatform.parent] is one of those platforms. Subclasses should record
  /// the settings to use when [PlatformPlugin.loadChannel] or
  /// [PlatformPlugin.load] is called with the given [platform]. It's guaranteed
  /// to be called before either `load` method.
  ///
  /// If a [platform] has user-provided settings *and* extensions that have
  /// their own settings, this is guaranteed to be called with the parent
  /// platform before it's called with the child. It's the plugin's
  /// responsibility to make sure that the child's settings respect and override
  /// the parent's.
  ///
  /// Subclasses should throw [SourceSpanFormatException]s if [settings]
  /// contains invalid configuration.
  void customizePlatform(TestPlatform platform, YamlMap settings);
}

(Note that we could probably provide a better API if we had stateful mixins, but that's not likely to happen in time.)

@grouma What do you think of the configuration format and plugin API here?

[grouma]

From a Google3 perspective the CustomizablePlatform will probably go unused given the fact that we don't make use of the configuration YAML. That being said, for those users that do, this certainly looks useful. On a semi-related note, I'm curious if you have any data around the number of users for the various package:test features.

[nex3]

On a semi-related note, I'm curious if you have any data around the number of users for the various package:test features.

I know @munificent has done studies across pub manually a few times, but I don't know if that resulted in any infrastructure that we could use to figure out usage of test features. And we'll never be able to see the use in application packages.

Was there any particular feature you had in mind?

[natebosch]

Do we have an idea of what types of settings we expect will be configurable for which platforms? The proposed implementation is very general so I imagine we're anticipating a lot of use cases with a lot of users. Should we validate that?

[nex3]

I want to give the plugin API as much flexibility as possible to do things I haven't necessarily anticipated, but here are some example types of configuration I can imagine:

• Executable paths and command-line flags for browsers or Node.js.
• Command-line flags for the Flutter VM.
• Browser/OS choices for BrowserStack.
• Settings for a WebDriver instance.

[nex3]

There's also a question here of what to do when a platform is overridden in the user's global configuration and their local configuration. Because we're treating the configuration as opaque YAML objects, we can't automatically merge them. We could just say that the local configuration takes precedence, but this will probably violate user expectations if they're configuring different values (for example, they might configure a browser's executable locations globally and run tests in a package that sets an extra flag).

I propose we require plugins to deal with this explicitly. CustomizablePlatform would then look like this:

abstract class CustomizablePlatform<T> extends PlatformPlugin {
  /// Parses user-provided [settings] for a custom platform into a
  /// plugin-defined format.
  ///
  /// The [settings] come from a user's configuration file. The parsed output
  /// will be passed to [customizePlatform].
  ///
  /// Subclasses should throw [SourceSpanFormatException]s if [settings]
  /// contains invalid configuration. Unrecognized fields should be ignored if
  /// 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
  /// registered with [Loader.registerPlatformPlugin], or a platform whose
  /// [TestPlatform.parent] is one of those platforms. Subclasses should
  /// customize the behavior for these platforms when [loadChannel] or [load] is
  /// called with the given [platform], using the [settings] which are parsed by
  /// [parsePlatformSettings]. This is guaranteed to be called before either
  /// `load` method.
  void customizePlatform(TestPlatform platform, T settings);
}

This looks more complex for plugin authors, but in practice the old API already required that they deal with merging settings, it just wasn't enforced as part of the shape of the API.

[grouma]

I prefer explicitly calling out the need to merge settings so I'm in favor of the new API proposal. However, its not clear to me which of settings1 and settings2 refers to the global settings. Similarly which of the two refers to the parent's settings.

[nex3]

It's not relevant to the plugin what the specific semantics of the two settings are, only that settings2 should take priority over settings1 in the case of a conflict. In practice, though, this means that settings2 will be the local/child settings.

[nex3]

If possible I'd like to find a better name than CustomizablePlatform, since we might want to use that later on for an interface for platform plugins that have plugin-level customization, rather than TestPlatform-level customization. I'm not sure what a better option is, though, and I'd love to hear some suggestions.