From 257edc918795e4a505dbf91885851fc0c5b8fd9d Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Tue, 4 Jun 2024 11:50:15 -0400 Subject: [PATCH] [tool] Provide better CI feedback for combo PRs Currently if a PR follows the recommended combo PR process for a federated plugin, the main PR will have CI errors that say the PR isn't allowed to do what it is doing, which is confusing, especially to new contributors or reviewers. This updates the tooling to detect the temporary overrides created by the tooling, and uses that to trigger a different error message that explains that the error is expected, and exists only to prevent accidental landing. --- script/tool/lib/src/common/core.dart | 4 + .../src/federation_safety_check_command.dart | 21 +++++ .../lib/src/make_deps_path_based_command.dart | 2 +- .../federation_safety_check_command_test.dart | 83 +++++++++++++++++++ 4 files changed, 109 insertions(+), 1 deletion(-) diff --git a/script/tool/lib/src/common/core.dart b/script/tool/lib/src/common/core.dart index 476106c924ae..63208d23f7e4 100644 --- a/script/tool/lib/src/common/core.dart +++ b/script/tool/lib/src/common/core.dart @@ -33,6 +33,10 @@ const String platformWindows = 'windows'; /// Key for enable experiment. const String kEnableExperiment = 'enable-experiment'; +/// A String to add to comments on temporarily-added changes that should not +/// land (e.g., dependency overrides in federated plugin combination PRs). +const String kDoNotLandWarning = 'DO NOT MERGE'; + /// Target platforms supported by Flutter. // ignore: public_member_api_docs enum FlutterPlatform { android, ios, linux, macos, web, windows } diff --git a/script/tool/lib/src/federation_safety_check_command.dart b/script/tool/lib/src/federation_safety_check_command.dart index 804ef8e3abc9..477fe4a2f61c 100644 --- a/script/tool/lib/src/federation_safety_check_command.dart +++ b/script/tool/lib/src/federation_safety_check_command.dart @@ -6,6 +6,7 @@ import 'package:file/file.dart'; import 'package:path/path.dart' as p; import 'package:pub_semver/pub_semver.dart'; +import 'common/core.dart'; import 'common/file_utils.dart'; import 'common/git_version_finder.dart'; import 'common/output_utils.dart'; @@ -112,6 +113,21 @@ class FederationSafetyCheckCommand extends PackageLoopingCommand { 'Platform interface changes are not validated.'); } + // Special-case combination PRs that are following repo process, so that + // they don't get an error that makes it sound like something is wrong with + // the PR (but is still an error so that the PR can't land without following + // the resolution process). + if (package.getExamples().any(_hasTemporaryDependencyOverrides)) { + printError('"$kDoNotLandWarning" found in pubspec.yaml, so this is ' + 'assumed to be the initial combination PR for a federated change, ' + 'following the standard repository procedure. This failure is ' + 'expected, in order to prevent accidentally landing the temporary ' + 'overrides, and will automatically be resolved when the temporary ' + 'overrides are replaced by dependency version bumps later in the ' + 'process.'); + return PackageResult.fail(['Unresolved combo PR.']); + } + // Uses basename to match _changedPackageFiles. final String basePackageName = package.directory.parent.basename; final String platformInterfacePackageName = @@ -216,4 +232,9 @@ class FederationSafetyCheckCommand extends PackageLoopingCommand { // against having the wrong (e.g., incorrectly empty) diff output. return foundComment; } + + bool _hasTemporaryDependencyOverrides(RepositoryPackage package) { + final String pubspecContents = package.pubspecFile.readAsStringSync(); + return pubspecContents.contains(kDoNotLandWarning); + } } diff --git a/script/tool/lib/src/make_deps_path_based_command.dart b/script/tool/lib/src/make_deps_path_based_command.dart index 7e5c095fb53a..b3682ea6f132 100644 --- a/script/tool/lib/src/make_deps_path_based_command.dart +++ b/script/tool/lib/src/make_deps_path_based_command.dart @@ -57,7 +57,7 @@ class MakeDepsPathBasedCommand extends PackageCommand { // Includes a reference to the docs so that reviewers who aren't familiar with // the federated plugin change process don't think it's a mistake. static const String _dependencyOverrideWarningComment = - '# FOR TESTING AND INITIAL REVIEW ONLY. DO NOT MERGE.\n' + '# FOR TESTING AND INITIAL REVIEW ONLY. $kDoNotLandWarning.\n' '# See https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#changing-federated-plugins'; @override diff --git a/script/tool/test/federation_safety_check_command_test.dart b/script/tool/test/federation_safety_check_command_test.dart index ac80173cde15..ca4b018a811b 100644 --- a/script/tool/test/federation_safety_check_command_test.dart +++ b/script/tool/test/federation_safety_check_command_test.dart @@ -262,6 +262,89 @@ index abc123..def456 100644 ); }); + test('fails with specific text for combo PRs using the recommended tooling', + () async { + final Directory pluginGroupDir = packagesDir.childDirectory('foo'); + final RepositoryPackage appFacing = createFakePlugin('foo', pluginGroupDir); + final RepositoryPackage implementation = + createFakePlugin('foo_bar', pluginGroupDir); + final RepositoryPackage platformInterface = + createFakePlugin('foo_platform_interface', pluginGroupDir); + + void addFakeTempPubspecOverrides(RepositoryPackage package) { + final String contents = package.pubspecFile.readAsStringSync(); + package.pubspecFile.writeAsStringSync(''' +$contents + +# FOR TESTING AND INITIAL REVIEW ONLY. $kDoNotLandWarning. +dependency_overrides: + foo_platform_interface: + path: ../../../foo/foo_platform_interface +'''); + } + + addFakeTempPubspecOverrides(appFacing.getExamples().first); + addFakeTempPubspecOverrides(implementation.getExamples().first); + + const String appFacingChanges = ''' +diff --git a/packages/foo/foo/lib/foo.dart b/packages/foo/foo/lib/foo.dart +index abc123..def456 100644 +--- a/packages/foo/foo/lib/foo.dart ++++ b/packages/foo/foo/lib/foo.dart +@@ -51,6 +51,9 @@ Future launchUrl( + return true; + } + ++// This is a new method ++bool foo() => true; ++ + // This in an existing method + void aMethod() { + // Do things. +'''; + + final String changedFileOutput = [ + appFacing.libDirectory.childFile('foo.dart'), + implementation.libDirectory.childFile('foo.dart'), + platformInterface.pubspecFile, + platformInterface.libDirectory.childFile('foo.dart'), + ].map((File file) => file.path).join('\n'); + processRunner.mockProcessesForExecutable['git-diff'] = [ + FakeProcessInfo(MockProcess(stdout: changedFileOutput)), + FakeProcessInfo(MockProcess(stdout: appFacingChanges), + ['', 'HEAD', '--', '/packages/foo/foo/lib/foo.dart']), + // The others diffs don't need to be specified, since empty diff is also + // treated as a non-comment change. + ]; + + Error? commandError; + final List output = await runCapturingPrint( + runner, ['federation-safety-check'], errorHandler: (Error e) { + commandError = e; + }); + + expect(commandError, isA()); + expect( + output, + containsAllInOrder([ + contains('Running for foo/foo...'), + contains('"DO NOT MERGE" found in pubspec.yaml, so this is assumed to ' + 'be the initial combination PR for a federated change, following ' + 'the standard repository procedure. This failure is expected, in ' + 'order to prevent accidentally landing the temporary overrides, ' + 'and will automatically be resolved when the temporary overrides ' + 'are replaced by dependency version bumps later in the process.'), + contains('Running for foo_bar...'), + contains('"DO NOT MERGE" found in pubspec.yaml'), + contains('The following packages had errors:'), + contains('foo/foo:\n' + ' Unresolved combo PR.'), + contains('foo_bar:\n' + ' Unresolved combo PR.'), + ]), + ); + }); + test('ignores test-only changes to interface packages', () async { final Directory pluginGroupDir = packagesDir.childDirectory('foo'); final RepositoryPackage appFacing = createFakePlugin('foo', pluginGroupDir);