[flutter_plugin_tools] Add a new base command for looping over packages (#4067)

Most of our commands are generally of the form:
```
  for (each plugin as defined by the tool flags)
    check some things for success or failure
  print a summary all of the failing things
  exit non-zero if anything failed
```

Currently all that logic not consistent, having been at various points copied and pasted around, modified, in some cases rewritten. There's unnecessary boilerplate in each new command, and there's unnecessary variation that makes it harder both to maintain the tool, and to consume the test output:
- There's no standard format for separating each plugin's run to search within a log
- There's no standard format for the summary at the end
- In some cases commands have been written to ToolExit on failure, which means we don't actually get the rest of the runs

This makes a new base class for commands that follow this structure to use, with shared code for all the common bits. This makes it harder to accidentally write new commands incorrectly, easier to maintain the code, and lets us standardize output so that searching within large logs will be easier.

This ports two commands over as a proof of concept to demonstrate that it works; more will be converted in follow-ups.

Related to https://github.com/flutter/flutter/issues/83413

script/tool/lib/src/common/core.dart

@@ -53,13 +53,25 @@ bool isFlutterPackage(FileSystemEntity entity) {
   }
 }
 
+/// Prints `successMessage` in green.
+void printSuccess(String successMessage) {
+  print(Colorize(successMessage)..green());
+}
+
 /// Prints `errorMessage` in red.
 void printError(String errorMessage) {
-  final Colorize redError = Colorize(errorMessage)..red();
-  print(redError);
+  print(Colorize(errorMessage)..red());
 }
 
 /// Error thrown when a command needs to exit with a non-zero exit code.
