Skip to content
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
37 changes: 37 additions & 0 deletions script/tool/lib/src/common/package_state_utils.dart
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,12 @@ Future<bool> _isDevChange(List<String> pathComponents,
_isExampleBuildFile(pathComponents) ||
// Test-only gradle depenedencies don't affect clients.
await _isGradleTestDependencyChange(pathComponents,
git: git, repoPath: repoPath) ||
// Implementation comments don't affect clients.
// This check is currently Dart-only since that's the only place
// this has come up in practice; it could be generalized to other
// languages if needed.
await _isDartImplementationCommentChange(pathComponents,
git: git, repoPath: repoPath);
}

Expand Down Expand Up @@ -231,3 +237,34 @@ Future<bool> _isGradleTestDependencyChange(List<String> pathComponents,
// against having the wrong (e.g., incorrectly empty) diff output.
return foundTestDependencyChange;
}

// Returns true if the given file is a Dart file whose only changes are
// implementation comments (i.e., not doc comments).
Future<bool> _isDartImplementationCommentChange(List<String> pathComponents,
{GitVersionFinder? git, String? repoPath}) async {
if (git == null) {
return false;
}
if (!pathComponents.last.endsWith('.dart')) {
return false;
}
final List<String> diff = await git.getDiffContents(targetPath: repoPath);
final RegExp changeLine = RegExp(r'^[+-] ');
final RegExp whitespaceLine = RegExp(r'^[+-]\s*$');
final RegExp nonDocCommentLine = RegExp(r'^[+-]\s*//\s');
bool foundIgnoredChange = false;
for (final String line in diff) {
if (!changeLine.hasMatch(line) ||
line.startsWith('--- ') ||
line.startsWith('+++ ')) {
continue;
}
if (!nonDocCommentLine.hasMatch(line) && !whitespaceLine.hasMatch(line)) {
return false;
}
foundIgnoredChange = true;
}
// Only return true if an ignored change was found, as a failsafe against
// having the wrong (e.g., incorrectly empty) diff output.
return foundIgnoredChange;
}
84 changes: 84 additions & 0 deletions script/tool/test/common/package_state_utils_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,90 @@ void main() {
expect(state.needsChangelogChange, true);
});

test(
'does not requires changelog or version change for '
'non-doc-comment-only changes', () async {
final RepositoryPackage package =
createFakePlugin('a_plugin', packagesDir);

const List<String> changedFiles = <String>[
'packages/a_plugin/lib/a_plugin.dart',
];

final GitVersionFinder git = FakeGitVersionFinder(<String, List<String>>{
'packages/a_plugin/lib/a_plugin.dart': <String>[
'- // Old comment.',
'+ // New comment.',
'+ ', // Allow whitespace line changes as part of comment changes.
]
});

final PackageChangeState state = await checkPackageChangeState(package,
changedPaths: changedFiles,
relativePackagePath: 'packages/a_plugin/',
git: git);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a good time for a trailing comma. Do we have style guidelines for this?

Copy link
Collaborator Author

@stuartmorgan-g stuartmorgan-g Jun 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#use-a-trailing-comma-for-arguments-parameters-and-list-items-but-only-if-they-each-have-their-own-line seems like it covers it. The style rules there don't always apply 100% since we use the auto-formatter, but in this case it's both better under the autoformatter and follows the style guide so it's a win all around :)

I definitely don't remember to use trailing commas as much as I should.


expect(state.hasChanges, true);
expect(state.needsVersionChange, false);
expect(state.needsChangelogChange, false);
});

test('requires changelog or version change for doc comment changes',
() async {
final RepositoryPackage package =
createFakePlugin('a_plugin', packagesDir);

const List<String> changedFiles = <String>[
'packages/a_plugin/lib/a_plugin.dart',
];

final GitVersionFinder git = FakeGitVersionFinder(<String, List<String>>{
'packages/a_plugin/lib/a_plugin.dart': <String>[
'- /// Old doc comment.',
'+ /// New doc comment.',
]
});

final PackageChangeState state = await checkPackageChangeState(
package,
changedPaths: changedFiles,
relativePackagePath: 'packages/a_plugin/',
git: git,
);

expect(state.hasChanges, true);
expect(state.needsVersionChange, true);
expect(state.needsChangelogChange, true);
});

