Merge branch 'dont-fail-danger-with-more-than-10-commits-with-squash' into 'master'

Lint MR title instead of commits when squash is enabled

Closes #194230 and #34148

See merge request gitlab-org/gitlab!22213

danger/changelog/Dangerfile

@@ -38,9 +38,13 @@ rescue StandardError => e
   warn "There was a problem trying to check the Changelog. Exception: #{e.name} - #{e.message}"
 end
 
+def sanitized_mr_title
+  helper.sanitize_mr_title(gitlab.mr_json["title"])
+end
+
 if git.modified_files.include?("CHANGELOG.md")
   fail "**CHANGELOG.md was edited.** Please remove the additions and create a CHANGELOG entry.\n\n" +
-    format(CREATE_CHANGELOG_MESSAGE, mr_iid: gitlab.mr_json["iid"], mr_title: changelog.sanitized_mr_title, labels: changelog.presented_no_changelog_labels)
+    format(CREATE_CHANGELOG_MESSAGE, mr_iid: gitlab.mr_json["iid"], mr_title: sanitized_mr_title, labels: changelog.presented_no_changelog_labels)
 end
 
 changelog_found = changelog.found
@@ -50,6 +54,6 @@ if changelog.needed?
     check_changelog(changelog_found)
   else
     message "**[CHANGELOG missing](https://docs.gitlab.com/ce/development/changelog.html)**: If this merge request [doesn't need a CHANGELOG entry](https://docs.gitlab.com/ee/development/changelog.html#what-warrants-a-changelog-entry), feel free to ignore this message.\n\n" +
-      format(CREATE_CHANGELOG_MESSAGE, mr_iid: gitlab.mr_json["iid"], mr_title: changelog.sanitized_mr_title, labels: changelog.presented_no_changelog_labels)
+      format(CREATE_CHANGELOG_MESSAGE, mr_iid: gitlab.mr_json["iid"], mr_title: sanitized_mr_title, labels: changelog.presented_no_changelog_labels)
   end
 end

danger/commit_messages/Dangerfile

 # frozen_string_literal: true
 
-require 'json'
+require_relative File.expand_path('../../lib/gitlab/danger/commit_linter', __dir__)
 
-URL_LIMIT_SUBJECT = "https://chris.beams.io/posts/git-commit/#limit-50"
 URL_GIT_COMMIT = "https://chris.beams.io/posts/git-commit/"
-
-# rubocop: disable Style/SignalException
-# rubocop: disable Metrics/CyclomaticComplexity
-# rubocop: disable Metrics/PerceivedComplexity
-
-# Perform various checks against commits. We're not using
-# https://github.com/jonallured/danger-commit_lint because its output is not
-# very helpful, and it doesn't offer the means of ignoring merge commits.
-
-class EmojiChecker
-  DIGESTS = File.expand_path('../../fixtures/emojis/digests.json', __dir__)
-  ALIASES = File.expand_path('../../fixtures/emojis/aliases.json', __dir__)
-
-  # A regex that indicates a piece of text _might_ include an Emoji. The regex
-  # alone is not enough, as we'd match `:foo:bar:baz`. Instead, we use this
-  # regex to save us from having to check for all possible emoji names when we
-  # know one definitely is not included.
-  LIKELY_EMOJI = /:[\+a-z0-9_\-]+:/.freeze
-
-  def initialize
-    names = JSON.parse(File.read(DIGESTS)).keys +
-      JSON.parse(File.read(ALIASES)).keys
-
-    @emoji = names.map { |name| ":#{name}:" }
-  end
-
-  def includes_emoji?(text)
-    return false unless text.match?(LIKELY_EMOJI)
-
-    @emoji.any? { |emoji| text.include?(emoji) }
-  end
-end
+MAX_COMMITS_COUNT = 10
 
 def gitlab_danger
   @gitlab_danger ||= GitlabDanger.new(helper.gitlab_helper)
 end
 
 def fail_commit(commit, message)
-  fail("#{commit.sha}: #{message}")
+  self.fail("#{commit.sha}: #{message}")
 end
 
 def warn_commit(commit, message)
-  warn("#{commit.sha}: #{message}")
+  self.warn("#{commit.sha}: #{message}")
 end
 
-def lines_changed_in_commit(commit)
-  commit.diff_parent.stats[:total][:lines]
+def squash_mr?
+  gitlab_danger.ci? ? gitlab.mr_json['squash'] : false
 end
 
