diff --git a/script/tool/CHANGELOG.md b/script/tool/CHANGELOG.md index bf549d4e79..f62509040a 100644 --- 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 index eb8fba6b76..b135424827 100644 --- 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 { ['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(['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 index 60d7ecc800..1d82c4e936 100644 --- 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 { '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,9 +310,9 @@ abstract class PackageCommand extends Command { Set packages = Set.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) { @@ -321,24 +320,28 @@ abstract class PackageCommand extends Command { '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 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 changedFiles = - await gitVersionFinder.getChangedFiles(); + await changedFileFinder.getChangedFiles(); if (!_changesRequireFullTest(changedFiles)) { packages = _getChangedPackageNames(changedFiles); } @@ -362,6 +365,7 @@ abstract class PackageCommand extends Command { .childDirectory('third_party') .childDirectory('packages'); + final Set excludedPackageNames = getExcludedPackageNames(); for (final Directory dir in [ packagesDir, if (thirdPartyPackagesDirectory.existsSync()) thirdPartyPackagesDirectory, diff --git a/script/tool/pubspec.yaml b/script/tool/pubspec.yaml index 246c2ade25..9e767ad724 100644 --- 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 index c3d1ee343f..be21b86bff 100644 --- 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([ 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([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'] = [ MockProcess(stdout: 'packages/plugin1/plugin1.dart'), ]; processRunner.mockProcessesForExecutable['git-rev-parse'] = [ MockProcess(stdout: 'a-branch'), ]; + processRunner.mockProcessesForExecutable['git-merge-base'] = [ + 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([ - 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', ['--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'] = [ 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 output = await runCapturingPrint( runner, ['sample', '--packages-for-branch']); - expect(command.plugins, - unorderedEquals([plugin1.path, plugin2.path])); + expect(command.plugins, unorderedEquals([plugin1.path])); expect( output, containsAllInOrder([ - 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', ['--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 output = await runCapturingPrint( runner, ['sample', '--packages-for-branch']); - expect(command.plugins, - unorderedEquals([plugin1.path, plugin2.path])); + expect(command.plugins, unorderedEquals([plugin1.path])); expect( output, containsAllInOrder([ - 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', ['--name-only', 'HEAD~', 'HEAD'], null), + )); }); test('throws if getting the branch fails', () async {