Merge branch 'eb-code-coverage-graph-storage' into 'master'

Store daily code coverage

See merge request gitlab-org/gitlab!24695

app/models/ci/build.rb

@@ -174,6 +174,8 @@ module Ci
               pipeline: Ci::Pipeline::PROJECT_ROUTE_AND_NAMESPACE_ROUTE)
     end
 
+    scope :with_coverage, -> { where.not(coverage: nil) }
+
     acts_as_taggable
 
     add_authentication_token_field :token, encrypted: :optional

app/models/ci/daily_report_result.rb

+# frozen_string_literal: true
+
+module Ci
+  class DailyReportResult < ApplicationRecord
+    extend Gitlab::Ci::Model
+
+    belongs_to :last_pipeline, class_name: 'Ci::Pipeline', foreign_key: :last_pipeline_id
+    belongs_to :project
+
+    # TODO: Refactor this out when BuildReportResult is implemented.
+    # They both need to share the same enum values for param.
+    REPORT_PARAMS = {
+      coverage: 0
+    }.freeze
+
+    enum param_type: REPORT_PARAMS
+
+    def self.upsert_reports(data)
+      upsert_all(data, unique_by: :index_daily_report_results_unique_columns) if data.any?
+    end
+  end
+end

app/models/ci/pipeline.rb

@@ -82,6 +82,8 @@ module Ci
 
     has_one :pipeline_config, class_name: 'Ci::PipelineConfig', inverse_of: :pipeline
 
+    has_many :daily_report_results, class_name: 'Ci::DailyReportResult', foreign_key: :last_pipeline_id
+
     accepts_nested_attributes_for :variables, reject_if: :persisted?
 
     delegate :id, to: :project, prefix: true
@@ -189,7 +191,10 @@ module Ci
       end
 
       after_transition [:created, :waiting_for_resource, :preparing, :pending, :running] => :success do |pipeline|
-        pipeline.run_after_commit { PipelineSuccessWorker.perform_async(pipeline.id) }
+        # We wait a little bit to ensure that all BuildFinishedWorkers finish first
+        # because this is where some metrics like code coverage is parsed and stored
+        # in CI build records which the daily build metrics worker relies on.
+        pipeline.run_after_commit { Ci::DailyReportResultsWorker.perform_in(10.minutes, pipeline.id) }
       end
 
       after_transition do |pipeline, transition|
@@ -941,6 +946,14 @@ module Ci
       Ci::PipelineEnums.ci_config_sources.key?(config_source.to_sym)
     end
 
+    def source_ref_path
+      if branch? || merge_request?
+        Gitlab::Git::BRANCH_REF_PREFIX + source_ref.to_s
+      elsif tag?
+        Gitlab::Git::TAG_REF_PREFIX + source_ref.to_s
+      end
+    end
+
     private
 
     def pipeline_data

app/models/project.rb

@@ -314,6 +314,8 @@ class Project < ApplicationRecord
 
   has_many :import_failures, inverse_of: :project
 
+  has_many :daily_report_results, class_name: 'Ci::DailyReportResult'
+
   accepts_nested_attributes_for :variables, allow_destroy: true
   accepts_nested_attributes_for :project_feature, update_only: true
   accepts_nested_attributes_for :import_data

app/services/ci/daily_report_result_service.rb

+# frozen_string_literal: true
+
+module Ci
+  class DailyReportResultService
+    def execute(pipeline)
+      return unless Feature.enabled?(:ci_daily_code_coverage, pipeline.project, default_enabled: true)
+
+      DailyReportResult.upsert_reports(coverage_reports(pipeline))
+    end
+
+    private
+
+    def coverage_reports(pipeline)
+      base_attrs = {
+        project_id: pipeline.project_id,
+        ref_path: pipeline.source_ref_path,
+        param_type: DailyReportResult.param_types[:coverage],
+        date: pipeline.created_at.to_date,
+        last_pipeline_id: pipeline.id
+      }
+
+      pipeline.builds.with_coverage.map do |build|
+        base_attrs.merge(
+          title: build.group_name,
+          value: build.coverage
+        )
+      end
+    end
+  end
+end

