diff --git a/script/tool/lib/src/common/package_state_utils.dart b/script/tool/lib/src/common/package_state_utils.dart index f52e70f146d5..bb0960cd1ce4 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 93a832eb0e29..86767f15f7a1 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 0699dc251c42..ac80173cde15 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);