From 48232e890b6d3315e4874769ce3cee76e3876468 Mon Sep 17 00:00:00 2001 From: stuartmorgan Date: Fri, 23 Jun 2023 13:06:07 -0400 Subject: [PATCH] [tool] Consider comment-only changes to be dev-only (#4279) Updates the state checker to inspect changed Dart files to see if the only changes are implementation (not documentation) comment lines. In particular, this will fix the problem of CI flagging changes that do nothing but add `// ignore:` comments (for federated package changes involving deprecation, framework changes that require temporary ignores in packages to support stable, etc.) as needing version and changelog changes --- .../lib/src/common/package_state_utils.dart | 37 ++++++++ .../test/common/package_state_utils_test.dart | 84 +++++++++++++++++++ .../tool/test/version_check_command_test.dart | 83 +++++++++++++++++- 3 files changed, 203 insertions(+), 1 deletion(-) 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 {