Merge branch 'ignore-runs-as-foss-from-mr-too-long' into 'master'

Ignore magic CI run options in mr title

See merge request gitlab-org/gitlab!49854

danger/commit_messages/Dangerfile

 # frozen_string_literal: true
 
 require_relative File.expand_path('../../lib/gitlab/danger/commit_linter', __dir__)
+require_relative File.expand_path('../../lib/gitlab/danger/merge_request_linter', __dir__)
 
 COMMIT_MESSAGE_GUIDELINES = "https://docs.gitlab.com/ee/development/contributing/merge_request_workflow.html#commit-messages-guidelines"
 MORE_INFO = "For more information, take a look at our [Commit message guidelines](#{COMMIT_MESSAGE_GUIDELINES})."
@@ -92,7 +93,7 @@ end
 def lint_mr_title(mr_title)
   commit = Struct.new(:message, :sha).new(mr_title)
 
-  Gitlab::Danger::CommitLinter.new(commit).lint_subject("merge request title")
+  Gitlab::Danger::MergeRequestLinter.new(commit).lint
 end
 
 def count_non_fixup_commits(commit_linters)

lib/gitlab/danger/base_linter.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Danger
+    class BaseLinter
+      MIN_SUBJECT_WORDS_COUNT = 3
+      MAX_LINE_LENGTH = 72
+      WIP_PREFIX = 'WIP: '
+
+      attr_reader :commit, :problems
+
+      def self.problems_mapping
+        {
+          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_starts_with_lowercase: "The %s must start with a capital letter",
+          subject_ends_with_a_period: "The %s must not end with a period"
+        }
+      end
+
+      def self.subject_description
+        'commit subject'
+      end
+
+      def initialize(commit)
+        @commit = commit
+        @problems = {}
+      end
+
+      def failed?
+        problems.any?
+      end
+
+      def add_problem(problem_key, *args)
+        @problems[problem_key] = sprintf(self.class.problems_mapping[problem_key], *args)
+      end
+
+      def lint_subject
+        if subject_too_short?
+          add_problem(:subject_too_short, self.class.subject_description)
+        end
+
+        if subject_too_long?
+          add_problem(:subject_too_long, self.class.subject_description)
+        end
+
+        if subject_starts_with_lowercase?
+          add_problem(:subject_starts_with_lowercase, self.class.subject_description)
+        end
+
+        if subject_ends_with_a_period?
+          add_problem(:subject_ends_with_a_period, self.class.subject_description)
+        end
+
+        self
+      end
+
+      private
+
+      def subject
+        message_parts[0].delete_prefix(WIP_PREFIX)
+      end
+
+      def subject_too_short?
+        subject.split(' ').length < MIN_SUBJECT_WORDS_COUNT
+      end
+
+      def subject_too_long?
+        line_too_long?(subject)
+      end
+
+      def line_too_long?(line)
+        line.length > MAX_LINE_LENGTH
+      end
+
+      def subject_starts_with_lowercase?
+        return false if ('A'..'Z').cover?(subject[0])
+
+        first_char = subject.sub(/\A(\[.+\]|\w+:)\s/, '')[0]
+        first_char_downcased = first_char.downcase
+        return true unless ('a'..'z').cover?(first_char_downcased)
+
+        first_char.downcase == first_char
+      end
+
+      def subject_ends_with_a_period?
+        subject.end_with?('.')
+      end
+
+      def message_parts
+        @message_parts ||= commit.message.split("\n", 3)
+      end
+    end
+  end
+end

lib/gitlab/danger/commit_linter.rb

 # frozen_string_literal: true
 
+require_relative 'base_linter'
+
 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
+    class CommitLinter < BaseLinter
       MAX_CHANGED_FILES_IN_COMMIT = 3
       MAX_CHANGED_LINES_IN_COMMIT = 30
       SHORT_REFERENCE_REGEX = %r{([\w\-\/]+)?(?<!`)(#|!|&|%)\d+(?<!`)}.freeze
-      DEFAULT_SUBJECT_DESCRIPTION = 'commit subject'
-      WIP_PREFIX = 'WIP: '
-      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_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 " \
+
+      def self.problems_mapping
+        super.merge(
+          {
+            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 " \
+            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_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 " \
+            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
+          }
+        )
+      end
 
       def initialize(commit)
-        @commit = commit
-        @problems = {}
+        super
+
         @linted = false
       end
 
