diff --git a/src/domains/configurations/Configuration.fixtures.ts b/src/domains/configurations/Configuration.fixtures.ts index 331f2625..1fa473a3 100644 --- a/src/domains/configurations/Configuration.fixtures.ts +++ b/src/domains/configurations/Configuration.fixtures.ts @@ -18,7 +18,7 @@ export function fakeConfiguration(overrides: ConfigurationTemplate = {}): Config }, rules: { noBlankSubjectLines: {}, - noExcessiveCommitsPerBranch: {}, + noExcessiveCommitsPerBranch: { maxCommits: 10 }, noMergeCommits: {}, noRepeatedSubjectLines: {}, noRestrictedFooterLines: {}, diff --git a/src/domains/configurations/GetConfiguration.tests.ts b/src/domains/configurations/GetConfiguration.tests.ts index 893d8fff..9042eaa1 100644 --- a/src/domains/configurations/GetConfiguration.tests.ts +++ b/src/domains/configurations/GetConfiguration.tests.ts @@ -19,7 +19,7 @@ describe("the default configuration in the command-line", () => { it.each` enabledRuleKey | expectedRuleOptions ${"noBlankSubjectLines"} | ${{}} - ${"noExcessiveCommitsPerBranch"} | ${{}} + ${"noExcessiveCommitsPerBranch"} | ${{ maxCommits: 10 }} ${"noMergeCommits"} | ${{}} ${"noSingleWordSubjectLines"} | ${{}} ${"noRestrictedFooterLines"} | ${{}} @@ -70,7 +70,7 @@ describe("the default configuration in GitHub Actions", () => { it.each` enabledRuleKey | expectedRuleOptions ${"noBlankSubjectLines"} | ${{}} - ${"noExcessiveCommitsPerBranch"} | ${{}} + ${"noExcessiveCommitsPerBranch"} | ${{ maxCommits: 10 }} ${"noMergeCommits"} | ${{}} ${"noRepeatedSubjectLines"} | ${{}} ${"noRestrictedFooterLines"} | ${{}} diff --git a/src/domains/configurations/GetDefaultConfiguration.ts b/src/domains/configurations/GetDefaultConfiguration.ts index 17438d2e..99836d8e 100644 --- a/src/domains/configurations/GetDefaultConfiguration.ts +++ b/src/domains/configurations/GetDefaultConfiguration.ts @@ -22,7 +22,7 @@ function getDefaultCliConfiguration(): Configuration { tokens: getDefaultTokenConfiguration(), rules: { noBlankSubjectLines: {}, - noExcessiveCommitsPerBranch: {}, + noExcessiveCommitsPerBranch: { maxCommits: 10 }, noMergeCommits: {}, noRepeatedSubjectLines: null, noRestrictedFooterLines: {}, @@ -51,7 +51,7 @@ function getDefaultGhaConfiguration(): Configuration { tokens: getDefaultTokenConfiguration(), rules: { noBlankSubjectLines: {}, - noExcessiveCommitsPerBranch: {}, + noExcessiveCommitsPerBranch: { maxCommits: 10 }, noMergeCommits: {}, noRepeatedSubjectLines: {}, noRestrictedFooterLines: {}, diff --git a/src/domains/rules/NoExcessiveCommitsPerBranch.tests.ts b/src/domains/rules/NoExcessiveCommitsPerBranch.tests.ts new file mode 100644 index 00000000..4fe688e6 --- /dev/null +++ b/src/domains/rules/NoExcessiveCommitsPerBranch.tests.ts @@ -0,0 +1,249 @@ +import { describe, expect, it } from "vitest" +import { fakeCommitFactory } from "#commits/Commit.fixtures.ts" +import type { Commit } from "#commits/Commit.ts" +import { fakeConfiguration } from "#configurations/Configuration.fixtures.ts" +import { commitConcern } from "#rules/concerns/CommitConcern.ts" +import type { Concerns } from "#rules/concerns/Concern.ts" +import { noExcessiveCommitsPerBranch } from "#rules/NoExcessiveCommitsPerBranch.ts" +import type { RuleKey, RuleOptions } from "#rules/Rule.ts" +import { fakeCommitSha } from "#types/CommitSha.fixtures.ts" +import type { Vector } from "#types/Vector.ts" + +const rule = "noExcessiveCommitsPerBranch" satisfies RuleKey +const enabled3: RuleOptions = { maxCommits: 3 } +const enabled10: RuleOptions = { maxCommits: 10 } + +const fakeCommit = fakeCommitFactory(fakeConfiguration()) + +describe("when verifying a set of 6 commits when up to 3 commits are allowed", () => { + const commits: Vector = [ + fakeCommit({ message: "Open the bakery dashboard" }), + fakeCommit({ message: "Add the cinnamon telemetry" }), + fakeCommit({ message: "Wire the oat milk alert" }), + fakeCommit({ message: "fix the suspicious croissant counter" }), + fakeCommit({ message: "refactor the jam queue" }), + fakeCommit({ message: "Test the emergency toaster" }), + ] + + describe("and the rule is enabled", () => { + const actualConcerns = noExcessiveCommitsPerBranch(commits, enabled3) + + it("raises concerns about commits 4-6", () => { + expect(actualConcerns).toEqual([ + commitConcern(rule, commits[3].sha), + commitConcern(rule, commits[4].sha), + commitConcern(rule, commits[5].sha), + ]) + }) + }) + + describe("and the rule is disabled", () => { + const actualConcerns = noExcessiveCommitsPerBranch(commits, null) + + it("does not raise any concerns", () => { + expect(actualConcerns).toEqual([]) + }) + }) +}) + +describe("when verifying a set of 3 commits when up to 3 commits are allowed", () => { + const commits: Vector = [ + fakeCommit({ message: "Replace guesswork with a tiny chart" }), + fakeCommit({ message: "Teach the kettle to apologise" }), + fakeCommit({ message: "Document the spooky button" }), + ] + + describe("and the rule is enabled", () => { + const actualConcerns = noExcessiveCommitsPerBranch(commits, enabled3) + + it("does not raise any concerns", () => { + expect(actualConcerns).toEqual([]) + }) + }) + + describe("and the rule is disabled", () => { + const actualConcerns = noExcessiveCommitsPerBranch(commits, null) + + it("does not raise any concerns", () => { + expect(actualConcerns).toEqual([]) + }) + }) +}) + +describe("when verifying a set of 1 commit when up to 3 commits are allowed", () => { + const commits: Vector = [fakeCommit({ message: "label the mystery switch" })] + + describe("and the rule is enabled", () => { + const actualConcerns = noExcessiveCommitsPerBranch(commits, enabled3) + + it("does not raise any concerns", () => { + expect(actualConcerns).toEqual([]) + }) + }) + + describe("and the rule is disabled", () => { + const actualConcerns = noExcessiveCommitsPerBranch(commits, null) + + it("does not raise any concerns", () => { + expect(actualConcerns).toEqual([]) + }) + }) +}) + +describe("when verifying a set of 12 commits when up to 10 commits are allowed", () => { + const commits: Vector = [ + fakeCommit({ message: "Open the emergency snack drawer" }), + fakeCommit({ message: "Close the emergency snack drawer" }), + fakeCommit({ message: "inventory the secret biscuit shelf" }), + fakeCommit({ message: "Name the suspicious toggle" }), + fakeCommit({ message: "Install the typo detector" }), + fakeCommit({ message: "Train the queue to wait politely" }), + fakeCommit({ message: "sort the dashboard mittens" }), + fakeCommit({ message: "Dust the release checklist" }), + fakeCommit({ message: "Label the pocket calculator" }), + fakeCommit({ message: "Patch the waffle endpoint" }), + fakeCommit({ message: "rebalance the pastry index" }), + fakeCommit({ message: "test the emergency toaster again" }), + ] + + describe("and the rule is enabled", () => { + const actualConcerns = noExcessiveCommitsPerBranch(commits, enabled10) + + it("raises concerns about commits 11-12", () => { + expect(actualConcerns).toEqual([ + commitConcern(rule, commits[10].sha), + commitConcern(rule, commits[11].sha), + ]) + }) + }) + + describe("and the rule is disabled", () => { + const actualConcerns = noExcessiveCommitsPerBranch(commits, null) + + it("does not raise any concerns", () => { + expect(actualConcerns).toEqual([]) + }) + }) +}) + +describe("when verifying a set of 10 commits when up to 10 commits are allowed", () => { + const commits: Vector = [ + fakeCommit({ message: "Replace guesswork with a tiny chart" }), + fakeCommit({ message: "Teach the kettle to apologise" }), + fakeCommit({ message: "Document the spooky button" }), + fakeCommit({ message: "shrink the noisy diff" }), + fakeCommit({ message: "Invite the parser to brunch" }), + fakeCommit({ message: "mend the build badge" }), + fakeCommit({ message: "Adjust the polite timeout" }), + fakeCommit({ message: "Archive the sleepy migration" }), + fakeCommit({ message: "Untangle the ribbon config" }), + fakeCommit({ message: "tidy the peanut gallery" }), + ] + + describe("and the rule is enabled", () => { + const actualConcerns = noExcessiveCommitsPerBranch(commits, enabled10) + + it("does not raise any concerns", () => { + expect(actualConcerns).toEqual([]) + }) + }) + + describe("and the rule is disabled", () => { + const actualConcerns = noExcessiveCommitsPerBranch(commits, null) + + it("does not raise any concerns", () => { + expect(actualConcerns).toEqual([]) + }) + }) +}) + +describe("when verifying a set of 4 commits when up to 10 commits are allowed", () => { + const commits: Vector = [ + fakeCommit({ message: "label the mystery switch" }), + fakeCommit({ message: "make the release notes less dramatic" }), + fakeCommit({ message: "Invite the parser to brunch" }), + fakeCommit({ message: "mend the build badge" }), + ] + + describe("and the rule is enabled", () => { + const actualConcerns = noExcessiveCommitsPerBranch(commits, enabled10) + + it("does not raise any concerns", () => { + expect(actualConcerns).toEqual([]) + }) + }) + + describe("and the rule is disabled", () => { + const actualConcerns = noExcessiveCommitsPerBranch(commits, null) + + it("does not raise any concerns", () => { + expect(actualConcerns).toEqual([]) + }) + }) +}) + +describe("when verifying a set of 6 commits when up to 3 commits are allowed and some commits are merge commits", () => { + const commits: Vector = [ + fakeCommit({ message: "Map the odd socks drawer" }), + fakeCommit({ + parents: [fakeCommitSha(), fakeCommitSha()], + message: "Merge branch 'main' into feature/odd-socks", + }), + fakeCommit({ message: "Add the cardigan forecast" }), + fakeCommit({ + parents: [fakeCommitSha(), fakeCommitSha(), fakeCommitSha()], + message: "Keep the branch vaguely current", + }), + fakeCommit({ message: "tune the lint whistle" }), + fakeCommit({ message: "Polish the biscuit dashboard" }), + ] + + describe("and the rule is enabled", () => { + const actualConcerns = noExcessiveCommitsPerBranch(commits, enabled3) + + it("ignores merge commits when determining excessive commits", () => { + expect(actualConcerns).toEqual([commitConcern(rule, commits[5].sha)]) + }) + }) + + describe("and the rule is disabled", () => { + const actualConcerns = noExcessiveCommitsPerBranch(commits, null) + + it("does not raise any concerns", () => { + expect(actualConcerns).toEqual([]) + }) + }) +}) + +describe("when verifying a set of 8 commits when up to 3 commits are allowed and some commits have squash markers", () => { + const commits: Vector = [ + fakeCommit({ message: "Teach the calendar to blink" }), + fakeCommit({ message: "fixup! Teach the calendar to blink" }), + fakeCommit({ message: "Replace the mysterious toggle" }), + fakeCommit({ message: "this commit is a lie" }), + fakeCommit({ message: "Document the noodle switch" }), + fakeCommit({ message: " squash! fixup! make the tests less theatrical" }), + fakeCommit({ message: "Calm down the invoice parser" }), + fakeCommit({ message: "I must have been drunk" }), + ] + + describe("and the rule is enabled", () => { + const actualConcerns = noExcessiveCommitsPerBranch(commits, enabled3) + + it("ignores commits with squash markers when determining excessive commits", () => { + expect(actualConcerns).toEqual([ + commitConcern(rule, commits[4].sha), + commitConcern(rule, commits[6].sha), + commitConcern(rule, commits[7].sha), + ]) + }) + }) + + describe("and the rule is disabled", () => { + const actualConcerns = noExcessiveCommitsPerBranch(commits, null) + + it("does not raise any concerns", () => { + expect(actualConcerns).toEqual([]) + }) + }) +}) diff --git a/src/domains/rules/NoExcessiveCommitsPerBranch.ts b/src/domains/rules/NoExcessiveCommitsPerBranch.ts index e8a904f6..04fc8e74 100644 --- a/src/domains/rules/NoExcessiveCommitsPerBranch.ts +++ b/src/domains/rules/NoExcessiveCommitsPerBranch.ts @@ -1,20 +1,39 @@ import type { Commit, Commits } from "#commits/Commit.ts" +import { commitConcern } from "#rules/concerns/CommitConcern.ts" import type { Concern, Concerns } from "#rules/concerns/Concern.ts" import type { RuleKey, RuleOptions } from "#rules/Rule.ts" -import type { EmptyObject } from "#types/EmptyObject.ts" -import { notNullish } from "#utilities/Arrays.ts" -const _rule = "noExcessiveCommitsPerBranch" satisfies RuleKey +const rule = "noExcessiveCommitsPerBranch" satisfies RuleKey +/** + * Verifies that the branch does not contain more than a given number of commits. + * + * Keeping pull requests small makes them easier to review and easier to revert if needed. + * It may also help to catch accidental rebases onto wrong branches or stale commits. + * + * It ignores merge commits and commits with squash markers. + */ export function noExcessiveCommitsPerBranch( commits: Commits, - options: EmptyObject | null, + options: { maxCommits: number } | null, ): Concerns { - return options !== null - ? commits.map((commit) => verifyCommit(commit, options)).filter(notNullish) - : [] + return options !== null ? [...verifyCommits(commits, options)] : [] } -function verifyCommit(_commit: Commit, _options: RuleOptions): Concern | null { - throw new Error("The `noExcessiveCommitsPerBranch` rule has not been implemented yet") // TODO: To be implemented. +function* verifyCommits(commits: Commits, options: RuleOptions): Generator { + let commitCount = 0 + + for (const commit of commits) { + if (!commit.isMergeCommit && !hasSquashMarker(commit)) { + commitCount += 1 + + if (commitCount > options.maxCommits) { + yield commitConcern(rule, commit.sha) + } + } + } +} + +function hasSquashMarker(commit: Commit): boolean { + return commit.subjectLine[0]?.type === "squash-marker" } diff --git a/src/domains/rules/reports/CommitwiseReport.tests.ts b/src/domains/rules/reports/CommitwiseReport.tests.ts index 92da194b..1a28c426 100644 --- a/src/domains/rules/reports/CommitwiseReport.tests.ts +++ b/src/domains/rules/reports/CommitwiseReport.tests.ts @@ -69,6 +69,81 @@ describe("when 'noBlankSubjectLines' has a concern about characters 15-16 of the }) }) +describe("when 'noExcessiveCommitsPerBranch' has a concern about an excessive commit when the limit is 1", () => { + const configuration = fakeConfiguration({ + rules: { noExcessiveCommitsPerBranch: { maxCommits: 1 } }, + }) + const fakeCommit = fakeCommitFactory(configuration) + + const commit = fakeCommit({ + sha: "9a7e6aa14b8c6e5dd49d7a6a18443bf1f67c520", + message: "invite the parser to brunch", + }) + const concern = commitConcern("noExcessiveCommitsPerBranch", commit.sha) + + it("describes the rule violation in the commit", () => { + const actualOutput = commitwiseReport([concern], [commit], configuration) + expect(actualOutput).toBe( + ` +9a7e6aa invite the parser to brunch + ╭──────────────────────────── + ╰─ Branches must not contain more than 1 commit. + (noExcessiveCommitsPerBranch) +`.trim(), + ) + }) +}) + +describe("when 'noExcessiveCommitsPerBranch' has a concern about an excessive commit when the limit is 3", () => { + const configuration = fakeConfiguration({ + rules: { noExcessiveCommitsPerBranch: { maxCommits: 3 } }, + }) + const fakeCommit = fakeCommitFactory(configuration) + + const commit = fakeCommit({ + sha: "75bedf83e2b573dc812eba9f14f2b7c6741e670", + message: "Refactor the jam queue", + }) + const concern = commitConcern("noExcessiveCommitsPerBranch", commit.sha) + + it("describes the rule violation in the commit", () => { + const actualOutput = commitwiseReport([concern], [commit], configuration) + expect(actualOutput).toBe( + ` +75bedf8 Refactor the jam queue + ╭─────────────────────── + ╰─ Branches must not contain more than 3 commits. + (noExcessiveCommitsPerBranch) +`.trim(), + ) + }) +}) + +describe("when 'noExcessiveCommitsPerBranch' has a concern about an excessive commit when the limit is 10", () => { + const configuration = fakeConfiguration({ + rules: { noExcessiveCommitsPerBranch: { maxCommits: 10 } }, + }) + const fakeCommit = fakeCommitFactory(configuration) + + const commit = fakeCommit({ + sha: "f753a247a54bb4c20e9968dca75ef915f2b1ca", + message: "last minute fix", + }) + const concern = commitConcern("noExcessiveCommitsPerBranch", commit.sha) + + it("describes the rule violation in the commit", () => { + const actualOutput = commitwiseReport([concern], [commit], configuration) + expect(actualOutput).toBe( + ` +f753a24 last minute fix + ╭──────────────── + ╰─ Branches must not contain more than 10 commits. + (noExcessiveCommitsPerBranch) +`.trim(), + ) + }) +}) + describe("when 'noMergeCommits' has a concern about the commit", () => { const configuration = fakeConfiguration() const fakeCommit = fakeCommitFactory(configuration) diff --git a/src/domains/rules/reports/CommitwiseReport.ts b/src/domains/rules/reports/CommitwiseReport.ts index fdc622b3..92500dc5 100644 --- a/src/domains/rules/reports/CommitwiseReport.ts +++ b/src/domains/rules/reports/CommitwiseReport.ts @@ -210,7 +210,9 @@ function getRuleMessage(rule: RuleKey, configuration: Configuration): RuleMessag return ruleMessage("Subject lines must contain at least one non-whitespace character.") } case "noExcessiveCommitsPerBranch": { - throw new Error(`Not implemented yet: ${rule}`) + const options = getRuleOptions(rule, configuration) + const commitPhrase = formatCount(options.maxCommits, "commit", "commits") + return ruleMessage(`Branches must not contain more than ${commitPhrase}.`) } case "noMergeCommits": { return ruleMessage("Merge commits are not allowed.")