[tool] Provide better CI feedback for combo PRs (flutter#6865)

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.

Fixes flutter/flutter#129303

Author: stuartmorgan-g

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 }

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(<String>['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);
+  }
 }

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

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<bool> launchUrl(
+   return true;
+ }
+
++// This is a new method
++bool foo() => true;
++
+ // This in an existing method
+ void aMethod() {
+   // Do things.
+''';
+
+    final String changedFileOutput = <File>[
+      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>[
+      FakeProcessInfo(MockProcess(stdout: changedFileOutput)),
+      FakeProcessInfo(MockProcess(stdout: appFacingChanges),
+          <String>['', '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<String> output = await runCapturingPrint(
+        runner, <String>['federation-safety-check'], errorHandler: (Error e) {
+      commandError = e;
+    });
+
+    expect(commandError, isA<ToolExit>());
+    expect(
+      output,
+      containsAllInOrder(<Matcher>[
+        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);