[tool] Only run postsubmit on changed packages (#6516)

flutter · Sep 30, 2022 · e8f19ed · e8f19ed
1 parent 35b9755
commit e8f19ed
Show file tree

Hide file tree

Showing 5 changed files with 71 additions and 34 deletions.
diff --git a/script/tool/CHANGELOG.md b/script/tool/CHANGELOG.md
@@ -1,5 +1,9 @@
-## 0.11.1
+## 12.0
 
+* Changes the behavior of `--packages-for-branch` on main/master to run for
+  packages changed in the last commit, rather than running for all packages.
+  This allows CI to test the same filtered set of packages in post-submit as are
+  tested in presubmit.
 * Adds a `fix` command to run `dart fix --apply` in target packages.
 
 ## 0.11

diff --git a/script/tool/lib/src/common/git_version_finder.dart b/script/tool/lib/src/common/git_version_finder.dart
@@ -103,7 +103,7 @@ class GitVersionFinder {
         <String>['merge-base', '--fork-point', 'FETCH_HEAD', 'HEAD'],
         throwOnError: false);
     final String stdout = (baseShaFromMergeBase.stdout as String? ?? '').trim();
-    final String stderr = (baseShaFromMergeBase.stdout as String? ?? '').trim();
+    final String stderr = (baseShaFromMergeBase.stderr as String? ?? '').trim();
     if (stderr.isNotEmpty || stdout.isEmpty) {
       baseShaFromMergeBase = await baseGitDir
           .runCommand(<String>['merge-base', 'FETCH_HEAD', 'HEAD']);

diff --git a/script/tool/lib/src/common/package_command.dart b/script/tool/lib/src/common/package_command.dart
@@ -84,9 +84,8 @@ abstract class PackageCommand extends Command<void> {
             'Cannot be combined with $_packagesArg.\n',
         hide: true);
     argParser.addFlag(_packagesForBranchArg,
-        help:
-            'This runs on all packages (equivalent to no package selection flag)\n'
-            'on main (or master), and behaves like --run-on-changed-packages on '
+        help: 'This runs on all packages changed in the last commit on main '
+            '(or master), and behaves like --run-on-changed-packages on '
             'any other branch.\n\n'
             'Cannot be combined with $_packagesArg.\n\n'
             'This is intended for use in CI.\n',
@@ -311,34 +310,38 @@ abstract class PackageCommand extends Command<void> {
 
     Set<String> packages = Set<String>.from(getStringListArg(_packagesArg));
 
-    final bool runOnChangedPackages;
+    final GitVersionFinder? changedFileFinder;
     if (getBoolArg(_runOnChangedPackagesArg)) {
-      runOnChangedPackages = true;
+      changedFileFinder = await retrieveVersionFinder();
     } else if (getBoolArg(_packagesForBranchArg)) {
       final String? branch = await _getBranch();
       if (branch == null) {
         printError('Unabled to determine branch; --$_packagesForBranchArg can '
             'only be used in a git repository.');
         throw ToolExit(exitInvalidArguments);
       } else {
-        runOnChangedPackages = branch != 'master' && branch != 'main';
-        // Log the mode for auditing what was intended to run.
-        print('--$_packagesForBranchArg: running on '
-            '${runOnChangedPackages ? 'changed' : 'all'} packages');
+        // Configure the change finder the correct mode for the branch.
+        final bool lastCommitOnly = branch == 'main' || branch == 'master';
+        if (lastCommitOnly) {
+          // Log the mode to make it easier to audit logs to see that the
+          // intended diff was used.
+          print('--$_packagesForBranchArg: running on default branch; '
+              'using parent commit as the diff base.');
+          changedFileFinder = GitVersionFinder(await gitDir, 'HEAD~');
+        } else {
+          changedFileFinder = await retrieveVersionFinder();
+        }
       }
     } else {
-      runOnChangedPackages = false;
+      changedFileFinder = null;
     }
 
-    final Set<String> excludedPackageNames = getExcludedPackageNames();
-
-    if (runOnChangedPackages) {
-      final GitVersionFinder gitVersionFinder = await retrieveVersionFinder();
-      final String baseSha = await gitVersionFinder.getBaseSha();
+    if (changedFileFinder != null) {
+      final String baseSha = await changedFileFinder.getBaseSha();
       print(
-          'Running for all packages that have changed relative to "$baseSha"\n');
+          'Running for all packages that have diffs relative to "$baseSha"\n');
       final List<String> changedFiles =
-          await gitVersionFinder.getChangedFiles();
+          await changedFileFinder.getChangedFiles();
       if (!_changesRequireFullTest(changedFiles)) {
         packages = _getChangedPackageNames(changedFiles);
       }
@@ -362,6 +365,7 @@ abstract class PackageCommand extends Command<void> {
         .childDirectory('third_party')
         .childDirectory('packages');
 
+    final Set<String> excludedPackageNames = getExcludedPackageNames();
     for (final Directory dir in <Directory>[
       packagesDir,
       if (thirdPartyPackagesDirectory.existsSync()) thirdPartyPackagesDirectory,

diff --git a/script/tool/pubspec.yaml b/script/tool/pubspec.yaml
@@ -1,7 +1,7 @@
 name: flutter_plugin_tools
 description: Productivity utils for flutter/plugins and flutter/packages
 repository: https://github.com/flutter/plugins/tree/main/script/tool
-version: 0.11.0
+version: 0.12.0
 
 dependencies:
   args: ^2.1.0

diff --git a/script/tool/test/common/package_command_test.dart b/script/tool/test/common/package_command_test.dart
@@ -523,7 +523,7 @@ packages/plugin1/CHANGELOG
             output,
             containsAllInOrder(<Matcher>[
               contains(
-                  'Running for all packages that have changed relative to "main"'),
+                  'Running for all packages that have diffs relative to "main"'),
             ]));
 
         expect(command.plugins, unorderedEquals(<String>[plugin1.path]));
@@ -732,13 +732,17 @@ packages/b_package/lib/src/foo.dart
   });
 
   group('--packages-for-branch', () {
-    test('only tests changed packages on a branch', () async {
+    test('only tests changed packages relative to the merge base on a branch',
+        () async {
       processRunner.mockProcessesForExecutable['git-diff'] = <Process>[
         MockProcess(stdout: 'packages/plugin1/plugin1.dart'),
       ];
       processRunner.mockProcessesForExecutable['git-rev-parse'] = <Process>[
         MockProcess(stdout: 'a-branch'),
       ];
+      processRunner.mockProcessesForExecutable['git-merge-base'] = <Process>[
+        MockProcess(stdout: 'abc123'),
+      ];
       final RepositoryPackage plugin1 =
           createFakePlugin('plugin1', packagesDir);
       createFakePlugin('plugin2', packagesDir);
@@ -750,11 +754,20 @@ packages/b_package/lib/src/foo.dart
       expect(
           output,
           containsAllInOrder(<Matcher>[
-            contains('--packages-for-branch: running on changed packages'),
+            contains(
+                'Running for all packages that have diffs relative to "abc123"'),
           ]));
+      // Ensure that it's diffing against the merge-base.
+      expect(
+          processRunner.recordedCalls,
+          contains(
+            const ProcessCall(
+                'git-diff', <String>['--name-only', 'abc123', 'HEAD'], null),
+          ));
     });
 
-    test('tests all packages on main', () async {
+    test('only tests changed packages relative to the previous commit on main',
+        () async {
       processRunner.mockProcessesForExecutable['git-diff'] = <Process>[
         MockProcess(stdout: 'packages/plugin1/plugin1.dart'),
       ];
@@ -763,19 +776,27 @@ packages/b_package/lib/src/foo.dart
       ];
       final RepositoryPackage plugin1 =
           createFakePlugin('plugin1', packagesDir);
-      final RepositoryPackage plugin2 =
-          createFakePlugin('plugin2', packagesDir);
+      createFakePlugin('plugin2', packagesDir);
 
       final List<String> output = await runCapturingPrint(
           runner, <String>['sample', '--packages-for-branch']);
 
-      expect(command.plugins,
-          unorderedEquals(<String>[plugin1.path, plugin2.path]));
+      expect(command.plugins, unorderedEquals(<String>[plugin1.path]));
       expect(
           output,
           containsAllInOrder(<Matcher>[
-            contains('--packages-for-branch: running on all packages'),
+            contains('--packages-for-branch: running on default branch; '
+                'using parent commit as the diff base'),
+            contains(
+                'Running for all packages that have diffs relative to "HEAD~"'),
           ]));
+      // Ensure that it's diffing against the prior commit.
+      expect(
+          processRunner.recordedCalls,
+          contains(
+            const ProcessCall(
+                'git-diff', <String>['--name-only', 'HEAD~', 'HEAD'], null),
+          ));
     });
 
     test('tests all packages on master', () async {
@@ -787,19 +808,27 @@ packages/b_package/lib/src/foo.dart
       ];
       final RepositoryPackage plugin1 =
           createFakePlugin('plugin1', packagesDir);
-      final RepositoryPackage plugin2 =
-          createFakePlugin('plugin2', packagesDir);
+      createFakePlugin('plugin2', packagesDir);
 
       final List<String> output = await runCapturingPrint(
           runner, <String>['sample', '--packages-for-branch']);
 
-      expect(command.plugins,
-          unorderedEquals(<String>[plugin1.path, plugin2.path]));
+      expect(command.plugins, unorderedEquals(<String>[plugin1.path]));
       expect(
           output,
           containsAllInOrder(<Matcher>[
-            contains('--packages-for-branch: running on all packages'),
+            contains('--packages-for-branch: running on default branch; '
+                'using parent commit as the diff base'),
+            contains(
+                'Running for all packages that have diffs relative to "HEAD~"'),
           ]));
+      // Ensure that it's diffing against the prior commit.
+      expect(
+          processRunner.recordedCalls,
+          contains(
+            const ProcessCall(
+                'git-diff', <String>['--name-only', 'HEAD~', 'HEAD'], null),
+          ));
     });
 
     test('throws if getting the branch fails', () async {