Merge branch 'mo-fix-codequality-mr-diff-report' into 'master'

Fix codequality mr diff report

See merge request gitlab-org/gitlab!59421

app/presenters/ci/pipeline_artifacts/code_quality_mr_diff_presenter.rb

@@ -5,18 +5,20 @@ module Ci
     class CodeQualityMrDiffPresenter < Gitlab::View::Presenter::Delegated
       include Gitlab::Utils::StrongMemoize
 
-      def for_files(filenames)
-        quality_files = raw_report["files"].select { |key| filenames.include?(key) }
+      def for_files(merge_request)
+        filenames = merge_request.new_paths
+        mr_diff_report = raw_report(merge_request.id)
+        quality_files = mr_diff_report["files"]&.select { |key| filenames.include?(key) }
 
         { files: quality_files }
       end
 
       private
 
-      def raw_report
+      def raw_report(merge_request_id)
         strong_memoize(:raw_report) do
           self.each_blob do |blob|
-            Gitlab::Json.parse(blob).with_indifferent_access
+            Gitlab::Json.parse(blob).with_indifferent_access.fetch("merge_request_#{merge_request_id}", {})
           end
         end
       end

app/services/ci/generate_codequality_mr_diff_report_service.rb

@@ -12,7 +12,7 @@ module Ci
       {
         status: :parsed,
         key: key(base_pipeline, head_pipeline),
-        data: head_pipeline.pipeline_artifacts.find_by_file_type(:code_quality_mr_diff).present.for_files(merge_request.new_paths)
+        data: head_pipeline.pipeline_artifacts.find_by_file_type(:code_quality_mr_diff).present.for_files(merge_request)
       }
     rescue StandardError => e
       Gitlab::ErrorTracking.track_exception(e, project_id: project.id)

app/services/ci/pipeline_artifacts/create_code_quality_mr_diff_report_service.rb

@@ -2,11 +2,18 @@
 module Ci
   module PipelineArtifacts
     class CreateCodeQualityMrDiffReportService
-      def execute(pipeline)
+      include Gitlab::Utils::StrongMemoize
+
+      def initialize(pipeline)
+        @pipeline = pipeline
+      end
+
+      def execute
         return unless pipeline.can_generate_codequality_reports?
         return if pipeline.has_codequality_mr_diff_report?
+        return unless new_errors_introduced?
 
