[ci] Add a web version of Dart unit tests (#4352)

stuartmorgan-g · web-flow · commit 704207985a21 · 2023-07-05T00:27:25.000Z
Adds new LUCI targets in bringup mode to run all possible Dart unit tests in Chrome. This is a new test (see linked issue), not a port of a Cirrus test, so it involves changes to tooling and packages: - The tooling now accepts an explicit platform. The default behavior if none is provided is the previous behavior of running in VM for everything but web plugin implementations (since that's convenient locally). - The tooling now has a basic understanding of `dart_test.yaml` `test_on` directives, to know when to skip. - Packages that don't support web have opt-out files. - Packages that do support web but have a few tests that fail on web have those tests opted out. - Packages that do support web but have a lot of failures on web have temporary opt-out files with TODOs + issue links. Most of flutter/flutter#128979
diff --git a/.ci.yaml b/.ci.yaml
@@ -136,6 +136,50 @@ targets:
       cores: "32"
       package_sharding: "--shardIndex 1 --shardCount 2"
 
+  - name: Linux_web web_dart_unit_test_shard_1 master
+    bringup: true # New target
+    recipe: packages/packages
+    timeout: 60
+    properties:
+      target_file: web_dart_unit_tests.yaml
+      channel: master
+      version_file: flutter_master.version
+      cores: "32"
+      package_sharding: "--shardIndex 0 --shardCount 2"
+
+  - name: Linux_web web_dart_unit_test_shard_2 master
+    bringup: true # New target
+    recipe: packages/packages
+    timeout: 60
+    properties:
+      target_file: web_dart_unit_tests.yaml
+      channel: master
+      version_file: flutter_master.version
+      cores: "32"
+      package_sharding: "--shardIndex 1 --shardCount 2"
+
+  - name: Linux_web web_dart_unit_test_shard_1 stable
+    bringup: true # New target
+    recipe: packages/packages
+    timeout: 60
+    properties:
+      target_file: web_dart_unit_tests.yaml
+      channel: stable
+      version_file: flutter_stable.version
+      cores: "32"
+      package_sharding: "--shardIndex 0 --shardCount 2"
+
+  - name: Linux_web web_dart_unit_test_shard_2 stable
+    bringup: true # New target
+    recipe: packages/packages
+    timeout: 60
+    properties:
+      target_file: web_dart_unit_tests.yaml
+      channel: stable
+      version_file: flutter_stable.version
+      cores: "32"
+      package_sharding: "--shardIndex 1 --shardCount 2"
+
   - name: Linux analyze master
     recipe: packages/packages
     timeout: 30
diff --git a/.ci/targets/dart_unit_tests.yaml b/.ci/targets/dart_unit_tests.yaml
@@ -3,7 +3,7 @@ tasks:
     script: .ci/scripts/prepare_tool.sh
   - name: Dart unit tests
     script: script/tool_runner.sh
-    args: ["dart-test", "--exclude=script/configs/dart_unit_tests_exceptions.yaml"]
+    args: ["dart-test", "--exclude=script/configs/dart_unit_tests_exceptions.yaml", "--platform=vm"]
     # Re-run tests with path-based dependencies to ensure that publishing
     # the changes won't break tests of other packages in the respository
     # that depend on it.
diff --git a/.ci/targets/web_dart_unit_tests.yaml b/.ci/targets/web_dart_unit_tests.yaml
@@ -0,0 +1,6 @@
+tasks:
+  - name: prepare tool
+    script: .ci/scripts/prepare_tool.sh
+  - name: Dart unit tests - web
+    script: script/tool_runner.sh
+    args: ["dart-test", "--exclude=script/configs/dart_unit_tests_exceptions.yaml", "--platform=chrome"]
diff --git a/.cirrus.yml b/.cirrus.yml
@@ -158,22 +158,6 @@ task:
     memory: 16G
   matrix:
     ### Platform-agnostic tasks ###
-    - name: dart_unit_tests
-      env:
-        matrix:
-          PACKAGE_SHARDING: "--shardIndex 0 --shardCount 2"
-          PACKAGE_SHARDING: "--shardIndex 1 --shardCount 2"
-        matrix:
-          CHANNEL: "master"
-          CHANNEL: "stable"
-      unit_test_script:
-        - ./script/tool_runner.sh dart-test --exclude=script/configs/dart_unit_tests_exceptions.yaml
-      pathified_unit_test_script:
-        # Run tests with path-based dependencies to ensure that publishing
-        # the changes won't break tests of other packages in the repository
-        # that depend on it.
-        - ./script/tool_runner.sh make-deps-path-based --target-dependencies-with-non-breaking-updates
-        - $PLUGIN_TOOL_COMMAND dart-test --run-on-dirty-packages --exclude=script/configs/dart_unit_tests_exceptions.yaml
     - name: linux-custom_package_tests
       env:
         PATH: $PATH:/usr/local/bin
diff --git a/packages/flutter_markdown/test/image_test.dart b/packages/flutter_markdown/test/image_test.dart
@@ -4,6 +4,7 @@
 
 import 'dart:io' as io;
 
+import 'package:flutter/foundation.dart';
 import 'package:flutter/gestures.dart';
 import 'package:flutter/material.dart';
 import 'package:flutter_markdown/flutter_markdown.dart';
@@ -90,7 +91,7 @@ void defineTests() {
     );
 
     testWidgets(
-      'local files should be files',
+      'local files should be files on non-web',
       (WidgetTester tester) async {
         const String data = '![alt](http.png)';
         await tester.pumpWidget(
@@ -105,6 +106,26 @@ void defineTests() {
 
         expect(image.image is FileImage, isTrue);
       },
+      skip: kIsWeb,
+    );
+
+    testWidgets(
+      'local files should be network on web',
+      (WidgetTester tester) async {
+        const String data = '![alt](http.png)';
+        await tester.pumpWidget(
+          boilerplate(
+            const Markdown(data: data),
+          ),
+        );
+
+        final Iterable<Widget> widgets = tester.allWidgets;
+        final Image image =
+            widgets.firstWhere((Widget widget) => widget is Image) as Image;
+
+        expect(image.image is NetworkImage, isTrue);
+      },
+      skip: !kIsWeb,
     );
 
     testWidgets(
@@ -150,6 +171,7 @@ void defineTests() {
             matchesGoldenFile(
                 'assets/images/golden/image_test/resource_asset_logo.png'));
       },
+      skip: kIsWeb, // Goldens are platform-specific.
     );
 
     testWidgets(
@@ -168,6 +190,7 @@ void defineTests() {
         expect(image.width, 50);
         expect(image.height, 50);
       },
+      skip: kIsWeb,
     );
 
     testWidgets(
@@ -360,6 +383,7 @@ void defineTests() {
             matchesGoldenFile(
                 'assets/images/golden/image_test/custom_builder_asset_logo.png'));
       },
+      skip: kIsWeb, // Goldens are platform-specific.
     );
   });
 }
