Skip to content

Commit

Permalink
[ci] Add a web version of Dart unit tests (flutter#4352)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
stuartmorgan authored Jul 5, 2023
1 parent b310246 commit 7042079
Show file tree
Hide file tree
Showing 21 changed files with 407 additions and 37 deletions.
44 changes: 44 additions & 0 deletions .ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion .ci/targets/dart_unit_tests.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
6 changes: 6 additions & 0 deletions .ci/targets/web_dart_unit_tests.yaml
Original file line number Diff line number Diff line change
@@ -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"]
16 changes: 0 additions & 16 deletions .cirrus.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
26 changes: 25 additions & 1 deletion packages/flutter_markdown/test/image_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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(
Expand All @@ -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(
Expand Down Expand Up @@ -150,6 +171,7 @@ void defineTests() {
matchesGoldenFile(
'assets/images/golden/image_test/resource_asset_logo.png'));
},
skip: kIsWeb, // Goldens are platform-specific.
);

testWidgets(
Expand All @@ -168,6 +190,7 @@ void defineTests() {
expect(image.width, 50);
expect(image.height, 50);
},
skip: kIsWeb,
);

testWidgets(
Expand Down Expand Up @@ -360,6 +383,7 @@ void defineTests() {
matchesGoldenFile(
'assets/images/golden/image_test/custom_builder_asset_logo.png'));
},
skip: kIsWeb, // Goldens are platform-specific.
);
});
}
10 changes: 8 additions & 2 deletions packages/flutter_markdown/test/link_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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);
},
);
Expand Down
1 change: 1 addition & 0 deletions packages/flutter_migrate/dart_test.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
test_on: vm
1 change: 1 addition & 0 deletions packages/go_router_builder/dart_test.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
test_on: vm
1 change: 1 addition & 0 deletions packages/go_router_builder/example/dart_test.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
test_on: vm
Original file line number Diff line number Diff line change
Expand Up @@ -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(() {
Expand Down
1 change: 1 addition & 0 deletions packages/metrics_center/dart_test.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
test_on: vm
1 change: 1 addition & 0 deletions packages/multicast_dns/dart_test.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
test_on: vm
3 changes: 3 additions & 0 deletions packages/palette_generator/dart_test.yaml
Original file line number Diff line number Diff line change
@@ -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
1 change: 1 addition & 0 deletions packages/pigeon/dart_test.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
test_on: vm
3 changes: 3 additions & 0 deletions packages/rfw/dart_test.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# TODO(stuartmorgan): Fix the web failures, and enable. See
# https://github.com/flutter/flutter/issues/129843
test_on: vm
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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();
Expand Down Expand Up @@ -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 =
Expand Down
1 change: 1 addition & 0 deletions packages/xdg_directories/dart_test.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
test_on: vm
1 change: 1 addition & 0 deletions script/tool/lib/src/common/package_state_utils.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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';
}
Expand Down
93 changes: 85 additions & 8 deletions script/tool/lib/src/dart_test_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand All @@ -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(
Expand All @@ -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(
Expand All @@ -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';
});
}
}
1 change: 1 addition & 0 deletions script/tool/test/common/package_state_utils_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Loading

0 comments on commit 7042079

Please sign in to comment.