Merge branch '353405-extract-broken-check' into 'master'

Extract broken? to mergeability checks framework

See merge request gitlab-org/gitlab!81970

app/models/merge_request.rb

@@ -1154,7 +1154,6 @@ class MergeRequest < ApplicationRecord
   def mergeable_state?(skip_ci_check: false, skip_discussions_check: false)
     return false unless open?
     return false if work_in_progress?
-    return false if broken?
 
     if Feature.enabled?(:improved_mergeability_checks, self.project, default_enabled: :yaml)
       additional_checks = MergeRequests::Mergeability::RunChecksService.new(
@@ -1166,6 +1165,7 @@ class MergeRequest < ApplicationRecord
       )
       additional_checks.execute.all?(&:success?)
     else
+      return false if broken?
       return false unless skip_discussions_check || mergeable_discussions_state?
       return false unless skip_ci_check || mergeable_ci_state?
 

app/services/merge_requests/mergeability/check_broken_status_service.rb

+# frozen_string_literal: true
+module MergeRequests
+  module Mergeability
+    class CheckBrokenStatusService < CheckBaseService
+      def execute
+        if merge_request.broken?
+          failure
+        else
+          success
+        end
+      end
+
+      def skip?
+        false
+      end
+
+      def cacheable?
+        false
+      end
+    end
+  end
+end

app/services/merge_requests/mergeability/run_checks_service.rb

@@ -7,6 +7,7 @@ module MergeRequests
       # We want to have the cheapest checks first in the list,
       # that way we can fail fast before running the more expensive ones
       CHECKS = [
+        CheckBrokenStatusService,
         CheckDiscussionsStatusService,
         CheckCiStatusService
       ].freeze

spec/models/merge_request_spec.rb

@@ -3225,52 +3225,44 @@ RSpec.describe MergeRequest, factory_default: :keep do
     end
 
     context 'when failed' do
-      shared_examples 'failed skip_ci_check' do
-        context 'when #mergeable_ci_state? is false' do
-          before do
-            allow(subject).to receive(:mergeable_ci_state?) { false }
-          end
-
-          it 'returns false' do
-            expect(subject.mergeable_state?).to be_falsey
-          end
-
-          it 'returns true when skipping ci check' do
-            expect(subject.mergeable_state?(skip_ci_check: true)).to be(true)
-          end
+      context 'when #mergeable_ci_state? is false' do
+        before do
+          allow(subject).to receive(:mergeable_ci_state?) { false }
         end
 
-        context 'when #mergeable_discussions_state? is false' do
-          before do
-            allow(subject).to receive(:mergeable_discussions_state?) { false }
-          end
-
-          it 'returns false' do
-            expect(subject.mergeable_state?).to be_falsey
-          end
-
-          it 'returns true when skipping discussions check' do
-            expect(subject.mergeable_state?(skip_discussions_check: true)).to be(true)
-          end
+        it 'returns false' do
+          expect(subject.mergeable_state?).to be_falsey
         end
-      end
 
-      context 'when improved_mergeability_checks is on' do
-        it_behaves_like 'failed skip_ci_check'
+        it 'returns true when skipping ci check' do
+          expect(subject.mergeable_state?(skip_ci_check: true)).to be(true)
+        end
       end
 
-      context 'when improved_mergeability_checks is off' do
+      context 'when #mergeable_discussions_state? is false' do
         before do
-          stub_feature_flags(improved_mergeability_checks: false)
+          allow(subject).to receive(:mergeable_discussions_state?) { false }
         end
 
-        it_behaves_like 'failed skip_ci_check'
+        it 'returns false' do
+          expect(subject.mergeable_state?).to be_falsey
+        end
+
+        it 'returns true when skipping discussions check' do
+          expect(subject.mergeable_state?(skip_discussions_check: true)).to be(true)
+        end
       end
     end
   end
 
   describe '#mergeable_state?' do
-    context 'when merge state caching is on' do
+    it_behaves_like 'for mergeable_state'
+
+    context 'when improved_mergeability_checks is off' do
+      before do
+        stub_feature_flags(improved_mergeability_checks: false)
+      end
+
       it_behaves_like 'for mergeable_state'
     end
 

spec/services/merge_requests/mergeability/check_broken_status_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MergeRequests::Mergeability::CheckBrokenStatusService do
+  subject(:check_broken_status) { described_class.new(merge_request: merge_request, params: {}) }
+
+  let(:merge_request) { build(:merge_request) }
+
+  describe '#execute' do
+    before do
+      expect(merge_request).to receive(:broken?).and_return(broken)
+    end
+
+    context 'when the merge request is broken' do
+      let(:broken) { true }
+
+      it 'returns a check result with status failed' do
+        expect(check_broken_status.execute.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::FAILED_STATUS
+      end
+    end
+
+    context 'when the merge request is not broken' do
+      let(:broken) { false }
+
+      it 'returns a check result with status success' do
+        expect(check_broken_status.execute.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::SUCCESS_STATUS
+      end
+    end
+  end
+
+  describe '#skip?' do
+    it 'returns false' do
+      expect(check_broken_status.skip?).to eq false
+    end
+  end
+
+  describe '#cacheable?' do
+    it 'returns false' do
+      expect(check_broken_status.cacheable?).to eq false
+    end
+  end
+end

spec/services/merge_requests/mergeability/run_checks_service_spec.rb

@@ -47,7 +47,7 @@ RSpec.describe MergeRequests::Mergeability::RunChecksService do
           expect(service).not_to receive(:execute)
         end
 
-        expect(execute).to match_array([success_result])
+        expect(execute).to match_array([success_result, success_result])
       end
     end