diff --git a/packages/flutter_markdown/test/link_test.dart b/packages/flutter_markdown/test/link_test.dart
@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+import 'package:flutter/foundation.dart';
 import 'package:flutter/gestures.dart';
 import 'package:flutter/widgets.dart';
 import 'package:flutter_markdown/flutter_markdown.dart';
@@ -1147,8 +1148,13 @@ void defineTests() {
         final Finder imageFinder = find.byType(Image);
         expect(imageFinder, findsOneWidget);
         final Image image = imageFinder.evaluate().first.widget as Image;
-        final FileImage fi = image.image as FileImage;
-        expect(fi.file.path, equals('uri3'));
+        if (kIsWeb) {
+          final NetworkImage fi = image.image as NetworkImage;
+          expect(fi.url.endsWith('uri3'), true);
+        } else {
+          final FileImage fi = image.image as FileImage;
+          expect(fi.file.path, equals('uri3'));
+        }
         expect(linkTapResults, isNull);
       },
     );
diff --git a/packages/flutter_migrate/dart_test.yaml b/packages/flutter_migrate/dart_test.yaml
@@ -0,0 +1 @@
+test_on: vm
diff --git a/packages/go_router_builder/dart_test.yaml b/packages/go_router_builder/dart_test.yaml
@@ -0,0 +1 @@
+test_on: vm
diff --git a/packages/go_router_builder/example/dart_test.yaml b/packages/go_router_builder/example/dart_test.yaml
@@ -0,0 +1 @@
+test_on: vm
diff --git a/packages/google_maps_flutter/google_maps_flutter_platform_interface/test/types/bitmap_test.dart b/packages/google_maps_flutter/google_maps_flutter_platform_interface/test/types/bitmap_test.dart
@@ -185,7 +185,9 @@ void main() {
 
           expect((mip.toJson() as List<dynamic>)[2], 1);
           expect((scaled.toJson() as List<dynamic>)[2], 3);
-        });
+        },
+            // TODO(stuartmorgan): Investigate timeout on web.
+            skip: kIsWeb);
 
         test('name cannot be null or empty', () {
           expect(() {
diff --git a/packages/metrics_center/dart_test.yaml b/packages/metrics_center/dart_test.yaml
@@ -0,0 +1 @@
+test_on: vm
diff --git a/packages/multicast_dns/dart_test.yaml b/packages/multicast_dns/dart_test.yaml
@@ -0,0 +1 @@
+test_on: vm
diff --git a/packages/palette_generator/dart_test.yaml b/packages/palette_generator/dart_test.yaml
@@ -0,0 +1,3 @@
+# TODO(stuartmorgan): Adjust the tests not to require dart:io
+# See https://github.com/flutter/flutter/issues/129839
+test_on: vm
diff --git a/packages/pigeon/dart_test.yaml b/packages/pigeon/dart_test.yaml
@@ -0,0 +1 @@
+test_on: vm
diff --git a/packages/rfw/dart_test.yaml b/packages/rfw/dart_test.yaml
@@ -0,0 +1,3 @@
+# TODO(stuartmorgan): Fix the web failures, and enable. See
+# https://github.com/flutter/flutter/issues/129843
+test_on: vm
diff --git a/packages/standard_message_codec/test/standard_message_codec_test.dart b/packages/standard_message_codec/test/standard_message_codec_test.dart
@@ -66,7 +66,7 @@ void main() {
       expect(written.lengthInBytes, equals(8));
       final ReadBuffer read = ReadBuffer(written);
       expect(read.getInt64(), equals(-9000000000000));
-    });
+    }, testOn: 'vm' /* Int64 isn't supported on web */);
 
     test('of 64-bit integer in big endian', () {
       final WriteBuffer write = WriteBuffer();
@@ -75,7 +75,7 @@ void main() {
       expect(written.lengthInBytes, equals(8));
       final ReadBuffer read = ReadBuffer(written);
       expect(read.getInt64(endian: Endian.big), equals(-9000000000000));
-    });
+    }, testOn: 'vm' /* Int64 isn't supported on web */);
 
     test('of double', () {
       final WriteBuffer write = WriteBuffer();
@@ -115,7 +115,7 @@ void main() {
       final ReadBuffer read = ReadBuffer(written);
       read.getUint8();
       expect(read.getInt64List(3), equals(integers));
-    });
+    }, testOn: 'vm' /* Int64 isn't supported on web */);
 
     test('of float list when unaligned', () {
       final Float32List floats =
diff --git a/packages/xdg_directories/dart_test.yaml b/packages/xdg_directories/dart_test.yaml
@@ -0,0 +1 @@
+test_on: vm
diff --git a/script/tool/lib/src/common/package_state_utils.dart b/script/tool/lib/src/common/package_state_utils.dart
@@ -111,6 +111,7 @@ bool _isTestChange(List<String> pathComponents) {
       pathComponents.contains('androidTest') ||
       pathComponents.contains('RunnerTests') ||
       pathComponents.contains('RunnerUITests') ||
+      pathComponents.last == 'dart_test.yaml' ||
       // Pigeon's custom platform tests.
       pathComponents.first == 'platform_tests';
 }
diff --git a/script/tool/lib/src/dart_test_command.dart b/script/tool/lib/src/dart_test_command.dart
@@ -27,8 +27,15 @@ class DartTestCommand extends PackageLoopingCommand {
           'See https://github.com/dart-lang/sdk/blob/main/docs/process/experimental-flags.md '
           'for details.',
     );
+    argParser.addOption(
+      _platformFlag,
+      help: 'Runs tests on the given platform instead of the default platform '
+          '("vm" in most cases, "chrome" for web plugin implementations).',
+    );
   }
 
+  static const String _platformFlag = 'platform';
+
   @override
   final String name = 'dart-test';
 
@@ -52,17 +59,57 @@ class DartTestCommand extends PackageLoopingCommand {
       return PackageResult.skip('No test/ directory.');
     }
 
+    String? platform = getNullableStringArg(_platformFlag);
+
+    // Skip running plugin tests for non-web-supporting plugins (or non-web
+    // federated plugin implementations) on web, since there's no reason to
+    // expect them to work.
+    final bool webPlatform = platform != null && platform != 'vm';
+    final bool explicitVMPlatform = platform == 'vm';
+    final bool isWebOnlyPluginImplementation = pluginSupportsPlatform(
+            platformWeb, package,
+            requiredMode: PlatformSupport.inline) &&
+        package.directory.basename.endsWith('_web');
+    if (webPlatform) {
+      if (isFlutterPlugin(package) &&
+          !pluginSupportsPlatform(platformWeb, package)) {
+        return PackageResult.skip(
+            "Non-web plugin tests don't need web testing.");
+      }
+      if (_requiresVM(package)) {
+        // This explict skip is necessary because trying to run tests in a mode
+        // that the package has opted out of returns a non-zero exit code.
+        return PackageResult.skip('Package has opted out of non-vm testing.');
+      }
+    } else if (explicitVMPlatform) {
+      if (isWebOnlyPluginImplementation) {
+        return PackageResult.skip("Web plugin tests don't need vm testing.");
+      }
+      if (_requiresNonVM(package)) {
+        // This explict skip is necessary because trying to run tests in a mode
+        // that the package has opted out of returns a non-zero exit code.
+        return PackageResult.skip('Package has opted out of vm testing.');
+      }
+    } else if (platform == null && isWebOnlyPluginImplementation) {
+      // If no explicit mode is requested, run web plugin implementations in
+      // Chrome since their tests are not expected to work in vm mode. This
+      // allows easily running all unit tests locally, without having to run
+      // both modes.
+      platform = 'chrome';
+    }
+
     bool passed;
     if (package.requiresFlutter()) {
-      passed = await _runFlutterTests(package);
+      passed = await _runFlutterTests(package, platform: platform);
     } else {
-      passed = await _runDartTests(package);
+      passed = await _runDartTests(package, platform: platform);
     }
     return passed ? PackageResult.success() : PackageResult.fail();
   }
 
   /// Runs the Dart tests for a Flutter package, returning true on success.
