Commit 6cd6c3f7 authored by Maxime Orefice's avatar Maxime Orefice Committed by Shinya Maeda

Fix codequality mr diff report

parent d12901d0
...@@ -5,18 +5,20 @@ module Ci ...@@ -5,18 +5,20 @@ module Ci
class CodeQualityMrDiffPresenter < Gitlab::View::Presenter::Delegated class CodeQualityMrDiffPresenter < Gitlab::View::Presenter::Delegated
include Gitlab::Utils::StrongMemoize include Gitlab::Utils::StrongMemoize
def for_files(filenames) def for_files(merge_request)
quality_files = raw_report["files"].select { |key| filenames.include?(key) } 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 } { files: quality_files }
end end
private private
def raw_report def raw_report(merge_request_id)
strong_memoize(:raw_report) do strong_memoize(:raw_report) do
self.each_blob do |blob| 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 end
end end
......
...@@ -12,7 +12,7 @@ module Ci ...@@ -12,7 +12,7 @@ module Ci
{ {
status: :parsed, status: :parsed,
key: key(base_pipeline, head_pipeline), 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 rescue StandardError => e
Gitlab::ErrorTracking.track_exception(e, project_id: project.id) Gitlab::ErrorTracking.track_exception(e, project_id: project.id)
......
...@@ -2,11 +2,18 @@ ...@@ -2,11 +2,18 @@
module Ci module Ci
module PipelineArtifacts module PipelineArtifacts
class CreateCodeQualityMrDiffReportService 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 unless pipeline.can_generate_codequality_reports?
return if pipeline.has_codequality_mr_diff_report? 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!( pipeline.pipeline_artifacts.create!(
project_id: pipeline.project_id, project_id: pipeline.project_id,
...@@ -20,18 +27,54 @@ module Ci ...@@ -20,18 +27,54 @@ module Ci
private 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( 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), filename: Ci::PipelineArtifact::DEFAULT_FILE_NAMES.fetch(:code_quality_mr_diff),
content_type: 'application/json' content_type: 'application/json'
) )
end end
def build_quality_mr_diff_report(pipeline) def build_quality_mr_diff_report(mr_diff_report)
mr_diff_report = Gitlab::Ci::Reports::CodequalityMrDiff.new(pipeline.codequality_reports) 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
Ci::CodequalityMrDiffReportSerializer.new.represent(mr_diff_report).to_json # rubocop: disable CodeReuse/Serializer end.to_json
end end
end end
end end
......
...@@ -15,7 +15,7 @@ module Ci ...@@ -15,7 +15,7 @@ module Ci
def perform(pipeline_id) def perform(pipeline_id)
Ci::Pipeline.find_by_id(pipeline_id).try do |pipeline| 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 end
end end
......
---
title: 'Fix false positive for codequality mr diff report'
merge_request: 59421
author:
type: fixed
...@@ -6,8 +6,8 @@ module Gitlab ...@@ -6,8 +6,8 @@ module Gitlab
class CodequalityMrDiff class CodequalityMrDiff
attr_reader :files attr_reader :files
def initialize(raw_report) def initialize(new_errors)
@raw_report = raw_report @new_errors = new_errors
@files = {} @files = {}
build_report! build_report!
end end
...@@ -15,7 +15,7 @@ module Gitlab ...@@ -15,7 +15,7 @@ module Gitlab
private private
def build_report! 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? unless codequality_files[degradation.dig(:location, :path)].present?
codequality_files[degradation.dig(:location, :path)] = [] codequality_files[degradation.dig(:location, :path)] = []
end end
......
...@@ -369,6 +369,12 @@ FactoryBot.define do ...@@ -369,6 +369,12 @@ FactoryBot.define do
end end
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 trait :terraform_reports do
after(:build) do |build| after(:build) do |build|
build.job_artifacts << create(:ci_job_artifact, :terraform, job: build) build.job_artifacts << create(:ci_job_artifact, :terraform, job: build)
......
{ {
"files": { "merge_request_123456789": {
"file_a.rb": [ "files": {
{ "file_a.rb": [
"line": 10, {
"description": "Avoid parameter lists longer than 5 parameters. [12/5]", "line": 10,
"severity": "major" "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.", "line": 10,
"severity": "minor" "description": "Method `new_array` has 12 arguments (exceeds 4 allowed). Consider refactoring.",
} "severity": "minor"
], }
"file_b.rb": [ ],
{ "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.", "line": 10,
"severity": "minor" "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"
] }
]
}
} }
} }
...@@ -9,14 +9,11 @@ RSpec.describe Gitlab::Ci::Reports::CodequalityMrDiff do ...@@ -9,14 +9,11 @@ RSpec.describe Gitlab::Ci::Reports::CodequalityMrDiff do
let(:degradation_3) { build(:codequality_degradation_3) } let(:degradation_3) { build(:codequality_degradation_3) }
describe '#initialize!' do describe '#initialize!' do
subject(:report) { described_class.new(codequality_report) } subject(:report) { described_class.new(new_degradations) }
context 'when quality has degradations' do context 'when quality has degradations' do
context 'with several degradations on the same line' do context 'with several degradations on the same line' do
before do let(:new_degradations) { [degradation_1, degradation_2] }
codequality_report.add_degradation(degradation_1)
codequality_report.add_degradation(degradation_2)
end
it 'generates quality report for mr diff' do it 'generates quality report for mr diff' do
expect(report.files).to match( expect(report.files).to match(
...@@ -29,11 +26,7 @@ RSpec.describe Gitlab::Ci::Reports::CodequalityMrDiff do ...@@ -29,11 +26,7 @@ RSpec.describe Gitlab::Ci::Reports::CodequalityMrDiff do
end end
context 'with several degradations on several files' do context 'with several degradations on several files' do
before do let(:new_degradations) { [degradation_1, degradation_2, degradation_3] }
codequality_report.add_degradation(degradation_1)
codequality_report.add_degradation(degradation_2)
codequality_report.add_degradation(degradation_3)
end
it 'returns quality report for mr diff' do it 'returns quality report for mr diff' do
expect(report.files).to match( expect(report.files).to match(
...@@ -50,6 +43,8 @@ RSpec.describe Gitlab::Ci::Reports::CodequalityMrDiff do ...@@ -50,6 +43,8 @@ RSpec.describe Gitlab::Ci::Reports::CodequalityMrDiff do
end end
context 'when quality has no degradation' do context 'when quality has no degradation' do
let(:new_degradations) { [] }
it 'returns an empty hash' do it 'returns an empty hash' do
expect(report.files).to match({}) expect(report.files).to match({})
end end
......
...@@ -7,7 +7,7 @@ RSpec.describe Gitlab::Ci::Reports::CodequalityReportsComparer do ...@@ -7,7 +7,7 @@ RSpec.describe Gitlab::Ci::Reports::CodequalityReportsComparer do
let(:base_report) { Gitlab::Ci::Reports::CodequalityReports.new } let(:base_report) { Gitlab::Ci::Reports::CodequalityReports.new }
let(:head_report) { Gitlab::Ci::Reports::CodequalityReports.new } let(:head_report) { Gitlab::Ci::Reports::CodequalityReports.new }
let(:major_degradation) { build(:codequality_degradation, :major) } 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(:critical_degradation) { build(:codequality_degradation, :critical) }
let(:blocker_degradation) { build(:codequality_degradation, :blocker) } let(:blocker_degradation) { build(:codequality_degradation, :blocker) }
......
...@@ -2255,7 +2255,7 @@ RSpec.describe MergeRequest, factory_default: :keep do ...@@ -2255,7 +2255,7 @@ RSpec.describe MergeRequest, factory_default: :keep do
describe '#find_codequality_mr_diff_reports' do describe '#find_codequality_mr_diff_reports' do
let(:project) { create(:project, :repository) } 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 } let(:pipeline) { merge_request.head_pipeline }
subject(:mr_diff_report) { merge_request.find_codequality_mr_diff_reports } subject(:mr_diff_report) { merge_request.find_codequality_mr_diff_reports }
......
...@@ -4,11 +4,12 @@ require 'spec_helper' ...@@ -4,11 +4,12 @@ require 'spec_helper'
RSpec.describe Ci::PipelineArtifacts::CodeQualityMrDiffPresenter do RSpec.describe Ci::PipelineArtifacts::CodeQualityMrDiffPresenter do
let(:pipeline_artifact) { create(:ci_pipeline_artifact, :with_codequality_mr_diff_report) } 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) } subject(:presenter) { described_class.new(pipeline_artifact) }
describe '#for_files' do 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 code quality has data' do
context 'when filenames is empty' do context 'when filenames is empty' do
......
...@@ -4,18 +4,18 @@ require 'spec_helper' ...@@ -4,18 +4,18 @@ require 'spec_helper'
RSpec.describe Ci::CodequalityMrDiffEntity do RSpec.describe Ci::CodequalityMrDiffEntity do
let(:entity) { described_class.new(mr_diff_report) } 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(:codequality_report) { Gitlab::Ci::Reports::CodequalityReports.new }
let(:degradation_1) { build(:codequality_degradation_1) } let(:major) { build(:codequality_degradation, :major) }
let(:degradation_2) { build(:codequality_degradation_2) } let(:minor) { build(:codequality_degradation, :minor) }
describe '#as_json' do describe '#as_json' do
subject(:report) { entity.as_json } subject(:report) { entity.as_json }
context 'when quality report has degradations' do context 'when quality report has degradations' do
before do before do
codequality_report.add_degradation(degradation_1) codequality_report.add_degradation(major)
codequality_report.add_degradation(degradation_2) codequality_report.add_degradation(minor)
end end
it 'contains correct codequality mr diff report', :aggregate_failures do it 'contains correct codequality mr diff report', :aggregate_failures do
......
...@@ -4,18 +4,18 @@ require 'spec_helper' ...@@ -4,18 +4,18 @@ require 'spec_helper'
RSpec.describe Ci::CodequalityMrDiffReportSerializer do RSpec.describe Ci::CodequalityMrDiffReportSerializer do
let(:serializer) { described_class.new.represent(mr_diff_report) } 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(:codequality_report) { Gitlab::Ci::Reports::CodequalityReports.new }
let(:degradation_1) { build(:codequality_degradation_1) } let(:major) { build(:codequality_degradation, :major) }
let(:degradation_2) { build(:codequality_degradation_2) } let(:minor) { build(:codequality_degradation, :minor) }
describe '#to_json' do describe '#to_json' do
subject { serializer.as_json } subject { serializer.as_json }
context 'when quality report has degradations' do context 'when quality report has degradations' do
before do before do
codequality_report.add_degradation(degradation_1) codequality_report.add_degradation(major)
codequality_report.add_degradation(degradation_2) codequality_report.add_degradation(minor)
end end
it 'matches the schema' do it 'matches the schema' do
......
...@@ -10,7 +10,7 @@ RSpec.describe Ci::GenerateCodequalityMrDiffReportService do ...@@ -10,7 +10,7 @@ RSpec.describe Ci::GenerateCodequalityMrDiffReportService do
subject { service.execute(base_pipeline, head_pipeline) } subject { service.execute(base_pipeline, head_pipeline) }
context 'when head pipeline has codequality mr diff report' do 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!(:service) { described_class.new(project, nil, id: merge_request.id) }
let!(:head_pipeline) { merge_request.head_pipeline } let!(:head_pipeline) { merge_request.head_pipeline }
let!(:base_pipeline) { nil } let!(:base_pipeline) { nil }
...@@ -18,7 +18,7 @@ RSpec.describe Ci::GenerateCodequalityMrDiffReportService do ...@@ -18,7 +18,7 @@ RSpec.describe Ci::GenerateCodequalityMrDiffReportService do
it 'returns status and data', :aggregate_failures do it 'returns status and data', :aggregate_failures do
expect_any_instance_of(Ci::PipelineArtifact) do |instance| expect_any_instance_of(Ci::PipelineArtifact) do |instance|
expect(instance).to receive(:present) 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 end
expect(subject[:status]).to eq(:parsed) expect(subject[:status]).to eq(:parsed)
......
...@@ -4,58 +4,76 @@ require 'spec_helper' ...@@ -4,58 +4,76 @@ require 'spec_helper'
RSpec.describe ::Ci::PipelineArtifacts::CreateCodeQualityMrDiffReportService do RSpec.describe ::Ci::PipelineArtifacts::CreateCodeQualityMrDiffReportService do
describe '#execute' 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 subject { described_class.new(head_pipeline).execute }
let(:project) { create(:project, :repository) }
describe 'pipeline completed status' do context 'when there are codequality reports' do
using RSpec::Parameterized::TableSyntax 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 it "does not persist a pipeline artifact" do
:success | 1 expect { subject }.not_to change { Ci::PipelineArtifact.count }
:failed | 1 end
:canceled | 1 end
:skipped | 1
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 it 'persists a pipeline artifact' do
let(:pipeline) { create(:ci_pipeline, :with_codequality_reports, status: status, project: project) } expect { subject }.to change { Ci::PipelineArtifact.count }.by(1)
end
it 'creates a pipeline artifact' do it 'persists the default file name' do
expect { pipeline_artifact }.to change(Ci::PipelineArtifact, :count).by(result) subject
end
it 'persists the default file name' do pipeline_artifact = Ci::PipelineArtifact.first
expect(pipeline_artifact.file.filename).to eq('code_quality_mr_diff.json')
end
it 'sets expire_at to 1 week' do expect(pipeline_artifact.file.filename).to eq('code_quality_mr_diff.json')
freeze_time do
expect(pipeline_artifact.expire_at).to eq(1.week.from_now)
end end
end
end
end
context 'when pipeline artifact has already been created' do it 'sets expire_at to 1 week' do
let(:pipeline) { create(:ci_pipeline, :with_codequality_reports, project: project) } 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 it 'does not persist the same artifact twice' do
2.times { described_class.new.execute(pipeline) } 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 end
end end
context 'when pipeline is not completed and codequality report does not exist' do context 'when there are no codequality reports for head pipeline' do
let(:pipeline) { create(:ci_pipeline, :running) } 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 context 'when there are no codequality reports for base pipeline' do
pipeline_artifact 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 end
end end
......
...@@ -21,8 +21,8 @@ RSpec.describe ::Ci::PipelineArtifacts::CreateQualityReportWorker do ...@@ -21,8 +21,8 @@ RSpec.describe ::Ci::PipelineArtifacts::CreateQualityReportWorker do
it_behaves_like 'an idempotent worker' do it_behaves_like 'an idempotent worker' do
let(:job_args) { pipeline_id } let(:job_args) { pipeline_id }
it 'creates a pipeline artifact' do it 'does not create another pipeline artifact if already has one' do
expect { subject }.to change { pipeline.pipeline_artifacts.count }.by(1) expect { subject }.not_to change { pipeline.pipeline_artifacts.count }
end end
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment