[flutter_plugin_tools] Improve and test 'format' (#4145)

- Adds unit tests, as there are currently none.
- Adds more graceful failure handling.
- Adds an internal ignore list to skip files that don't need to be
  formatted that showed up during local testing.
- Adds a note explaining that it's intentially not using the new base
  command due to performance issues.

script/tool/lib/src/format_command.dart

@@ -14,6 +14,11 @@ import 'common/core.dart';
 import 'common/plugin_command.dart';
 import 'common/process_runner.dart';
 
+const int _exitClangFormatFailed = 3;
+const int _exitFlutterFormatFailed = 4;
+const int _exitJavaFormatFailed = 5;
+const int _exitGitFailed = 6;
+
 final Uri _googleFormatterUrl = Uri.https('github.com',
     '/google/google-java-format/releases/download/google-java-format-1.3/google-java-format-1.3-all-deps.jar');
 
@@ -43,14 +48,18 @@ class FormatCommand extends PluginCommand {
   Future<void> run() async {
     final String googleFormatterPath = await _getGoogleFormatterPath();
 
-    await _formatDart();
+    // This class is not based on PackageLoopingCommand because running the
-    await _formatJava(googleFormatterPath);
+    // formatters separately for each package is an order of magnitude slower,
-    await _formatCppAndObjectiveC();
+    // due to the startup overhead of the formatters.
+    final Iterable<String> files = await _getFilteredFilePaths(getFiles());
+    await _formatDart(files);
+    await _formatJava(files, googleFormatterPath);
+    await _formatCppAndObjectiveC(files);
 
     if (getBoolArg('fail-on-change')) {
       final bool modified = await _didModifyAnything();
       if (modified) {
-        throw ToolExit(1);
+        throw ToolExit(exitCommandFoundErrors);
       }
     }
   }
@@ -60,9 +69,12 @@ class FormatCommand extends PluginCommand {
       'git',
       <String>['ls-files', '--modified'],
       workingDir: packagesDir,
-      exitOnError: true,
       logOnError: true,
     );
+    if (modifiedFiles.exitCode != 0) {
+      printError('Unable to determine changed files.');
+      throw ToolExit(_exitGitFailed);
+    }
 
     print('\n\n');
 
@@ -79,66 +91,105 @@ class FormatCommand extends PluginCommand {
         'pub global run flutter_plugin_tools format" or copy-paste '
         'this command into your terminal:');
 
-    print('patch -p1 <<DONE');
     final io.ProcessResult diff = await processRunner.run(
       'git',
       <String>['diff'],
       workingDir: packagesDir,
-      exitOnError: true,
       logOnError: true,
     );
+    if (diff.exitCode != 0) {
+      printError('Unable to determine diff.');
+      throw ToolExit(_exitGitFailed);
+    }
+    print('patch -p1 <<DONE');
     print(diff.stdout);
     print('DONE');
     return true;
   }
 
-  Future<void> _formatCppAndObjectiveC() async {
+  Future<void> _formatCppAndObjectiveC(Iterable<String> files) async {
-    print('Formatting all .cc, .cpp, .mm, .m, and .h files...');
+    final Iterable<String> clangFiles = _getPathsWithExtensions(
-    final Iterable<String> allFiles = <String>[
+        files, <String>{'.h', '.m', '.mm', '.cc', '.cpp'});
-      ...await _getFilesWithExtension('.h'),
+    if (clangFiles.isNotEmpty) {
-      ...await _getFilesWithExtension('.m'),
+      print('Formatting .cc, .cpp, .h, .m, and .mm files...');
-      ...await _getFilesWithExtension('.mm'),
+      final Iterable<List<String>> batches = partition(clangFiles, 100);
-      ...await _getFilesWithExtension('.cc'),
+      int exitCode = 0;
-      ...await _getFilesWithExtension('.cpp'),
+      for (final List<String> batch in batches) {
-    ];
+        batch.sort(); // For ease of testing; partition changes the order.
-    // Split this into multiple invocations to avoid a
+        exitCode = await processRunner.runAndStream(
-    // 'ProcessException: Argument list too long'.
+            getStringArg('clang-format'),
-    final Iterable<List<String>> batches = partition(allFiles, 100);
+            <String>['-i', '--style=Google', ...batch],
-    for (final List<String> batch in batches) {
+            workingDir: packagesDir);
-      await processRunner.runAndStream(getStringArg('clang-format'),
+        if (exitCode != 0) {
-          <String>['-i', '--style=Google', ...batch],
+          break;
-          workingDir: packagesDir, exitOnError: true);
+        }
+      }
+      if (exitCode != 0) {
+        printError(
+            'Failed to format C, C++, and Objective-C files: exit code $exitCode.');
+        throw ToolExit(_exitClangFormatFailed);
+      }
     }
   }
 
-  Future<void> _formatJava(String googleFormatterPath) async {
+  Future<void> _formatJava(
-    print('Formatting all .java files...');
+      Iterable<String> files, String googleFormatterPath) async {
-    final Iterable<String> javaFiles = await _getFilesWithExtension('.java');
+    final Iterable<String> javaFiles =
-    await processRunner.runAndStream('java',
+        _getPathsWithExtensions(files, <String>{'.java'});
-        <String>['-jar', googleFormatterPath, '--replace', ...javaFiles],
+    if (javaFiles.isNotEmpty) {
-        workingDir: packagesDir, exitOnError: true);
+      print('Formatting .java files...');
+      final int exitCode = await processRunner.runAndStream('java',
+          <String>['-jar', googleFormatterPath, '--replace', ...javaFiles],
+          workingDir: packagesDir);
+      if (exitCode != 0) {
+        printError('Failed to format Java files: exit code $exitCode.');
+        throw ToolExit(_exitJavaFormatFailed);
+      }
+    }
   }
 
-  Future<void> _formatDart() async {
+  Future<void> _formatDart(Iterable<String> files) async {
-    // This actually should be fine for non-Flutter Dart projects, no need to
+    final Iterable<String> dartFiles =
-    // specifically shell out to dartfmt -w in that case.
+        _getPathsWithExtensions(files, <String>{'.dart'});
-    print('Formatting all .dart files...');
+    if (dartFiles.isNotEmpty) {
-    final Iterable<String> dartFiles = await _getFilesWithExtension('.dart');
+      print('Formatting .dart files...');
-    if (dartFiles.isEmpty) {
+      // `flutter format` doesn't require the project to actually be a Flutter
-      print(
+      // project.
-          'No .dart files to format. If you set the `--exclude` flag, most likey they were skipped');
+      final int exitCode = await processRunner.runAndStream(
-    } else {
-      await processRunner.runAndStream(
           'flutter', <String>['format', ...dartFiles],
-          workingDir: packagesDir, exitOnError: true);
+          workingDir: packagesDir);
+      if (exitCode != 0) {
+        printError('Failed to format Dart files: exit code $exitCode.');
+        throw ToolExit(_exitFlutterFormatFailed);
+      }
     }
   }
 
-  Future<List<String>> _getFilesWithExtension(String extension) async =>
+  Future<Iterable<String>> _getFilteredFilePaths(Stream<File> files) async {
-      getFiles()
+    // Returns a pattern to check for [directories] as a subset of a file path.
-          .where((File file) => p.extension(file.path) == extension)
+    RegExp pathFragmentForDirectories(List<String> directories) {
-          .map((File file) => file.path)
+      final String s = p.separator;
-          .toList();
+      return RegExp('(?:^|$s)${p.joinAll(directories)}$s');
+    }
+
+    return files
+        .map((File file) => file.path)
+        .where((String path) =>
+            // Ignore files in build/ directories (e.g., headers of frameworks)
+            // to avoid useless extra work in local repositories.
+            !path.contains(
+                pathFragmentForDirectories(<String>['example', 'build'])) &&
+            // Ignore files in Pods, which are not part of the repository.
+            !path.contains(pathFragmentForDirectories(<String>['Pods'])) &&
+            // Ignore .dart_tool/, which can have various intermediate files.
+            !path.contains(pathFragmentForDirectories(<String>['.dart_tool'])))
+        .toList();
+  }
+
+  Iterable<String> _getPathsWithExtensions(
+      Iterable<String> files, Set<String> extensions) {
+    return files.where((String path) => extensions.contains(p.extension(path)));
+  }
 
   Future<String> _getGoogleFormatterPath() async {
     final String javaFormatterPath = p.join(

script/tool/test/format_command_test.dart

@@ -0,0 +1,344 @@
+// 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 'dart:io' as io;
+
+import 'package:args/command_runner.dart';
+import 'package:file/file.dart';
+import 'package:file/memory.dart';
+import 'package:flutter_plugin_tools/src/common/core.dart';
+import 'package:flutter_plugin_tools/src/format_command.dart';
+import 'package:path/path.dart' as p;
+import 'package:test/test.dart';
+
+import 'mocks.dart';
+import 'util.dart';
+
+void main() {
+  late FileSystem fileSystem;
+  late Directory packagesDir;
+  late RecordingProcessRunner processRunner;
+  late CommandRunner<void> runner;
+  late String javaFormatPath;
+
+  setUp(() {
+    fileSystem = MemoryFileSystem();
+    packagesDir = createPackagesDirectory(fileSystem: fileSystem);
+    processRunner = RecordingProcessRunner();
+    final FormatCommand analyzeCommand =
+        FormatCommand(packagesDir, processRunner: processRunner);
+
+    // Create the java formatter file that the command checks for, to avoid
+    // a download.
+    javaFormatPath = p.join(p.dirname(p.fromUri(io.Platform.script)),
+        'google-java-format-1.3-all-deps.jar');
+    fileSystem.file(javaFormatPath).createSync(recursive: true);
+
+    runner = CommandRunner<void>('format_command', 'Test for format_command');
+    runner.addCommand(analyzeCommand);
+  });
+
+  List<String> _getAbsolutePaths(
+      Directory package, List<String> relativePaths) {
+    return relativePaths
+        .map((String path) => p.join(package.path, path))
+        .toList();
+  }
+
+  test('formats .dart files', () async {
+    const List<String> files = <String>[
+      'lib/a.dart',
+      'lib/src/b.dart',
+      'lib/src/c.dart',
+    ];
+    final Directory pluginDir = createFakePlugin(
+      'a_plugin',
+      packagesDir,
+      extraFiles: files,
+    );
+
+    await runCapturingPrint(runner, <String>['format']);
+
+    expect(
+        processRunner.recordedCalls,
+        orderedEquals(<ProcessCall>[
+          ProcessCall(
+              'flutter',
+              <String>['format', ..._getAbsolutePaths(pluginDir, files)],
+              packagesDir.path),
+        ]));
+  });
+
+  test('fails if flutter format fails', () async {
+    const List<String> files = <String>[
+      'lib/a.dart',
+      'lib/src/b.dart',
+      'lib/src/c.dart',
+    ];
+    createFakePlugin('a_plugin', packagesDir, extraFiles: files);
+
+    processRunner.mockProcessesForExecutable['flutter'] = <io.Process>[
+      MockProcess.failing()
+    ];
+    Error? commandError;
+    final List<String> output = await runCapturingPrint(
+        runner, <String>['format'], errorHandler: (Error e) {
+      commandError = e;
+    });
+
+    expect(commandError, isA<ToolExit>());
+    expect(
+        output,
+        containsAllInOrder(<Matcher>[
+          contains('Failed to format Dart files: exit code 1.'),
+        ]));
+  });
+
+  test('formats .java files', () async {
+    const List<String> files = <String>[
+      'android/src/main/java/io/flutter/plugins/a_plugin/a.java',
+      'android/src/main/java/io/flutter/plugins/a_plugin/b.java',
+    ];
+    final Directory pluginDir = createFakePlugin(
+      'a_plugin',
+      packagesDir,
+      extraFiles: files,
+    );
+
+    await runCapturingPrint(runner, <String>['format']);
+
+    expect(
+        processRunner.recordedCalls,
+        orderedEquals(<ProcessCall>[
+          ProcessCall(
+              'java',
+              <String>[
+                '-jar',
+                javaFormatPath,
+                '--replace',
+                ..._getAbsolutePaths(pluginDir, files)
+              ],
+              packagesDir.path),
+        ]));
+  });
+
+  test('fails if Java formatter fails', () async {
+    const List<String> files = <String>[
+      'android/src/main/java/io/flutter/plugins/a_plugin/a.java',
+      'android/src/main/java/io/flutter/plugins/a_plugin/b.java',
+    ];
+    createFakePlugin('a_plugin', packagesDir, extraFiles: files);
+
+    processRunner.mockProcessesForExecutable['java'] = <io.Process>[
+      MockProcess.failing()
+    ];
+    Error? commandError;
+    final List<String> output = await runCapturingPrint(
+        runner, <String>['format'], errorHandler: (Error e) {
+      commandError = e;
+    });
+
+    expect(commandError, isA<ToolExit>());
+    expect(
+        output,
+        containsAllInOrder(<Matcher>[
+          contains('Failed to format Java files: exit code 1.'),
+        ]));
+  });
+
+  test('formats c-ish files', () async {
+    const List<String> files = <String>[
+      'ios/Classes/Foo.h',
+      'ios/Classes/Foo.m',
+      'linux/foo_plugin.cc',
+      'macos/Classes/Foo.h',
+      'macos/Classes/Foo.mm',
+      'windows/foo_plugin.cpp',
+    ];
+    final Directory pluginDir = createFakePlugin(
+      'a_plugin',
+      packagesDir,
+      extraFiles: files,
+    );
+
+    await runCapturingPrint(runner, <String>['format']);
+
+    expect(
+        processRunner.recordedCalls,
+        orderedEquals(<ProcessCall>[
+          ProcessCall(
+              'clang-format',
+              <String>[
+                '-i',
+                '--style=Google',
+                ..._getAbsolutePaths(pluginDir, files)
+              ],
+              packagesDir.path),
+        ]));
+  });
+
+  test('fails if clang-format fails', () async {
+    const List<String> files = <String>[
+      'linux/foo_plugin.cc',
+      'macos/Classes/Foo.h',
+    ];
+    createFakePlugin('a_plugin', packagesDir, extraFiles: files);
+
+    processRunner.mockProcessesForExecutable['clang-format'] = <io.Process>[
+      MockProcess.failing()
+    ];
+    Error? commandError;
+    final List<String> output = await runCapturingPrint(
+        runner, <String>['format'], errorHandler: (Error e) {
+      commandError = e;
+    });
+
+    expect(commandError, isA<ToolExit>());
+    expect(
+        output,
+        containsAllInOrder(<Matcher>[
+          contains(
+              'Failed to format C, C++, and Objective-C files: exit code 1.'),
+        ]));
+  });
+
+  test('skips known non-repo files', () async {
+    const List<String> skipFiles = <String>[
+      '/example/build/SomeFramework.framework/Headers/SomeFramework.h',
+      '/example/Pods/APod.framework/Headers/APod.h',
+      '.dart_tool/internals/foo.cc',
+      '.dart_tool/internals/Bar.java',
+      '.dart_tool/internals/baz.dart',
+    ];
+    const List<String> clangFiles = <String>['ios/Classes/Foo.h'];
+    const List<String> dartFiles = <String>['lib/a.dart'];
+    const List<String> javaFiles = <String>[
+      'android/src/main/java/io/flutter/plugins/a_plugin/a.java'
+    ];
+    final Directory pluginDir = createFakePlugin(
+      'a_plugin',
+      packagesDir,
+      extraFiles: <String>[
+        ...skipFiles,
+        // Include some files that should be formatted to validate that it's
+        // correctly filtering even when running the commands.
+        ...clangFiles,
+        ...dartFiles,
+        ...javaFiles,
+      ],
+    );
+
+    await runCapturingPrint(runner, <String>['format']);
+
+    expect(
+        processRunner.recordedCalls,
+        containsAll(<ProcessCall>[
+          ProcessCall(
+              'clang-format',
+              <String>[
+                '-i',
+                '--style=Google',
+                ..._getAbsolutePaths(pluginDir, clangFiles)
+              ],
+              packagesDir.path),
+          ProcessCall(
+              'flutter',
+              <String>['format', ..._getAbsolutePaths(pluginDir, dartFiles)],
+              packagesDir.path),
+          ProcessCall(
+              'java',
+              <String>[
+                '-jar',
+                javaFormatPath,
+                '--replace',
+                ..._getAbsolutePaths(pluginDir, javaFiles)
+              ],
+              packagesDir.path),
+        ]));
+  });
+
+  test('fails if files are changed with --file-on-change', () async {
+    const List<String> files = <String>[
+      'linux/foo_plugin.cc',
+      'macos/Classes/Foo.h',
+    ];
+    createFakePlugin('a_plugin', packagesDir, extraFiles: files);
+
+    processRunner.mockProcessesForExecutable['git'] = <io.Process>[
+      MockProcess.succeeding(),
+    ];
+    const String changedFilePath = 'packages/a_plugin/linux/foo_plugin.cc';
+    processRunner.resultStdout = changedFilePath;
+    Error? commandError;
+    final List<String> output =
+        await runCapturingPrint(runner, <String>['format', '--fail-on-change'],
+            errorHandler: (Error e) {
+      commandError = e;
+    });
+
+    expect(commandError, isA<ToolExit>());
+    expect(
+        output,
+        containsAllInOrder(<Matcher>[
+          contains('These files are not formatted correctly'),
+          contains(changedFilePath),
+          contains('patch -p1 <<DONE'),
+        ]));
+  });
+
+  test('fails if git ls-files fails', () async {
+    const List<String> files = <String>[
+      'linux/foo_plugin.cc',
+      'macos/Classes/Foo.h',
+    ];
+    createFakePlugin('a_plugin', packagesDir, extraFiles: files);
+
+    processRunner.mockProcessesForExecutable['git'] = <io.Process>[
+      MockProcess.failing()
+    ];
+    Error? commandError;
+    final List<String> output =
+        await runCapturingPrint(runner, <String>['format', '--fail-on-change'],
+            errorHandler: (Error e) {
+      commandError = e;
+    });
+
+    expect(commandError, isA<ToolExit>());
+    expect(
+        output,
+        containsAllInOrder(<Matcher>[
+          contains('Unable to determine changed files.'),
+        ]));
+  });
+
+  test('reports git diff failures', () async {
+    const List<String> files = <String>[
+      'linux/foo_plugin.cc',
+      'macos/Classes/Foo.h',
+    ];
+    createFakePlugin('a_plugin', packagesDir, extraFiles: files);
+
+    processRunner.mockProcessesForExecutable['git'] = <io.Process>[
+      MockProcess.succeeding(), // ls-files
+      MockProcess.failing(), // diff
+    ];
+    const String changedFilePath = 'packages/a_plugin/linux/foo_plugin.cc';
+    processRunner.resultStdout = changedFilePath;
+    Error? commandError;
+    final List<String> output =
+        await runCapturingPrint(runner, <String>['format', '--fail-on-change'],
+            errorHandler: (Error e) {
+      commandError = e;
+    });
+
+    expect(commandError, isA<ToolExit>());
+    expect(
+        output,
+        containsAllInOrder(<Matcher>[
+          contains('These files are not formatted correctly'),
+          contains(changedFilePath),
+          contains('Unable to determine diff.'),
+        ]));
+  });
+}