Skip to content

[tools] Ignore comments in federated safety check #4028

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions script/tool/lib/src/common/package_state_utils.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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) ||
Expand Down
30 changes: 29 additions & 1 deletion script/tool/lib/src/federation_safety_check_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down Expand Up @@ -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;
}
}
158 changes: 158 additions & 0 deletions script/tool/test/federation_safety_check_command_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand Down