From afe2722ba3c4f6d8809e42415a31923ece0b6ef8 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Thu, 22 Jun 2023 15:27:58 -0400 Subject: [PATCH 1/2] [tool] Consider comment-only changes to be dev-only 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 | 82 ++++++++++++++++++ .../tool/test/version_check_command_test.dart | 83 ++++++++++++++++++- 3 files changed, 201 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 fdc816143ab..3885cde8e7a 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 a900c274123..63427ff2614 100644 --- a/script/tool/test/common/package_state_utils_test.dart +++ b/script/tool/test/common/package_state_utils_test.dart @@ -289,6 +289,88 @@ 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 c6d93398264..da864de409a 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 { From 6002a3ae899e1acff2fead7ad82913293764e426 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Thu, 22 Jun 2023 19:00:16 -0400 Subject: [PATCH 2/2] Review feedback --- script/tool/test/common/package_state_utils_test.dart | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/script/tool/test/common/package_state_utils_test.dart b/script/tool/test/common/package_state_utils_test.dart index 63427ff2614..9c8c2c04731 100644 --- a/script/tool/test/common/package_state_utils_test.dart +++ b/script/tool/test/common/package_state_utils_test.dart @@ -333,10 +333,12 @@ void main() { ] }); - final PackageChangeState state = await checkPackageChangeState(package, - changedPaths: changedFiles, - relativePackagePath: 'packages/a_plugin/', - git: git); + final PackageChangeState state = await checkPackageChangeState( + package, + changedPaths: changedFiles, + relativePackagePath: 'packages/a_plugin/', + git: git, + ); expect(state.hasChanges, true); expect(state.needsVersionChange, true);