-  Future<bool> _runFlutterTests(RepositoryPackage package) async {
+  Future<bool> _runFlutterTests(RepositoryPackage package,
+      {String? platform}) async {
     final String experiment = getStringArg(kEnableExperiment);
 
     final int exitCode = await processRunner.runAndStream(
@@ -71,18 +118,16 @@ class DartTestCommand extends PackageLoopingCommand {
         'test',
         '--color',
         if (experiment.isNotEmpty) '--enable-experiment=$experiment',
-        // TODO(ditman): Remove this once all plugins are migrated to 'drive'.
-        if (pluginSupportsPlatform(platformWeb, package,
-            requiredMode: PlatformSupport.inline))
-          '--platform=chrome',
+        if (platform != null) '--platform=$platform',
       ],
       workingDir: package.directory,
     );
     return exitCode == 0;
   }
 
   /// Runs the Dart tests for a non-Flutter package, returning true on success.
-  Future<bool> _runDartTests(RepositoryPackage package) async {
+  Future<bool> _runDartTests(RepositoryPackage package,
+      {String? platform}) async {
     // Unlike `flutter test`, `pub run test` does not automatically get
     // packages
     int exitCode = await processRunner.runAndStream(
@@ -103,10 +148,42 @@ class DartTestCommand extends PackageLoopingCommand {
         'run',
         if (experiment.isNotEmpty) '--enable-experiment=$experiment',
         'test',
+        if (platform != null) '--platform=$platform',
       ],
       workingDir: package.directory,
     );
 
     return exitCode == 0;
   }
+
+  bool _requiresVM(RepositoryPackage package) {
+    final File testConfig = package.directory.childFile('dart_test.yaml');
+    if (!testConfig.existsSync()) {
+      return false;
+    }
+    // test_on lines can be very complex, but in pratice the packages in this
+    // repo currently only need the ability to require vm or not, so that
+    // simple directive is all that is currently supported.
+    final RegExp vmRequrimentRegex = RegExp(r'^test_on:\s*vm$');
+    return testConfig
+        .readAsLinesSync()
+        .any((String line) => vmRequrimentRegex.hasMatch(line));
+  }
+
+  bool _requiresNonVM(RepositoryPackage package) {
+    final File testConfig = package.directory.childFile('dart_test.yaml');
+    if (!testConfig.existsSync()) {
+      return false;
+    }
+    // test_on lines can be very complex, but in pratice the packages in this
+    // repo currently only need the ability to require vm or not, so a simple
+    // one-target directive is all that's supported currently. Making it
+    // deliberately strict avoids the possibility of accidentally skipping vm
+    // coverage due to a complex expression that's not handled correctly.
+    final RegExp testOnRegex = RegExp(r'^test_on:\s*([a-z])*\s*$');
+    return testConfig.readAsLinesSync().any((String line) {
+      final RegExpMatch? match = testOnRegex.firstMatch(line);
+      return match != null && match.group(1) != 'vm';
+    });
+  }
 }
diff --git a/script/tool/test/common/package_state_utils_test.dart b/script/tool/test/common/package_state_utils_test.dart
@@ -72,6 +72,7 @@ void main() {
         'packages/a_plugin/pigeons/messages.dart',
         // Test scripts.
         'packages/a_plugin/run_tests.sh',
+        'packages/a_plugin/dart_test.yaml',
         // Tools.
         'packages/a_plugin/tool/a_development_tool.dart',
         // Example build files.
diff --git a/script/tool/test/dart_test_command_test.dart b/script/tool/test/dart_test_command_test.dart