app/workers/all_queues.yml

@@ -605,6 +605,13 @@
   :resource_boundary: :unknown
   :weight: 1
   :idempotent: 
+- :name: pipeline_background:ci_daily_report_results
+  :feature_category: :continuous_integration
+  :has_external_dependencies: 
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
 - :name: pipeline_cache:expire_job_cache
   :feature_category: :continuous_integration
   :has_external_dependencies: 
@@ -745,13 +752,6 @@
   :resource_boundary: :unknown
   :weight: 5
   :idempotent: 
-- :name: pipeline_processing:pipeline_success
-  :feature_category: :continuous_integration
-  :has_external_dependencies: 
-  :urgency: :high
-  :resource_boundary: :unknown
-  :weight: 5
-  :idempotent: 
 - :name: pipeline_processing:pipeline_update
   :feature_category: :continuous_integration
   :has_external_dependencies: 

app/workers/ci/daily_report_results_worker.rb

+# frozen_string_literal: true
+
+module Ci
+  class DailyReportResultsWorker
+    include ApplicationWorker
+    include PipelineBackgroundQueue
+
+    idempotent!
+
+    def perform(pipeline_id)
+      Ci::Pipeline.find_by_id(pipeline_id).try do |pipeline|
+        Ci::DailyReportResultService.new.execute(pipeline)
+      end
+    end
+  end
+end

app/workers/pipeline_success_worker.rb

-# frozen_string_literal: true
-
-class PipelineSuccessWorker # rubocop:disable Scalability/IdempotentWorker
-  include ApplicationWorker
-  include PipelineQueue
-
-  queue_namespace :pipeline_processing
-  urgency :high
-
-  def perform(pipeline_id)
-    # no-op
-  end
-end

changelogs/unreleased/eb-code-coverage-graph-storage.yml

+---
+title: Store daily code coverages into ci_daily_report_results table
+merge_request: 24695
+author:
+type: added

db/migrate/20200204131831_create_daily_report_results.rb

+# frozen_string_literal: true
+
+class CreateDailyReportResults < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def change
+    create_table :ci_daily_report_results do |t|
+      t.date :date, null: false
+      t.bigint :project_id, null: false
+      t.bigint :last_pipeline_id, null: false
+      t.float :value, null: false
+      t.integer :param_type, limit: 8, null: false
+      t.string :ref_path, null: false # rubocop:disable Migration/AddLimitToStringColumns
+      t.string :title, null: false # rubocop:disable Migration/AddLimitToStringColumns
+
+      t.index :last_pipeline_id
+      t.index [:project_id, :ref_path, :param_type, :date, :title], name: 'index_daily_report_results_unique_columns', unique: true
+      t.foreign_key :projects, on_delete: :cascade
+      t.foreign_key :ci_pipelines, column: :last_pipeline_id, on_delete: :cascade
+    end
+  end
+end

db/schema.rb

@@ -738,6 +738,18 @@ ActiveRecord::Schema.define(version: 2020_03_13_123934) do
     t.index ["build_id"], name: "index_ci_builds_runner_session_on_build_id", unique: true
   end
 
+  create_table "ci_daily_report_results", force: :cascade do |t|
+    t.date "date", null: false
+    t.bigint "project_id", null: false
+    t.bigint "last_pipeline_id", null: false
+    t.float "value", null: false
+    t.bigint "param_type", null: false
+    t.string "ref_path", null: false
+    t.string "title", null: false
+    t.index ["last_pipeline_id"], name: "index_ci_daily_report_results_on_last_pipeline_id"
+    t.index ["project_id", "ref_path", "param_type", "date", "title"], name: "index_daily_report_results_unique_columns", unique: true
+  end
+
   create_table "ci_group_variables", id: :serial, force: :cascade do |t|
     t.string "key", null: false
     t.text "value"