-        file = build_carrierwave_file(pipeline)
+        file = build_carrierwave_file!
 
         pipeline.pipeline_artifacts.create!(
           project_id: pipeline.project_id,
@@ -20,18 +27,54 @@ module Ci
 
       private
 
-      def build_carrierwave_file(pipeline)
+      attr_reader :pipeline
+
+      def merge_requests
+        strong_memoize(:merge_requests) do
+          pipeline.merge_requests_as_head_pipeline
+        end
+      end
+
+      def head_report
+        strong_memoize(:head_report) do
+          pipeline.codequality_reports
+        end
+      end
+
+      def base_report(merge_request)
+        strong_memoize(:base_report) do
+          merge_request&.base_pipeline&.codequality_reports
+        end
+      end
+
+      def mr_diff_report_by_merge_requests
+        strong_memoize(:mr_diff_report_by_merge_requests) do
+          merge_requests.each_with_object({}) do |merge_request, hash|
+            key = "merge_request_#{merge_request.id}"
+            new_errors = Gitlab::Ci::Reports::CodequalityReportsComparer.new(base_report(merge_request), head_report).new_errors
+            next if new_errors.empty?
+
+            hash[key] = Gitlab::Ci::Reports::CodequalityMrDiff.new(new_errors)
+          end
+        end
+      end
+
+      def new_errors_introduced?
+        mr_diff_report_by_merge_requests.present?
+      end
+
+      def build_carrierwave_file!
         CarrierWaveStringFile.new_file(
-          file_content: build_quality_mr_diff_report(pipeline),
+          file_content: build_quality_mr_diff_report(mr_diff_report_by_merge_requests),
           filename: Ci::PipelineArtifact::DEFAULT_FILE_NAMES.fetch(:code_quality_mr_diff),
           content_type: 'application/json'
         )
       end
 
-      def build_quality_mr_diff_report(pipeline)
-        mr_diff_report = Gitlab::Ci::Reports::CodequalityMrDiff.new(pipeline.codequality_reports)
-
-        Ci::CodequalityMrDiffReportSerializer.new.represent(mr_diff_report).to_json # rubocop: disable CodeReuse/Serializer
+      def build_quality_mr_diff_report(mr_diff_report)
+        mr_diff_report.each_with_object({}) do |diff_report, hash|
+          hash[diff_report.first] = Ci::CodequalityMrDiffReportSerializer.new.represent(diff_report.second) # rubocop: disable CodeReuse/Serializer
+        end.to_json
       end
     end
   end

app/workers/ci/pipeline_artifacts/create_quality_report_worker.rb

@@ -15,7 +15,7 @@ module Ci
 
       def perform(pipeline_id)
         Ci::Pipeline.find_by_id(pipeline_id).try do |pipeline|
-          Ci::PipelineArtifacts::CreateCodeQualityMrDiffReportService.new.execute(pipeline)
+          Ci::PipelineArtifacts::CreateCodeQualityMrDiffReportService.new(pipeline).execute
         end
       end
     end

changelogs/unreleased/mo-fix-codequality-mr-diff-report.yml

+---
+title: 'Fix false positive for codequality mr diff report'
+merge_request: 59421
+author:
+type: fixed

lib/gitlab/ci/reports/codequality_mr_diff.rb

@@ -6,8 +6,8 @@ module Gitlab
       class CodequalityMrDiff
         attr_reader :files
 
-        def initialize(raw_report)
-          @raw_report = raw_report
+        def initialize(new_errors)
+          @new_errors = new_errors
           @files = {}
           build_report!
         end
@@ -15,7 +15,7 @@ module Gitlab
         private
 
         def build_report!
-          codequality_files = @raw_report.all_degradations.each_with_object({}) do |degradation, codequality_files|
+          codequality_files = @new_errors.each_with_object({}) do |degradation, codequality_files|
             unless codequality_files[degradation.dig(:location, :path)].present?
               codequality_files[degradation.dig(:location, :path)] = []
             end

spec/factories/ci/builds.rb

@@ -369,6 +369,12 @@ FactoryBot.define do
       end
     end
 
+    trait :codequality_reports_without_degradation do
+      after(:build) do |build|
+        build.job_artifacts << create(:ci_job_artifact, :codequality_without_errors, job: build)
+      end
+    end
+
     trait :terraform_reports do
       after(:build) do |build|
         build.job_artifacts << create(:ci_job_artifact, :terraform, job: build)

spec/fixtures/pipeline_artifacts/code_quality_mr_diff.json

 {
-  "files": {
-    "file_a.rb": [
-      {
-        "line": 10,
-        "description": "Avoid parameter lists longer than 5 parameters. [12/5]",
-        "severity": "major"
-      },
-      {
-        "line": 10,
-        "description": "Method `new_array` has 12 arguments (exceeds 4 allowed). Consider refactoring.",
-        "severity": "minor"
-      }
-    ],
-    "file_b.rb": [
-      {
-        "line": 10,
-        "description": "This cop checks for methods with too many parameters.\nThe maximum number of parameters is configurable.\nKeyword arguments can optionally be excluded from the total count.",
-        "severity": "minor"
-      }
-    ]
+  "merge_request_123456789": {
+    "files": {
+      "file_a.rb": [
+        {
+          "line": 10,
+          "description": "Avoid parameter lists longer than 5 parameters. [12/5]",
+          "severity": "major"
+        },
+        {
+          "line": 10,
+          "description": "Method `new_array` has 12 arguments (exceeds 4 allowed). Consider refactoring.",
+          "severity": "minor"
+        }
+      ],
+      "file_b.rb": [
+        {
+          "line": 10,
+          "description": "This cop checks for methods with too many parameters.\nThe maximum number of parameters is configurable.\nKeyword arguments can optionally be excluded from the total count.",
+          "severity": "minor"
+        }
+      ]
+    }
   }
 }

spec/lib/gitlab/ci/reports/codequality_mr_diff_spec.rb

@@ -9,14 +9,11 @@ RSpec.describe Gitlab::Ci::Reports::CodequalityMrDiff do
   let(:degradation_3) { build(:codequality_degradation_3) }
 
   describe '#initialize!' do
-    subject(:report) { described_class.new(codequality_report) }
+    subject(:report) { described_class.new(new_degradations) }
 
     context 'when quality has degradations' do
       context 'with several degradations on the same line' do
-        before do
-          codequality_report.add_degradation(degradation_1)
-          codequality_report.add_degradation(degradation_2)
-        end
+        let(:new_degradations) { [degradation_1, degradation_2] }
 
         it 'generates quality report for mr diff' do
           expect(report.files).to match(
@@ -29,11 +26,7 @@ RSpec.describe Gitlab::Ci::Reports::CodequalityMrDiff do
       end
 
       context 'with several degradations on several files' do
-        before do
-          codequality_report.add_degradation(degradation_1)
-          codequality_report.add_degradation(degradation_2)
-          codequality_report.add_degradation(degradation_3)
-        end
+        let(:new_degradations) { [degradation_1, degradation_2, degradation_3] }
 
         it 'returns quality report for mr diff' do
           expect(report.files).to match(
@@ -50,6 +43,8 @@ RSpec.describe Gitlab::Ci::Reports::CodequalityMrDiff do
     end
 
     context 'when quality has no degradation' do
+      let(:new_degradations) { [] }
+
       it 'returns an empty hash' do
         expect(report.files).to match({})
       end

spec/lib/gitlab/ci/reports/codequality_reports_comparer_spec.rb

@@ -7,7 +7,7 @@ RSpec.describe Gitlab::Ci::Reports::CodequalityReportsComparer do
   let(:base_report) { Gitlab::Ci::Reports::CodequalityReports.new }
   let(:head_report) { Gitlab::Ci::Reports::CodequalityReports.new }
   let(:major_degradation) { build(:codequality_degradation, :major) }
-  let(:minor_degradation) { build(:codequality_degradation, :major) }
+  let(:minor_degradation) { build(:codequality_degradation, :minor) }
   let(:critical_degradation) { build(:codequality_degradation, :critical) }
   let(:blocker_degradation) { build(:codequality_degradation, :blocker) }
 

spec/models/merge_request_spec.rb

@@ -2255,7 +2255,7 @@ RSpec.describe MergeRequest, factory_default: :keep do
 
   describe '#find_codequality_mr_diff_reports' do
     let(:project) { create(:project, :repository) }
-    let(:merge_request) { create(:merge_request, :with_codequality_mr_diff_reports, source_project: project) }
+    let(:merge_request) { create(:merge_request, :with_codequality_mr_diff_reports, source_project: project, id: 123456789) }
     let(:pipeline) { merge_request.head_pipeline }
 
     subject(:mr_diff_report) { merge_request.find_codequality_mr_diff_reports }

spec/presenters/ci/pipeline_artifacts/code_quality_mr_diff_presenter_spec.rb

@@ -4,11 +4,12 @@ require 'spec_helper'
 
 RSpec.describe Ci::PipelineArtifacts::CodeQualityMrDiffPresenter do
   let(:pipeline_artifact) { create(:ci_pipeline_artifact, :with_codequality_mr_diff_report) }
+  let(:merge_request) { double(id: 123456789, new_paths: filenames) }
 
   subject(:presenter) { described_class.new(pipeline_artifact) }
 
   describe '#for_files' do
-    subject(:quality_data) { presenter.for_files(filenames) }
+    subject(:quality_data) { presenter.for_files(merge_request) }
 
     context 'when code quality has data' do
       context 'when filenames is empty' do

spec/serializers/ci/codequality_mr_diff_entity_spec.rb

@@ -4,18 +4,18 @@ require 'spec_helper'
 
 RSpec.describe Ci::CodequalityMrDiffEntity do
   let(:entity) { described_class.new(mr_diff_report) }
-  let(:mr_diff_report) { Gitlab::Ci::Reports::CodequalityMrDiff.new(codequality_report) }
+  let(:mr_diff_report) { Gitlab::Ci::Reports::CodequalityMrDiff.new(codequality_report.all_degradations) }
   let(:codequality_report) { Gitlab::Ci::Reports::CodequalityReports.new }
-  let(:degradation_1) { build(:codequality_degradation_1) }
-  let(:degradation_2) { build(:codequality_degradation_2) }
+  let(:major) { build(:codequality_degradation, :major) }
+  let(:minor) { build(:codequality_degradation, :minor) }
 
   describe '#as_json' do
     subject(:report) { entity.as_json }
 
     context 'when quality report has degradations' do
       before do
-        codequality_report.add_degradation(degradation_1)
-        codequality_report.add_degradation(degradation_2)
+        codequality_report.add_degradation(major)
+        codequality_report.add_degradation(minor)
       end
 
       it 'contains correct codequality mr diff report', :aggregate_failures do

spec/serializers/ci/codequality_mr_diff_report_serializer_spec.rb

@@ -4,18 +4,18 @@ require 'spec_helper'
 
 RSpec.describe Ci::CodequalityMrDiffReportSerializer do
   let(:serializer) { described_class.new.represent(mr_diff_report) }
-  let(:mr_diff_report) { Gitlab::Ci::Reports::CodequalityMrDiff.new(codequality_report) }
+  let(:mr_diff_report) { Gitlab::Ci::Reports::CodequalityMrDiff.new(codequality_report.all_degradations) }
   let(:codequality_report) { Gitlab::Ci::Reports::CodequalityReports.new }
-  let(:degradation_1) { build(:codequality_degradation_1) }
-  let(:degradation_2) { build(:codequality_degradation_2) }
+  let(:major) { build(:codequality_degradation, :major) }
+  let(:minor) { build(:codequality_degradation, :minor) }
 
   describe '#to_json' do
     subject { serializer.as_json }
 
     context 'when quality report has degradations' do
       before do
-        codequality_report.add_degradation(degradation_1)
-        codequality_report.add_degradation(degradation_2)
+        codequality_report.add_degradation(major)
+        codequality_report.add_degradation(minor)
       end
 
       it 'matches the schema' do

spec/services/ci/generate_codequality_mr_diff_report_service_spec.rb

@@ -10,7 +10,7 @@ RSpec.describe Ci::GenerateCodequalityMrDiffReportService do
     subject { service.execute(base_pipeline, head_pipeline) }
 
     context 'when head pipeline has codequality mr diff report' do
-      let!(:merge_request) { create(:merge_request, :with_codequality_mr_diff_reports, source_project: project) }
+      let!(:merge_request) { create(:merge_request, :with_codequality_mr_diff_reports, source_project: project, id: 123456789) }
       let!(:service) { described_class.new(project, nil, id: merge_request.id) }
       let!(:head_pipeline) { merge_request.head_pipeline }
       let!(:base_pipeline) { nil }
@@ -18,7 +18,7 @@ RSpec.describe Ci::GenerateCodequalityMrDiffReportService do
       it 'returns status and data', :aggregate_failures do
         expect_any_instance_of(Ci::PipelineArtifact) do |instance|
           expect(instance).to receive(:present)
-          expect(instance).to receive(:for_files).with(merge_request.new_paths).and_call_original
+          expect(instance).to receive(:for_files).with(merge_request).and_call_original
         end
 
         expect(subject[:status]).to eq(:parsed)

spec/services/ci/pipeline_artifacts/create_code_quality_mr_diff_report_service_spec.rb

@@ -4,58 +4,76 @@ require 'spec_helper'
 
 RSpec.describe ::Ci::PipelineArtifacts::CreateCodeQualityMrDiffReportService do
   describe '#execute' do
-    subject(:pipeline_artifact) { described_class.new.execute(pipeline) }
+    let(:merge_request) { create(:merge_request) }
+    let(:project) { merge_request.project }
+    let(:head_pipeline) { create(:ci_pipeline, :success, :with_codequality_reports, project: project, merge_requests_as_head_pipeline: [merge_request]) }
+    let(:base_pipeline) { create(:ci_pipeline, :success, project: project, ref: merge_request.target_branch, sha: merge_request.diff_base_sha) }
 
-    context 'when pipeline has codequality reports' do
-      let(:project) { create(:project, :repository) }
+    subject { described_class.new(head_pipeline).execute }
 
-      describe 'pipeline completed status' do
-        using RSpec::Parameterized::TableSyntax
+    context 'when there are codequality reports' do
+      context 'when pipeline passes' do
+        context 'when degradations are present' do
+          context 'when degradations already present in target branch pipeline' do
+            before do
+              create(:ci_build, :success, :codequality_reports, name: 'codequality', pipeline: base_pipeline, project: project)
+            end
 
-        where(:status, :result) do
-          :success   | 1
-          :failed    | 1
-          :canceled  | 1
-          :skipped   | 1
-        end
+            it "does not persist a pipeline artifact" do
+              expect { subject }.not_to change { Ci::PipelineArtifact.count }
+            end
+          end
+
+          context 'when degradation is not present in target branch pipeline' do
+            before do
+              create(:ci_build, :success, :codequality_reports_without_degradation, name: 'codequality', pipeline: base_pipeline, project: project)
+            end
 
-        with_them do
-          let(:pipeline) { create(:ci_pipeline, :with_codequality_reports, status: status, project: project) }
+            it 'persists a pipeline artifact' do
+              expect { subject }.to change { Ci::PipelineArtifact.count }.by(1)
+            end
 
-          it 'creates a pipeline artifact' do
-            expect { pipeline_artifact }.to change(Ci::PipelineArtifact, :count).by(result)
-          end
+            it 'persists the default file name' do
+              subject
 
-          it 'persists the default file name' do
-            expect(pipeline_artifact.file.filename).to eq('code_quality_mr_diff.json')
-          end
+              pipeline_artifact = Ci::PipelineArtifact.first
 
-          it 'sets expire_at to 1 week' do
-            freeze_time do
-              expect(pipeline_artifact.expire_at).to eq(1.week.from_now)
+              expect(pipeline_artifact.file.filename).to eq('code_quality_mr_diff.json')
             end
-          end
-        end
-      end
 
-      context 'when pipeline artifact has already been created' do
-        let(:pipeline) { create(:ci_pipeline, :with_codequality_reports, project: project) }
+            it 'sets expire_at to 1 week' do
+              freeze_time do
+                subject
+
+                pipeline_artifact = Ci::PipelineArtifact.first
+
+                expect(pipeline_artifact.expire_at).to eq(1.week.from_now)
+              end
+            end
 
-        it 'does not persist the same artifact twice' do
-          2.times { described_class.new.execute(pipeline) }
+            it 'does not persist the same artifact twice' do
+              2.times { described_class.new(head_pipeline).execute }
 
-          expect(Ci::PipelineArtifact.count).to eq(1)
+              expect { subject }.not_to change { Ci::PipelineArtifact.count }
+            end
+          end
         end
       end
     end
 
-    context 'when pipeline is not completed and codequality report does not exist' do
-      let(:pipeline) { create(:ci_pipeline, :running) }
+    context 'when there are no codequality reports for head pipeline' do
+      let(:head_pipeline) { create(:ci_pipeline, :success, project: project, merge_requests_as_head_pipeline: [merge_request]) }
+
+      it "does not persist a pipeline artifact" do
+        expect { subject }.not_to change { Ci::PipelineArtifact.count }
+      end
+    end
 
-      it 'does not persist data' do
-        pipeline_artifact
+    context 'when there are no codequality reports for base pipeline' do
+      let(:head_pipeline) { create(:ci_pipeline, :success, project: project, merge_requests_as_head_pipeline: [merge_request]) }
 
-        expect(Ci::PipelineArtifact.count).to eq(0)
+      it "does not persist a pipeline artifact" do
+        expect { subject }.not_to change { Ci::PipelineArtifact.count }
       end
     end
   end

spec/workers/ci/pipeline_artifacts/create_quality_report_worker_spec.rb

@@ -21,8 +21,8 @@ RSpec.describe ::Ci::PipelineArtifacts::CreateQualityReportWorker do
       it_behaves_like 'an idempotent worker' do
         let(:job_args) { pipeline_id }
 
-        it 'creates a pipeline artifact' do
-          expect { subject }.to change { pipeline.pipeline_artifacts.count }.by(1)
+        it 'does not create another pipeline artifact if already has one' do
+          expect { subject }.not_to change { pipeline.pipeline_artifacts.count }
         end
       end
     end