From 1ee7bef5133ba0b09a5286e94fe32aa3e698ef5e Mon Sep 17 00:00:00 2001 From: stuartmorgan Date: Tue, 3 Aug 2021 16:24:46 -0700 Subject: [PATCH] [flutter_plugin_tools] Track and log exclusions (#4205) Makes commands that use the package-looping base command track and report exclusions. This will make it much easier to debug/audit situations where tests aren't running when expected (e.g., when enabling a new type of test for a package that previously had to be explicitly excluded from that test to avoid failing for having no tests, but forgetting to remove the package from the exclusion list). Also fixes a latent issue with using different exclusion lists on different commands in a single CI task when using sharding could cause unexpected failures due to different sets of plugins being included for each step (e.g., build+drive with an exclude list on drive could potentially try to drive a plugin that hadn't been built in that shard) by sharding before filtering out excluded packages. Adds testing for sharding in general, as there was previously none. --- script/tool/CHANGELOG.md | 2 + script/tool/lib/src/analyze_command.dart | 6 +- .../src/common/package_looping_command.dart | 90 +++++-- .../tool/lib/src/common/plugin_command.dart | 147 +++++++---- .../src/create_all_plugins_app_command.dart | 9 +- script/tool/lib/src/list_command.dart | 15 +- .../common/package_looping_command_test.dart | 76 ++++++ .../tool/test/common/plugin_command_test.dart | 239 +++++++++++++++--- 8 files changed, 460 insertions(+), 124 deletions(-) diff --git a/script/tool/CHANGELOG.md b/script/tool/CHANGELOG.md index 7d1eac01b7..7f326ff3c8 100644 --- a/script/tool/CHANGELOG.md +++ b/script/tool/CHANGELOG.md @@ -21,6 +21,8 @@ `--no-integration`. - **Breaking change**: Replaced `java-test` with Android unit test support for the new `native-test` command. +- Commands that print a run summary at the end now track and log exclusions + similarly to skips for easier auditing. ## 0.4.1 diff --git a/script/tool/lib/src/analyze_command.dart b/script/tool/lib/src/analyze_command.dart index 4fd15f027f..2b728e2b90 100644 --- a/script/tool/lib/src/analyze_command.dart +++ b/script/tool/lib/src/analyze_command.dart @@ -5,6 +5,7 @@ import 'dart:async'; import 'package:file/file.dart'; +import 'package:flutter_plugin_tools/src/common/plugin_command.dart'; import 'package:platform/platform.dart'; import 'package:yaml/yaml.dart'; @@ -84,7 +85,10 @@ class AnalyzeCommand extends PackageLoopingCommand { /// Ensures that the dependent packages have been fetched for all packages /// (including their sub-packages) that will be analyzed. Future _runPackagesGetOnTargetPackages() async { - final List packageDirectories = await getPackages().toList(); + final List packageDirectories = + await getTargetPackagesAndSubpackages() + .map((PackageEnumerationEntry package) => package.directory) + .toList(); final Set packagePaths = packageDirectories.map((Directory dir) => dir.path).toSet(); packageDirectories.removeWhere((Directory directory) { diff --git a/script/tool/lib/src/common/package_looping_command.dart b/script/tool/lib/src/common/package_looping_command.dart index 0bcde6d296..0e0976ecc6 100644 --- a/script/tool/lib/src/common/package_looping_command.dart +++ b/script/tool/lib/src/common/package_looping_command.dart @@ -22,6 +22,10 @@ enum RunState { /// The command was skipped for the package. skipped, + /// The command was skipped for the package because it was explicitly excluded + /// in the command arguments. + excluded, + /// The command failed for the package. failed, } @@ -35,6 +39,9 @@ class PackageResult { PackageResult.skip(String reason) : this._(RunState.skipped, [reason]); + /// A run that was excluded by the command invocation. + PackageResult.exclude() : this._(RunState.excluded); + /// A run that failed. /// /// If [errors] are provided, they will be listed in the summary, otherwise @@ -70,13 +77,14 @@ abstract class PackageLoopingCommand extends PluginCommand { processRunner: processRunner, platform: platform, gitDir: gitDir); /// Packages that had at least one [logWarning] call. - final Set _packagesWithWarnings = {}; + final Set _packagesWithWarnings = + {}; /// Number of warnings that happened outside of a [runForPackage] call. int _otherWarningCount = 0; /// The package currently being run by [runForPackage]. - Directory? _currentPackage; + PackageEnumerationEntry? _currentPackage; /// Called during [run] before any calls to [runForPackage]. This provides an /// opportunity to fail early if the command can't be run (e.g., because the @@ -215,15 +223,24 @@ abstract class PackageLoopingCommand extends PluginCommand { await initializeRun(); - final List packages = includeSubpackages - ? await getPackages().toList() - : await getPlugins().toList(); + final List packages = includeSubpackages + ? await getTargetPackagesAndSubpackages(filterExcluded: false).toList() + : await getTargetPackages(filterExcluded: false).toList(); - final Map results = {}; - for (final Directory package in packages) { + final Map results = + {}; + for (final PackageEnumerationEntry package in packages) { _currentPackage = package; _printPackageHeading(package); - final PackageResult result = await runForPackage(package); + + // Command implementations should never see excluded packages; they are + // included at this level only for logging. + if (package.excluded) { + results[package] = PackageResult.exclude(); + continue; + } + + final PackageResult result = await runForPackage(package.directory); if (result.state == RunState.skipped) { final String message = '${indentation}SKIPPING: ${result.details.first}'; @@ -266,8 +283,11 @@ abstract class PackageLoopingCommand extends PluginCommand { /// Something is always printed to make it easier to distinguish between /// a command running for a package and producing no output, and a command /// not having been run for a package. - void _printPackageHeading(Directory package) { - String heading = 'Running for ${getPackageDescription(package)}'; + void _printPackageHeading(PackageEnumerationEntry package) { + final String packageDisplayName = getPackageDescription(package.directory); + String heading = package.excluded + ? 'Not running for $packageDisplayName; excluded' + : 'Running for $packageDisplayName'; if (hasLongOutput) { heading = ''' @@ -275,24 +295,35 @@ abstract class PackageLoopingCommand extends PluginCommand { || $heading ============================================================ '''; - } else { + } else if (!package.excluded) { heading = '$heading...'; } - captureOutput ? print(heading) : print(Colorize(heading)..cyan()); + if (captureOutput) { + print(heading); + } else { + final Colorize colorizeHeading = Colorize(heading); + print(package.excluded + ? colorizeHeading.darkGray() + : colorizeHeading.cyan()); + } } /// Prints a summary of packges run, packages skipped, and warnings. - void _printRunSummary( - List packages, Map results) { - final Set skippedPackages = results.entries - .where((MapEntry entry) => + void _printRunSummary(List packages, + Map results) { + final Set skippedPackages = results.entries + .where((MapEntry entry) => entry.value.state == RunState.skipped) - .map((MapEntry entry) => entry.key) + .map((MapEntry entry) => + entry.key) .toSet(); - final int skipCount = skippedPackages.length; + final int skipCount = skippedPackages.length + + packages + .where((PackageEnumerationEntry package) => package.excluded) + .length; // Split the warnings into those from packages that ran, and those that // were skipped. - final Set _skippedPackagesWithWarnings = + final Set _skippedPackagesWithWarnings = _packagesWithWarnings.intersection(skippedPackages); final int skippedWarningCount = _skippedPackagesWithWarnings.length; final int runWarningCount = @@ -318,14 +349,17 @@ abstract class PackageLoopingCommand extends PluginCommand { /// Prints a one-line-per-package overview of the run results for each /// package. - void _printPerPackageRunOverview(List packages, - {required Set skipped}) { + void _printPerPackageRunOverview(List packages, + {required Set skipped}) { print('Run overview:'); - for (final Directory package in packages) { + for (final PackageEnumerationEntry package in packages) { final bool hadWarning = _packagesWithWarnings.contains(package); Styles style; String summary; - if (skipped.contains(package)) { + if (package.excluded) { + summary = 'excluded'; + style = Styles.DARK_GRAY; + } else if (skipped.contains(package)) { summary = 'skipped'; style = hadWarning ? Styles.LIGHT_YELLOW : Styles.DARK_GRAY; } else { @@ -339,17 +373,17 @@ abstract class PackageLoopingCommand extends PluginCommand { if (!captureOutput) { summary = (Colorize(summary)..apply(style)).toString(); } - print(' ${getPackageDescription(package)} - $summary'); + print(' ${getPackageDescription(package.directory)} - $summary'); } print(''); } /// Prints a summary of all of the failures from [results]. - void _printFailureSummary( - List packages, Map results) { + void _printFailureSummary(List packages, + Map results) { const String indentation = ' '; _printError(failureListHeader); - for (final Directory package in packages) { + for (final PackageEnumerationEntry package in packages) { final PackageResult result = results[package]!; if (result.state == RunState.failed) { final String errorIndentation = indentation * 2; @@ -359,7 +393,7 @@ abstract class PackageLoopingCommand extends PluginCommand { ':\n$errorIndentation${result.details.join('\n$errorIndentation')}'; } _printError( - '$indentation${getPackageDescription(package)}$errorDetails'); + '$indentation${getPackageDescription(package.directory)}$errorDetails'); } } _printError(failureListFooter); diff --git a/script/tool/lib/src/common/plugin_command.dart b/script/tool/lib/src/common/plugin_command.dart index 7781eee0d9..db0a821fd2 100644 --- a/script/tool/lib/src/common/plugin_command.dart +++ b/script/tool/lib/src/common/plugin_command.dart @@ -15,6 +15,19 @@ import 'core.dart'; import 'git_version_finder.dart'; import 'process_runner.dart'; +/// An entry in package enumeration for APIs that need to include extra +/// data about the entry. +class PackageEnumerationEntry { + /// Creates a new entry for the given package directory. + PackageEnumerationEntry(this.directory, {required this.excluded}); + + /// The package's location. + final Directory directory; + + /// Whether or not this package was excluded by the command invocation. + final bool excluded; +} + /// Interface definition for all commands in this tool. // TODO(stuartmorgan): Move most of this logic to PackageLoopingCommand. abstract class PluginCommand extends Command { @@ -97,6 +110,9 @@ abstract class PluginCommand extends Command { int? _shardIndex; int? _shardCount; + // Cached set of explicitly excluded packages. + Set? _excludedPackages; + /// A context that matches the default for [platform]. p.Context get path => platform.isWindows ? p.windows : p.posix; @@ -174,60 +190,82 @@ abstract class PluginCommand extends Command { _shardCount = shardCount; } - /// Returns the root Dart package folders of the plugins involved in this - /// command execution. - // TODO(stuartmorgan): Rename/restructure this, _getAllPlugins, and - // getPackages, as the current naming is very confusing. - Stream getPlugins() async* { + /// Returns the set of plugins to exclude based on the `--exclude` argument. + Set _getExcludedPackageName() { + final Set excludedPackages = _excludedPackages ?? + getStringListArg(_excludeArg).expand((String item) { + if (item.endsWith('.yaml')) { + final File file = packagesDir.fileSystem.file(item); + return (loadYaml(file.readAsStringSync()) as YamlList) + .toList() + .cast(); + } + return [item]; + }).toSet(); + // Cache for future calls. + _excludedPackages = excludedPackages; + return excludedPackages; + } + + /// Returns the root diretories of the packages involved in this command + /// execution. + /// + /// Depending on the command arguments, this may be a user-specified set of + /// packages, the set of packages that should be run for a given diff, or all + /// packages. + /// + /// By default, packages excluded via --exclude will not be in the stream, but + /// they can be included by passing false for [filterExcluded]. + Stream getTargetPackages( + {bool filterExcluded = true}) async* { // To avoid assuming consistency of `Directory.list` across command // invocations, we collect and sort the plugin folders before sharding. // This is considered an implementation detail which is why the API still // uses streams. - final List allPlugins = await _getAllPlugins().toList(); - allPlugins.sort((Directory d1, Directory d2) => d1.path.compareTo(d2.path)); - // Sharding 10 elements into 3 shards should yield shard sizes 4, 4, 2. - // Sharding 9 elements into 3 shards should yield shard sizes 3, 3, 3. - // Sharding 2 elements into 3 shards should yield shard sizes 1, 1, 0. + final List allPlugins = + await _getAllPackages().toList(); + allPlugins.sort((PackageEnumerationEntry p1, PackageEnumerationEntry p2) => + p1.directory.path.compareTo(p2.directory.path)); final int shardSize = allPlugins.length ~/ shardCount + (allPlugins.length % shardCount == 0 ? 0 : 1); final int start = min(shardIndex * shardSize, allPlugins.length); final int end = min(start + shardSize, allPlugins.length); - for (final Directory plugin in allPlugins.sublist(start, end)) { - yield plugin; + for (final PackageEnumerationEntry plugin + in allPlugins.sublist(start, end)) { + if (!(filterExcluded && plugin.excluded)) { + yield plugin; + } } } - /// Returns the root Dart package folders of the plugins involved in this - /// command execution, assuming there is only one shard. + /// Returns the root Dart package folders of the packages involved in this + /// command execution, assuming there is only one shard. Depending on the + /// command arguments, this may be a user-specified set of packages, the + /// set of packages that should be run for a given diff, or all packages. /// - /// Plugin packages can exist in the following places relative to the packages + /// This will return packages that have been excluded by the --exclude + /// parameter, annotated in the entry as excluded. + /// + /// Packages can exist in the following places relative to the packages /// directory: /// /// 1. As a Dart package in a directory which is a direct child of the - /// packages directory. This is a plugin where all of the implementations - /// exist in a single Dart package. + /// packages directory. This is a non-plugin package, or a non-federated + /// plugin. /// 2. Several plugin packages may live in a directory which is a direct /// child of the packages directory. This directory groups several Dart - /// packages which implement a single plugin. This directory contains a - /// "client library" package, which declares the API for the plugin, as - /// well as one or more platform-specific implementations. + /// packages which implement a single plugin. This directory contains an + /// "app-facing" package which declares the API for the plugin, a + /// platform interface package which declares the API for implementations, + /// and one or more platform-specific implementation packages. /// 3./4. Either of the above, but in a third_party/packages/ directory that /// is a sibling of the packages directory. This is used for a small number /// of packages in the flutter/packages repository. - Stream _getAllPlugins() async* { + Stream _getAllPackages() async* { Set plugins = Set.from(getStringListArg(_packagesArg)); - final Set excludedPlugins = - getStringListArg(_excludeArg).expand((String item) { - if (item.endsWith('.yaml')) { - final File file = packagesDir.fileSystem.file(item); - return (loadYaml(file.readAsStringSync()) as YamlList) - .toList() - .cast(); - } - return [item]; - }).toSet(); + final Set excludedPluginNames = _getExcludedPackageName(); final bool runOnChangedPackages = getBoolArg(_runOnChangedPackagesArg); if (plugins.isEmpty && @@ -248,9 +286,9 @@ abstract class PluginCommand extends Command { in dir.list(followLinks: false)) { // A top-level Dart package is a plugin package. if (_isDartPackage(entity)) { - if (!excludedPlugins.contains(entity.basename) && - (plugins.isEmpty || plugins.contains(p.basename(entity.path)))) { - yield entity as Directory; + if (plugins.isEmpty || plugins.contains(p.basename(entity.path))) { + yield PackageEnumerationEntry(entity as Directory, + excluded: excludedPluginNames.contains(entity.basename)); } } else if (entity is Directory) { // Look for Dart packages under this top-level directory. @@ -264,13 +302,13 @@ abstract class PluginCommand extends Command { path.relative(subdir.path, from: dir.path); final String packageName = path.basename(subdir.path); final String basenamePath = path.basename(entity.path); - if (!excludedPlugins.contains(basenamePath) && - !excludedPlugins.contains(packageName) && - !excludedPlugins.contains(relativePath) && - (plugins.isEmpty || - plugins.contains(relativePath) || - plugins.contains(basenamePath))) { - yield subdir as Directory; + if (plugins.isEmpty || + plugins.contains(relativePath) || + plugins.contains(basenamePath)) { + yield PackageEnumerationEntry(subdir as Directory, + excluded: excludedPluginNames.contains(basenamePath) || + excludedPluginNames.contains(packageName) || + excludedPluginNames.contains(relativePath)); } } } @@ -279,27 +317,30 @@ abstract class PluginCommand extends Command { } } - /// Returns the example Dart package folders of the plugins involved in this - /// command execution. - Stream getExamples() => - getPlugins().expand(getExamplesForPlugin); - - /// Returns all Dart package folders (typically, plugin + example) of the - /// plugins involved in this command execution. - Stream getPackages() async* { - await for (final Directory plugin in getPlugins()) { + /// Returns all Dart package folders (typically, base package + example) of + /// the packages involved in this command execution. + /// + /// By default, packages excluded via --exclude will not be in the stream, but + /// they can be included by passing false for [filterExcluded]. + Stream getTargetPackagesAndSubpackages( + {bool filterExcluded = true}) async* { + await for (final PackageEnumerationEntry plugin + in getTargetPackages(filterExcluded: filterExcluded)) { yield plugin; - yield* plugin + yield* plugin.directory .list(recursive: true, followLinks: false) .where(_isDartPackage) - .cast(); + .map((FileSystemEntity directory) => PackageEnumerationEntry( + directory as Directory, // _isDartPackage guarantees this works. + excluded: plugin.excluded)); } } /// Returns the files contained, recursively, within the plugins /// involved in this command execution. Stream getFiles() { - return getPlugins() + return getTargetPackages() + .map((PackageEnumerationEntry entry) => entry.directory) .asyncExpand((Directory folder) => getFilesForPackage(folder)); } diff --git a/script/tool/lib/src/create_all_plugins_app_command.dart b/script/tool/lib/src/create_all_plugins_app_command.dart index ed70144560..d4eccb8a31 100644 --- a/script/tool/lib/src/create_all_plugins_app_command.dart +++ b/script/tool/lib/src/create_all_plugins_app_command.dart @@ -156,13 +156,14 @@ class CreateAllPluginsAppCommand extends PluginCommand { final Map pathDependencies = {}; - await for (final Directory package in getPlugins()) { - final String pluginName = package.basename; - final File pubspecFile = package.childFile('pubspec.yaml'); + await for (final PackageEnumerationEntry package in getTargetPackages()) { + final Directory pluginDirectory = package.directory; + final String pluginName = pluginDirectory.basename; + final File pubspecFile = pluginDirectory.childFile('pubspec.yaml'); final Pubspec pubspec = Pubspec.parse(pubspecFile.readAsStringSync()); if (pubspec.publishTo != 'none') { - pathDependencies[pluginName] = PathDependency(package.path); + pathDependencies[pluginName] = PathDependency(pluginDirectory.path); } } return pathDependencies; diff --git a/script/tool/lib/src/list_command.dart b/script/tool/lib/src/list_command.dart index 20f01ff98f..29a8ceb127 100644 --- a/script/tool/lib/src/list_command.dart +++ b/script/tool/lib/src/list_command.dart @@ -39,18 +39,23 @@ class ListCommand extends PluginCommand { Future run() async { switch (getStringArg(_type)) { case _plugin: - await for (final Directory package in getPlugins()) { - print(package.path); + await for (final PackageEnumerationEntry package + in getTargetPackages()) { + print(package.directory.path); } break; case _example: - await for (final Directory package in getExamples()) { + final Stream examples = getTargetPackages() + .map((PackageEnumerationEntry entry) => entry.directory) + .expand(getExamplesForPlugin); + await for (final Directory package in examples) { print(package.path); } break; case _package: - await for (final Directory package in getPackages()) { - print(package.path); + await for (final PackageEnumerationEntry package + in getTargetPackagesAndSubpackages()) { + print(package.directory.path); } break; case _file: diff --git a/script/tool/test/common/package_looping_command_test.dart b/script/tool/test/common/package_looping_command_test.dart index 542e91af64..00e64ddc21 100644 --- a/script/tool/test/common/package_looping_command_test.dart +++ b/script/tool/test/common/package_looping_command_test.dart @@ -185,6 +185,28 @@ void main() { package.childDirectory('example').path, ])); }); + + test('excludes subpackages when main package is excluded', () async { + final Directory excluded = createFakePlugin('a_plugin', packagesDir, + examples: ['example1', 'example2']); + final Directory included = createFakePackage('a_package', packagesDir); + + final TestPackageLoopingCommand command = + createTestCommand(includeSubpackages: true); + await runCommand(command, arguments: ['--exclude=a_plugin']); + + expect( + command.checkedPackages, + unorderedEquals([ + included.path, + included.childDirectory('example').path, + ])); + expect(command.checkedPackages, isNot(contains(excluded.path))); + expect(command.checkedPackages, + isNot(contains(excluded.childDirectory('example1').path))); + expect(command.checkedPackages, + isNot(contains(excluded.childDirectory('example2').path))); + }); }); group('output', () { @@ -376,6 +398,23 @@ void main() { ])); }); + test('logs exclusions', () async { + createFakePackage('package_a', packagesDir); + createFakePackage('package_b', packagesDir); + + final TestPackageLoopingCommand command = + createTestCommand(hasLongOutput: false); + final List output = + await runCommand(command, arguments: ['--exclude=package_b']); + + expect( + output, + containsAllInOrder([ + '${_startHeadingColor}Running for package_a...$_endColor', + '${_startSkipColor}Not running for package_b; excluded$_endColor', + ])); + }); + test('logs warnings', () async { final Directory warnPackage = createFakePackage('package_a', packagesDir); warnPackage @@ -435,6 +474,24 @@ void main() { expect(output, isNot(contains(contains('package a - ran')))); }); + test('counts exclusions as skips in run summary', () async { + createFakePackage('package_a', packagesDir); + + final TestPackageLoopingCommand command = + createTestCommand(hasLongOutput: false); + final List output = + await runCommand(command, arguments: ['--exclude=package_a']); + + expect( + output, + containsAllInOrder([ + '------------------------------------------------------------', + 'Skipped 1 package(s)', + '\n', + '${_startSuccessColor}No issues found!$_endColor', + ])); + }); + test('prints long-form run summary for long-output commands', () async { final Directory warnPackage1 = createFakePackage('package_a', packagesDir); @@ -478,6 +535,25 @@ void main() { ])); }); + test('prints exclusions as skips in long-form run summary', () async { + createFakePackage('package_a', packagesDir); + + final TestPackageLoopingCommand command = + createTestCommand(hasLongOutput: true); + final List output = + await runCommand(command, arguments: ['--exclude=package_a']); + + expect( + output, + containsAllInOrder([ + ' package_a - ${_startSkipColor}excluded$_endColor', + '', + 'Skipped 1 package(s)', + '\n', + '${_startSuccessColor}No issues found!$_endColor', + ])); + }); + test('handles warnings outside of runForPackage', () async { createFakePackage('package_a', packagesDir); diff --git a/script/tool/test/common/plugin_command_test.dart b/script/tool/test/common/plugin_command_test.dart index 7f67acfb2d..2f332aa8eb 100644 --- a/script/tool/test/common/plugin_command_test.dart +++ b/script/tool/test/common/plugin_command_test.dart @@ -22,12 +22,12 @@ import 'plugin_command_test.mocks.dart'; @GenerateMocks([GitDir]) void main() { late RecordingProcessRunner processRunner; + late SamplePluginCommand command; late CommandRunner runner; late FileSystem fileSystem; late MockPlatform mockPlatform; late Directory packagesDir; late Directory thirdPartyPackagesDir; - late List plugins; late List?> gitDirCommands; late String gitDiffResponse; @@ -53,9 +53,7 @@ void main() { return Future.value(mockProcessResult); }); processRunner = RecordingProcessRunner(); - plugins = []; - final SamplePluginCommand samplePluginCommand = SamplePluginCommand( - plugins, + command = SamplePluginCommand( packagesDir, processRunner: processRunner, platform: mockPlatform, @@ -63,7 +61,7 @@ void main() { ); runner = CommandRunner('common_command', 'Test for common functionality'); - runner.addCommand(samplePluginCommand); + runner.addCommand(command); }); group('plugin iteration', () { @@ -71,7 +69,8 @@ void main() { final Directory plugin1 = createFakePlugin('plugin1', packagesDir); final Directory plugin2 = createFakePlugin('plugin2', packagesDir); await runCapturingPrint(runner, ['sample']); - expect(plugins, unorderedEquals([plugin1.path, plugin2.path])); + expect(command.plugins, + unorderedEquals([plugin1.path, plugin2.path])); }); test('includes both plugins and packages', () async { @@ -81,7 +80,7 @@ void main() { final Directory package4 = createFakePackage('package4', packagesDir); await runCapturingPrint(runner, ['sample']); expect( - plugins, + command.plugins, unorderedEquals([ plugin1.path, plugin2.path, @@ -96,7 +95,7 @@ void main() { final Directory plugin3 = createFakePlugin('plugin3', thirdPartyPackagesDir); await runCapturingPrint(runner, ['sample']); - expect(plugins, + expect(command.plugins, unorderedEquals([plugin1.path, plugin2.path, plugin3.path])); }); @@ -108,7 +107,7 @@ void main() { await runCapturingPrint( runner, ['sample', '--packages=plugin1,package4']); expect( - plugins, + command.plugins, unorderedEquals([ plugin1.path, package4.path, @@ -123,7 +122,7 @@ void main() { await runCapturingPrint( runner, ['sample', '--plugins=plugin1,package4']); expect( - plugins, + command.plugins, unorderedEquals([ plugin1.path, package4.path, @@ -138,7 +137,7 @@ void main() { '--packages=plugin1,plugin2', '--exclude=plugin1' ]); - expect(plugins, unorderedEquals([plugin2.path])); + expect(command.plugins, unorderedEquals([plugin2.path])); }); test('exclude packages when packages flag isn\'t specified', () async { @@ -146,7 +145,7 @@ void main() { createFakePlugin('plugin2', packagesDir); await runCapturingPrint( runner, ['sample', '--exclude=plugin1,plugin2']); - expect(plugins, unorderedEquals([])); + expect(command.plugins, unorderedEquals([])); }); test('exclude federated plugins when packages flag is specified', () async { @@ -157,7 +156,7 @@ void main() { '--packages=federated/plugin1,plugin2', '--exclude=federated/plugin1' ]); - expect(plugins, unorderedEquals([plugin2.path])); + expect(command.plugins, unorderedEquals([plugin2.path])); }); test('exclude entire federated plugins when packages flag is specified', @@ -169,7 +168,7 @@ void main() { '--packages=federated/plugin1,plugin2', '--exclude=federated' ]); - expect(plugins, unorderedEquals([plugin2.path])); + expect(command.plugins, unorderedEquals([plugin2.path])); }); test('exclude accepts config files', () async { @@ -182,7 +181,7 @@ void main() { '--packages=plugin1', '--exclude=${configFile.path}' ]); - expect(plugins, unorderedEquals([])); + expect(command.plugins, unorderedEquals([])); }); group('test run-on-changed-packages', () { @@ -195,7 +194,8 @@ void main() { '--run-on-changed-packages' ]); - expect(plugins, unorderedEquals([plugin1.path, plugin2.path])); + expect(command.plugins, + unorderedEquals([plugin1.path, plugin2.path])); }); test( @@ -210,7 +210,8 @@ void main() { '--run-on-changed-packages' ]); - expect(plugins, unorderedEquals([plugin1.path, plugin2.path])); + expect(command.plugins, + unorderedEquals([plugin1.path, plugin2.path])); }); test('all plugins should be tested if .cirrus.yml changes.', () async { @@ -226,7 +227,8 @@ packages/plugin1/CHANGELOG '--run-on-changed-packages' ]); - expect(plugins, unorderedEquals([plugin1.path, plugin2.path])); + expect(command.plugins, + unorderedEquals([plugin1.path, plugin2.path])); }); test('all plugins should be tested if .ci.yaml changes', () async { @@ -242,7 +244,8 @@ packages/plugin1/CHANGELOG '--run-on-changed-packages' ]); - expect(plugins, unorderedEquals([plugin1.path, plugin2.path])); + expect(command.plugins, + unorderedEquals([plugin1.path, plugin2.path])); }); test('all plugins should be tested if anything in .ci/ changes', @@ -259,7 +262,8 @@ packages/plugin1/CHANGELOG '--run-on-changed-packages' ]); - expect(plugins, unorderedEquals([plugin1.path, plugin2.path])); + expect(command.plugins, + unorderedEquals([plugin1.path, plugin2.path])); }); test('all plugins should be tested if anything in script changes.', @@ -276,7 +280,8 @@ packages/plugin1/CHANGELOG '--run-on-changed-packages' ]); - expect(plugins, unorderedEquals([plugin1.path, plugin2.path])); + expect(command.plugins, + unorderedEquals([plugin1.path, plugin2.path])); }); test('all plugins should be tested if the root analysis options change.', @@ -293,7 +298,8 @@ packages/plugin1/CHANGELOG '--run-on-changed-packages' ]); - expect(plugins, unorderedEquals([plugin1.path, plugin2.path])); + expect(command.plugins, + unorderedEquals([plugin1.path, plugin2.path])); }); test('all plugins should be tested if formatting options change.', @@ -310,7 +316,8 @@ packages/plugin1/CHANGELOG '--run-on-changed-packages' ]); - expect(plugins, unorderedEquals([plugin1.path, plugin2.path])); + expect(command.plugins, + unorderedEquals([plugin1.path, plugin2.path])); }); test('Only changed plugin should be tested.', () async { @@ -323,7 +330,7 @@ packages/plugin1/CHANGELOG '--run-on-changed-packages' ]); - expect(plugins, unorderedEquals([plugin1.path])); + expect(command.plugins, unorderedEquals([plugin1.path])); }); test('multiple files in one plugin should also test the plugin', @@ -340,7 +347,7 @@ packages/plugin1/ios/plugin1.m '--run-on-changed-packages' ]); - expect(plugins, unorderedEquals([plugin1.path])); + expect(command.plugins, unorderedEquals([plugin1.path])); }); test('multiple plugins changed should test all the changed plugins', @@ -358,7 +365,8 @@ packages/plugin2/ios/plugin2.m '--run-on-changed-packages' ]); - expect(plugins, unorderedEquals([plugin1.path, plugin2.path])); + expect(command.plugins, + unorderedEquals([plugin1.path, plugin2.path])); }); test( @@ -379,7 +387,31 @@ packages/plugin1/plugin1_web/plugin1_web.dart '--run-on-changed-packages' ]); - expect(plugins, unorderedEquals([plugin1.path])); + expect(command.plugins, unorderedEquals([plugin1.path])); + }); + + test( + 'changing one plugin in a federated group should include all plugins in the group', + () async { + gitDiffResponse = ''' +packages/plugin1/plugin1/plugin1.dart +'''; + final Directory plugin1 = + createFakePlugin('plugin1', packagesDir.childDirectory('plugin1')); + final Directory plugin2 = createFakePlugin('plugin1_platform_interface', + packagesDir.childDirectory('plugin1')); + final Directory plugin3 = createFakePlugin( + 'plugin1_web', packagesDir.childDirectory('plugin1')); + await runCapturingPrint(runner, [ + 'sample', + '--base-sha=master', + '--run-on-changed-packages' + ]); + + expect( + command.plugins, + unorderedEquals( + [plugin1.path, plugin2.path, plugin3.path])); }); test( @@ -401,7 +433,8 @@ packages/plugin3/plugin3.dart '--run-on-changed-packages' ]); - expect(plugins, unorderedEquals([plugin1.path, plugin2.path])); + expect(command.plugins, + unorderedEquals([plugin1.path, plugin2.path])); }); test('--exclude flag works with --run-on-changed-packages', () async { @@ -421,15 +454,155 @@ packages/plugin3/plugin3.dart '--run-on-changed-packages' ]); - expect(plugins, unorderedEquals([plugin1.path])); + expect(command.plugins, unorderedEquals([plugin1.path])); }); }); }); + + group('sharding', () { + test('distributes evenly when evenly divisible', () async { + final List> expectedShards = >[ + [ + createFakePackage('package1', packagesDir), + createFakePackage('package2', packagesDir), + createFakePackage('package3', packagesDir), + ], + [ + createFakePackage('package4', packagesDir), + createFakePackage('package5', packagesDir), + createFakePackage('package6', packagesDir), + ], + [ + createFakePackage('package7', packagesDir), + createFakePackage('package8', packagesDir), + createFakePackage('package9', packagesDir), + ], + ]; + + for (int i = 0; i < expectedShards.length; ++i) { + final SamplePluginCommand localCommand = SamplePluginCommand( + packagesDir, + processRunner: processRunner, + platform: mockPlatform, + gitDir: MockGitDir(), + ); + final CommandRunner localRunner = + CommandRunner('common_command', 'Shard testing'); + localRunner.addCommand(localCommand); + + await runCapturingPrint(localRunner, [ + 'sample', + '--shardIndex=$i', + '--shardCount=3', + ]); + expect( + localCommand.plugins, + unorderedEquals(expectedShards[i] + .map((Directory package) => package.path) + .toList())); + } + }); + + test('distributes as evenly as possible when not evenly divisible', + () async { + final List> expectedShards = >[ + [ + createFakePackage('package1', packagesDir), + createFakePackage('package2', packagesDir), + createFakePackage('package3', packagesDir), + ], + [ + createFakePackage('package4', packagesDir), + createFakePackage('package5', packagesDir), + createFakePackage('package6', packagesDir), + ], + [ + createFakePackage('package7', packagesDir), + createFakePackage('package8', packagesDir), + ], + ]; + + for (int i = 0; i < expectedShards.length; ++i) { + final SamplePluginCommand localCommand = SamplePluginCommand( + packagesDir, + processRunner: processRunner, + platform: mockPlatform, + gitDir: MockGitDir(), + ); + final CommandRunner localRunner = + CommandRunner('common_command', 'Shard testing'); + localRunner.addCommand(localCommand); + + await runCapturingPrint(localRunner, [ + 'sample', + '--shardIndex=$i', + '--shardCount=3', + ]); + expect( + localCommand.plugins, + unorderedEquals(expectedShards[i] + .map((Directory package) => package.path) + .toList())); + } + }); + + // In CI (which is the use case for sharding) we often want to run muliple + // commands on the same set of packages, but the exclusion lists for those + // commands may be different. In those cases we still want all the commands + // to operate on a consistent set of plugins. + // + // E.g., some commands require running build-examples in a previous step; + // excluding some plugins from the later step shouldn't change what's tested + // in each shard, as it may no longer align with what was built. + test('counts excluded plugins when sharding', () async { + final List> expectedShards = >[ + [ + createFakePackage('package1', packagesDir), + createFakePackage('package2', packagesDir), + createFakePackage('package3', packagesDir), + ], + [ + createFakePackage('package4', packagesDir), + createFakePackage('package5', packagesDir), + createFakePackage('package6', packagesDir), + ], + [ + createFakePackage('package7', packagesDir), + ], + ]; + // These would be in the last shard, but are excluded. + createFakePackage('package8', packagesDir); + createFakePackage('package9', packagesDir); + + for (int i = 0; i < expectedShards.length; ++i) { + final SamplePluginCommand localCommand = SamplePluginCommand( + packagesDir, + processRunner: processRunner, + platform: mockPlatform, + gitDir: MockGitDir(), + ); + final CommandRunner localRunner = + CommandRunner('common_command', 'Shard testing'); + localRunner.addCommand(localCommand); + + await runCapturingPrint(localRunner, [ + 'sample', + '--shardIndex=$i', + '--shardCount=3', + '--exclude=package8,package9', + ]); + expect( + localCommand.plugins, + unorderedEquals(expectedShards[i] + .map((Directory package) => package.path) + .toList())); + } + }); + }); } class SamplePluginCommand extends PluginCommand { SamplePluginCommand( - this._plugins, Directory packagesDir, { ProcessRunner processRunner = const ProcessRunner(), Platform platform = const LocalPlatform(), @@ -437,7 +610,7 @@ class SamplePluginCommand extends PluginCommand { }) : super(packagesDir, processRunner: processRunner, platform: platform, gitDir: gitDir); - final List _plugins; + final List plugins = []; @override final String name = 'sample'; @@ -447,8 +620,8 @@ class SamplePluginCommand extends PluginCommand { @override Future run() async { - await for (final Directory package in getPlugins()) { - _plugins.add(package.path); + await for (final PackageEnumerationEntry package in getTargetPackages()) { + plugins.add(package.directory.path); } } }