@@ -58,19 +55,11 @@ module Gitlab
         !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")
+      def lint
         return self if @linted
 
         @linted = true
-        lint_subject(subject_description)
+        lint_subject
         lint_separator
         lint_details
         lint_message
@@ -78,26 +67,6 @@ module Gitlab
         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)
-        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
@@ -155,10 +124,6 @@ module Gitlab
         files_changed > MAX_CHANGED_FILES_IN_COMMIT && lines_changed > MAX_CHANGED_LINES_IN_COMMIT
       end
 
-      def subject
-        message_parts[0].delete_prefix(WIP_PREFIX)
-      end
-
       def separator
         message_parts[1]
       end
@@ -167,32 +132,6 @@ module Gitlab
         message_parts[2]&.gsub(/^Signed-off-by.*$/, '')
       end
 
-      def line_too_long?(line)
-        line.length > MAX_LINE_LENGTH
-      end
-
-      def subject_too_short?
-        subject.split(' ').length < MIN_SUBJECT_WORDS_COUNT
-      end
-
-      def subject_too_long?
-        line_too_long?(subject)
-      end
-
-      def subject_starts_with_lowercase?
-        return false if ('A'..'Z').cover?(subject[0])
-
-        first_char = subject.sub(/\A(\[.+\]|\w+:)\s/, '')[0]
-        first_char_downcased = first_char.downcase
-        return true unless ('a'..'z').cover?(first_char_downcased)
-
-        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
@@ -208,10 +147,6 @@ module Gitlab
       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/merge_request_linter.rb

+# frozen_string_literal: true
+
+require_relative 'base_linter'
+
+module Gitlab
+  module Danger
+    class MergeRequestLinter < BaseLinter
+      alias_method :lint, :lint_subject
+
+      def self.subject_description
+        'merge request title'
+      end
+
+      def self.mr_run_options_regex
+        [
+          'RUN AS-IF-FOSS',
+          'UPDATE CACHE',
+          'RUN ALL RSPEC',
+          'SKIP RSPEC FAIL-FAST'
+        ].join('|')
+      end
+
+      private
+
+      def subject
+        super.gsub(/\[?(#{self.class.mr_run_options_regex})\]?/, '').strip
+      end
+    end
+  end
+end

spec/lib/gitlab/danger/base_linter_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+require_relative 'danger_spec_helper'
+
+require 'gitlab/danger/base_linter'
+
+RSpec.describe Gitlab::Danger::BaseLinter do
+  let(:commit_class) do
+    Struct.new(:message, :sha, :diff_parent)
+  end
+
+  let(:commit_message) { 'A commit message' }
+  let(:commit) { commit_class.new(commit_message, anything, anything) }
+
+  subject(:commit_linter) { described_class.new(commit) }
+
+  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(:subject_too_long, described_class.subject_description)
+      end
+
+      it { expect(commit_linter).to be_failed }
+    end
+  end
+
+  describe '#add_problem' do
+    it 'stores messages in #failures' do
+      commit_linter.add_problem(:subject_too_long, '%s')
+
+      expect(commit_linter.problems).to eq({ subject_too_long: described_class.problems_mapping[:subject_too_long] })
+    end
+  end
+
+  shared_examples 'a valid commit' do
+    it 'does not have any problem' do
+      commit_linter.lint_subject
+
+      expect(commit_linter.problems).to be_empty
+    end
+  end
+
+  describe '#lint_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.subject_description)
+
+        commit_linter.lint_subject
+      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.subject_description)
+
+        commit_linter.lint_subject
+      end
+    end
+
+    context 'when subject is a WIP' do
+      let(:final_message) { 'A B C' }
+      # commit message with prefix will be over max length. commit message without prefix will be of maximum size
+      let(:commit_message) { described_class::WIP_PREFIX + final_message + 'D' * (described_class::MAX_LINE_LENGTH - final_message.size) }
+
+      it 'does not have any problems' do
+        commit_linter.lint_subject
+
+        expect(commit_linter.problems).to be_empty
+      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.subject_description)
+        expect(commit_linter).to receive(:add_problem).with(:subject_too_long, described_class.subject_description)
+
+        commit_linter.lint_subject
+      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.subject_description)
+
+        commit_linter.lint_subject
+      end
+    end
+
+    [
+      '[ci skip] A commit message',
+      '[Ci skip] A commit message',
+      '[API] A commit message',
+      'api: A commit message',
+      'API: A commit message',
+      'API: a commit message',
+      'API: a commit message'
+    ].each do |message|
+      context "when subject is '#{message}'" do
+        let(:commit_message) { message }
+
+        it 'does not add a problem' do
+          expect(commit_linter).not_to receive(:add_problem)
+
+          commit_linter.lint_subject
+        end
+      end
+    end
+
+    [
+      '[ci skip]A commit message',
+      '[Ci skip]  A commit message',
+      '[ci skip] a commit message',
+      'api: a commit message',
+      '! A commit message'
+    ].each do |message|
+      context "when subject is '#{message}'" do
+        let(:commit_message) { message }
+
+        it 'adds a problem' do
+          expect(commit_linter).to receive(:add_problem).with(:subject_starts_with_lowercase, described_class.subject_description)
+
+          commit_linter.lint_subject
+        end
+      end
+    end
+
+    context 'when subject ends 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.subject_description)
+
+        commit_linter.lint_subject
+      end
+    end
+  end
+end