+///
+/// While there is no specific definition of the meaning of different non-zero
+/// exit codes for this tool, commands should follow the general convention:
+///   1: The command ran correctly, but found errors.
+///   2: The command failed to run because the arguments were invalid.
+///  >2: The command failed to run correctly for some other reason. Ideally,
+///      each such failure should have a unique exit code within the context of
+///      that command.
 class ToolExit extends Error {
   /// Creates a tool exit with the given [exitCode].
   ToolExit(this.exitCode);
@@ -67,3 +79,9 @@ class ToolExit extends Error {
   /// The code that the process should exit with.
   final int exitCode;
 }
+
+/// A exit code for [ToolExit] for a successful run that found errors.
+const int exitCommandFoundErrors = 1;
+
+/// A exit code for [ToolExit] for a failure to run due to invalid arguments.
+const int exitInvalidArguments = 2;

script/tool/lib/src/common/package_looping_command.dart

@@ -0,0 +1,161 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+import 'package:colorize/colorize.dart';
+import 'package:file/file.dart';
+import 'package:git/git.dart';
+import 'package:path/path.dart' as p;
+
+import 'core.dart';
+import 'plugin_command.dart';
+import 'process_runner.dart';
+
+/// An abstract base class for a command that iterates over a set of packages
+/// controlled by a standard set of flags, running some actions on each package,
+/// and collecting and reporting the success/failure of those actions.
+abstract class PackageLoopingCommand extends PluginCommand {
+  /// Creates a command to operate on [packagesDir] with the given environment.
+  PackageLoopingCommand(
+    Directory packagesDir, {
+    ProcessRunner processRunner = const ProcessRunner(),
+    GitDir? gitDir,
+  }) : super(packagesDir, processRunner: processRunner, gitDir: gitDir);
+
+  /// 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
+  /// arguments are invalid), and to set up any run-level state.
+  Future<void> initializeRun() async {}
+
+  /// Runs the command for [package], returning a list of errors.
+  ///
+  /// Errors may either be an empty string if there is no context that should
+  /// be included in the final error summary (e.g., a command that only has a
+  /// single failure mode), or strings that should be listed for that package
+  /// in the final summary. An empty list indicates success.
+  Future<List<String>> runForPackage(Directory package);
+
+  /// Whether or not the output (if any) of [runForPackage] is long, or short.
+  ///
+  /// This changes the logging that happens at the start of each package's
+  /// run; long output gets a banner-style message to make it easier to find,
+  /// while short output gets a single-line entry.
+  ///
+  /// When this is false, runForPackage output should be indented if possible,
+  /// to make the output structure easier to follow.
+  bool get hasLongOutput => true;
+
+  /// Whether to loop over all packages (e.g., including example/), rather than
+  /// only top-level packages.
+  bool get includeSubpackages => false;
+
+  /// The text to output at the start when reporting one or more failures.
+  /// This will be followed by a list of packages that reported errors, with
+  /// the per-package details if any.
+  ///
+  /// This only needs to be overridden if the summary should provide extra
+  /// context.
+  String get failureListHeader => 'The following packages had errors:';
+
+  /// The text to output at the end when reporting one or more failures. This
+  /// will be printed immediately after the a list of packages that reported
+  /// errors.
+  ///
+  /// This only needs to be overridden if the summary should provide extra
+  /// context.
+  String get failureListFooter => 'See above for full details.';
+
+  // ----------------------------------------
+
+  /// A convenience constant for [runForPackage] success that's more
+  /// self-documenting than the value.
+  static const List<String> success = <String>[];
+
+  /// A convenience constant for [runForPackage] failure without additional
+  /// context that's more self-documenting than the value.
+  static const List<String> failure = <String>[''];
+
+  /// Prints a message using a standard format indicating that the package was
+  /// skipped, with an explanation of why.
+  void printSkip(String reason) {
+    print(Colorize('SKIPPING: $reason')..darkGray());
+  }
+
+  /// Returns the identifying name to use for [package].
+  ///
+  /// Implementations should not expect a specific format for this string, since
+  /// it uses heuristics to try to be precise without being overly verbose. If
+  /// an exact format (e.g., published name, or basename) is required, that
+  /// should be used instead.
+  String getPackageDescription(Directory package) {
+    String packageName = p.relative(package.path, from: packagesDir.path);
+    final List<String> components = p.split(packageName);
+    // For the common federated plugin pattern of `foo/foo_subpackage`, drop
+    // the first part since it's not useful.
+    if (components.length == 2 &&
+        components[1].startsWith('${components[0]}_')) {
+      packageName = components[1];
+    }
+    return packageName;
+  }
+
+  // ----------------------------------------
+
+  @override
+  Future<void> run() async {
+    await initializeRun();
+
+    final List<Directory> packages = includeSubpackages
+        ? await getPackages().toList()
+        : await getPlugins().toList();
+
+    final Map<Directory, List<String>> results = <Directory, List<String>>{};
+    for (final Directory package in packages) {
+      _printPackageHeading(package);
+      results[package] = await runForPackage(package);
+    }
+
+    // If there were any errors reported, summarize them and exit.
+    if (results.values.any((List<String> failures) => failures.isNotEmpty)) {
+      const String indentation = '  ';
+      printError(failureListHeader);
+      for (final Directory package in packages) {
+        final List<String> errors = results[package]!;
+        if (errors.isNotEmpty) {
+          final String errorIndentation = indentation * 2;
+          String errorDetails = errors.join('\n$errorIndentation');
+          if (errorDetails.isNotEmpty) {
+            errorDetails = ':\n$errorIndentation$errorDetails';
+          }
+          printError(
+              '$indentation${getPackageDescription(package)}$errorDetails');
+        }
+      }
+      printError(failureListFooter);
+      throw ToolExit(exitCommandFoundErrors);
+    }
+
+    printSuccess('\n\nNo issues found!');
+  }
+
+  /// Prints the status message indicating that the command is being run for
+  /// [package].
+  ///
+  /// 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)}';
+    if (hasLongOutput) {
+      heading = '''
+
+============================================================
+|| $heading
+============================================================
+''';
+    } else {
+      heading = '$heading...';
+    }
+    print(Colorize(heading)..cyan());
+  }
+}

script/tool/lib/src/common/plugin_command.dart

@@ -14,6 +14,7 @@ import 'git_version_finder.dart';
 import 'process_runner.dart';
 
 /// Interface definition for all commands in this tool.
