Commit caa86179 authored by Shinya Maeda's avatar Shinya Maeda

Merge branch 'mo-introduce-quality-endpoint' into 'master'

Add codequality mr diff report endpoint

See merge request gitlab-org/gitlab!52843
parents 97c2def3 9b3a5413
......@@ -23,7 +23,8 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
:coverage_reports,
:terraform_reports,
:accessibility_reports,
:codequality_reports
:codequality_reports,
:codequality_mr_diff_reports
]
before_action :set_issuables_index, only: [:index]
before_action :authenticate_user!, only: [:assign_related_issues]
......@@ -67,7 +68,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
:toggle_award_emoji, :toggle_subscription, :update
]
feature_category :code_testing, [:test_reports, :coverage_reports]
feature_category :code_testing, [:test_reports, :coverage_reports, :codequality_mr_diff_reports]
feature_category :accessibility_testing, [:accessibility_reports]
feature_category :infrastructure_as_code, [:terraform_reports]
......@@ -196,6 +197,10 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
end
end
def codequality_mr_diff_reports
reports_response(@merge_request.find_codequality_mr_diff_reports)
end
def codequality_reports
reports_response(@merge_request.compare_codequality_reports)
end
......
......@@ -1003,8 +1003,8 @@ module Ci
has_reports?(Ci::JobArtifact.coverage_reports)
end
def has_codequality_reports?
pipeline_artifacts&.has_report?(:code_quality)
def has_codequality_mr_diff_report?
pipeline_artifacts&.has_report?(:code_quality_mr_diff)
end
def can_generate_codequality_reports?
......
......@@ -15,7 +15,7 @@ module Ci
DEFAULT_FILE_NAMES = {
code_coverage: 'code_coverage.json',
code_quality: 'code_quality.json'
code_quality_mr_diff: 'code_quality_mr_diff.json'
}.freeze
belongs_to :project, class_name: "Project", inverse_of: :pipeline_artifacts
......@@ -32,7 +32,7 @@ module Ci
enum file_type: {
code_coverage: 1,
code_quality: 2
code_quality_mr_diff: 2
}
class << self
......
......@@ -1484,6 +1484,24 @@ class MergeRequest < ApplicationRecord
compare_reports(Ci::GenerateCoverageReportsService)
end
def has_codequality_mr_diff_report?
return false unless ::Gitlab::Ci::Features.display_quality_on_mr_diff?(project)
actual_head_pipeline&.has_codequality_mr_diff_report?
end
# TODO: this method and compare_test_reports use the same
# result type, which is handled by the controller's #reports_response.
# we should minimize mistakes by isolating the common parts.
# issue: https://gitlab.com/gitlab-org/gitlab/issues/34224
def find_codequality_mr_diff_reports
unless has_codequality_mr_diff_report?
return { status: :error, status_reason: 'This merge request does not have codequality mr diff reports' }
end
compare_reports(Ci::GenerateCodequalityMrDiffReportService)
end
def has_codequality_reports?
return false unless ::Gitlab::Ci::Features.display_quality_on_mr_diff?(project)
......
......@@ -2,7 +2,7 @@
module Ci
module PipelineArtifacts
class CodeCoveragePresenter < ProcessablePresenter
class CodeCoveragePresenter < Gitlab::View::Presenter::Delegated
include Gitlab::Utils::StrongMemoize
def for_files(filenames)
......
# frozen_string_literal: true
module Ci
module PipelineArtifacts
class CodeQualityMrDiffPresenter < Gitlab::View::Presenter::Delegated
include Gitlab::Utils::StrongMemoize
def for_files(filenames)
quality_files = raw_report["files"].select { |key| filenames.include?(key) }
{ files: quality_files }
end
private
def raw_report
strong_memoize(:raw_report) do
self.each_blob do |blob|
Gitlab::Json.parse(blob).with_indifferent_access
end
end
end
end
end
end
# frozen_string_literal: true
module Ci
# TODO: a couple of points with this approach:
# + reuses existing architecture and reactive caching
# - it's not a report comparison and some comparing features must be turned off.
# see CompareReportsBaseService for more notes.
# issue: https://gitlab.com/gitlab-org/gitlab/issues/34224
class GenerateCodequalityMrDiffReportService < CompareReportsBaseService
def execute(base_pipeline, head_pipeline)
merge_request = MergeRequest.find_by_id(params[:id])
{
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)
}
rescue => e
Gitlab::ErrorTracking.track_exception(e, project_id: project.id)
{
status: :error,
key: key(base_pipeline, head_pipeline),
status_reason: _('An error occurred while fetching codequality mr diff reports.')
}
end
def latest?(base_pipeline, head_pipeline, data)
data&.fetch(:key, nil) == key(base_pipeline, head_pipeline)
end
end
end
# frozen_string_literal: true
module Ci
module PipelineArtifacts
class CreateQualityReportService
class CreateCodeQualityMrDiffReportService
def execute(pipeline)
return unless pipeline.can_generate_codequality_reports?
return if pipeline.has_codequality_reports?
return if pipeline.has_codequality_mr_diff_report?
file = build_carrierwave_file(pipeline)
pipeline.pipeline_artifacts.create!(
project_id: pipeline.project_id,
file_type: :code_quality,
file_type: :code_quality_mr_diff,
file_format: :raw,
size: file["tempfile"].size,
file: file,
......@@ -23,7 +23,7 @@ module Ci
def build_carrierwave_file(pipeline)
CarrierWaveStringFile.new_file(
file_content: build_quality_mr_diff_report(pipeline),
filename: Ci::PipelineArtifact::DEFAULT_FILE_NAMES.fetch(:code_quality),
filename: Ci::PipelineArtifact::DEFAULT_FILE_NAMES.fetch(:code_quality_mr_diff),
content_type: 'application/json'
)
end
......
......@@ -12,7 +12,7 @@ module Ci
def perform(pipeline_id)
Ci::Pipeline.find_by_id(pipeline_id).try do |pipeline|
Ci::PipelineArtifacts::CreateQualityReportService.new.execute(pipeline)
Ci::PipelineArtifacts::CreateCodeQualityMrDiffReportService.new.execute(pipeline)
end
end
end
......
......@@ -18,6 +18,7 @@ resources :merge_requests, concerns: :awardable, except: [:new, :create, :show],
get :coverage_reports
get :terraform_reports
get :codequality_reports
get :codequality_mr_diff_reports
scope constraints: ->(req) { req.format == :json }, as: :json do
get :commits
......
......@@ -3152,6 +3152,9 @@ msgstr ""
msgid "An error occurred while fetching branches. Retry the search."
msgstr ""
msgid "An error occurred while fetching codequality mr diff reports."
msgstr ""
msgid "An error occurred while fetching commits. Retry the search."
msgstr ""
......
......@@ -1118,6 +1118,108 @@ RSpec.describe Projects::MergeRequestsController do
end
end
describe 'GET codequality_mr_diff_reports' do
let_it_be(:merge_request) do
create(:merge_request,
:with_merge_request_pipeline,
target_project: project,
source_project: project)
end
let(:pipeline) do
create(:ci_pipeline,
:success,
project: merge_request.source_project,
ref: merge_request.source_branch,
sha: merge_request.diff_head_sha)
end
before do
allow_any_instance_of(MergeRequest)
.to receive(:find_codequality_mr_diff_reports)
.and_return(report)
allow_any_instance_of(MergeRequest)
.to receive(:actual_head_pipeline)
.and_return(pipeline)
end
subject(:get_codequality_mr_diff_reports) do
get :codequality_mr_diff_reports, params: {
namespace_id: project.namespace.to_param,
project_id: project,
id: merge_request.iid
},
format: :json
end
context 'permissions on a public project with private CI/CD' do
let(:project) { create :project, :repository, :public, :builds_private }
let(:report) { { status: :parsed, data: { 'files' => {} } } }
context 'while signed out' do
before do
sign_out(user)
end
it 'responds with a 404' do
get_codequality_mr_diff_reports
expect(response).to have_gitlab_http_status(:not_found)
expect(response.body).to be_blank
end
end
context 'while signed in as an unrelated user' do
before do
sign_in(create(:user))
end
it 'responds with a 404' do
get_codequality_mr_diff_reports
expect(response).to have_gitlab_http_status(:not_found)
expect(response.body).to be_blank
end
end
end
context 'when pipeline has jobs with codequality mr diff report' do
before do
allow_any_instance_of(MergeRequest)
.to receive(:has_codequality_mr_diff_report?)
.and_return(true)
end
context 'when processing codequality mr diff report is in progress' do
let(:report) { { status: :parsing } }
it 'sends polling interval' do
expect(Gitlab::PollingInterval).to receive(:set_header)
get_codequality_mr_diff_reports
end
it 'returns 204 HTTP status' do
get_codequality_mr_diff_reports
expect(response).to have_gitlab_http_status(:no_content)
end
end
context 'when processing codequality mr diff report is completed' do
let(:report) { { status: :parsed, data: { 'files' => {} } } }
it 'returns codequality mr diff report' do
get_codequality_mr_diff_reports
expect(response).to have_gitlab_http_status(:ok)
expect(json_response).to eq({ 'files' => {} })
end
end
end
end
describe 'GET terraform_reports' do
let_it_be(:merge_request) do
create(:merge_request,
......
......@@ -49,12 +49,12 @@ FactoryBot.define do
size { 1.megabyte }
end
trait :with_codequality_report do
file_type { :code_quality }
trait :with_codequality_mr_diff_report do
file_type { :code_quality_mr_diff }
after(:build) do |artifact, _evaluator|
artifact.file = fixture_file_upload(
Rails.root.join('spec/fixtures/pipeline_artifacts/code_quality.json'), 'application/json')
Rails.root.join('spec/fixtures/pipeline_artifacts/code_quality_mr_diff.json'), 'application/json')
end
size { file.size }
......
......@@ -155,9 +155,9 @@ FactoryBot.define do
end
end
trait :with_quality_report_artifact do
trait :with_codequality_mr_diff_report do
after(:build) do |pipeline, evaluator|
pipeline.pipeline_artifacts << build(:ci_pipeline_artifact, :with_codequality_report, pipeline: pipeline, project: pipeline.project)
pipeline.pipeline_artifacts << build(:ci_pipeline_artifact, :with_codequality_mr_diff_report, pipeline: pipeline, project: pipeline.project)
end
end
......
......@@ -200,6 +200,18 @@ FactoryBot.define do
end
end
trait :with_codequality_mr_diff_reports do
after(:build) do |merge_request|
merge_request.head_pipeline = build(
:ci_pipeline,
:success,
:with_codequality_mr_diff_report,
project: merge_request.source_project,
ref: merge_request.source_branch,
sha: merge_request.diff_head_sha)
end
end
trait :with_terraform_reports do
after(:build) do |merge_request|
merge_request.head_pipeline = build(
......
{
"files": {
"demo.rb": [
{
"line": 5,
"description": "Method `new_array` has 8 arguments (exceeds 4 allowed). Consider refactoring.",
"severity": "major"
},
{
"line": 5,
"description": "Avoid parameter lists longer than 5 parameters. [8/5]",
"severity": "minor"
}
]
}
}
{
"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"
}
]
}
}
......@@ -97,18 +97,18 @@ RSpec.describe Ci::PipelineArtifact, type: :model do
end
end
context 'when file_type is code_quality' do
let(:file_type) { :code_quality }
context 'when file_type is code_quality_mr_diff' do
let(:file_type) { :code_quality_mr_diff }
context 'when pipeline artifact has a quality report' do
let!(:pipeline_artifact) { create(:ci_pipeline_artifact, :with_codequality_report) }
context 'when pipeline artifact has a codequality mr diff report' do
let!(:pipeline_artifact) { create(:ci_pipeline_artifact, :with_codequality_mr_diff_report) }
it 'returns true' do
expect(pipeline_artifact).to be_truthy
end
end
context 'when pipeline artifact does not have a quality report' do
context 'when pipeline artifact does not have a codequality mr diff report' do
it 'returns false' do
expect(pipeline_artifact).to be_falsey
end
......@@ -145,14 +145,14 @@ RSpec.describe Ci::PipelineArtifact, type: :model do
end
end
context 'when file_type is code_quality' do
let(:file_type) { :code_quality }
context 'when file_type is code_quality_mr_diff' do
let(:file_type) { :code_quality_mr_diff }
context 'when pipeline artifact has a quality report' do
let!(:coverage_report) { create(:ci_pipeline_artifact, :with_codequality_report) }
let!(:coverage_report) { create(:ci_pipeline_artifact, :with_codequality_mr_diff_report) }
it 'returns a pipeline artifact with a quality report' do
expect(pipeline_artifact.file_type).to eq('code_quality')
expect(pipeline_artifact.file_type).to eq('code_quality_mr_diff')
end
end
......
......@@ -3510,16 +3510,16 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
end
end
describe '#has_codequality_reports?' do
subject { pipeline.has_codequality_reports? }
describe '#has_codequality_mr_diff_report?' do
subject { pipeline.has_codequality_mr_diff_report? }
context 'when pipeline has a codequality artifact' do
let(:pipeline) { create(:ci_pipeline, :with_quality_report_artifact, :running, project: project) }
context 'when pipeline has a codequality mr diff report' do
let(:pipeline) { create(:ci_pipeline, :with_codequality_mr_diff_report, :running, project: project) }
it { expect(subject).to be_truthy }
end
context 'when pipeline does not have a codequality artifact' do
context 'when pipeline does not have a codequality mr diff report' do
let(:pipeline) { create(:ci_pipeline, :success, project: project) }
it { expect(subject).to be_falsey }
......
......@@ -1982,6 +1982,30 @@ RSpec.describe MergeRequest, factory_default: :keep do
end
end
describe '#has_codequality_mr_diff_report?' do
subject { merge_request.has_codequality_mr_diff_report? }
context 'when head pipeline has codequality mr diff report' do
let(:merge_request) { create(:merge_request, :with_codequality_mr_diff_reports) }
it { is_expected.to be_truthy }
context 'when feature flag is disabled' do
before do
stub_feature_flags(codequality_mr_diff: false)
end
it { is_expected.to be_falsey }
end
end
context 'when head pipeline does not have codeqquality mr diff report' do
let(:merge_request) { create(:merge_request) }
it { is_expected.to be_falsey }
end
end
describe '#has_codequality_reports?' do
subject { merge_request.has_codequality_reports? }
......@@ -2155,6 +2179,54 @@ RSpec.describe MergeRequest, factory_default: :keep do
end
end
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(:pipeline) { merge_request.head_pipeline }
subject(:mr_diff_report) { merge_request.find_codequality_mr_diff_reports }
context 'when head pipeline has coverage reports' do
context 'when reactive cache worker is parsing results asynchronously' do
it 'returns status' do
expect(mr_diff_report[:status]).to eq(:parsing)
end
end
context 'when reactive cache worker is inline' do
before do
synchronous_reactive_cache(merge_request)
end
it 'returns status and data' do
expect(mr_diff_report[:status]).to eq(:parsed)
end
context 'when an error occurrs' do
before do
merge_request.update!(head_pipeline: nil)
end
it 'returns an error message' do
expect(mr_diff_report[:status]).to eq(:error)
end
end
context 'when cached results is not latest' do
before do
allow_next_instance_of(Ci::GenerateCodequalityMrDiffReportService) do |service|
allow(service).to receive(:latest?).and_return(false)
end
end
it 'raises and InvalidateReactiveCache error' do
expect { mr_diff_report }.to raise_error(ReactiveCaching::InvalidateReactiveCache)
end
end
end
end
end
describe '#compare_test_reports' do
subject { merge_request.compare_test_reports }
......
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe Ci::PipelineArtifacts::CodeQualityMrDiffPresenter do
let(:pipeline_artifact) { create(:ci_pipeline_artifact, :with_codequality_mr_diff_report) }
subject(:presenter) { described_class.new(pipeline_artifact) }
describe '#for_files' do
subject(:quality_data) { presenter.for_files(filenames) }
context 'when code quality has data' do
context 'when filenames is empty' do
let(:filenames) { %w() }
it 'returns hash without quality' do
expect(quality_data).to match(files: {})
end
end
context 'when filenames do not match code quality data' do
let(:filenames) { %w(demo.rb) }
it 'returns hash without quality' do
expect(quality_data).to match(files: {})
end
end
context 'when filenames matches code quality data' do
context 'when asking for one filename' do
let(:filenames) { %w(file_a.rb) }
it 'returns quality for the given filename' do
expect(quality_data).to match(
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" }
]
}
)
end
end
context 'when asking for multiple filenames' do
let(:filenames) { %w(file_a.rb file_b.rb) }
it 'returns quality for the given filenames' do
expect(quality_data).to match(
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" }
]
}
)
end
end
end
end
end
end
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe Ci::GenerateCodequalityMrDiffReportService do
let(:service) { described_class.new(project) }
let(:project) { create(:project, :repository) }
describe '#execute' 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!(:service) { described_class.new(project, nil, id: merge_request.id) }
let!(:head_pipeline) { merge_request.head_pipeline }
let!(:base_pipeline) { nil }
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
end
expect(subject[:status]).to eq(:parsed)
expect(subject[:data]).to eq(files: {})
end
end
context 'when head pipeline does not have a codequality mr diff report' do
let!(:merge_request) { create(:merge_request, source_project: project) }
let!(:service) { described_class.new(project, nil, id: merge_request.id) }
let!(:head_pipeline) { merge_request.head_pipeline }
let!(:base_pipeline) { nil }
it 'returns status and error message' do
expect(subject[:status]).to eq(:error)
expect(subject[:status_reason]).to include('An error occurred while fetching codequality mr diff reports.')
end
end
context 'when head pipeline has codequality mr diff report and no merge request associated' do
let!(:head_pipeline) { create(:ci_pipeline, :with_codequality_mr_diff_report, project: project) }
let!(:base_pipeline) { nil }
it 'returns status and error message' do
expect(subject[:status]).to eq(:error)
expect(subject[:status_reason]).to include('An error occurred while fetching codequality mr diff reports.')
end
end
end
end
......@@ -2,7 +2,7 @@
require 'spec_helper'
RSpec.describe ::Ci::PipelineArtifacts::CreateQualityReportService do
RSpec.describe ::Ci::PipelineArtifacts::CreateCodeQualityMrDiffReportService do
describe '#execute' do
subject(:pipeline_artifact) { described_class.new.execute(pipeline) }
......@@ -27,7 +27,7 @@ RSpec.describe ::Ci::PipelineArtifacts::CreateQualityReportService do
end
it 'persists the default file name' do
expect(pipeline_artifact.file.filename).to eq('code_quality.json')
expect(pipeline_artifact.file.filename).to eq('code_quality_mr_diff.json')
end
it 'sets expire_at to 1 week' do
......
......@@ -11,7 +11,7 @@ RSpec.describe ::Ci::PipelineArtifacts::CreateQualityReportWorker do
let(:pipeline_id) { pipeline.id }
it 'calls pipeline codequality report service' do
expect_next_instance_of(::Ci::PipelineArtifacts::CreateQualityReportService) do |quality_report_service|
expect_next_instance_of(::Ci::PipelineArtifacts::CreateCodeQualityMrDiffReportService) do |quality_report_service|
expect(quality_report_service).to receive(:execute)
end
......@@ -31,7 +31,7 @@ RSpec.describe ::Ci::PipelineArtifacts::CreateQualityReportWorker do
let(:pipeline_id) { non_existing_record_id }
it 'does not call pipeline codequality report service' do
expect(Ci::PipelineArtifacts::CreateQualityReportService).not_to receive(:execute)
expect(Ci::PipelineArtifacts::CreateCodeQualityMrDiffReportService).not_to receive(:execute)
subject
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