From bc5a8f4d37bcff2af21547ff8644f0a519efca69 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Tue, 1 Jun 2021 21:00:22 +0200 Subject: [PATCH] feat(dev-infra): validate deprecation notes in commit messages (#42436) Currently the commit message validation tool from `ng-dev` validates the `BREAKING CHANGE:` commit message notes. This commit adds a similar check for `DEPRECATED:` commit message notes. Additionally, the check for breaking changes is reworked slightly to be more tolerant (i.e. if there is only a single line break after the summary; this is acceptable as per the parser and commonly done in the COMP repo). The checks have been updated to capture wrong keywords that are commonly used instead of the correct one. e.g. if a commit message uses `DEPRECATIONS:` instead of `DEPRECATED:`, the validation will fail. This prevents changelog generation issues where breaking change notes, or deprecations are missing. This happened in the COMP repo where the `DEPRECATED:` keyword was used incorrectly. See: https://github.com/angular/components/commit/99391e79391d20c6ef2f95a3ea4fd6901dcb631d PR Close #42436 --- dev-infra/commit-message/parse.ts | 2 +- dev-infra/commit-message/validate.spec.ts | 139 ++++++++++++++++++++-- dev-infra/commit-message/validate.ts | 43 ++++--- dev-infra/ng-dev.js | 41 ++++--- 4 files changed, 184 insertions(+), 41 deletions(-) diff --git a/dev-infra/commit-message/parse.ts b/dev-infra/commit-message/parse.ts index bfaf566bcc..1892121a0e 100644 --- a/dev-infra/commit-message/parse.ts +++ b/dev-infra/commit-message/parse.ts @@ -111,7 +111,7 @@ const parseOptions: Options&{notesPattern: (keywords: string) => RegExp} = { headerPattern, headerCorrespondence, noteKeywords: [NoteSections.BREAKING_CHANGE, NoteSections.DEPRECATED], - notesPattern: (keywords: string) => new RegExp(`(${keywords})(?:: ?)(.*)`), + notesPattern: (keywords: string) => new RegExp(`(${keywords}): ?(.*)`), }; /** Parse a commit message into its composite parts. */ diff --git a/dev-infra/commit-message/validate.spec.ts b/dev-infra/commit-message/validate.spec.ts index 0d92bdccb2..0c0b534ea5 100644 --- a/dev-infra/commit-message/validate.spec.ts +++ b/dev-infra/commit-message/validate.spec.ts @@ -8,6 +8,7 @@ // Imports import * as validateConfig from './config'; +import {parseCommitMessage} from './parse'; import {validateCommitMessage, ValidateCommitMessageResult} from './validate'; type CommitMessageConfig = validateConfig.CommitMessageConfig; @@ -278,6 +279,78 @@ describe('validate-commit-message.js', () => { }); }); + describe('deprecations', () => { + it('should allow valid deprecation notes in commit messages', () => { + const msgWithListOfDeprecations = + 'feat(compiler): this is just a usual commit message title\n\n' + + 'This is a normal commit message body which does not exceed the max length\n' + + 'limit. For more details see the following super long URL:\n\n' + + 'DEPRECATED:\n' + + ' * A to be removed\n' + + ' * B to be removed'; + expectValidationResult(validateCommitMessage(msgWithListOfDeprecations), VALID); + expect(parseCommitMessage(msgWithListOfDeprecations).deprecations.length).toBe(1); + + const msgWithSummary = 'feat(compiler): this is just a usual commit message title\n\n' + + 'This is a normal commit message body which does not exceed the max length\n' + + 'limit. For more details see the following super long URL:\n\n' + + 'DEPRECATED: All methods in X to be removed in v12.'; + + expectValidationResult(validateCommitMessage(msgWithSummary), VALID); + expect(parseCommitMessage(msgWithSummary).deprecations.length).toBe(1); + + const msgWithSummaryAndDescription = + 'feat(compiler): this is just a usual commit message title\n\n' + + 'This is a normal commit message body which does not exceed the max length\n' + + 'limit. For more details see the following super long URL:\n\n' + + 'DEPRECATED: All methods in X to be removed in v12.\n' + + '' + + 'This is the more detailed description about the deprecation of X.'; + + expectValidationResult(validateCommitMessage(msgWithSummaryAndDescription), VALID); + expect(parseCommitMessage(msgWithSummaryAndDescription).deprecations.length).toBe(1); + + const msgWithNoDeprecation = + 'feat(compiler): this is just a usual commit message title\n\n' + + 'This is not a\n' + + 'deprecation commit.'; + expectValidationResult(validateCommitMessage(msgWithNoDeprecation), VALID); + expect(parseCommitMessage(msgWithNoDeprecation).deprecations.length).toBe(0); + }); + + it('should fail for non-valid deprecation notes in commit messages', () => { + const incorrectKeyword1 = 'feat(compiler): this is just a usual commit message title\n\n' + + 'This is a normal commit message body which does not exceed the max length\n' + + 'limit. For more details see the following super long URL:\n\n' + + 'DEPRECATE:\n' + + ' * A to be removed\n' + + ' * B to be removed'; + expectValidationResult( + validateCommitMessage(incorrectKeyword1), INVALID, + ['The commit message body contains an invalid deprecation note.']); + + const incorrectKeyword2 = 'feat(compiler): this is just a usual commit message title\n\n' + + 'This is a normal commit message body which does not exceed the max length\n' + + 'limit. For more details see the following super long URL:\n\n' + + 'DEPRECATES:\n' + + ' * A to be removed\n' + + ' * B to be removed'; + expectValidationResult( + validateCommitMessage(incorrectKeyword2), INVALID, + ['The commit message body contains an invalid deprecation note.']); + + const incorrectKeyword3 = 'feat(compiler): this is just a usual commit message title\n\n' + + 'This is a normal commit message body which does not exceed the max length\n' + + 'limit. For more details see the following super long URL:\n\n' + + 'DEPRECATIONS:\n' + + ' * A to be removed\n' + + ' * B to be removed'; + expectValidationResult( + validateCommitMessage(incorrectKeyword3), INVALID, + ['The commit message body contains an invalid deprecation note.']); + }); + }); + describe('breaking change', () => { it('should allow valid breaking change commit descriptions', () => { const msgWithSummary = 'feat(compiler): this is just a usual commit message title\n\n' + @@ -285,13 +358,31 @@ describe('validate-commit-message.js', () => { 'limit. For more details see the following super long URL:\n\n' + 'BREAKING CHANGE: This is a summary of a breaking change.'; expectValidationResult(validateCommitMessage(msgWithSummary), VALID); + expect(parseCommitMessage(msgWithSummary).breakingChanges.length).toBe(1); - const msgWithDescription = 'feat(compiler): this is just a usual commit message title\n\n' + + const msgWithDescriptionDoubleLineBreakSeparator = + 'feat(compiler): this is just a usual commit message title\n\n' + 'This is a normal commit message body which does not exceed the max length\n' + 'limit. For more details see the following super long URL:\n\n' + 'BREAKING CHANGE:\n\n' + 'This is a full description of the breaking change.'; - expectValidationResult(validateCommitMessage(msgWithDescription), VALID); + expectValidationResult( + validateCommitMessage(msgWithDescriptionDoubleLineBreakSeparator), VALID); + expect( + parseCommitMessage(msgWithDescriptionDoubleLineBreakSeparator).breakingChanges.length) + .toBe(1); + + const msgWithDescriptionSingleLineBreakSeparator = + 'feat(compiler): this is just a usual commit message title\n\n' + + 'This is a normal commit message body which does not exceed the max length\n' + + 'limit. For more details see the following super long URL:\n\n' + + 'BREAKING CHANGE:\n' + + 'This is a full description of the breaking change.'; + expectValidationResult( + validateCommitMessage(msgWithDescriptionSingleLineBreakSeparator), VALID); + expect( + parseCommitMessage(msgWithDescriptionSingleLineBreakSeparator).breakingChanges.length) + .toBe(1); const msgWithSummaryAndDescription = 'feat(compiler): this is just a usual commit message title\n\n' + @@ -300,11 +391,13 @@ describe('validate-commit-message.js', () => { 'BREAKING CHANGE: This is a summary of a breaking change.\n\n' + 'This is a full description of the breaking change.'; expectValidationResult(validateCommitMessage(msgWithSummaryAndDescription), VALID); + expect(parseCommitMessage(msgWithSummaryAndDescription).breakingChanges.length).toBe(1); const msgWithNonBreaking = 'feat(compiler): this is just a usual commit message title\n\n' + 'This is not a\n' + 'breaking change commit.'; expectValidationResult(validateCommitMessage(msgWithNonBreaking), VALID); + expect(parseCommitMessage(msgWithNonBreaking).breakingChanges.length).toBe(0); }); it('should fail for non-valid breaking change commit descriptions', () => { @@ -314,7 +407,7 @@ describe('validate-commit-message.js', () => { 'BREAKING CHANGE This is a summary of a breaking change.'; expectValidationResult( validateCommitMessage(msgWithSummary), INVALID, - [`The commit message body contains an invalid breaking change description.`]); + [`The commit message body contains an invalid breaking change note.`]); const msgWithPlural = 'feat(compiler): this is just a usual commit message title\n\n' + 'This is a normal commit message body which does not exceed the max length\n' + @@ -322,16 +415,17 @@ describe('validate-commit-message.js', () => { 'BREAKING CHANGES: This is a summary of a breaking change.'; expectValidationResult( validateCommitMessage(msgWithPlural), INVALID, - [`The commit message body contains an invalid breaking change description.`]); + [`The commit message body contains an invalid breaking change note.`]); - const msgWithDescription = 'feat(compiler): this is just a usual commit message title\n\n' + + const msgWithWithDashedKeyword = + 'feat(compiler): this is just a usual commit message title\n\n' + 'This is a normal commit message body which does not exceed the max length\n' + 'limit. For more details see the following super long URL:\n\n' + - 'BREAKING CHANGE:\n' + + 'BREAKING-CHANGE:' + 'This is a full description of the breaking change.'; expectValidationResult( - validateCommitMessage(msgWithDescription), INVALID, - [`The commit message body contains an invalid breaking change description.`]); + validateCommitMessage(msgWithWithDashedKeyword), INVALID, + [`The commit message body contains an invalid breaking change note.`]); const msgWithSummaryAndDescription = 'feat(compiler): this is just a usual commit message title\n\n' + @@ -341,7 +435,34 @@ describe('validate-commit-message.js', () => { 'This is a full description of the breaking change.'; expectValidationResult( validateCommitMessage(msgWithSummaryAndDescription), INVALID, - [`The commit message body contains an invalid breaking change description.`]); + [`The commit message body contains an invalid breaking change note.`]); + + const incorrectKeyword1 = 'feat(compiler): this is just a usual commit message title\n\n' + + 'This is a normal commit message body which does not exceed the max length\n' + + 'limit. For more details see the following super long URL:\n\n' + + 'BREAKING CHANGES:\n' + + ' * A has been removed\n'; + expectValidationResult( + validateCommitMessage(incorrectKeyword1), INVALID, + ['The commit message body contains an invalid breaking change note.']); + + const incorrectKeyword2 = 'feat(compiler): this is just a usual commit message title\n\n' + + 'This is a normal commit message body which does not exceed the max length\n' + + 'limit. For more details see the following super long URL:\n\n' + + 'BREAKING-CHANGE:\n' + + ' * A has been removed\n'; + expectValidationResult( + validateCommitMessage(incorrectKeyword2), INVALID, + ['The commit message body contains an invalid breaking change note.']); + + const incorrectKeyword3 = 'feat(compiler): this is just a usual commit message title\n\n' + + 'This is a normal commit message body which does not exceed the max length\n' + + 'limit. For more details see the following super long URL:\n\n' + + 'BREAKING-CHANGES:\n' + + ' * A has been removed\n'; + expectValidationResult( + validateCommitMessage(incorrectKeyword3), INVALID, + ['The commit message body contains an invalid breaking change note.']); }); }); }); diff --git a/dev-infra/commit-message/validate.ts b/dev-infra/commit-message/validate.ts index 4b630f7a7d..4a9215d602 100644 --- a/dev-infra/commit-message/validate.ts +++ b/dev-infra/commit-message/validate.ts @@ -26,16 +26,29 @@ export interface ValidateCommitMessageResult { /** Regex matching a URL for an entire commit body line. */ const COMMIT_BODY_URL_LINE_RE = /^https?:\/\/.*$/; + /** - * Regex matching a breaking change. + * Regular expression matching potential misuse of the `BREAKING CHANGE:` marker in a + * commit message. Commit messages containing one of the following snippets will fail: * - * - Starts with BREAKING CHANGE - * - Followed by a colon - * - Followed by a single space or two consecutive new lines - * - * NB: Anything after `BREAKING CHANGE` is optional to facilitate the validation. + * - `BREAKING CHANGE ` | Here we assume the colon is missing by accident. + * - `BREAKING-CHANGE: ` | The wrong keyword is used here. + * - `BREAKING CHANGES: ` | The wrong keyword is used here. + * - `BREAKING-CHANGES: ` | The wrong keyword is used here. */ -const COMMIT_BODY_BREAKING_CHANGE_RE = /^BREAKING CHANGE(:( |\n{2}))?/m; +const INCORRECT_BREAKING_CHANGE_BODY_RE = + /^(BREAKING CHANGE[^:]|BREAKING-CHANGE|BREAKING[ -]CHANGES)/m; + +/** + * Regular expression matching potential misuse of the `DEPRECATED:` marker in a commit + * message. Commit messages containing one of the following snippets will fail: + * + * - `DEPRECATED ` | Here we assume the colon is missing by accident. + * - `DEPRECATIONS: ` | The wrong keyword is used here. + * - `DEPRECATE: ` | The wrong keyword is used here. + * - `DEPRECATES: ` | The wrong keyword is used here. + */ +const INCORRECT_DEPRECATION_BODY_RE = /^(DEPRECATED[^:]|DEPRECATIONS|DEPRECATE:|DEPRECATES)/m; /** Validate a commit message against using the local repo's config. */ export function validateCommitMessage( @@ -161,14 +174,14 @@ export function validateCommitMessage( // Breaking change // Check if the commit message contains a valid break change description. // https://github.com/angular/angular/blob/88fbc066775ab1a2f6a8c75f933375b46d8fa9a4/CONTRIBUTING.md#commit-message-footer - const hasBreakingChange = COMMIT_BODY_BREAKING_CHANGE_RE.exec(commit.fullText); - if (hasBreakingChange !== null) { - const [, breakingChangeDescription] = hasBreakingChange; - if (!breakingChangeDescription) { - // Not followed by :, space or two consecutive new lines, - errors.push(`The commit message body contains an invalid breaking change description.`); - return false; - } + if (INCORRECT_BREAKING_CHANGE_BODY_RE.test(commit.fullText)) { + errors.push(`The commit message body contains an invalid breaking change note.`); + return false; + } + + if (INCORRECT_DEPRECATION_BODY_RE.test(commit.fullText)) { + errors.push(`The commit message body contains an invalid deprecation note.`); + return false; } return true; diff --git a/dev-infra/ng-dev.js b/dev-infra/ng-dev.js index fdf9379078..0d27dd2800 100755 --- a/dev-infra/ng-dev.js +++ b/dev-infra/ng-dev.js @@ -1843,7 +1843,7 @@ const parseOptions = { headerPattern, headerCorrespondence, noteKeywords: [NoteSections.BREAKING_CHANGE, NoteSections.DEPRECATED], - notesPattern: (keywords) => new RegExp(`(${keywords})(?:: ?)(.*)`), + notesPattern: (keywords) => new RegExp(`(${keywords}): ?(.*)`), }; /** Parse a commit message into its composite parts. */ const parseCommitMessage = parseInternal; @@ -1902,15 +1902,25 @@ function parseInternal(fullText) { /** Regex matching a URL for an entire commit body line. */ const COMMIT_BODY_URL_LINE_RE = /^https?:\/\/.*$/; /** - * Regex matching a breaking change. + * Regular expression matching potential misuse of the `BREAKING CHANGE:` marker in a + * commit message. Commit messages containing one of the following snippets will fail: * - * - Starts with BREAKING CHANGE - * - Followed by a colon - * - Followed by a single space or two consecutive new lines - * - * NB: Anything after `BREAKING CHANGE` is optional to facilitate the validation. + * - `BREAKING CHANGE ` | Here we assume the colon is missing by accident. + * - `BREAKING-CHANGE: ` | The wrong keyword is used here. + * - `BREAKING CHANGES: ` | The wrong keyword is used here. + * - `BREAKING-CHANGES: ` | The wrong keyword is used here. */ -const COMMIT_BODY_BREAKING_CHANGE_RE = /^BREAKING CHANGE(:( |\n{2}))?/m; +const INCORRECT_BREAKING_CHANGE_BODY_RE = /^(BREAKING CHANGE[^:]|BREAKING-CHANGE|BREAKING[ -]CHANGES)/m; +/** + * Regular expression matching potential misuse of the `DEPRECATED:` marker in a commit + * message. Commit messages containing one of the following snippets will fail: + * + * - `DEPRECATED ` | Here we assume the colon is missing by accident. + * - `DEPRECATIONS: ` | The wrong keyword is used here. + * - `DEPRECATE: ` | The wrong keyword is used here. + * - `DEPRECATES: ` | The wrong keyword is used here. + */ +const INCORRECT_DEPRECATION_BODY_RE = /^(DEPRECATED[^:]|DEPRECATIONS|DEPRECATE:|DEPRECATES)/m; /** Validate a commit message against using the local repo's config. */ function validateCommitMessage(commitMsg, options = {}) { const config = getCommitMessageConfig().commitMessage; @@ -2008,14 +2018,13 @@ function validateCommitMessage(commitMsg, options = {}) { // Breaking change // Check if the commit message contains a valid break change description. // https://github.com/angular/angular/blob/88fbc066775ab1a2f6a8c75f933375b46d8fa9a4/CONTRIBUTING.md#commit-message-footer - const hasBreakingChange = COMMIT_BODY_BREAKING_CHANGE_RE.exec(commit.fullText); - if (hasBreakingChange !== null) { - const [, breakingChangeDescription] = hasBreakingChange; - if (!breakingChangeDescription) { - // Not followed by :, space or two consecutive new lines, - errors.push(`The commit message body contains an invalid breaking change description.`); - return false; - } + if (INCORRECT_BREAKING_CHANGE_BODY_RE.test(commit.fullText)) { + errors.push(`The commit message body contains an invalid breaking change note.`); + return false; + } + if (INCORRECT_DEPRECATION_BODY_RE.test(commit.fullText)) { + errors.push(`The commit message body contains an invalid deprecation note.`); + return false; } return true; }