+// TODO(stuartmorgan): Move most of this logic to PackageLoopingCommand.
 abstract class PluginCommand extends Command<void> {
   /// Creates a command to operate on [packagesDir] with the given environment.
   PluginCommand(
@@ -136,6 +137,8 @@ abstract class PluginCommand extends Command<void> {
 
   /// 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<Directory> getPlugins() async* {
     // To avoid assuming consistency of `Directory.list` across command
     // invocations, we collect and sort the plugin folders before sharding.

script/tool/lib/src/pubspec_check_command.dart

@@ -4,11 +4,9 @@
 
 import 'package:file/file.dart';
 import 'package:git/git.dart';
-import 'package:path/path.dart' as p;
 import 'package:pubspec_parse/pubspec_parse.dart';
 
-import 'common/core.dart';
-import 'common/plugin_command.dart';
+import 'common/package_looping_command.dart';
 import 'common/process_runner.dart';
 
 /// A command to enforce pubspec conventions across the repository.
@@ -16,7 +14,7 @@ import 'common/process_runner.dart';
 /// This both ensures that repo best practices for which optional fields are
 /// used are followed, and that the structure is consistent to make edits
 /// across multiple pubspec files easier.
-class PubspecCheckCommand extends PluginCommand {
+class PubspecCheckCommand extends PackageLoopingCommand {
   /// Creates an instance of the version check command.
   PubspecCheckCommand(
     Directory packagesDir, {
@@ -52,29 +50,20 @@ class PubspecCheckCommand extends PluginCommand {
       'Checks that pubspecs follow repository conventions.';
 
   @override
-  Future<void> run() async {
-    final List<String> failingPackages = <String>[];
-    await for (final Directory package in getPackages()) {
-      final String relativePackagePath =
-          p.relative(package.path, from: packagesDir.path);
-      print('Checking $relativePackagePath...');
-      final File pubspec = package.childFile('pubspec.yaml');
-      final bool passesCheck = !pubspec.existsSync() ||
-          await _checkPubspec(pubspec, packageName: package.basename);
-      if (!passesCheck) {
-        failingPackages.add(relativePackagePath);
-      }
-    }
+  bool get hasLongOutput => false;
 
-    if (failingPackages.isNotEmpty) {
-      print('The following packages have pubspec issues:');
-      for (final String package in failingPackages) {
-        print('  $package');
-      }
-      throw ToolExit(1);
-    }
+  @override
+  bool get includeSubpackages => true;
 
-    print('\nNo pubspec issues found!');
+  @override
+  Future<List<String>> runForPackage(Directory package) async {
+    final File pubspec = package.childFile('pubspec.yaml');
+    final bool passesCheck = !pubspec.existsSync() ||
+        await _checkPubspec(pubspec, packageName: package.basename);
+    if (!passesCheck) {
+      return PackageLoopingCommand.failure;
+    }
+    return PackageLoopingCommand.success;
   }
 
   Future<bool> _checkPubspec(

script/tool/lib/src/xctest_command.dart

@@ -9,7 +9,7 @@ import 'package:file/file.dart';
 import 'package:path/path.dart' as p;
 
 import 'common/core.dart';
-import 'common/plugin_command.dart';
+import 'common/package_looping_command.dart';
 import 'common/plugin_utils.dart';
 import 'common/process_runner.dart';
 
@@ -19,12 +19,15 @@ const String _kXCRunCommand = 'xcrun';
 const String _kFoundNoSimulatorsMessage =
     'Cannot find any available simulators, tests failed';
 
+const int _exitFindingSimulatorsFailed = 3;
+const int _exitNoSimulators = 4;
+
 /// The command to run XCTests (XCUnitTest and XCUITest) in plugins.
 /// The tests target have to be added to the Xcode project of the example app,
 /// usually at "example/{ios,macos}/Runner.xcworkspace".
 ///
 /// The static analyzer is also run.
-class XCTestCommand extends PluginCommand {
+class XCTestCommand extends PackageLoopingCommand {
   /// Creates an instance of the test command.
   XCTestCommand(
     Directory packagesDir, {
@@ -41,6 +44,9 @@ class XCTestCommand extends PluginCommand {
     argParser.addFlag(kPlatformMacos, help: 'Runs the macOS tests');
   }
 
+  // The device destination flags for iOS tests.
+  List<String> _iosDestinationFlags = <String>[];
+
   @override
   final String name = 'xctest';
 
@@ -50,62 +56,53 @@ class XCTestCommand extends PluginCommand {
       'This command requires "flutter" and "xcrun" to be in your path.';
 
   @override
-  Future<void> run() async {
-    final bool testIos = getBoolArg(kPlatformIos);
-    final bool testMacos = getBoolArg(kPlatformMacos);
+  String get failureListHeader => 'The following packages are failing XCTests:';
 
-    if (!(testIos || testMacos)) {
-      print('At least one platform flag must be provided.');
-      throw ToolExit(2);
+  @override
+  Future<void> initializeRun() async {
+    final bool shouldTestIos = getBoolArg(kPlatformIos);
+    final bool shouldTestMacos = getBoolArg(kPlatformMacos);
+
+    if (!(shouldTestIos || shouldTestMacos)) {
+      printError('At least one platform flag must be provided.');
+      throw ToolExit(exitInvalidArguments);
     }
 
-    List<String> iosDestinationFlags = <String>[];
-    if (testIos) {
+    if (shouldTestIos) {
       String destination = getStringArg(_kiOSDestination);
       if (destination.isEmpty) {
         final String? simulatorId = await _findAvailableIphoneSimulator();
         if (simulatorId == null) {
-          print(_kFoundNoSimulatorsMessage);
-          throw ToolExit(1);
+          printError(_kFoundNoSimulatorsMessage);
+          throw ToolExit(_exitNoSimulators);
         }
         destination = 'id=$simulatorId';
       }
-      iosDestinationFlags = <String>[
+      _iosDestinationFlags = <String>[
         '-destination',
         destination,
       ];
     }
+  }
 
-    final List<String> failingPackages = <String>[];
-    await for (final Directory plugin in getPlugins()) {
-      final String packageName =
-          p.relative(plugin.path, from: packagesDir.path);
-      print('============================================================');
-      print('Start running for $packageName...');
-      bool passed = true;
-      if (testIos) {
-        passed &= await _testPlugin(plugin, 'iOS',
-            extraXcrunFlags: iosDestinationFlags);
-      }
-      if (testMacos) {
-        passed &= await _testPlugin(plugin, 'macOS');
-      }
-      if (!passed) {
-        failingPackages.add(packageName);
-      }
-    }
+  @override
+  Future<List<String>> runForPackage(Directory package) async {
+    final List<String> failures = <String>[];
+    final bool testIos = getBoolArg(kPlatformIos);
+    final bool testMacos = getBoolArg(kPlatformMacos);
+    // Only provide the failing platform(s) in the summary if testing multiple
+    // platforms, otherwise it's just noise.
+    final bool provideErrorDetails = testIos && testMacos;
 
-    // Command end, print reports.
-    if (failingPackages.isEmpty) {
-      print('All XCTests have passed!');
-    } else {
-      print(
-          'The following packages are failing XCTests (see above for details):');
-      for (final String package in failingPackages) {
-        print(' * $package');
-      }
-      throw ToolExit(1);
+    if (testIos &&
+        !await _testPlugin(package, 'iOS',
+            extraXcrunFlags: _iosDestinationFlags)) {
+      failures.add(provideErrorDetails ? 'iOS' : '');
     }
+    if (testMacos && !await _testPlugin(package, 'macOS')) {
+      failures.add(provideErrorDetails ? 'macOS' : '');
+    }
+    return failures;
   }
 
   /// Runs all applicable tests for [plugin], printing status and returning
@@ -117,8 +114,7 @@ class XCTestCommand extends PluginCommand {
   }) async {
     if (!pluginSupportsPlatform(platform.toLowerCase(), plugin,
         requiredMode: PlatformSupport.inline)) {
-      print('$platform is not implemented by this plugin package.');
-      print('\n');
+      printSkip('$platform is not implemented by this plugin package.\n');
       return true;
     }
     bool passing = true;
@@ -136,7 +132,7 @@ class XCTestCommand extends PluginCommand {
             extraFlags: extraXcrunFlags);
       }
       if (exitCode == 0) {
-        print('Successfully ran $platform xctest for $examplePath');
+        printSuccess('Successfully ran $platform xctest for $examplePath');
       } else {
         passing = false;
       }
@@ -184,9 +180,10 @@ class XCTestCommand extends PluginCommand {
     final io.ProcessResult findSimulatorsResult =
         await processRunner.run(_kXCRunCommand, findSimulatorsArguments);
     if (findSimulatorsResult.exitCode != 0) {
-      print('Error occurred while running "$findSimulatorCompleteCommand":\n'
+      printError(
+          'Error occurred while running "$findSimulatorCompleteCommand":\n'
           '${findSimulatorsResult.stderr}');
-      throw ToolExit(1);
+      throw ToolExit(_exitFindingSimulatorsFailed);
     }
     final Map<String, dynamic> simulatorListJson =
         jsonDecode(findSimulatorsResult.stdout as String)