From a78a9137fac915ef08ea07da3fbe47f62ce03eba Mon Sep 17 00:00:00 2001 From: stuartmorgan Date: Wed, 17 May 2023 13:28:24 -0700 Subject: [PATCH] [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 https://github.com/flutter/flutter/issues/122390 --- .../lib/src/common/package_state_utils.dart | 4 +- .../src/federation_safety_check_command.dart | 30 +++- .../federation_safety_check_command_test.dart | 158 ++++++++++++++++++ 3 files changed, 189 insertions(+), 3 deletions(-) diff --git a/script/tool/lib/src/common/package_state_utils.dart b/script/tool/lib/src/common/package_state_utils.dart index f52e70f146..bb0960cd1c 100644 --- 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 _isGradleTestDependencyChange(List pathComponents, return false; } final List 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 index 93a832eb0e..86767f15f7 100644 --- 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] ??= []; _changedDartFiles[packageName]! .add(p.posix.joinAll(relativeComponents)); @@ -196,4 +197,31 @@ class FederationSafetyCheckCommand extends PackageLoopingCommand { } return pubspec.version != previousVersion; } + + Future _changeIsCommentOnly( + GitVersionFinder git, String repoPath) async { + if (git == null) { + return false; + } + final List 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 index 0699dc251c..ac80173cde 100644 --- 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 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'), @@ -210,6 +227,12 @@ void main() { ].map((File file) => file.path).join('\n'); processRunner.mockProcessesForExecutable['git-diff'] = [ 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), + ['', '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 = [ + 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 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 launchUrl( + } + + void foo() { ++ // ignore: exhaustive_cases + switch(a_foo) { + case a: + // Do things +'''; + + processRunner.mockProcessesForExecutable['git-diff'] = [ + FakeProcessInfo( + MockProcess(stdout: changedFileOutput), ['--name-only']), + FakeProcessInfo(MockProcess(stdout: implementationChanges), + ['', 'HEAD', '--', '/packages/foo/foo_bar/lib/foo.dart']), + FakeProcessInfo(MockProcess(stdout: platformInterfaceChanges), [ + '', + 'HEAD', + '--', + '/packages/foo/foo_platform_interface/lib/foo.dart' + ]), + ]; + + final List output = + await runCapturingPrint(runner, ['federation-safety-check']); + + expect( + output, + containsAllInOrder([ + 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 = [ + 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 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 launchUrl( + } + + void foo() { ++ new code; + existing code; + ... + ... +'''; + + processRunner.mockProcessesForExecutable['git-diff'] = [ + FakeProcessInfo( + MockProcess(stdout: changedFileOutput), ['--name-only']), + FakeProcessInfo(MockProcess(stdout: implementationChanges), + ['', 'HEAD', '--', '/packages/foo/foo_bar/lib/foo.dart']), + FakeProcessInfo(MockProcess(stdout: platformInterfaceChanges), [ + '', + 'HEAD', + '--', + '/packages/foo/foo_platform_interface/lib/foo.dart' + ]), + ]; + + final List output = + await runCapturingPrint(runner, ['federation-safety-check']); + + expect( + output, + containsAllInOrder([ + 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);