[tools] Ignore comments in federated safety check (#4028)

Because we treat analysis warnings as errors, there are common cases where non-breaking changes to part of a federated plugin (adding an enum value, deprecating a method, etc.) are CI-breaking for us. Currently we can't ignore those issues in the same PR where they are added, because doing so would violate the federated saftey check, adding extra complexity to those PRs. This improves the change detection logic for the federated safety check to ignore comment-only changes in Dart code, which should allow us to add temporary `// ignore:`s in the PRs that create the need for them. Fixes flutter/flutter#122390
flutter · May 17, 2023 · a78a913 · a78a913
1 parent baeca88
commit a78a913
Show file tree

Hide file tree

Showing 3 changed files with 189 additions and 3 deletions.
diff --git a/script/tool/lib/src/common/package_state_utils.dart b/script/tool/lib/src/common/package_state_utils.dart
@@ -211,9 +211,9 @@ Future<bool> _isGradleTestDependencyChange(List<String> pathComponents,
     return false;
   }
   final List<String> diff = await git.getDiffContents(targetPath: repoPath);
-  final RegExp changeLine = RegExp(r'[+-] ');
+  final RegExp changeLine = RegExp(r'^[+-] ');
   final RegExp testDependencyLine =
-      RegExp(r'[+-]\s*(?:androidT|t)estImplementation\s');
+      RegExp(r'^[+-]\s*(?:androidT|t)estImplementation\s');
   bool foundTestDependencyChange = false;
   for (final String line in diff) {
     if (!changeLine.hasMatch(line) ||

diff --git a/script/tool/lib/src/federation_safety_check_command.dart b/script/tool/lib/src/federation_safety_check_command.dart
@@ -85,7 +85,8 @@ class FederationSafetyCheckCommand extends PackageLoopingCommand {
         packageName = relativeComponents.removeAt(0);
       }
 
-      if (relativeComponents.last.endsWith('.dart')) {
+      if (relativeComponents.last.endsWith('.dart') &&
+          !await _changeIsCommentOnly(gitVersionFinder, path)) {
         _changedDartFiles[packageName] ??= <String>[];
         _changedDartFiles[packageName]!
             .add(p.posix.joinAll(relativeComponents));
@@ -196,4 +197,31 @@ class FederationSafetyCheckCommand extends PackageLoopingCommand {
     }
     return pubspec.version != previousVersion;
   }
+
+  Future<bool> _changeIsCommentOnly(
+      GitVersionFinder git, String repoPath) async {
+    if (git == null) {
+      return false;
+    }
+    final List<String> diff = await git.getDiffContents(targetPath: repoPath);
+    final RegExp changeLine = RegExp(r'^[+-] ');
+    // This will not catch /**/-style comments, but false negatives are fine
+    // (and in practice, we almost never use that comment style in Dart code).
+    final RegExp commentLine = RegExp(r'^[+-]\s*//');
+    bool foundComment = false;
+    for (final String line in diff) {
+      if (!changeLine.hasMatch(line) ||
+          line.startsWith('--- ') ||
+          line.startsWith('+++ ')) {
+        continue;
+      }
+      if (!commentLine.hasMatch(line)) {
+        return false;
+      }
+      foundComment = true;
+    }
+    // Only return true if a comment change was found, as a fail-safe against
+    // against having the wrong (e.g., incorrectly empty) diff output.
+    return foundComment;
+  }
 }
diff --git a/script/tool/test/federation_safety_check_command_test.dart b/script/tool/test/federation_safety_check_command_test.dart
@@ -202,6 +202,23 @@ void main() {
     final RepositoryPackage platformInterface =
         createFakePlugin('foo_platform_interface', pluginGroupDir);
 
+    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'),
@@ -210,6 +227,12 @@ void main() {
     ].map((File file) => file.path).join('\n');
     processRunner.mockProcessesForExecutable['git-diff'] = <FakeProcessInfo>[
       FakeProcessInfo(MockProcess(stdout: changedFileOutput)),
+      // Ensure that a change with both a comment and non-comment addition is
+      // counted, to validate change analysis.
+      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;
@@ -308,6 +331,141 @@ void main() {
     );
   });
 
+  test('ignores comment-only changes in implementation packages', () async {
+    final Directory pluginGroupDir = packagesDir.childDirectory('foo');
+    final RepositoryPackage implementation =
+        createFakePlugin('foo_bar', pluginGroupDir);
+    final RepositoryPackage platformInterface =
+        createFakePlugin('foo_platform_interface', pluginGroupDir);
+
+    final String changedFileOutput = <File>[
+      implementation.libDirectory.childFile('foo.dart'),
+      platformInterface.pubspecFile,
+      platformInterface.libDirectory.childFile('foo.dart'),
+    ].map((File file) => file.path).join('\n');
+
+    const String platformInterfaceChanges = '''
+diff --git a/packages/foo/foo_platform_interface/lib/foo.dart b/packages/foo/foo_platform_interface/lib/foo.dart
+index abc123..def456 100644
+--- a/packages/foo/foo_platform_interface/lib/foo.dart
++++ b/packages/foo/foo_platform_interface/lib/foo.dart
+@@ -51,6 +51,7 @@ Future<bool> launchUrl(
+ enum Foo {
+   a,
+   b,
++  c,
+   d,
+   e,
+ }
+''';
+    const String implementationChanges = '''
+diff --git a/packages/foo/foo_bar/lib/foo.dart b/packages/foo/foo_bar/lib/foo.dart
+index abc123..def456 100644
+--- a/packages/foo/foo_bar/lib/foo.dart
++++ b/packages/foo/foo_bar/lib/foo.dart
+@@ -51,6 +51,7 @@ Future<bool> launchUrl(
+ }
+
+ void foo() {
++  // ignore: exhaustive_cases
+   switch(a_foo) {
+     case a:
+       // Do things
+''';
+
+    processRunner.mockProcessesForExecutable['git-diff'] = <FakeProcessInfo>[
+      FakeProcessInfo(
+          MockProcess(stdout: changedFileOutput), <String>['--name-only']),
+      FakeProcessInfo(MockProcess(stdout: implementationChanges),
+          <String>['', 'HEAD', '--', '/packages/foo/foo_bar/lib/foo.dart']),
+      FakeProcessInfo(MockProcess(stdout: platformInterfaceChanges), <String>[
+        '',
+        'HEAD',
+        '--',
+        '/packages/foo/foo_platform_interface/lib/foo.dart'
+      ]),
+    ];
+
+    final List<String> output =
+        await runCapturingPrint(runner, <String>['federation-safety-check']);
+
+    expect(
+      output,
+      containsAllInOrder(<Matcher>[
+        contains('Running for foo_bar...'),
+        contains('No Dart changes.'),
+      ]),
+    );
+  });
+
+  test('ignores comment-only changes in platform interface packages', () async {
+    final Directory pluginGroupDir = packagesDir.childDirectory('foo');
+    final RepositoryPackage implementation =
+        createFakePlugin('foo_bar', pluginGroupDir);
+    final RepositoryPackage platformInterface =
+        createFakePlugin('foo_platform_interface', pluginGroupDir);
+
+    final String changedFileOutput = <File>[
+      implementation.libDirectory.childFile('foo.dart'),
+      platformInterface.pubspecFile,
+      platformInterface.libDirectory.childFile('foo.dart'),
+    ].map((File file) => file.path).join('\n');
+
+    const String platformInterfaceChanges = '''
+diff --git a/packages/foo/foo_platform_interface/lib/foo.dart b/packages/foo/foo_platform_interface/lib/foo.dart
+index abc123..def456 100644
+--- a/packages/foo/foo_platform_interface/lib/foo.dart
++++ b/packages/foo/foo_platform_interface/lib/foo.dart
+@@ -51,6 +51,8 @@ Future<bool> launchUrl(
+   // existing comment
+   // existing comment
+   // existing comment
++  //
++  // additional comment
+   void foo() {
+     some code;
+   }
+''';
+    const String implementationChanges = '''
+diff --git a/packages/foo/foo_bar/lib/foo.dart b/packages/foo/foo_bar/lib/foo.dart
+index abc123..def456 100644
+--- a/packages/foo/foo_bar/lib/foo.dart
++++ b/packages/foo/foo_bar/lib/foo.dart
+@@ -51,6 +51,7 @@ Future<bool> launchUrl(
+ }
+
+ void foo() {
++  new code;
+   existing code;
+   ...
+   ...
+''';
+
+    processRunner.mockProcessesForExecutable['git-diff'] = <FakeProcessInfo>[
+      FakeProcessInfo(
+          MockProcess(stdout: changedFileOutput), <String>['--name-only']),
+      FakeProcessInfo(MockProcess(stdout: implementationChanges),
+          <String>['', 'HEAD', '--', '/packages/foo/foo_bar/lib/foo.dart']),
+      FakeProcessInfo(MockProcess(stdout: platformInterfaceChanges), <String>[
+        '',
+        'HEAD',
+        '--',
+        '/packages/foo/foo_platform_interface/lib/foo.dart'
+      ]),
+    ];
+
+    final List<String> output =
+        await runCapturingPrint(runner, <String>['federation-safety-check']);
+
+    expect(
+      output,
+      containsAllInOrder(<Matcher>[
+        contains('Running for foo_bar...'),
+        contains('No public code changes for foo_platform_interface.'),
+      ]),
+    );
+  });
+
   test('allows things that look like mass changes, with warning', () async {
     final Directory pluginGroupDir = packagesDir.childDirectory('foo');
     final RepositoryPackage appFacing = createFakePlugin('foo', pluginGroupDir);