[tool] Consider comment-only changes to be dev-only (#4279)

stuartmorgan-g · web-flow · commit 48232e890b6d · 2023-06-23T17:06:07.000Z
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
diff --git 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<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);
 }
 
@@ -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;
+}
diff --git 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<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);
+
+      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 {
diff --git 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>[
+          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 {
