Merge branch '337108-fix-pipeline-coverage-comparison' into 'master'

Fix errors in coverage approval rule sync when either pipeline does not have a coverage

See merge request gitlab-org/gitlab!75087

ee/app/services/ci/sync_reports_to_approval_rules_service.rb

@@ -67,6 +67,8 @@ module Ci
       return unless pipeline.complete?
 
       pipeline.update_builds_coverage
+      return unless pipeline.coverage.present?
+
       remove_required_approvals_for(ApprovalMergeRequestRule.code_coverage, merge_requests_approved_coverage)
     end
 
@@ -109,13 +111,21 @@ module Ci
 
     def merge_requests_approved_coverage
       pipeline.merge_requests_as_head_pipeline.reject do |merge_request|
-        base_pipeline = merge_request.base_pipeline
-
-        # if base pipeline is missing we just default to not require approval.
-        pipeline.coverage < base_pipeline.coverage if base_pipeline.present?
+        require_coverage_approval?(merge_request)
       end
     end
 
+    def require_coverage_approval?(merge_request)
+      base_pipeline = merge_request.base_pipeline
+
+      # if base pipeline is missing we just default to not require approval.
+      return false unless base_pipeline.present?
+
+      return true unless base_pipeline.coverage.present?
+
+      pipeline.coverage < base_pipeline.coverage
+    end
+
     def merge_requests_approved_security_reports
       pipeline.merge_requests_as_head_pipeline.reject do |merge_request|
         reports.present? && reports.violates_default_policy_against?(merge_request.base_pipeline&.security_reports, project_rule_vulnerabilities_allowed, project_rule_severity_levels, project_rule_vulnerability_states)

ee/spec/services/ci/sync_reports_to_approval_rules_service_spec.rb

@@ -21,6 +21,12 @@ RSpec.describe Ci::SyncReportsToApprovalRulesService, '#execute' do
     stub_licensed_features(dependency_scanning: true, dast: true, license_scanning: true)
   end
 
+  shared_examples 'a successful execution' do
+    it "is successful" do
+      expect(sync_rules[:status]).to eq(:success)
+    end
+  end
+
   shared_context 'security reports with vulnerabilities' do
     context 'when there are security reports' do
       context 'when pipeline passes' do
@@ -244,6 +250,8 @@ RSpec.describe Ci::SyncReportsToApprovalRulesService, '#execute' do
           ]
         end
 
+        it_behaves_like 'a successful execution'
+
         it "won't lower approvals_required count" do
           expect { sync_rules }
             .not_to change { report_approver_rule.reload.approvals_required }
@@ -259,6 +267,8 @@ RSpec.describe Ci::SyncReportsToApprovalRulesService, '#execute' do
           ]
         end
 
+        it_behaves_like 'a successful execution'
+
         it "lowers approvals_required count" do
           expect { sync_rules }
             .to change { report_approver_rule.reload.approvals_required }.from(2).to(0)
@@ -267,6 +277,8 @@ RSpec.describe Ci::SyncReportsToApprovalRulesService, '#execute' do
         context 'when MR is merged' do
           let!(:merge_request) { create(:merge_request, :merged, source_project: project) }
 
+          it_behaves_like 'a successful execution'
+
           it "won't change approvals_required count" do
             expect { subject }
               .not_to change { report_approver_rule.reload.approvals_required }
@@ -283,11 +295,34 @@ RSpec.describe Ci::SyncReportsToApprovalRulesService, '#execute' do
           ]
         end
 
+        it_behaves_like 'a successful execution'
+
         it "lowers approvals_required count" do
           expect { sync_rules }
             .to change { report_approver_rule.reload.approvals_required }.from(2).to(0)
         end
       end
+
+      context 'and head pipeline does not have coverage' do
+        let!(:head_pipeline_builds) do
+          [
+            create(:ci_build, :success, coverage: nil, pipeline: pipeline)
+          ]
+        end
+
+        let!(:base_pipeline_builds) do
+          [
+            create(:ci_build, :success, coverage: 60.0, pipeline: base_pipeline)
+          ]
+        end
+
+        it_behaves_like 'a successful execution'
+
+        it "does not lower approvals_required count" do
+          expect { sync_rules }
+            .not_to change { report_approver_rule.reload.approvals_required }
+        end
+      end
     end
 
     context 'when pipeline is incomplete' do
@@ -302,6 +337,8 @@ RSpec.describe Ci::SyncReportsToApprovalRulesService, '#execute' do
         allow(pipeline).to receive(:complete?).and_return(false)
       end
 
+      it_behaves_like 'a successful execution'
+
       it "won't lower approvals_required count" do
         expect { sync_rules }
           .not_to change { report_approver_rule.reload.approvals_required }
@@ -313,11 +350,32 @@ RSpec.describe Ci::SyncReportsToApprovalRulesService, '#execute' do
         allow(pipeline).to receive(:complete?).and_return(true)
       end
 
+      it_behaves_like 'a successful execution'
+
       it "lowers approvals_required count" do
         expect { sync_rules }
           .to change { report_approver_rule.reload.approvals_required }.from(2).to(0)
       end
     end
+
+    context 'when base pipeline does not have coverage' do
+      before do
+        allow(pipeline).to receive(:complete?).and_return(true)
+      end
+
+      let!(:base_pipeline_builds) do
+        [
+          create(:ci_build, :success, coverage: nil, pipeline: base_pipeline)
+        ]
+      end
+
+      it_behaves_like 'a successful execution'
+
+      it "does not lower approvals_required count" do
+        expect { sync_rules }
+          .not_to change { report_approver_rule.reload.approvals_required }
+      end
+    end
   end
 
   context 'with security orchestration rules' do