-def subject_starts_with_capital?(subject)
-  first_char = subject.chars.first
-
-  first_char.upcase == first_char
+def wip_mr?
+  gitlab_danger.ci? ? gitlab.mr_json['work_in_progress'] : false
 end
 
-def too_many_changed_lines?(commit)
-  commit.diff_parent.stats[:total][:files] > 3 &&
-    lines_changed_in_commit(commit) >= 30
-end
-
-def emoji_checker
-  @emoji_checker ||= EmojiChecker.new
-end
-
-def unicode_emoji_regex
-  @unicode_emoji_regex ||= %r((
-    [\u{1F300}-\u{1F5FF}] |
-    [\u{1F1E6}-\u{1F1FF}] |
-    [\u{2700}-\u{27BF}] |
-    [\u{1F900}-\u{1F9FF}] |
-    [\u{1F600}-\u{1F64F}] |
-    [\u{1F680}-\u{1F6FF}] |
-    [\u{2600}-\u{26FF}]
-  ))x
-end
-
-def count_filtered_commits(commits)
-  commits.count do |commit|
-    !commit.message.start_with?('fixup!', 'squash!')
-  end
-end
+# Perform various checks against commits. We're not using
+# https://github.com/jonallured/danger-commit_lint because its output is not
+# very helpful, and it doesn't offer the means of ignoring merge commits.
+def lint_commit(commit)
+  linter = Gitlab::Danger::CommitLinter.new(commit)
 
-def lint_commit(commit) # rubocop:disable Metrics/AbcSize
   # For now we'll ignore merge commits, as getting rid of those is a problem
   # separate from enforcing good commit messages.
-  return false if commit.message.start_with?('Merge branch')
+  return linter if linter.merge?
 
   # We ignore revert commits as they are well structured by Git already
-  return false if commit.message.start_with?('Revert "')
+  return linter if linter.revert?
 
-  is_squash = gitlab_danger.ci? ? gitlab.mr_json['squash'] : false
-  is_wip = gitlab_danger.ci? ? gitlab.mr_json['work_in_progress'] : false
-  is_fixup = commit.message.start_with?('fixup!', 'squash!')
-
-  if is_fixup
-    # The MR is set to squash - Danger adds an informative notice
-    # The MR is not set to squash - Danger fails. if also WIP warn only, not error
-    if is_squash
-      return false
-    end
+  # If MR is set to squash, we ignore fixup commits
+  return linter if linter.fixup? && squash_mr?
 
-    if is_wip
-      warn_commit(
-        commit,
-        'Squash or Fixup commits must be squashed before merge, or enable squash merge option'
-      )
+  if linter.fixup?
+    msg = 'Squash or fixup commits must be squashed before merge, or enable squash merge option'
+    if wip_mr? || squash_mr?
+      warn_commit(commit, msg)
     else
-      fail_commit(
-        commit,
-        'Squash or Fixup commits must be squashed before merge, or enable squash merge option'
-      )
+      fail_commit(commit, msg)
     end
 
     # Makes no sense to process other rules for fixup commits, they trigger just more noise
-    return false
+    return linter
   end
 
   # Fail if a suggestion commit is used and squash is not enabled
-  if commit.message.start_with?('Apply suggestion to')
-    if is_squash
-      return false
-    else
-      fail_commit(
-        commit,
-        'If you are applying suggestions, enable squash in the merge request and re-run the failed job'
-      )
-      return true
+  if linter.suggestion?
+    unless squash_mr?
+      fail_commit(commit, "If you are applying suggestions, enable squash in the merge request and re-run the `danger-review` job")
     end
+
+    return linter
   end
 
-  failures = false
-  subject, separator, details = commit.message.split("\n", 3)
+  linter.lint
+end
 
-  if subject.split.length < 3
-    fail_commit(
-      commit,
-      'The commit subject must contain at least three words'
-    )
+def lint_mr_title(mr_title)
+  commit = Struct.new(:message, :sha).new(mr_title)
 
-    failures = true
-  end
+  Gitlab::Danger::CommitLinter.new(commit).lint_subject("merge request title")
+end
 
-  if subject.length > 72
-    fail_commit(
-      commit,
-      'The commit subject may not be longer than 72 characters'
-    )
-
-    failures = true
-  elsif subject.length > 50
-    warn_commit(
-      commit,
-      "This commit's subject line is acceptable, but please try to [reduce it to 50 characters](#{URL_LIMIT_SUBJECT})."
-    )
-  end
+def count_non_fixup_commits(commit_linters)
+  commit_linters.count { |commit_linter| !commit_linter.fixup? }
+end
 