@@ -4765,6 +4777,8 @@ ActiveRecord::Schema.define(version: 2020_03_13_123934) do
   add_foreign_key "ci_builds_metadata", "ci_builds", column: "build_id", on_delete: :cascade
   add_foreign_key "ci_builds_metadata", "projects", on_delete: :cascade
   add_foreign_key "ci_builds_runner_session", "ci_builds", column: "build_id", on_delete: :cascade
+  add_foreign_key "ci_daily_report_results", "ci_pipelines", column: "last_pipeline_id", on_delete: :cascade
+  add_foreign_key "ci_daily_report_results", "projects", on_delete: :cascade
   add_foreign_key "ci_group_variables", "namespaces", column: "group_id", name: "fk_33ae4d58d8", on_delete: :cascade
   add_foreign_key "ci_job_artifacts", "ci_builds", column: "job_id", on_delete: :cascade
   add_foreign_key "ci_job_artifacts", "projects", on_delete: :cascade

spec/db/schema_spec.rb

@@ -132,6 +132,7 @@ describe 'Database schema' do
     'Ci::Build' => %w[failure_reason],
     'Ci::BuildMetadata' => %w[timeout_source],
     'Ci::BuildTraceChunk' => %w[data_store],
+    'Ci::DailyReportResult' => %w[param_type],
     'Ci::JobArtifact' => %w[file_type],
     'Ci::Pipeline' => %w[source config_source failure_reason],
     'Ci::Processable' => %w[failure_reason],

spec/factories/ci/daily_report_results.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :ci_daily_report_result, class: 'Ci::DailyReportResult' do
+    ref_path { Gitlab::Git::BRANCH_REF_PREFIX + 'master' }
+    date { Time.zone.now.to_date }
+    project
+    last_pipeline factory: :ci_pipeline
+    param_type { Ci::DailyReportResult.param_types[:coverage] }
+    title { 'rspec' }
+    value { 77.0 }
+  end
+end

spec/lib/gitlab/import_export/all_models.yml

@@ -208,6 +208,7 @@ ci_pipelines:
 - vulnerability_findings
 - pipeline_config
 - security_scans
+- daily_report_results
 pipeline_variables:
 - pipeline
 stages:
@@ -470,6 +471,7 @@ project:
 - status_page_setting
 - requirements
 - export_jobs
+- daily_report_results
 award_emoji:
 - awardable
 - user

spec/models/ci/daily_report_result_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Ci::DailyReportResult do
+  describe '.upsert_reports' do
+    let!(:rspec_coverage) do
+      create(
+        :ci_daily_report_result,
+        title: 'rspec',
+        date: '2020-03-09',
+        value: 71.2
+      )
+    end
+    let!(:new_pipeline) { create(:ci_pipeline) }
+
+    it 'creates or updates matching report results' do
+      described_class.upsert_reports([
+        {
+          project_id: rspec_coverage.project_id,
+          ref_path: rspec_coverage.ref_path,
+          param_type: described_class.param_types[rspec_coverage.param_type],
+          last_pipeline_id: new_pipeline.id,
+          date: rspec_coverage.date,
+          title: 'rspec',
+          value: 81.0
+        },
+        {
+          project_id: rspec_coverage.project_id,
+          ref_path: rspec_coverage.ref_path,
+          param_type: described_class.param_types[rspec_coverage.param_type],
+          last_pipeline_id: new_pipeline.id,
+          date: rspec_coverage.date,
+          title: 'karma',
+          value: 87.0
+        }
+      ])
+
+      rspec_coverage.reload
+
+      expect(rspec_coverage).to have_attributes(
+        last_pipeline_id: new_pipeline.id,
+        value: 81.0
+      )
+
+      expect(described_class.find_by_title('karma')).to have_attributes(
+        project_id: rspec_coverage.project_id,
+        ref_path: rspec_coverage.ref_path,
+        param_type: rspec_coverage.param_type,
+        last_pipeline_id: new_pipeline.id,
+        date: rspec_coverage.date,
+        value: 87.0
+      )
+    end
+
+    context 'when given data is empty' do
+      it 'does nothing' do
+        expect { described_class.upsert_reports([]) }.not_to raise_error
+      end
+    end
+  end
+end

