diff --git a/src/plugins/commit-message/index.js b/src/plugins/commit-message/index.js index be0739c..dade862 100644 --- a/src/plugins/commit-message/index.js +++ b/src/plugins/commit-message/index.js @@ -9,6 +9,12 @@ const { getCommitMessageForPR } = require("../utils"); const TAG_REGEX = /^((?:Breaking|Build|Chore|Docs|Fix|New|Update|Upgrade):|Revert )/; + +const POTENTIAL_ISSUE_REF_REGEX = /#\d+/; + +const VALID_ISSUE_REF = "(?:(?:fixes|refs) #\\d+)"; +const CORRECT_ISSUE_REF_REGEX = new RegExp(` \\(${VALID_ISSUE_REF}(?:, ${VALID_ISSUE_REF})*\\)$`); + const MESSAGE_LENGTH_LIMIT = 72; const EXCLUDED_REPOSITORY_NAMES = new Set([ @@ -23,7 +29,21 @@ const EXCLUDED_REPOSITORY_NAMES = new Set([ * @private */ function checkCommitMessage(message) { - return TAG_REGEX.test(message) && message.split(/\r?\n/)[0].length <= MESSAGE_LENGTH_LIMIT; + + // First, check tag and summary length + let isValid = TAG_REGEX.test(message) && message.split(/\r?\n/)[0].length <= MESSAGE_LENGTH_LIMIT; + + // Then, if there appears to be an issue reference, test for correctness + if (isValid && POTENTIAL_ISSUE_REF_REGEX.test(message)) { + const issueSuffixMatch = CORRECT_ISSUE_REF_REGEX.exec(message); + + // If no suffix, or issue ref occurs before suffix, message is invalid + if (!issueSuffixMatch || POTENTIAL_ISSUE_REF_REGEX.test(message.slice(0, issueSuffixMatch.index))) { + isValid = false; + } + } + + return isValid; } /** diff --git a/tests/plugins/commit-message/index.js b/tests/plugins/commit-message/index.js index 65ecd4d..c7956e6 100644 --- a/tests/plugins/commit-message/index.js +++ b/tests/plugins/commit-message/index.js @@ -190,6 +190,75 @@ describe("commit-message", () => { expect(nockScope.isDone()).toBeTruthy(); }); + // Tests for malformed issue references + [ + "#1", + "fixes #1", + "(fix #1)", + "(ref #1)", + "(closes #1)", + "(fixes #1, #2)", + + // Unexpected issue number with valid suffix should fail + "#1 (fixes #1)" + ].forEach(suffix => { + test(`Posts failure status if the commit message references issue improperly: ${suffix}`, async() => { + mockSingleCommitWithMessage( + `New: foo ${suffix}` + ); + + const nockScope = nock("https://api.github.com") + .post("/repos/test/repo-test/statuses/first-sha", req => req.state === "failure") + .reply(201); + + await emitBotEvent(bot, { action }); + expect(nockScope.isDone()).toBeTruthy(); + }); + + test(`Posts failure status if multiple commits and PR title references issue improperly: ${suffix}`, async() => { + mockMultipleCommits(); + + const nockScope = nock("https://api.github.com") + .post("/repos/test/repo-test/statuses/second-sha", req => req.state === "failure") + .reply(201); + + await emitBotEvent(bot, { action, pull_request: { number: 1, title: `New: foo ${suffix}` } }); + expect(nockScope.isDone()).toBeTruthy(); + }); + }); + + // Tests for correct issue references + [ + "(fixes #1)", + "(refs #1)", + "(fixes #1, fixes #2)", + "(fixes #1, refs #2, fixes #3)" + ].forEach(suffix => { + test(`Posts success status if the commit message references issue correctly: ${suffix}`, async() => { + mockSingleCommitWithMessage( + `New: foo ${suffix}` + ); + + const nockScope = nock("https://api.github.com") + .post("/repos/test/repo-test/statuses/first-sha", req => req.state === "success") + .reply(201); + + await emitBotEvent(bot, { action }); + expect(nockScope.isDone()).toBeTruthy(); + }); + + test(`Posts success status if multiple commits and PR title references issue correctly: ${suffix}`, async() => { + mockMultipleCommits(); + + const nockScope = nock("https://api.github.com") + .post("/repos/test/repo-test/statuses/second-sha", req => req.state === "success") + .reply(201); + + await emitBotEvent(bot, { action, pull_request: { number: 1, title: `New: foo ${suffix}` } }); + expect(nockScope.isDone()).toBeTruthy(); + }); + }); + test("Does not post a status if the repository is excluded", async() => { await emitBotEvent(bot, { action: "opened",