-  unless subject_starts_with_capital?(subject)
-    fail_commit(commit, 'The commit subject must start with a capital letter')
-    failures = true
-  end
+def lint_commits(commits)
+  commit_linters = commits.map { |commit| lint_commit(commit) }
+  failed_commit_linters = commit_linters.select { |commit_linter| commit_linter.failed? }
+  warn_or_fail_commits(failed_commit_linters, default_to_fail: !squash_mr?)
 
-  if subject.end_with?('.')
-    fail_commit(commit, 'The commit subject must not end with a period')
-    failures = true
+  if count_non_fixup_commits(commit_linters) > MAX_COMMITS_COUNT
+    level = squash_mr? ? :warn : :fail
+    self.__send__(level, # rubocop:disable GitlabSecurity/PublicSend
+      "This merge request includes more than #{MAX_COMMITS_COUNT} commits. " \
+        'Please rebase these commits into a smaller number of commits or split ' \
+        'this merge request into multiple smaller merge requests.')
   end
 
-  if separator && !separator.empty?
-    fail_commit(
-      commit,
-      'The commit subject and body must be separated by a blank line'
-    )
+  if squash_mr?
+    multi_line_commit_linter = commit_linters.detect { |commit_linter| commit_linter.multi_line? }
 
-    failures = true
+    if multi_line_commit_linter && multi_line_commit_linter.lint.failed?
+      warn_or_fail_commits(multi_line_commit_linter)
+      fail_message('The commit message that will be used in the squash commit does not meet our Git commit message standards.')
+    else
+      title_linter = lint_mr_title(gitlab.mr_json['title'])
+      if title_linter.failed?
+        warn_or_fail_commits(title_linter)
+        fail_message('The merge request title that will be used in the squash commit does not meet our Git commit message standards.')
       end
