[flutter_plugin_tool] Don't allow NEXT on version bumps (#4246)

The special "NEXT" entry in a CHANGELOG should never be present in a
commit that bumped the version. This validates that this is true even if
the CHANGELOG would be correct for a non-version-change state, to catch
someone incorrectly resolving a merge conflict by leaving both parts of
the conflict, rather than folding the 'NEXT' entry's list into the new
version's notes.

Fixes https://github.com/flutter/flutter/issues/85584

script/tool/CHANGELOG.md

@@ -23,6 +23,8 @@
   the new `native-test` command.
 - Commands that print a run summary at the end now track and log exclusions
   similarly to skips for easier auditing.
+- `version-check` now validates that `NEXT` is not present when changing
+  the version.
 
 ## 0.4.1
 

script/tool/lib/src/version_check_command.dart

@@ -32,6 +32,21 @@ enum NextVersionType {
   RELEASE,
 }
 
+/// The state of a package's version relative to the comparison base.
+enum _CurrentVersionState {
+  /// The version is unchanged.
+  unchanged,
+
+  /// The version has changed, and the transition is valid.
+  validChange,
+
+  /// The version has changed, and the transition is invalid.
+  invalidChange,
+
+  /// There was an error determining the version state.
+  unknown,
+}
+
 /// Returns the set of allowed next versions, with their change type, for
 /// [version].
 ///