spec/models/ci/pipeline_spec.rb

@@ -1120,7 +1120,7 @@ describe Ci::Pipeline, :mailer do
           let(:from_status) { status }
 
           it 'schedules pipeline success worker' do
-            expect(PipelineSuccessWorker).to receive(:perform_async).with(pipeline.id)
+            expect(Ci::DailyReportResultsWorker).to receive(:perform_in).with(10.minutes, pipeline.id)
 
             pipeline.succeed
           end
@@ -3114,4 +3114,25 @@ describe Ci::Pipeline, :mailer do
       end
     end
   end
+
+  describe '#source_ref_path' do
+    subject { pipeline.source_ref_path }
+
+    context 'when pipeline is for a branch' do
+      it { is_expected.to eq(Gitlab::Git::BRANCH_REF_PREFIX + pipeline.source_ref.to_s) }
+    end
+
+    context 'when pipeline is for a merge request' do
+      let(:merge_request) { create(:merge_request, source_project: project) }
+      let(:pipeline) { create(:ci_pipeline, project: project, head_pipeline_of: merge_request) }
+
+      it { is_expected.to eq(Gitlab::Git::BRANCH_REF_PREFIX + pipeline.source_ref.to_s) }
+    end
+
+    context 'when pipeline is for a tag' do
+      let(:pipeline) { create(:ci_pipeline, project: project, tag: true) }
+
+      it { is_expected.to eq(Gitlab::Git::TAG_REF_PREFIX + pipeline.source_ref.to_s) }
+    end
+  end
 end