test('requires changelog or version change for Dart code change', () async {
final RepositoryPackage package =
createFakePlugin('a_plugin', packagesDir);

const List<String> changedFiles = <String>[
'packages/a_plugin/lib/a_plugin.dart',
];

final GitVersionFinder git = FakeGitVersionFinder(<String, List<String>>{
'packages/a_plugin/lib/a_plugin.dart': <String>[
// Include inline comments to ensure the comment check doesn't have
// false positives for lines that include comment changes but aren't
// only comment changes.
'- callOldMethod(); // inline comment',
'+ callNewMethod(); // inline comment',
]
});

final PackageChangeState state = await checkPackageChangeState(package,
changedPaths: changedFiles,
relativePackagePath: 'packages/a_plugin/',
git: git);

expect(state.hasChanges, true);
expect(state.needsVersionChange, true);
expect(state.needsChangelogChange, true);
});

test(
'requires changelog or version change if build.gradle diffs cannot '
'be checked', () async {
Expand Down
83 changes: 82 additions & 1 deletion script/tool/test/version_check_command_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1114,7 +1114,8 @@ packages/plugin/android/build.gradle
);
});

test('allows missing CHANGELOG and version change for dev-only changes',
test(
'allows missing CHANGELOG and version change for dev-only-file changes',
() async {
final RepositoryPackage plugin =
createFakePlugin('plugin', packagesDir, version: '1.0.0');
Expand Down Expand Up @@ -1147,6 +1148,86 @@ packages/plugin/run_tests.sh
]),
);
});

test(
'allows missing CHANGELOG and version change for dev-only line-level '
'changes in production files', () async {
final RepositoryPackage plugin =
createFakePlugin('plugin', packagesDir, version: '1.0.0');

const String changelog = '''
## 1.0.0
* Some changes.
''';
plugin.changelogFile.writeAsStringSync(changelog);
processRunner.mockProcessesForExecutable['git-show'] =
<FakeProcessInfo>[
FakeProcessInfo(MockProcess(stdout: 'version: 1.0.0')),
];
processRunner.mockProcessesForExecutable['git-diff'] =
<FakeProcessInfo>[
// File list.
FakeProcessInfo(MockProcess(stdout: '''
packages/plugin/lib/plugin.dart
''')),
// Dart file diff.
FakeProcessInfo(MockProcess(stdout: '''
+ // TODO(someone): Fix this.
+ // ignore: some_lint
'''), <String>['main', 'HEAD', '--', 'packages/plugin/lib/plugin.dart']),
];

final List<String> output =
await runWithMissingChangeDetection(<String>[]);

expect(
output,
containsAllInOrder(<Matcher>[
contains('Running for plugin'),
]),
);
});

test('documentation comments are not exempt', () async {
final RepositoryPackage plugin =
createFakePlugin('plugin', packagesDir, version: '1.0.0');

const String changelog = '''
## 1.0.0
* Some changes.
''';
plugin.changelogFile.writeAsStringSync(changelog);
processRunner.mockProcessesForExecutable['git-show'] =
<FakeProcessInfo>[
FakeProcessInfo(MockProcess(stdout: 'version: 1.0.0')),
];
processRunner.mockProcessesForExecutable['git-diff'] =
<FakeProcessInfo>[
FakeProcessInfo(MockProcess(stdout: '''
packages/plugin/lib/plugin.dart
''')),
// Dart file diff.
FakeProcessInfo(MockProcess(stdout: '''
+ /// Important new information for API clients!
'''), <String>['main', 'HEAD', '--', 'packages/plugin/lib/plugin.dart']),
];

Error? commandError;
final List<String> output = await runWithMissingChangeDetection(
<String>[], errorHandler: (Error e) {
commandError = e;
});

expect(commandError, isA<ToolExit>());
expect(
output,
containsAllInOrder(<Matcher>[
contains('No version change found'),
contains('plugin:\n'
' Missing version change'),
]),
);
});
});

test('allows valid against pub', () async {
Expand Down