-
-  details&.each_line do |line|
-    line = line.strip
-
-    next if line.length <= 72
-
-    url_size = line.scan(%r((https?://\S+))).sum { |(url)| url.length }
-
-    # If the line includes a URL, we'll allow it to exceed 72 characters, but
-    # only if the line _without_ the URL does not exceed this limit.
-    next if line.length - url_size <= 72
-
-    fail_commit(
-      commit,
-      'The commit body should not contain more than 72 characters per line'
-    )
-
-    failures = true
     end
-
-  if !details && too_many_changed_lines?(commit)
-    fail_commit(
-      commit,
-      'Commits that change 30 or more lines across at least three files ' \
-        'must describe these changes in the commit body'
-    )
-
-    failures = true
+  else
+    if failed_commit_linters.any?
+      fail_message('One or more commit messages do not meet our Git commit message standards.')
     end
-
-  if emoji_checker.includes_emoji?(commit.message)
-    warn_commit(
-      commit,
-      'Avoid the use of Markdown Emoji such as `:+1:`. ' \
-        'These add limited value to the commit message, ' \
-        'and are displayed as plain text outside of GitLab'
-    )
-
-    failures = true
   end
+end
 
-  if commit.message.match?(unicode_emoji_regex)
-    fail_commit(
-      commit,
-      'Avoid the use of Unicode Emoji. ' \
-        'These add no value to the commit message, ' \
-        'and may not be displayed properly everywhere'
-    )
+def warn_or_fail_commits(failed_linters, default_to_fail: true)
+  level = default_to_fail ? :fail : :warn
 
-    failures = true
+  Array(failed_linters).each do |linter|
+    linter.problems.each do |problem_key, problem_desc|
+      case problem_key
+      when :subject_above_warning
+        warn_commit(linter.commit, problem_desc)
+      else
+        self.__send__("#{level}_commit", linter.commit, problem_desc) # rubocop:disable GitlabSecurity/PublicSend
       end
-
-  if commit.message.match?(%r(([\w\-\/]+)?(#|!|&|%)\d+\b))
-    fail_commit(
-      commit,
-      'Use full URLs instead of short references ' \
-        '(`gitlab-org/gitlab#123` or `!123`), as short references are ' \
-        'displayed as plain text outside of GitLab'
-    )
-
-    failures = true
     end
-
-  failures
-end
-
-def lint_commits(commits)
-  failed = commits.select do |commit|
-    lint_commit(commit)
   end
+end
 
-  if failed.any?
+def fail_message(intro)
   markdown(<<~MARKDOWN)
     ## Commit message standards
 
-      One or more commit messages do not meet our Git commit message standards.
+    #{intro}
+
     For more information on how to write a good commit message, take a look at
     [How to Write a Git Commit Message](#{URL_GIT_COMMIT}).
 
@@ -282,14 +157,6 @@ def lint_commits(commits)
     This commit message is bad because although it tells us that README.md is
     updated, it doesn't tell us why or how it was updated.
   MARKDOWN
-  end
 end
 
 lint_commits(git.commits)
-
-if count_filtered_commits(git.commits) > 10
-  fail(
-    'This merge request includes more than 10 commits. ' \
-      'Please rebase these commits into a smaller number of commits.'
-  )
-end

lib/gitlab/danger/commit_linter.rb

+# frozen_string_literal: true
+
+emoji_checker_path = File.expand_path('emoji_checker', __dir__)
+defined?(Rails) ? require_dependency(emoji_checker_path) : require_relative(emoji_checker_path)
+
+module Gitlab
+  module Danger
+    class CommitLinter
+      MIN_SUBJECT_WORDS_COUNT = 3
+      MAX_LINE_LENGTH = 72
+      WARN_SUBJECT_LENGTH = 50
+      URL_LIMIT_SUBJECT = "https://chris.beams.io/posts/git-commit/#limit-50"
+      MAX_CHANGED_FILES_IN_COMMIT = 3
+      MAX_CHANGED_LINES_IN_COMMIT = 30
+      SHORT_REFERENCE_REGEX = %r{([\w\-\/]+)?(#|!|&|%)\d+\b}.freeze
+      DEFAULT_SUBJECT_DESCRIPTION = 'commit subject'
+      PROBLEMS = {
+        subject_too_short: "The %s must contain at least #{MIN_SUBJECT_WORDS_COUNT} words",
+        subject_too_long: "The %s may not be longer than #{MAX_LINE_LENGTH} characters",
+        subject_above_warning: "The %s length is acceptable, but please try to [reduce it to #{WARN_SUBJECT_LENGTH} characters](#{URL_LIMIT_SUBJECT}).",
+        subject_starts_with_lowercase: "The %s must start with a capital letter",
+        subject_ends_with_a_period: "The %s must not end with a period",
+        separator_missing: "The commit subject and body must be separated by a blank line",
+        details_too_many_changes: "Commits that change #{MAX_CHANGED_LINES_IN_COMMIT} or more lines across " \
+          "at least #{MAX_CHANGED_FILES_IN_COMMIT} files must describe these changes in the commit body",
+        details_line_too_long: "The commit body should not contain more than #{MAX_LINE_LENGTH} characters per line",
+        message_contains_text_emoji: "Avoid the use of Markdown Emoji such as `:+1:`. These add limited value " \
+          "to the commit message, and are displayed as plain text outside of GitLab",
+        message_contains_unicode_emoji: "Avoid the use of Unicode Emoji. These add no value to the commit " \
+          "message, and may not be displayed properly everywhere",
+        message_contains_short_reference: "Use full URLs instead of short references (`gitlab-org/gitlab#123` or " \
+          "`!123`), as short references are displayed as plain text outside of GitLab"
+      }.freeze
+
+      attr_reader :commit, :problems
+
+      def initialize(commit)
+        @commit = commit
+        @problems = {}
+        @linted = false
+      end
+
+      def fixup?
+        commit.message.start_with?('fixup!', 'squash!')
+      end
+
+      def suggestion?
+        commit.message.start_with?('Apply suggestion to')
+      end
+
+      def merge?
+        commit.message.start_with?('Merge branch')
+      end
+
+      def revert?
+        commit.message.start_with?('Revert "')
+      end
+
+      def multi_line?
+        !details.nil? && !details.empty?
+      end
+
+      def failed?
+        problems.any?
+      end
+
+      def add_problem(problem_key, *args)
+        @problems[problem_key] = sprintf(PROBLEMS[problem_key], *args)
+      end
+
+      def lint(subject_description = "commit subject")
+        return self if @linted
+
+        @linted = true
+        lint_subject(subject_description)
+        lint_separator
+        lint_details
+        lint_message
+
+        self
+      end
+
+      def lint_subject(subject_description)
+        if subject_too_short?
+          add_problem(:subject_too_short, subject_description)
+        end
+
+        if subject_too_long?
+          add_problem(:subject_too_long, subject_description)
+        elsif subject_above_warning?
+          add_problem(:subject_above_warning, subject_description)
+        end
+
+        if subject_starts_with_lowercase?
+          add_problem(:subject_starts_with_lowercase, subject_description)
+        end
+
+        if subject_ends_with_a_period?
+          add_problem(:subject_ends_with_a_period, subject_description)
+        end
+
+        self
+      end
+
+      private
+
+      def lint_separator
+        return self unless separator && !separator.empty?
+
+        add_problem(:separator_missing)
+
+        self
+      end
+
+      def lint_details
+        if !multi_line? && many_changes?
+          add_problem(:details_too_many_changes)
+        end
+
+        details&.each_line do |line|
+          line = line.strip
+
+          next unless line_too_long?(line)
+
+          url_size = line.scan(%r((https?://\S+))).sum { |(url)| url.length } # rubocop:disable CodeReuse/ActiveRecord
+
+          # If the line includes a URL, we'll allow it to exceed MAX_LINE_LENGTH characters, but
+          # only if the line _without_ the URL does not exceed this limit.
+          next unless line_too_long?(line.length - url_size)
+
+          add_problem(:details_line_too_long)
+          break
+        end
+
+        self
+      end
+
+      def lint_message
+        if message_contains_text_emoji?
+          add_problem(:message_contains_text_emoji)
+        end
+
+        if message_contains_unicode_emoji?
+          add_problem(:message_contains_unicode_emoji)
+        end
+
+        if message_contains_short_reference?
+          add_problem(:message_contains_short_reference)
+        end
+
+        self
+      end
+
+      def files_changed
+        commit.diff_parent.stats[:total][:files]
+      end
+
+      def lines_changed
+        commit.diff_parent.stats[:total][:lines]
+      end
+
+      def many_changes?
+        files_changed > MAX_CHANGED_FILES_IN_COMMIT && lines_changed > MAX_CHANGED_LINES_IN_COMMIT
+      end
+
+      def subject
+        message_parts[0]
+      end
+
+      def separator
+        message_parts[1]
+      end
+
+      def details
+        message_parts[2]
+      end
+
+      def line_too_long?(line)
+        case line
+        when String
+          line.length > MAX_LINE_LENGTH
+        when Integer
+          line > MAX_LINE_LENGTH
+        else
+          raise ArgumentError, "The line argument (#{line}) should be a String or an Integer! #{line.class} given."
+        end
+      end
+
+      def subject_too_short?
+        subject.split(' ').length < MIN_SUBJECT_WORDS_COUNT
+      end
+
+      def subject_too_long?
+        line_too_long?(subject)
+      end
+
+      def subject_above_warning?
+        subject.length > WARN_SUBJECT_LENGTH
+      end
+
+      def subject_starts_with_lowercase?
+        first_char = subject[0]
+
+        first_char.downcase == first_char
+      end
+
+      def subject_ends_with_a_period?
+        subject.end_with?('.')
+      end
+
+      def message_contains_text_emoji?
+        emoji_checker.includes_text_emoji?(commit.message)
+      end
+
+      def message_contains_unicode_emoji?
+        emoji_checker.includes_unicode_emoji?(commit.message)
+      end
+
+      def message_contains_short_reference?
+        commit.message.match?(SHORT_REFERENCE_REGEX)
+      end
+
+      def emoji_checker
+        @emoji_checker ||= Gitlab::Danger::EmojiChecker.new
+      end
+
+      def message_parts
+        @message_parts ||= commit.message.split("\n", 3)
+      end
+    end
+  end
+end

lib/gitlab/danger/emoji_checker.rb

+# frozen_string_literal: true
+
+require 'json'
+
+module Gitlab
+  module Danger
+    class EmojiChecker
+      DIGESTS = File.expand_path('../../../fixtures/emojis/digests.json', __dir__)
+      ALIASES = File.expand_path('../../../fixtures/emojis/aliases.json', __dir__)
+
+      # A regex that indicates a piece of text _might_ include an Emoji. The regex
+      # alone is not enough, as we'd match `:foo:bar:baz`. Instead, we use this
+      # regex to save us from having to check for all possible emoji names when we
+      # know one definitely is not included.
+      LIKELY_EMOJI = /:[\+a-z0-9_\-]+:/.freeze
+
+      UNICODE_EMOJI_REGEX = %r{(
+        [\u{1F300}-\u{1F5FF}] |
+        [\u{1F1E6}-\u{1F1FF}] |
+        [\u{2700}-\u{27BF}] |
+        [\u{1F900}-\u{1F9FF}] |
+        [\u{1F600}-\u{1F64F}] |
+        [\u{1F680}-\u{1F6FF}] |
+        [\u{2600}-\u{26FF}]
+      )}x.freeze
+
+      def initialize
+        names = JSON.parse(File.read(DIGESTS)).keys +
+          JSON.parse(File.read(ALIASES)).keys
+
+        @emoji = names.map { |name| ":#{name}:" }
+      end
+
+      def includes_text_emoji?(text)
+        return false unless text.match?(LIKELY_EMOJI)
+
+        @emoji.any? { |emoji| text.include?(emoji) }
+      end
+
+      def includes_unicode_emoji?(text)
+        text.match?(UNICODE_EMOJI_REGEX)
+      end
+    end
+  end
+end

lib/gitlab/danger/helper.rb

@@ -174,6 +174,10 @@ module Gitlab
         labels - current_mr_labels
       end
 
+      def sanitize_mr_title(title)
+        title.gsub(/^WIP: */, '').gsub(/`/, '\\\`')
+      end
+
       def security_mr?
         return false unless gitlab_helper
 

spec/lib/gitlab/danger/changelog_spec.rb

@@ -106,18 +106,6 @@ describe Gitlab::Danger::Changelog do
     end
   end
 
-  describe '#sanitized_mr_title' do
-    subject { changelog.sanitized_mr_title }
-
-    [
-      'WIP: My MR title',
-      'My MR title'
-    ].each do |mr_title|
-      let(:mr_json) { { "title" => mr_title } }
-      it { is_expected.to eq("My MR title") }
-    end
-  end
-
   describe '#ee_changelog?' do
     context 'is ee changelog' do
       [

spec/lib/gitlab/danger/commit_linter_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+require 'rspec-parameterized'
+require_relative 'danger_spec_helper'
+
+require 'gitlab/danger/commit_linter'
+
+describe Gitlab::Danger::CommitLinter do
+  using RSpec::Parameterized::TableSyntax
+
+  let(:total_files_changed) { 2 }
+  let(:total_lines_changed) { 10 }
+  let(:stats) { { total: { files: total_files_changed, lines: total_lines_changed } } }
+  let(:diff_parent) { Struct.new(:stats).new(stats) }
+  let(:commit_class) do
+    Struct.new(:message, :sha, :diff_parent)
+  end
+  let(:commit_message) { 'A commit message' }
+  let(:commit_sha) { 'abcd1234' }
+  let(:commit) { commit_class.new(commit_message, commit_sha, diff_parent) }
+
+  subject(:commit_linter) { described_class.new(commit) }
+
+  describe '#fixup?' do
+    where(:commit_message, :is_fixup) do
+      'A commit message' | false
+      'fixup!' | true
+      'fixup! A commit message' | true
+      'squash!' | true
+      'squash! A commit message' | true
+    end
+
+    with_them do
+      it 'is true when commit message starts with "fixup!" or "squash!"' do
+        expect(commit_linter.fixup?).to be(is_fixup)
+      end
+    end
+  end
+
+  describe '#suggestion?' do
+    where(:commit_message, :is_suggestion) do
+      'A commit message' | false
+      'Apply suggestion to' | true
+      'Apply suggestion to "A commit message"' | true
+    end
+
+    with_them do
+      it 'is true when commit message starts with "Apply suggestion to"' do
+        expect(commit_linter.suggestion?).to be(is_suggestion)
+      end
+    end
+  end
+
+  describe '#merge?' do
+    where(:commit_message, :is_merge) do
+      'A commit message' | false
+      'Merge branch' | true
+      'Merge branch "A commit message"' | true
+    end
+
+    with_them do
+      it 'is true when commit message starts with "Merge branch"' do
+        expect(commit_linter.merge?).to be(is_merge)
+      end
+    end
+  end
+
+  describe '#revert?' do
+    where(:commit_message, :is_revert) do
+      'A commit message' | false
+      'Revert' | false
+      'Revert "' | true
+      'Revert "A commit message"' | true
+    end
+
+    with_them do
+      it 'is true when commit message starts with "Revert \""' do
+        expect(commit_linter.revert?).to be(is_revert)
+      end
+    end
+  end
+
+  describe '#multi_line?' do
+    where(:commit_message, :is_multi_line) do
+      "A commit message" | false
+      "A commit message\n" | false
+      "A commit message\n\n" | false
+      "A commit message\n\nWith details" | true
+    end
+
+    with_them do
+      it 'is true when commit message contains details' do
+        expect(commit_linter.multi_line?).to be(is_multi_line)
+      end
+    end
+  end
+
+  describe '#failed?' do
+    context 'with no failures' do
+      it { expect(commit_linter).not_to be_failed }
+    end
+
+    context 'with failures' do
+      before do
+        commit_linter.add_problem(:details_line_too_long)
+      end
+
+      it { expect(commit_linter).to be_failed }
+    end
+  end
+
+  describe '#add_problem' do
+    it 'stores messages in #failures' do
+      commit_linter.add_problem(:details_line_too_long)
+
+      expect(commit_linter.problems).to eq({ details_line_too_long: described_class::PROBLEMS[:details_line_too_long] })
+    end
+  end
+
+  shared_examples 'a valid commit' do
+    it 'does not have any problem' do
+      commit_linter.lint
+
+      expect(commit_linter.problems).to be_empty
+    end
+  end
+
+  describe '#lint' do
+    describe 'subject' do
+      context 'when subject valid' do
+        it_behaves_like 'a valid commit'
+      end
+
+      context 'when subject is too short' do
+        let(:commit_message) { 'A B' }
+
+        it 'adds a problem' do
+          expect(commit_linter).to receive(:add_problem).with(:subject_too_short, described_class::DEFAULT_SUBJECT_DESCRIPTION)
+
+          commit_linter.lint
+        end
+      end
+
+      context 'when subject is too long' do
+        let(:commit_message) { 'A B ' + 'C' * described_class::MAX_LINE_LENGTH }
+
+        it 'adds a problem' do
+          expect(commit_linter).to receive(:add_problem).with(:subject_too_long, described_class::DEFAULT_SUBJECT_DESCRIPTION)
+
+          commit_linter.lint
+        end
+      end
+
+      context 'when subject is too short and too long' do
+        let(:commit_message) { 'A ' + 'B' * described_class::MAX_LINE_LENGTH }
+
+        it 'adds a problem' do
+          expect(commit_linter).to receive(:add_problem).with(:subject_too_short, described_class::DEFAULT_SUBJECT_DESCRIPTION)
+          expect(commit_linter).to receive(:add_problem).with(:subject_too_long, described_class::DEFAULT_SUBJECT_DESCRIPTION)
+
+          commit_linter.lint
+        end
+      end
+
+      context 'when subject is above warning' do
+        let(:commit_message) { 'A B ' + 'C' * described_class::WARN_SUBJECT_LENGTH }
+
+        it 'adds a problem' do
+          expect(commit_linter).to receive(:add_problem).with(:subject_above_warning, described_class::DEFAULT_SUBJECT_DESCRIPTION)
+
+          commit_linter.lint
+        end
+      end
+
+      context 'when subject starts with lowercase' do
+        let(:commit_message) { 'a B C' }
+
+        it 'adds a problem' do
+          expect(commit_linter).to receive(:add_problem).with(:subject_starts_with_lowercase, described_class::DEFAULT_SUBJECT_DESCRIPTION)
+
+          commit_linter.lint
+        end
+      end
+
+      context 'when subject ands with a period' do
+        let(:commit_message) { 'A B C.' }
+
+        it 'adds a problem' do
+          expect(commit_linter).to receive(:add_problem).with(:subject_ends_with_a_period, described_class::DEFAULT_SUBJECT_DESCRIPTION)
+
+          commit_linter.lint
+        end
+      end
+    end
+
+    describe 'separator' do
+      context 'when separator is missing' do
+        let(:commit_message) { "A B C\n" }
+
+        it_behaves_like 'a valid commit'
+      end
+
+      context 'when separator is a blank line' do
+        let(:commit_message) { "A B C\n\nMore details." }
+
+        it_behaves_like 'a valid commit'
+      end
+
+      context 'when separator is missing' do
+        let(:commit_message) { "A B C\nMore details." }
+
+        it 'adds a problem' do
+          expect(commit_linter).to receive(:add_problem).with(:separator_missing)
+
+          commit_linter.lint
+        end
+      end
+    end
+
+    describe 'details' do
+      context 'when details are valid' do
+        let(:commit_message) { "A B C\n\nMore details." }
+
+        it_behaves_like 'a valid commit'
+      end
+
+      context 'when no details are given and many files are changed' do
+        let(:total_files_changed) { described_class::MAX_CHANGED_FILES_IN_COMMIT + 1 }
+
+        it_behaves_like 'a valid commit'
+      end
+
+      context 'when no details are given and many lines are changed' do
+        let(:total_lines_changed) { described_class::MAX_CHANGED_LINES_IN_COMMIT + 1 }
+
+        it_behaves_like 'a valid commit'
+      end
+
+      context 'when no details are given and many files and lines are changed' do
+        let(:total_files_changed) { described_class::MAX_CHANGED_FILES_IN_COMMIT + 1 }
+        let(:total_lines_changed) { described_class::MAX_CHANGED_LINES_IN_COMMIT + 1 }
+
+        it 'adds a problem' do
+          expect(commit_linter).to receive(:add_problem).with(:details_too_many_changes)
+
+          commit_linter.lint
+        end
+      end
+
+      context 'when details exceeds the max line length' do
+        let(:commit_message) { "A B C\n\n" + 'D' * (described_class::MAX_LINE_LENGTH + 1) }
+
+        it 'adds a problem' do
+          expect(commit_linter).to receive(:add_problem).with(:details_line_too_long)
+
+          commit_linter.lint
+        end
+      end
+
+      context 'when details exceeds the max line length including a URL' do
+        let(:commit_message) { "A B C\n\nhttps://gitlab.com" + 'D' * described_class::MAX_LINE_LENGTH }
+
+        it_behaves_like 'a valid commit'
+      end
+    end
+
+    describe 'message' do
+      context 'when message includes a text emoji' do
+        let(:commit_message) { "A commit message :+1:" }
+
+        it 'adds a problem' do
+          expect(commit_linter).to receive(:add_problem).with(:message_contains_text_emoji)
+
+          commit_linter.lint
+        end
+      end
+
+      context 'when message includes a unicode emoji' do
+        let(:commit_message) { "A commit message 🚀" }
+
+        it 'adds a problem' do
+          expect(commit_linter).to receive(:add_problem).with(:message_contains_unicode_emoji)
+
+          commit_linter.lint
+        end
+      end
+
+      context 'when message includes a short reference' do
+        [
+          'A commit message to fix #1234',
+          'A commit message to fix !1234',
+          'A commit message to fix &1234',
+          'A commit message to fix %1234',
+          'A commit message to fix gitlab#1234',
+          'A commit message to fix gitlab!1234',
+          'A commit message to fix gitlab&1234',
+          'A commit message to fix gitlab%1234',
+          'A commit message to fix gitlab-org/gitlab#1234',
+          'A commit message to fix gitlab-org/gitlab!1234',
+          'A commit message to fix gitlab-org/gitlab&1234',
+          'A commit message to fix gitlab-org/gitlab%1234'
+        ].each do |message|
+          let(:commit_message) { message }
+
+          it 'adds a problem' do
+            expect(commit_linter).to receive(:add_problem).with(:message_contains_short_reference)
+
+            commit_linter.lint
+          end
+        end
+      end
+    end
+  end
+end

spec/lib/gitlab/danger/emoji_checker_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+require 'rspec-parameterized'
+
+require 'gitlab/danger/emoji_checker'
+
+describe Gitlab::Danger::EmojiChecker do
+  using RSpec::Parameterized::TableSyntax
+
+  describe '#includes_text_emoji?' do
+    where(:text, :includes_emoji) do
+      'Hello World!' | false
+      ':+1:' | true
+      'Hello World! :+1:' | true
+    end
+
+    with_them do
+      it 'is true when text includes a text emoji' do
+        expect(subject.includes_text_emoji?(text)).to be(includes_emoji)
+      end
+    end
+  end
+
+  describe '#includes_unicode_emoji?' do
+    where(:text, :includes_emoji) do
+      'Hello World!' | false
+      '🚀' | true
+      'Hello World! 🚀' | true
+    end
+
+    with_them do
+      it 'is true when text includes a text emoji' do
+        expect(subject.includes_unicode_emoji?(text)).to be(includes_emoji)
+      end
+    end
+  end
+end

spec/lib/gitlab/danger/helper_spec.rb

@@ -313,6 +313,19 @@ describe Gitlab::Danger::Helper do
     end
   end
 
+  describe '#sanitize_mr_title' do
+    where(:mr_title, :expected_mr_title) do
+      'My MR title'      | 'My MR title'
+      'WIP: My MR title' | 'My MR title'
+    end
+
+    with_them do
+      subject { helper.sanitize_mr_title(mr_title) }
+
+      it { is_expected.to eq(expected_mr_title) }
+    end
+  end
+
   describe '#security_mr?' do
     it 'returns false when `gitlab_helper` is unavailable' do
       expect(helper).to receive(:gitlab_helper).and_return(nil)