Store test platforms as strings in Configuration (flutter#692)

This will make it easier to define custom test platforms (both via flutter#391 and flutter#99) in the future. Because those platforms will be loaded based on the configuration, requiring knowledge of them to parse the user's platform arguments would produce an unresolvable circular dependency.
rhencke · Oct 3, 2017 · bfd24ca · bfd24ca
1 parent fa5620d
commit bfd24ca
Show file tree

Hide file tree

Showing 10 changed files with 45 additions and 38 deletions.
diff --git a/lib/src/runner.dart b/lib/src/runner.dart
@@ -128,7 +128,9 @@ class Runner {
     if (testOn == PlatformSelector.all) return;
 
     var unsupportedPlatforms = _config.suiteDefaults.platforms
-        .where((platform) => !testOn.evaluate(platform, os: currentOS))
+        .map(TestPlatform.find)
+        .where((platform) =>
+            platform != null && !testOn.evaluate(platform, os: currentOS))
         .toList();
     if (unsupportedPlatforms.isEmpty) return;
 
@@ -351,7 +353,7 @@ class Runner {
   /// Loads each suite in [suites] in order, pausing after load for platforms
   /// that support debugging.
   Future<bool> _loadThenPause(Stream<LoadSuite> suites) async {
-    if (_config.suiteDefaults.platforms.contains(TestPlatform.vm)) {
+    if (_config.suiteDefaults.platforms.contains(TestPlatform.vm.identifier)) {
       warn("Debugging is currently unsupported on the Dart VM.",
           color: _config.color);
     }

diff --git a/lib/src/runner/browser/platform.dart b/lib/src/runner/browser/platform.dart
@@ -194,7 +194,7 @@ class BrowserPlatform extends PlatformPlugin {
   /// Throws an [ArgumentError] if [browser] isn't a browser platform.
   Future<RunnerSuite> load(
       String path, TestPlatform browser, SuiteConfiguration suiteConfig) async {
-    assert(suiteConfig.platforms.contains(browser));
+    assert(suiteConfig.platforms.contains(browser.identifier));
 
     if (!browser.isBrowser) {
       throw new ArgumentError("$browser is not a browser.");

diff --git a/lib/src/runner/configuration.dart b/lib/src/runner/configuration.dart
@@ -10,7 +10,6 @@ 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;
@@ -220,7 +219,7 @@ class Configuration {
       Iterable<String> dart2jsArgs,
       String precompiledPath,
       Iterable<Pattern> patterns,
-      Iterable<TestPlatform> platforms,
+      Iterable<String> platforms,
       BooleanSelector includeTags,
       BooleanSelector excludeTags,
       Map<BooleanSelector, SuiteConfiguration> tags,
@@ -467,7 +466,7 @@ class Configuration {
       Iterable<String> dart2jsArgs,
       String precompiledPath,
       Iterable<Pattern> patterns,
-      Iterable<TestPlatform> platforms,
+      Iterable<String> platforms,
       BooleanSelector includeTags,
       BooleanSelector excludeTags,
       Map<BooleanSelector, SuiteConfiguration> tags,

diff --git a/lib/src/runner/configuration/args.dart b/lib/src/runner/configuration/args.dart
@@ -220,8 +220,7 @@ class _Parser {
         totalShards: totalShards,
         timeout: _parseOption('timeout', (value) => new Timeout.parse(value)),
         patterns: patterns,
-        platforms:
-            (_ifParsed('platform') as List<String>)?.map(TestPlatform.find),
+        platforms: _ifParsed('platform') as List<String>,
         runSkipped: _ifParsed('run-skipped'),
         chosenPresets: _ifParsed('preset') as List<String>,
         paths: _options.rest.isEmpty ? null : _options.rest,

diff --git a/lib/src/runner/configuration/load.dart b/lib/src/runner/configuration/load.dart
@@ -206,15 +206,13 @@ class _ConfigurationLoader {
 
     var concurrency = _getInt("concurrency");
 
-    var allPlatformIdentifiers =
-        TestPlatform.all.map((platform) => platform.identifier).toSet();
     var platforms = _getList("platforms", (platformNode) {
-      _validate(platformNode, "Platforms must be strings.",
-          (value) => value is String);
-      _validate(platformNode, 'Unknown platform "${platformNode.value}".',
-          allPlatformIdentifiers.contains);
-
-      return TestPlatform.find(platformNode.value);
+      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 chosenPresets = _getList("add_presets",

diff --git a/lib/src/runner/configuration/suite.dart b/lib/src/runner/configuration/suite.dart
@@ -51,8 +51,8 @@ class SuiteConfiguration {
   final Set<Pattern> patterns;
 
   /// The set of platforms on which to run tests.
-  List<TestPlatform> get platforms => _platforms ?? const [TestPlatform.vm];
-  final List<TestPlatform> _platforms;
+  List<String> get platforms => _platforms ?? const ["vm"];
+  final List<String> _platforms;
 
   /// Only run tests whose tags match this selector.
   ///
@@ -124,7 +124,7 @@ class SuiteConfiguration {
       Iterable<String> dart2jsArgs,
       String precompiledPath,
       Iterable<Pattern> patterns,
-      Iterable<TestPlatform> platforms,
+      Iterable<String> platforms,
       BooleanSelector includeTags,
       BooleanSelector excludeTags,
       Map<BooleanSelector, SuiteConfiguration> tags,
@@ -172,7 +172,7 @@ class SuiteConfiguration {
       Iterable<String> dart2jsArgs,
       this.precompiledPath,
       Iterable<Pattern> patterns,
-      Iterable<TestPlatform> platforms,
+      Iterable<String> platforms,
       BooleanSelector includeTags,
       BooleanSelector excludeTags,
       Map<BooleanSelector, SuiteConfiguration> tags,
@@ -249,7 +249,7 @@ class SuiteConfiguration {
       Iterable<String> dart2jsArgs,
       String precompiledPath,
       Iterable<Pattern> patterns,
-      Iterable<TestPlatform> platforms,
+      Iterable<String> platforms,
       BooleanSelector includeTags,
       BooleanSelector excludeTags,
       Map<BooleanSelector, SuiteConfiguration> tags,

diff --git a/lib/src/runner/loader.dart b/lib/src/runner/loader.dart
@@ -137,7 +137,9 @@ class Loader {
       return;
     }
 
-    for (var platform in suiteConfig.platforms) {
+    for (var platformName in suiteConfig.platforms) {
+      var platform = TestPlatform.find(platformName);
+
       if (!suiteConfig.metadata.testOn.evaluate(platform, os: currentOS)) {
         continue;
       }

diff --git a/test/runner/browser/loader_test.dart b/test/runner/browser/loader_test.dart
@@ -21,7 +21,8 @@ import '../../utils.dart';
 Loader _loader;
 
 /// A configuration that loads suites on Chrome.
-final _chrome = new SuiteConfiguration(platforms: [TestPlatform.chrome]);
+final _chrome =
+    new SuiteConfiguration(platforms: [TestPlatform.chrome.identifier]);
 
 void main() {
   setUp(() async {
@@ -124,8 +125,10 @@ Future main() {
     var suites = await _loader
         .loadFile(
             path,
-            new SuiteConfiguration(
-                platforms: [TestPlatform.vm, TestPlatform.chrome]))
+            new SuiteConfiguration(platforms: [
+              TestPlatform.vm.identifier,
+              TestPlatform.chrome.identifier
+            ]))
         .asyncMap((loadSuite) => loadSuite.getSuite())
         .toList();
     expect(suites[0].platform, equals(TestPlatform.vm));

diff --git a/test/runner/configuration/suite_test.dart b/test/runner/configuration/suite_test.dart
@@ -18,33 +18,34 @@ void main() {
         expect(merged.jsTrace, isFalse);
         expect(merged.runSkipped, isFalse);
         expect(merged.precompiledPath, isNull);
-        expect(merged.platforms, equals([TestPlatform.vm]));
+        expect(merged.platforms, equals([TestPlatform.vm.identifier]));
       });
 
       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]).merge(new SuiteConfiguration());
+                jsTrace: true,
+                runSkipped: true,
+                precompiledPath: "/tmp/js",
+                platforms: [TestPlatform.chrome.identifier])
+            .merge(new SuiteConfiguration());
 
         expect(merged.jsTrace, isTrue);
         expect(merged.runSkipped, isTrue);
         expect(merged.precompiledPath, equals("/tmp/js"));
-        expect(merged.platforms, equals([TestPlatform.chrome]));
+        expect(merged.platforms, equals([TestPlatform.chrome.identifier]));
       });
 
       test("if only the new configuration's is defined, uses it", () {
         var merged = new SuiteConfiguration().merge(new SuiteConfiguration(
             jsTrace: true,
             runSkipped: true,
             precompiledPath: "/tmp/js",
-            platforms: [TestPlatform.chrome]));
+            platforms: [TestPlatform.chrome.identifier]));
 
         expect(merged.jsTrace, isTrue);
         expect(merged.runSkipped, isTrue);
         expect(merged.precompiledPath, equals("/tmp/js"));
-        expect(merged.platforms, equals([TestPlatform.chrome]));
+        expect(merged.platforms, equals([TestPlatform.chrome.identifier]));
       });
 
       test(
@@ -54,18 +55,18 @@ void main() {
             jsTrace: false,
             runSkipped: true,
             precompiledPath: "/tmp/js",
-            platforms: [TestPlatform.chrome]);
+            platforms: [TestPlatform.chrome.identifier]);
         var newer = new SuiteConfiguration(
             jsTrace: true,
             runSkipped: false,
             precompiledPath: "../js",
-            platforms: [TestPlatform.dartium]);
+            platforms: [TestPlatform.dartium.identifier]);
         var merged = older.merge(newer);
 
         expect(merged.jsTrace, isTrue);
         expect(merged.runSkipped, isFalse);
         expect(merged.precompiledPath, equals("../js"));
-        expect(merged.platforms, equals([TestPlatform.dartium]));
+        expect(merged.platforms, equals([TestPlatform.dartium.identifier]));
       });
     });
 

diff --git a/test/runner/configuration/top_level_error_test.dart b/test/runner/configuration/top_level_error_test.dart
@@ -281,7 +281,8 @@ void main() {
           .create();
 
       var test = await runTest(["test.dart"]);
-      expect(test.stderr, containsInOrder(["Platforms must be strings", "^^"]));
+      expect(test.stderr,
+          containsInOrder(["Platform name must be a string", "^^"]));
       await test.shouldExit(exit_codes.data);
     });
 
@@ -294,7 +295,9 @@ void main() {
               }))
           .create();
 
-      var test = await runTest(["test.dart"]);
+      await d.dir("test").create();
+
+      var test = await runTest([]);
       expect(test.stderr, containsInOrder(['Unknown platform "foo"', "^^^^^"]));
       await test.shouldExit(exit_codes.data);
     });