spec/services/ci/daily_report_result_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Ci::DailyReportResultService, '#execute' do
+  let!(:pipeline) { create(:ci_pipeline, created_at: '2020-02-06 00:01:10') }
+  let!(:rspec_job) { create(:ci_build, pipeline: pipeline, name: '3/3 rspec', coverage: 80) }
+  let!(:karma_job) { create(:ci_build, pipeline: pipeline, name: '2/2 karma', coverage: 90) }
+  let!(:extra_job) { create(:ci_build, pipeline: pipeline, name: 'extra', coverage: nil) }
+
+  it 'creates daily code coverage record for each job in the pipeline that has coverage value' do
+    described_class.new.execute(pipeline)
+
+    Ci::DailyReportResult.find_by(title: 'rspec').tap do |coverage|
+      expect(coverage).to have_attributes(
+        project_id: pipeline.project.id,
+        last_pipeline_id: pipeline.id,
+        ref_path: pipeline.source_ref_path,
+        param_type: 'coverage',
+        title: rspec_job.group_name,
+        value: rspec_job.coverage,
+        date: pipeline.created_at.to_date
+      )
+    end
+
+    Ci::DailyReportResult.find_by(title: 'karma').tap do |coverage|
+      expect(coverage).to have_attributes(
+        project_id: pipeline.project.id,
+        last_pipeline_id: pipeline.id,
+        ref_path: pipeline.source_ref_path,
+        param_type: 'coverage',
+        title: karma_job.group_name,
+        value: karma_job.coverage,
+        date: pipeline.created_at.to_date
+      )
+    end
+
+    expect(Ci::DailyReportResult.find_by(title: 'extra')).to be_nil
+  end
+
+  context 'when there is an existing daily code coverage for the matching date, project, ref_path, and group name' do
+    let!(:new_pipeline) do
+      create(
+        :ci_pipeline,
+        project: pipeline.project,
+        ref: pipeline.ref,
+        created_at: '2020-02-06 00:02:20'
+      )
+    end
+    let!(:new_rspec_job) { create(:ci_build, pipeline: new_pipeline, name: '4/4 rspec', coverage: 84) }
+    let!(:new_karma_job) { create(:ci_build, pipeline: new_pipeline, name: '3/3 karma', coverage: 92) }
+
+    before do
+      # Create the existing daily code coverage records
+      described_class.new.execute(pipeline)
+    end
+
+    it "updates the existing record's coverage value and last_pipeline_id" do
+      rspec_coverage = Ci::DailyReportResult.find_by(title: 'rspec')
+      karma_coverage = Ci::DailyReportResult.find_by(title: 'karma')
+
+      # Bump up the coverage values
+      described_class.new.execute(new_pipeline)
+
+      rspec_coverage.reload
+      karma_coverage.reload
+
+      expect(rspec_coverage).to have_attributes(
+        last_pipeline_id: new_pipeline.id,
+        value: new_rspec_job.coverage
+      )
+
+      expect(karma_coverage).to have_attributes(
+        last_pipeline_id: new_pipeline.id,
+        value: new_karma_job.coverage
+      )
+    end
+  end
+
+  context 'when the ID of the pipeline is older than the last_pipeline_id' do
+    let!(:new_pipeline) do
+      create(
+        :ci_pipeline,
+        project: pipeline.project,
+        ref: pipeline.ref,
+        created_at: '2020-02-06 00:02:20'
+      )
+    end
+    let!(:new_rspec_job) { create(:ci_build, pipeline: new_pipeline, name: '4/4 rspec', coverage: 84) }
+    let!(:new_karma_job) { create(:ci_build, pipeline: new_pipeline, name: '3/3 karma', coverage: 92) }
+
+    before do
+      # Create the existing daily code coverage records
+      # but in this case, for the newer pipeline first.
+      described_class.new.execute(new_pipeline)
+    end
+
+    it 'updates the existing daily code coverage records' do
+      rspec_coverage = Ci::DailyReportResult.find_by(title: 'rspec')
+      karma_coverage = Ci::DailyReportResult.find_by(title: 'karma')
+
+      # Run another one but for the older pipeline.
+      # This simulates the scenario wherein the success worker
+      # of an older pipeline, for some network hiccup, was delayed
+      # and only got executed right after the newer pipeline's success worker.
+      # Ideally, we don't want to bump the coverage value with an older one
+      # but given this can be a rare edge case and can be remedied by re-running
+      # the pipeline we'll just let it be for now. In return, we are able to use
+      # Rails 6 shiny new method, upsert_all, and simplify the code a lot.
+      described_class.new.execute(pipeline)
+
+      rspec_coverage.reload
+      karma_coverage.reload
+
+      expect(rspec_coverage).to have_attributes(
+        last_pipeline_id: pipeline.id,
+        value: rspec_job.coverage
+      )
+
+      expect(karma_coverage).to have_attributes(
+        last_pipeline_id: pipeline.id,
+        value: karma_job.coverage
+      )
+    end
+  end
+
+  context 'when pipeline has no builds with coverage' do
+    let!(:new_pipeline) do
+      create(
+        :ci_pipeline,
+        created_at: '2020-02-06 00:02:20'
+      )
+    end
+    let!(:some_job) { create(:ci_build, pipeline: new_pipeline, name: 'foo') }
+
+    it 'does nothing' do
+      expect { described_class.new.execute(new_pipeline) }.not_to raise_error
+    end
+  end
+end

spec/workers/ci/daily_report_results_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Ci::DailyReportResultsWorker do
+  describe '#perform' do
+    let!(:pipeline) { create(:ci_pipeline) }
+
+    subject { described_class.new.perform(pipeline_id) }
+
+    context 'when pipeline is found' do
+      let(:pipeline_id) { pipeline.id }
+
+      it 'executes service' do
+        expect_any_instance_of(Ci::DailyReportResultService)
+          .to receive(:execute).with(pipeline)
+
+        subject
+      end
+    end
+
+    context 'when pipeline is not found' do
+      let(:pipeline_id) { 123 }
+
+      it 'does not execute service' do
+        expect_any_instance_of(Ci::DailyReportResultService)
+          .not_to receive(:execute)
+
+        expect { subject }
+          .not_to raise_error
+      end
+    end
+  end
+end