diff --git a/script/tool/CHANGELOG.md b/script/tool/CHANGELOG.md index 7f326ff3c8..584ea571f0 100644 --- a/script/tool/CHANGELOG.md +++ b/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 diff --git a/script/tool/lib/src/version_check_command.dart b/script/tool/lib/src/version_check_command.dart index c08600c3f6..67c5637828 100644 --- a/script/tool/lib/src/version_check_command.dart +++ b/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 errors = []; - if (!await _hasValidVersionChange(package, pubspec: pubspec)) { - errors.add('Disallowed version change.'); + bool versionChanged; + 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 - /// the comparison base (git or pub, depending on flags), or is a valid - /// version transition. - Future _hasValidVersionChange( + /// Returns the state of the verison of [package] relative to the comparison + /// base (git or pub, depending on flags). + Future<_CurrentVersionState> _getVersionState( 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 - /// [plugin] match. - Future _hasConsistentVersion( + /// Checks whether or not [package]'s CHANGELOG's versioning is correct, + /// both that it matches [pubspec] and that NEXT is used correctly, printing + /// the results of its checks. + /// + /// Returns false if the CHANGELOG fails validation. + Future _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 ' - 'NEXT section should be incorporated into the new version\'s ' - 'release notes.'); + printError(badNextErrorMessage); return false; } } diff --git a/script/tool/test/version_check_command_test.dart b/script/tool/test/version_check_command_test.dart index 587de1a58c..7765073feb 100644 --- a/script/tool/test/version_check_command_test.dart +++ b/script/tool/test/version_check_command_test.dart @@ -373,6 +373,10 @@ void main() { * Some other changes. '''; createFakeCHANGELOG(pluginDirectory, changelog); + gitShowResponses = { + 'master:packages/plugin/pubspec.yaml': 'version: 1.0.0', + }; + final List output = await runCapturingPrint( runner, ['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', - () async { + test('Fail if NEXT appears after a version', () 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 = { + 'master:packages/plugin/pubspec.yaml': 'version: 1.0.0', + }; + bool hasError = false; final List output = await runCapturingPrint(runner, [ 'version-check', @@ -444,8 +455,43 @@ void main() { expect( output, containsAllInOrder([ - contains('Found NEXT; validating next version in the CHANGELOG.'), - contains('Versions in CHANGELOG.md and pubspec.yaml do not match.'), + contains('When bumping the version for release, the NEXT section ' + '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 = { + 'master:packages/plugin/pubspec.yaml': 'version: 1.0.0', + }; + + bool hasError = false; + final List output = await runCapturingPrint(runner, [ + 'version-check', + '--base-sha=master', + '--against-pub' + ], errorHandler: (Error e) { + expect(e, isA()); + hasError = true; + }); + expect(hasError, isTrue); + + expect( + output, + containsAllInOrder([ + contains('When bumping the version for release, the NEXT section ' + 'should be incorporated into the new version\'s release notes.') ]), ); });