@@ -140,11 +155,28 @@ class VersionCheckCommand extends PackageLoopingCommand {
 
     final List<String> errors = <String>[];
 
-    if (!await _hasValidVersionChange(package, pubspec: pubspec)) {
+    bool versionChanged;
-      errors.add('Disallowed version change.');
+    final _CurrentVersionState versionState =
+        await _getVersionState(package, pubspec: pubspec);
+    switch (versionState) {
+      case _CurrentVersionState.unchanged:
+        versionChanged = false;
+        break;
+      case _CurrentVersionState.validChange:
+        versionChanged = true;
+        break;
+      case _CurrentVersionState.invalidChange:
+        versionChanged = true;
+        errors.add('Disallowed version change.');
+        break;
+      case _CurrentVersionState.unknown:
+        versionChanged = false;
+        errors.add('Unable to determine previous version.');
+        break;
     }
 
-    if (!(await _hasConsistentVersion(package, pubspec: pubspec))) {
+    if (!(await _validateChangelogVersion(package,
+        pubspec: pubspec, pubspecVersionChanged: versionChanged))) {
       errors.add('pubspec.yaml and CHANGELOG.md have different versions');
     }
 
@@ -195,10 +227,9 @@ ${indentation}HTTP response: ${pubVersionFinderResponse.httpResponse.body}
     return await gitVersionFinder.getPackageVersion(gitPath);
   }
 
-  /// Returns true if the version of [package] is either unchanged relative to
+  /// Returns the state of the verison of [package] relative to the comparison
-  /// the comparison base (git or pub, depending on flags), or is a valid
+  /// base (git or pub, depending on flags).
-  /// version transition.
+  Future<_CurrentVersionState> _getVersionState(
-  Future<bool> _hasValidVersionChange(
     Directory package, {
     required Pubspec pubspec,
   }) async {
@@ -208,7 +239,7 @@ ${indentation}HTTP response: ${pubVersionFinderResponse.httpResponse.body}
     if (getBoolArg(_againstPubFlag)) {
       previousVersion = await _fetchPreviousVersionFromPub(pubspec.name);
       if (previousVersion == null) {
-        return false;
+        return _CurrentVersionState.unknown;
       }
       if (previousVersion != Version.none) {
         print(
@@ -225,12 +256,12 @@ ${indentation}HTTP response: ${pubVersionFinderResponse.httpResponse.body}
           '${getBoolArg(_againstPubFlag) ? 'on pub server' : 'at git base'}.');
       logWarning(
           '${indentation}If this plugin is not new, something has gone wrong.');
-      return true;
+      return _CurrentVersionState.validChange; // Assume new, thus valid.
     }
 
     if (previousVersion == currentVersion) {
       print('${indentation}No version change.');
-      return true;
+      return _CurrentVersionState.unchanged;
     }
 
     // Check for reverts when doing local validation.
@@ -241,9 +272,9 @@ ${indentation}HTTP response: ${pubVersionFinderResponse.httpResponse.body}
       // to be a revert rather than a typo by checking that the transition
       // from the lower version to the new version would have been valid.
       if (possibleVersionsFromNewVersion.containsKey(previousVersion)) {
-        print('${indentation}New version is lower than previous version. '
+        logWarning('${indentation}New version is lower than previous version. '
             'This is assumed to be a revert.');
-        return true;
+        return _CurrentVersionState.validChange;
       }
     }
 
@@ -257,7 +288,7 @@ ${indentation}HTTP response: ${pubVersionFinderResponse.httpResponse.body}
       printError('${indentation}Incorrectly updated version.\n'
           '${indentation}HEAD: $currentVersion, $source: $previousVersion.\n'
           '${indentation}Allowed versions: $allowedNextVersions');
-      return false;
+      return _CurrentVersionState.invalidChange;
     }
 
     final bool isPlatformInterface =
@@ -268,16 +299,20 @@ ${indentation}HTTP response: ${pubVersionFinderResponse.httpResponse.body}
         allowedNextVersions[currentVersion] == NextVersionType.BREAKING_MAJOR) {
       printError('${indentation}Breaking change detected.\n'
           '${indentation}Breaking changes to platform interfaces are strongly discouraged.\n');
-      return false;
+      return _CurrentVersionState.invalidChange;
     }
-    return true;
+    return _CurrentVersionState.validChange;
   }
 
-  /// Returns whether or not the pubspec version and CHANGELOG version for
+  /// Checks whether or not [package]'s CHANGELOG's versioning is correct,
-  /// [plugin] match.
+  /// both that it matches [pubspec] and that NEXT is used correctly, printing
-  Future<bool> _hasConsistentVersion(
+  /// the results of its checks.
+  ///
+  /// Returns false if the CHANGELOG fails validation.
+  Future<bool> _validateChangelogVersion(
     Directory package, {
     required Pubspec pubspec,
+    required bool pubspecVersionChanged,
   }) async {
     // This method isn't called unless `version` is non-null.
     final Version fromPubspec = pubspec.version!;
@@ -296,10 +331,19 @@ ${indentation}HTTP response: ${pubVersionFinderResponse.httpResponse.body}
     // Remove all leading mark down syntax from the version line.
     String? versionString = firstLineWithText?.split(' ').last;
 
+    final String badNextErrorMessage = '${indentation}When bumping the version '
+        'for release, the NEXT section should be incorporated into the new '
+        'version\'s release notes.';
+
     // Skip validation for the special NEXT version that's used to accumulate
     // changes that don't warrant publishing on their own.
     final bool hasNextSection = versionString == 'NEXT';
     if (hasNextSection) {
+      // NEXT should not be present in a commit that changes the version.
+      if (pubspecVersionChanged) {
+        printError(badNextErrorMessage);
+        return false;
+      }
       print(
           '${indentation}Found NEXT; validating next version in the CHANGELOG.');
       // Ensure that the version in pubspec hasn't changed without updating
@@ -334,9 +378,7 @@ ${indentation}The first version listed in CHANGELOG.md is $fromChangeLog.
     if (!hasNextSection) {
       final RegExp nextRegex = RegExp(r'^#+\s*NEXT\s*$');
       if (lines.any((String line) => nextRegex.hasMatch(line))) {
-        printError('${indentation}When bumping the version for release, the '
+        printError(badNextErrorMessage);
-            'NEXT section should be incorporated into the new version\'s '
-            'release notes.');
         return false;
       }
     }

script/tool/test/version_check_command_test.dart

@@ -373,6 +373,10 @@ void main() {
 * Some other changes.
 ''';
       createFakeCHANGELOG(pluginDirectory, changelog);
+      gitShowResponses = <String, String>{
+        'master:packages/plugin/pubspec.yaml': 'version: 1.0.0',
+      };
+
       final List<String> output = await runCapturingPrint(
           runner, <String>['version-check', '--base-sha=master']);
       await expectLater(
@@ -384,8 +388,7 @@ void main() {
       );
     });
 
-    test('Fail if NEXT is left in the CHANGELOG when adding a version bump',
+    test('Fail if NEXT appears after a version', () async {
-        () async {
       const String version = '1.0.1';
       final Directory pluginDirectory =
           createFakePlugin('plugin', packagesDir, version: version);
@@ -419,17 +422,25 @@ void main() {
       );
     });
 
-    test('Fail if the version changes without replacing NEXT', () async {
+    test('Fail if NEXT is left in the CHANGELOG when adding a version bump',
+        () async {
+      const String version = '1.0.1';
       final Directory pluginDirectory =
-          createFakePlugin('plugin', packagesDir, version: '1.0.1');
+          createFakePlugin('plugin', packagesDir, version: version);
 
       const String changelog = '''
 ## NEXT
-* Some changes that should be listed as part of 1.0.1.
+* Some changes that should have been folded in 1.0.1.
+## $version
+* Some changes.
 ## 1.0.0
 * Some other changes.
 ''';
       createFakeCHANGELOG(pluginDirectory, changelog);
+      gitShowResponses = <String, String>{
+        'master:packages/plugin/pubspec.yaml': 'version: 1.0.0',
+      };
+
       bool hasError = false;
       final List<String> output = await runCapturingPrint(runner, <String>[
         'version-check',
@@ -444,8 +455,43 @@ void main() {
       expect(
         output,
         containsAllInOrder(<Matcher>[
-          contains('Found NEXT; validating next version in the CHANGELOG.'),
+          contains('When bumping the version for release, the NEXT section '
-          contains('Versions in CHANGELOG.md and pubspec.yaml do not match.'),
+              'should be incorporated into the new version\'s release notes.')
+        ]),
+      );
+    });
+
+    test('Fail if the version changes without replacing NEXT', () async {
+      final Directory pluginDirectory =
+          createFakePlugin('plugin', packagesDir, version: '1.0.1');
+
+      const String changelog = '''
+## NEXT
+* Some changes that should be listed as part of 1.0.1.
+## 1.0.0
+* Some other changes.
+''';
+      createFakeCHANGELOG(pluginDirectory, changelog);
+      gitShowResponses = <String, String>{
+        'master:packages/plugin/pubspec.yaml': 'version: 1.0.0',
+      };
+
+      bool hasError = false;
+      final List<String> output = await runCapturingPrint(runner, <String>[
+        'version-check',
+        '--base-sha=master',
+        '--against-pub'
+      ], errorHandler: (Error e) {
+        expect(e, isA<ToolExit>());
+        hasError = true;
+      });
+      expect(hasError, isTrue);
+
+      expect(
+        output,
+        containsAllInOrder(<Matcher>[
+          contains('When bumping the version for release, the NEXT section '
+              'should be incorporated into the new version\'s release notes.')
         ]),
       );
     });