Use numeric value in pipeline coverage

This fixes bug where pipeline coverage comparison
was previously comparing string values, leading to
unexpected result in comparison such as '5' < '10'.

Changing to a numeric value requires
updating the presentation to format the value into
a string with 2 decimals.

app/models/ci/pipeline.rb

@@ -648,7 +648,7 @@ module Ci
     def coverage
       coverage_array = latest_statuses.map(&:coverage).compact
       if coverage_array.size >= 1
-        '%.2f' % (coverage_array.reduce(:+) / coverage_array.size)
+        coverage_array.reduce(:+) / coverage_array.size
       end
     end
 

app/models/merge_request.rb

@@ -1802,7 +1802,7 @@ class MergeRequest < ApplicationRecord
 
   def pipeline_coverage_delta
     if base_pipeline&.coverage && head_pipeline&.coverage
-      '%.2f' % (head_pipeline.coverage.to_f - base_pipeline.coverage.to_f)
+      head_pipeline.coverage - base_pipeline.coverage
     end
   end
 

app/presenters/ci/pipeline_presenter.rb

@@ -62,6 +62,13 @@ module Ci
       localized_names.fetch(pipeline.merge_request_event_type, s_('Pipeline|Pipeline'))
     end
 
+    delegator_override :coverage
+    def coverage
+      return unless pipeline.coverage.present?
+
+      '%.2f' % pipeline.coverage
+    end
+
     def ref_text
       if pipeline.detached_merge_request_pipeline?
         _("for %{link_to_merge_request} with %{link_to_merge_request_source_branch}")

app/presenters/merge_request_presenter.rb

@@ -254,6 +254,13 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
     end
   end
 
+  delegator_override :pipeline_coverage_delta
+  def pipeline_coverage_delta
+    return unless merge_request.pipeline_coverage_delta.present?
+
+    '%.2f' % merge_request.pipeline_coverage_delta
+  end
+
   private
 
   def cached_can_be_reverted?

app/serializers/ci/pipeline_entity.rb

@@ -4,7 +4,7 @@ class Ci::PipelineEntity < Grape::Entity
   include RequestAwareEntity
   include Gitlab::Utils::StrongMemoize
 
-  delegate :name, :failure_reason, to: :presented_pipeline
+  delegate :name, :failure_reason, :coverage, to: :presented_pipeline
 
   expose :id
   expose :iid

app/serializers/merge_requests/pipeline_entity.rb

@@ -43,7 +43,9 @@ class MergeRequests::PipelineEntity < Grape::Entity
   # Coverage isn't always necessary (e.g. when displaying project pipelines in
   # the UI). Instead of creating an entirely different entity we just allow the
   # disabling of this specific field whenever necessary.
-  expose :coverage, unless: proc { options[:disable_coverage] }
+  expose :coverage, unless: proc { options[:disable_coverage] } do |pipeline|
+    pipeline.present.coverage
+  end
 
   expose :ref do
     expose :branch?, as: :branch

lib/gitlab/ci/badge/coverage/report.rb

@@ -30,7 +30,7 @@ module Gitlab::Ci
           @coverage ||= raw_coverage
           return unless @coverage
 
-          @coverage.to_f.round(2)
+          @coverage.round(2)
         end
 
         def metadata

spec/models/ci/pipeline_spec.rb

@@ -757,23 +757,23 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
     context 'with multiple pipelines' do
       before_all do
         create(:ci_build, name: "rspec", coverage: 30, pipeline: pipeline)
-        create(:ci_build, name: "rubocop", coverage: 40, pipeline: pipeline)
+        create(:ci_build, name: "rubocop", coverage: 35, pipeline: pipeline)
       end
 
       it "calculates average when there are two builds with coverage" do
-        expect(pipeline.coverage).to eq("35.00")
+        expect(pipeline.coverage).to be_within(0.001).of(32.5)
       end
 
       it "calculates average when there are two builds with coverage and one with nil" do
         create(:ci_build, pipeline: pipeline)
 