spec/lib/gitlab/danger/commit_linter_spec.rb

@@ -98,28 +98,6 @@ RSpec.describe Gitlab::Danger::CommitLinter do
     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
@@ -129,113 +107,6 @@ RSpec.describe Gitlab::Danger::CommitLinter do
   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 a WIP' do
-        let(:final_message) { 'A B C' }
-        # commit message with prefix will be over max length. commit message without prefix will be of maximum size
-        let(:commit_message) { described_class::WIP_PREFIX + final_message + 'D' * (described_class::MAX_LINE_LENGTH - final_message.size) }
-
-        it 'does not have any problems' do
-          commit_linter.lint
-
-          expect(commit_linter.problems).to be_empty
-        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 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
-
-      [
-        '[ci skip] A commit message',
-        '[Ci skip] A commit message',
-        '[API] A commit message',
-        'api: A commit message',
-        'API: A commit message',
-        'API: a commit message',
-        'API: a commit message'
-      ].each do |message|
-        context "when subject is '#{message}'" do
-          let(:commit_message) { message }
-
-          it 'does not add a problem' do
-            expect(commit_linter).not_to receive(:add_problem)
-
-            commit_linter.lint
-          end
-        end
-      end
-
-      [
-        '[ci skip]A commit message',
-        '[Ci skip]  A commit message',
-        '[ci skip] a commit message',
-        'api: a commit message',
-        '! A commit message'
-      ].each do |message|
-        context "when subject is '#{message}'" do
-          let(:commit_message) { message }
-
-          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
-      end
-
-      context 'when subject ends 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" }

spec/lib/gitlab/danger/merge_request_linter_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+require 'rspec-parameterized'
+require_relative 'danger_spec_helper'
+
+require 'gitlab/danger/merge_request_linter'
+
+RSpec.describe Gitlab::Danger::MergeRequestLinter do
+  using RSpec::Parameterized::TableSyntax
+
+  let(:mr_class) do
+    Struct.new(:message, :sha, :diff_parent)
+  end
+
+  let(:mr_title) { 'A B ' + 'C' }
+  let(:merge_request) { mr_class.new(mr_title, anything, anything) }
+
+  describe '#lint_subject' do
+    subject(:mr_linter) { described_class.new(merge_request) }
+
+    shared_examples 'a valid mr title' do
+      it 'does not have any problem' do
+        mr_linter.lint
+
+        expect(mr_linter.problems).to be_empty
+      end
+    end
+
+    context 'when subject valid' do
+      it_behaves_like 'a valid mr title'
+    end
+
+    context 'when it is too long' do
+      let(:mr_title) { 'A B ' + 'C' * described_class::MAX_LINE_LENGTH }
+
+      it 'adds a problem' do
+        expect(mr_linter).to receive(:add_problem).with(:subject_too_long, described_class.subject_description)
+
+        mr_linter.lint
+      end
+    end
+
+    describe 'using magic mr run options' do
+      where(run_option: described_class.mr_run_options_regex.split('|') +
+        described_class.mr_run_options_regex.split('|').map! { |x| "[#{x}]" })
+
+      with_them do
+        let(:mr_title) { run_option + ' A B ' + 'C' * (described_class::MAX_LINE_LENGTH - 5) }
+
+        it_behaves_like 'a valid mr title'
+      end
+    end
+  end
+end