diff --git a/script/tool/lib/src/common/package_state_utils.dart b/script/tool/lib/src/common/package_state_utils.dart index fdc816143a..3885cde8e7 100644 --- a/script/tool/lib/src/common/package_state_utils.dart +++ b/script/tool/lib/src/common/package_state_utils.dart @@ -184,6 +184,12 @@ Future _isDevChange(List 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); } @@ -231,3 +237,34 @@ Future _isGradleTestDependencyChange(List 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 _isDartImplementationCommentChange(List pathComponents, + {GitVersionFinder? git, String? repoPath}) async { + if (git == null) { + return false; + } + if (!pathComponents.last.endsWith('.dart')) { + return false; + } + final List 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; +} diff --git a/script/tool/test/common/package_state_utils_test.dart b/script/tool/test/common/package_state_utils_test.dart index a900c27412..9c8c2c0473 100644 --- a/script/tool/test/common/package_state_utils_test.dart +++ b/script/tool/test/common/package_state_utils_test.dart @@ -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 changedFiles = [ + 'packages/a_plugin/lib/a_plugin.dart', + ]; + + final GitVersionFinder git = FakeGitVersionFinder(>{ + 'packages/a_plugin/lib/a_plugin.dart': [ + '- // 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); + + 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 changedFiles = [ + 'packages/a_plugin/lib/a_plugin.dart', + ]; + + final GitVersionFinder git = FakeGitVersionFinder(>{ + 'packages/a_plugin/lib/a_plugin.dart': [ + '- /// 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 changedFiles = [ + 'packages/a_plugin/lib/a_plugin.dart', + ]; + + final GitVersionFinder git = FakeGitVersionFinder(>{ + 'packages/a_plugin/lib/a_plugin.dart': [ + // 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 { diff --git a/script/tool/test/version_check_command_test.dart b/script/tool/test/version_check_command_test.dart index c6d9339826..da864de409 100644 --- a/script/tool/test/version_check_command_test.dart +++ b/script/tool/test/version_check_command_test.dart @@ -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'); @@ -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(MockProcess(stdout: 'version: 1.0.0')), + ]; + processRunner.mockProcessesForExecutable['git-diff'] = + [ + // File list. + FakeProcessInfo(MockProcess(stdout: ''' +packages/plugin/lib/plugin.dart +''')), + // Dart file diff. + FakeProcessInfo(MockProcess(stdout: ''' ++ // TODO(someone): Fix this. ++ // ignore: some_lint +'''), ['main', 'HEAD', '--', 'packages/plugin/lib/plugin.dart']), + ]; + + final List output = + await runWithMissingChangeDetection([]); + + expect( + output, + containsAllInOrder([ + 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(MockProcess(stdout: 'version: 1.0.0')), + ]; + processRunner.mockProcessesForExecutable['git-diff'] = + [ + FakeProcessInfo(MockProcess(stdout: ''' +packages/plugin/lib/plugin.dart +''')), + // Dart file diff. + FakeProcessInfo(MockProcess(stdout: ''' ++ /// Important new information for API clients! +'''), ['main', 'HEAD', '--', 'packages/plugin/lib/plugin.dart']), + ]; + + Error? commandError; + final List output = await runWithMissingChangeDetection( + [], errorHandler: (Error e) { + commandError = e; + }); + + expect(commandError, isA()); + expect( + output, + containsAllInOrder([ + contains('No version change found'), + contains('plugin:\n' + ' Missing version change'), + ]), + ); + }); }); test('allows valid against pub', () async {