-        expect(pipeline.coverage).to eq("35.00")
+        expect(pipeline.coverage).to be_within(0.001).of(32.5)
       end
 
       it "calculates average when there are two builds with coverage and one is retried" do
         create(:ci_build, name: "rubocop", coverage: 30, pipeline: pipeline, retried: true)
 
-        expect(pipeline.coverage).to eq("35.00")
+        expect(pipeline.coverage).to be_within(0.001).of(32.5)
       end
     end
 

spec/models/merge_request_spec.rb

@@ -3960,7 +3960,7 @@ RSpec.describe MergeRequest, factory_default: :keep do
         create_build(source_pipeline, 60.2, 'test:1')
         create_build(target_pipeline, 50, 'test:2')
 
-        expect(merge_request.pipeline_coverage_delta).to eq('10.20')
+        expect(merge_request.pipeline_coverage_delta).to be_within(0.001).of(10.2)
       end
     end
 

spec/presenters/ci/pipeline_presenter_spec.rb

@@ -122,6 +122,30 @@ RSpec.describe Ci::PipelinePresenter do
     end
   end
 
+  describe '#coverage' do
+    subject { presenter.coverage }
+
+    context 'when pipeline has coverage' do
+      before do
+        allow(pipeline).to receive(:coverage).and_return(35.0)
+      end
+
+      it 'formats coverage into 2 decimal points' do
+        expect(subject).to eq('35.00')
+      end
+    end
+
+    context 'when pipeline does not have coverage' do
+      before do
+        allow(pipeline).to receive(:coverage).and_return(nil)
+      end
+
+      it 'returns nil' do
+        expect(subject).to be_nil
+      end
+    end
+  end
+
   describe '#ref_text' do
     subject { presenter.ref_text }
 

spec/presenters/merge_request_presenter_spec.rb

@@ -632,4 +632,28 @@ RSpec.describe MergeRequestPresenter do
 
     it { is_expected.to eq(expose_path("/api/v4/projects/#{project.id}/merge_requests/#{resource.iid}/unapprove")) }
   end
+
+  describe '#pipeline_coverage_delta' do
+    subject { described_class.new(resource, current_user: user).pipeline_coverage_delta }
+
+    context 'when merge request has pipeline coverage delta' do
+      before do
+        allow(resource).to receive(:pipeline_coverage_delta).and_return(35.0)
+      end
+
+      it 'formats coverage into 2 decimal points' do
+        expect(subject).to eq('35.00')
+      end
+    end
+
+    context 'when merge request does not have pipeline coverage delta' do
+      before do
+        allow(resource).to receive(:pipeline_coverage_delta).and_return(nil)
+      end
+
+      it 'returns nil' do
+        expect(subject).to be_nil
+      end
+    end
+  end
 end

spec/serializers/ci/pipeline_entity_spec.rb

@@ -260,5 +260,17 @@ RSpec.describe Ci::PipelineEntity do
         end
       end
     end
+
+    context 'when pipeline has coverage' do
+      let_it_be(:pipeline) { create(:ci_pipeline, project: project, user: user) }
+
+      before do
+        allow(pipeline).to receive(:coverage).and_return(35.0)
+      end
+
+      it 'exposes the coverage' do
+        expect(subject[:coverage]).to eq('35.00')
+      end
+    end
   end
 end

spec/serializers/merge_requests/pipeline_entity_spec.rb

@@ -14,6 +14,7 @@ RSpec.describe MergeRequests::PipelineEntity do
 
     allow(request).to receive(:current_user).and_return(user)
     allow(request).to receive(:project).and_return(project)
+    allow(pipeline).to receive(:coverage).and_return(35.0)
   end
 
   let(:entity) do
@@ -35,6 +36,10 @@ RSpec.describe MergeRequests::PipelineEntity do
       expect(subject[:flags]).to include(:merge_request_pipeline)
     end
 
+    it 'returns presented coverage' do
+      expect(subject[:coverage]).to eq('35.00')
+    end
+
     it 'excludes coverage data when disabled' do
       entity = described_class
         .represent(pipeline, request: request, disable_coverage: true)