summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorKamil Trzciński <ayufan@ayufan.eu>2018-06-05 17:14:39 +0000
committerKamil Trzciński <ayufan@ayufan.eu>2018-06-05 17:14:39 +0000
commit538110740acc1768315af097ef74213a6d8953d2 (patch)
tree3f5f7d0ad49102812204d5288b452cfaf88ca000
parent245f60f2b9a2d016288b4dce5a6aba5f05e34e5c (diff)
parentced0b4453e1928353b123051dab8ca1ed0137b0e (diff)
downloadgitlab-ce-538110740acc1768315af097ef74213a6d8953d2.tar.gz
Merge branch 'backstage/gb/use-persisted-stages-to-improve-pipelines-table' into 'master'
Improve pipeline index action performance by using persisted stages Closes #43132 See merge request gitlab-org/gitlab-ce!19063
-rw-r--r--app/controllers/projects/pipelines_controller.rb4
-rw-r--r--app/models/ci/group.rb8
-rw-r--r--app/models/ci/legacy_stage.rb6
-rw-r--r--app/models/ci/pipeline.rb24
-rw-r--r--app/models/ci/stage.rb32
-rw-r--r--app/models/concerns/has_status.rb2
-rw-r--r--app/models/project.rb9
-rw-r--r--app/serializers/pipeline_details_entity.rb2
-rw-r--r--app/serializers/pipeline_serializer.rb17
-rw-r--r--db/migrate/20180530135500_add_index_to_stages_position.rb15
-rw-r--r--db/schema.rb1
-rw-r--r--lib/gitlab/ci/pipeline/preloader.rb48
-rw-r--r--lib/gitlab/ci/status/stage/common.rb4
-rw-r--r--spec/controllers/projects/pipelines_controller_spec.rb109
-rw-r--r--spec/lib/gitlab/ci/pipeline/preloader_spec.rb47
-rw-r--r--spec/lib/gitlab/import_export/all_models.yml1
-rw-r--r--spec/models/ci/group_spec.rb51
-rw-r--r--spec/models/ci/pipeline_spec.rb118
-rw-r--r--spec/models/ci/stage_spec.rb142
-rw-r--r--spec/serializers/pipeline_serializer_spec.rb13
20 files changed, 579 insertions, 74 deletions
diff --git a/app/controllers/projects/pipelines_controller.rb b/app/controllers/projects/pipelines_controller.rb
index 6b40fc2fe68..768595ceeb4 100644
--- a/app/controllers/projects/pipelines_controller.rb
+++ b/app/controllers/projects/pipelines_controller.rb
@@ -23,8 +23,6 @@ class Projects::PipelinesController < Projects::ApplicationController
@finished_count = limited_pipelines_count(project, 'finished')
@pipelines_count = limited_pipelines_count(project)
- Gitlab::Ci::Pipeline::Preloader.preload(@pipelines)
-
respond_to do |format|
format.html
format.json do
@@ -34,7 +32,7 @@ class Projects::PipelinesController < Projects::ApplicationController
pipelines: PipelineSerializer
.new(project: @project, current_user: @current_user)
.with_pagination(request, response)
- .represent(@pipelines, disable_coverage: true),
+ .represent(@pipelines, disable_coverage: true, preload: true),
count: {
all: @pipelines_count,
running: @running_count,
diff --git a/app/models/ci/group.rb b/app/models/ci/group.rb
index 87898b086c6..9c1046e8715 100644
--- a/app/models/ci/group.rb
+++ b/app/models/ci/group.rb
@@ -31,6 +31,14 @@ module Ci
end
end
+ def self.fabricate(stage)
+ stage.statuses.ordered.latest
+ .sort_by(&:sortable_name).group_by(&:group_name)
+ .map do |group_name, grouped_statuses|
+ self.new(stage, name: group_name, jobs: grouped_statuses)
+ end
+ end
+
private
def commit_statuses
diff --git a/app/models/ci/legacy_stage.rb b/app/models/ci/legacy_stage.rb
index 9b536af672b..ce691875e42 100644
--- a/app/models/ci/legacy_stage.rb
+++ b/app/models/ci/legacy_stage.rb
@@ -16,11 +16,7 @@ module Ci
end
def groups
- @groups ||= statuses.ordered.latest
- .sort_by(&:sortable_name).group_by(&:group_name)
- .map do |group_name, grouped_statuses|
- Ci::Group.new(self, name: group_name, jobs: grouped_statuses)
- end
+ @groups ||= Ci::Group.fabricate(self)
end
def to_param
diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index 5eb30f4aaa0..eecd86349e4 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -18,7 +18,7 @@ module Ci
s&.project&.pipelines&.maximum(:iid) || s&.project&.pipelines&.count
end
- has_many :stages
+ has_many :stages, -> { order(position: :asc) }, inverse_of: :pipeline
has_many :statuses, class_name: 'CommitStatus', foreign_key: :commit_id, inverse_of: :pipeline
has_many :builds, foreign_key: :commit_id, inverse_of: :pipeline
has_many :trigger_requests, dependent: :destroy, foreign_key: :commit_id # rubocop:disable Cop/ActiveRecordDependent
@@ -254,6 +254,20 @@ module Ci
stage unless stage.statuses_count.zero?
end
+ ##
+ # TODO We do not completely switch to persisted stages because of
+ # race conditions with setting statuses gitlab-ce#23257.
+ #
+ def ordered_stages
+ return legacy_stages unless complete?
+
+ if Feature.enabled?('ci_pipeline_persisted_stages')
+ stages
+ else
+ legacy_stages
+ end
+ end
+
def legacy_stages
# TODO, this needs refactoring, see gitlab-ce#26481.
@@ -416,7 +430,7 @@ module Ci
def number_of_warnings
BatchLoader.for(id).batch(default_value: 0) do |pipeline_ids, loader|
- Build.where(commit_id: pipeline_ids)
+ ::Ci::Build.where(commit_id: pipeline_ids)
.latest
.failed_but_allowed
.group(:commit_id)
@@ -508,7 +522,8 @@ module Ci
def update_status
retry_optimistic_lock(self) do
- case latest_builds_status
+ case latest_builds_status.to_s
+ when 'created' then nil
when 'pending' then enqueue
when 'running' then run
when 'success' then succeed
@@ -516,6 +531,9 @@ module Ci
when 'canceled' then cancel
when 'skipped' then skip
when 'manual' then block
+ else
+ raise HasStatus::UnknownStatusError,
+ "Unknown status `#{latest_builds_status}`"
end
end
end
diff --git a/app/models/ci/stage.rb b/app/models/ci/stage.rb
index 5a1eeb966aa..ea07f37e6c1 100644
--- a/app/models/ci/stage.rb
+++ b/app/models/ci/stage.rb
@@ -68,16 +68,44 @@ module Ci
def update_status
retry_optimistic_lock(self) do
case statuses.latest.status
+ when 'created' then nil
when 'pending' then enqueue
when 'running' then run
when 'success' then succeed
when 'failed' then drop
when 'canceled' then cancel
when 'manual' then block
- when 'skipped' then skip
- else skip
+ when 'skipped', nil then skip
+ else
+ raise HasStatus::UnknownStatusError,
+ "Unknown status `#{statuses.latest.status}`"
end
end
end
+
+ def groups
+ @groups ||= Ci::Group.fabricate(self)
+ end
+
+ def has_warnings?
+ number_of_warnings.positive?
+ end
+
+ def number_of_warnings
+ BatchLoader.for(id).batch(default_value: 0) do |stage_ids, loader|
+ ::Ci::Build.where(stage_id: stage_ids)
+ .latest
+ .failed_but_allowed
+ .group(:stage_id)
+ .count
+ .each { |id, amount| loader.call(id, amount) }
+ end
+ end
+
+ def detailed_status(current_user)
+ Gitlab::Ci::Status::Stage::Factory
+ .new(self, current_user)
+ .fabricate!
+ end
end
end
diff --git a/app/models/concerns/has_status.rb b/app/models/concerns/has_status.rb
index 7c3ed96bc28..72c236a0fc7 100644
--- a/app/models/concerns/has_status.rb
+++ b/app/models/concerns/has_status.rb
@@ -11,6 +11,8 @@ module HasStatus
STATUSES_ENUM = { created: 0, pending: 1, running: 2, success: 3,
failed: 4, canceled: 5, skipped: 6, manual: 7 }.freeze
+ UnknownStatusError = Class.new(StandardError)
+
class_methods do
def status_sql
scope_relevant = respond_to?(:exclude_ignored) ? exclude_ignored : all
diff --git a/app/models/project.rb b/app/models/project.rb
index e17eabbb174..562198e2369 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -228,6 +228,7 @@ class Project < ActiveRecord::Base
has_many :commit_statuses
has_many :pipelines, class_name: 'Ci::Pipeline', inverse_of: :project
+ has_many :stages, class_name: 'Ci::Stage', inverse_of: :project
# Ci::Build objects store data on the file system such as artifact files and
# build traces. Currently there's no efficient way of removing this data in
@@ -1425,8 +1426,14 @@ class Project < ActiveRecord::Base
Ci::Runner.from("(#{union.to_sql}) ci_runners")
end
+ def active_runners
+ strong_memoize(:active_runners) do
+ all_runners.active
+ end
+ end
+
def any_runners?(&block)
- all_runners.active.any?(&block)
+ active_runners.any?(&block)
end
def valid_runners_token?(token)
diff --git a/app/serializers/pipeline_details_entity.rb b/app/serializers/pipeline_details_entity.rb
index 130968a44c1..8ba9cac53c4 100644
--- a/app/serializers/pipeline_details_entity.rb
+++ b/app/serializers/pipeline_details_entity.rb
@@ -1,6 +1,6 @@
class PipelineDetailsEntity < PipelineEntity
expose :details do
- expose :legacy_stages, as: :stages, using: StageEntity
+ expose :ordered_stages, as: :stages, using: StageEntity
expose :artifacts, using: BuildArtifactEntity
expose :manual_actions, using: BuildActionEntity
end
diff --git a/app/serializers/pipeline_serializer.rb b/app/serializers/pipeline_serializer.rb
index 7181f8a6b04..17a022539bb 100644
--- a/app/serializers/pipeline_serializer.rb
+++ b/app/serializers/pipeline_serializer.rb
@@ -1,14 +1,11 @@
class PipelineSerializer < BaseSerializer
include WithPagination
-
- InvalidResourceError = Class.new(StandardError)
-
entity PipelineDetailsEntity
def represent(resource, opts = {})
if resource.is_a?(ActiveRecord::Relation)
-
resource = resource.preload([
+ :stages,
:retryable_builds,
:cancelable_statuses,
:trigger_requests,
@@ -20,10 +17,14 @@ class PipelineSerializer < BaseSerializer
end
if paginated?
- super(@paginator.paginate(resource), opts)
- else
- super(resource, opts)
+ resource = paginator.paginate(resource)
end
+
+ if opts.delete(:preload)
+ resource = Gitlab::Ci::Pipeline::Preloader.preload!(resource)
+ end
+
+ super(resource, opts)
end
def represent_status(resource)
@@ -36,7 +37,7 @@ class PipelineSerializer < BaseSerializer
def represent_stages(resource)
return {} unless resource.present?
- data = represent(resource, { only: [{ details: [:stages] }] })
+ data = represent(resource, { only: [{ details: [:stages] }], preload: true })
data.dig(:details, :stages) || []
end
end
diff --git a/db/migrate/20180530135500_add_index_to_stages_position.rb b/db/migrate/20180530135500_add_index_to_stages_position.rb
new file mode 100644
index 00000000000..61150f33a25
--- /dev/null
+++ b/db/migrate/20180530135500_add_index_to_stages_position.rb
@@ -0,0 +1,15 @@
+class AddIndexToStagesPosition < ActiveRecord::Migration
+ include Gitlab::Database::MigrationHelpers
+
+ DOWNTIME = false
+
+ disable_ddl_transaction!
+
+ def up
+ add_concurrent_index :ci_stages, [:pipeline_id, :position]
+ end
+
+ def down
+ remove_concurrent_index :ci_stages, [:pipeline_id, :position]
+ end
+end
diff --git a/db/schema.rb b/db/schema.rb
index dc34736c76b..b7d7cd89c14 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -520,6 +520,7 @@ ActiveRecord::Schema.define(version: 20180531220618) do
end
add_index "ci_stages", ["pipeline_id", "name"], name: "index_ci_stages_on_pipeline_id_and_name", unique: true, using: :btree
+ add_index "ci_stages", ["pipeline_id", "position"], name: "index_ci_stages_on_pipeline_id_and_position", using: :btree
add_index "ci_stages", ["pipeline_id"], name: "index_ci_stages_on_pipeline_id", using: :btree
add_index "ci_stages", ["project_id"], name: "index_ci_stages_on_project_id", using: :btree
diff --git a/lib/gitlab/ci/pipeline/preloader.rb b/lib/gitlab/ci/pipeline/preloader.rb
index e7a2e5511cf..db0a1ea4dab 100644
--- a/lib/gitlab/ci/pipeline/preloader.rb
+++ b/lib/gitlab/ci/pipeline/preloader.rb
@@ -5,23 +5,47 @@ module Gitlab
module Pipeline
# Class for preloading data associated with pipelines such as commit
# authors.
- module Preloader
- def self.preload(pipelines)
- # This ensures that all the pipeline commits are eager loaded before we
- # start using them.
+ class Preloader
+ def self.preload!(pipelines)
+ ##
+ # This preloads all commits at once, because `Ci::Pipeline#commit` is
+ # using a lazy batch loading, what results in only one batched Gitaly
+ # call.
+ #
pipelines.each(&:commit)
pipelines.each do |pipeline|
- # This preloads the author of every commit. We're using "lazy_author"
- # here since "author" immediately loads the data on the first call.
- pipeline.commit.try(:lazy_author)
-
- # This preloads the number of warnings for every pipeline, ensuring
- # that Ci::Pipeline#has_warnings? doesn't execute any additional
- # queries.
- pipeline.number_of_warnings
+ self.new(pipeline).tap do |preloader|
+ preloader.preload_commit_authors
+ preloader.preload_pipeline_warnings
+ preloader.preload_stages_warnings
+ end
end
end
+
+ def initialize(pipeline)
+ @pipeline = pipeline
+ end
+
+ def preload_commit_authors
+ # This also preloads the author of every commit. We're using "lazy_author"
+ # here since "author" immediately loads the data on the first call.
+ @pipeline.commit.try(:lazy_author)
+ end
+
+ def preload_pipeline_warnings
+ # This preloads the number of warnings for every pipeline, ensuring
+ # that Ci::Pipeline#has_warnings? doesn't execute any additional
+ # queries.
+ @pipeline.number_of_warnings
+ end
+
+ def preload_stages_warnings
+ # This preloads the number of warnings for every stage, ensuring
+ # that Ci::Stage#has_warnings? doesn't execute any additional
+ # queries.
+ @pipeline.stages.each { |stage| stage.number_of_warnings }
+ end
end
end
end
diff --git a/lib/gitlab/ci/status/stage/common.rb b/lib/gitlab/ci/status/stage/common.rb
index bc99d925347..f60a7662075 100644
--- a/lib/gitlab/ci/status/stage/common.rb
+++ b/lib/gitlab/ci/status/stage/common.rb
@@ -8,7 +8,9 @@ module Gitlab
end
def details_path
- project_pipeline_path(subject.project, subject.pipeline, anchor: subject.name)
+ project_pipeline_path(subject.pipeline.project,
+ subject.pipeline,
+ anchor: subject.name)
end
def has_action?
diff --git a/spec/controllers/projects/pipelines_controller_spec.rb b/spec/controllers/projects/pipelines_controller_spec.rb
index 9e7bc20a6d1..92886e93077 100644
--- a/spec/controllers/projects/pipelines_controller_spec.rb
+++ b/spec/controllers/projects/pipelines_controller_spec.rb
@@ -17,44 +17,103 @@ describe Projects::PipelinesController do
describe 'GET index.json' do
before do
- %w(pending running created success).each_with_index do |status, index|
- sha = project.commit("HEAD~#{index}")
- create(:ci_empty_pipeline, status: status, project: project, sha: sha)
+ %w(pending running success failed canceled).each_with_index do |status, index|
+ create_pipeline(status, project.commit("HEAD~#{index}"))
end
end
- subject do
- get :index, namespace_id: project.namespace, project_id: project, format: :json
+ context 'when using persisted stages', :request_store do
+ before do
+ stub_feature_flags(ci_pipeline_persisted_stages: true)
+ end
+
+ it 'returns serialized pipelines', :request_store do
+ queries = ActiveRecord::QueryRecorder.new do
+ get_pipelines_index_json
+ end
+
+ expect(response).to have_gitlab_http_status(:ok)
+ expect(response).to match_response_schema('pipeline')
+
+ expect(json_response).to include('pipelines')
+ expect(json_response['pipelines'].count).to eq 5
+ expect(json_response['count']['all']).to eq '5'
+ expect(json_response['count']['running']).to eq '1'
+ expect(json_response['count']['pending']).to eq '1'
+ expect(json_response['count']['finished']).to eq '3'
+
+ json_response.dig('pipelines', 0, 'details', 'stages').tap do |stages|
+ expect(stages.count).to eq 3
+ end
+
+ expect(queries.count).to be
+ end
end
- it 'returns JSON with serialized pipelines' do
- subject
+ context 'when using legacy stages', :request_store do
+ before do
+ stub_feature_flags(ci_pipeline_persisted_stages: false)
+ end
- expect(response).to have_gitlab_http_status(:ok)
- expect(response).to match_response_schema('pipeline')
+ it 'returns JSON with serialized pipelines', :request_store do
+ queries = ActiveRecord::QueryRecorder.new do
+ get_pipelines_index_json
+ end
+
+ expect(response).to have_gitlab_http_status(:ok)
+ expect(response).to match_response_schema('pipeline')
- expect(json_response).to include('pipelines')
- expect(json_response['pipelines'].count).to eq 4
- expect(json_response['count']['all']).to eq '4'
- expect(json_response['count']['running']).to eq '1'
- expect(json_response['count']['pending']).to eq '1'
- expect(json_response['count']['finished']).to eq '1'
+ expect(json_response).to include('pipelines')
+ expect(json_response['pipelines'].count).to eq 5
+ expect(json_response['count']['all']).to eq '5'
+ expect(json_response['count']['running']).to eq '1'
+ expect(json_response['count']['pending']).to eq '1'
+ expect(json_response['count']['finished']).to eq '3'
+
+ json_response.dig('pipelines', 0, 'details', 'stages').tap do |stages|
+ expect(stages.count).to eq 3
+ end
+
+ expect(queries.count).to be_within(3).of(30)
+ end
end
it 'does not include coverage data for the pipelines' do
- subject
+ get_pipelines_index_json
expect(json_response['pipelines'][0]).not_to include('coverage')
end
context 'when performing gitaly calls', :request_store do
it 'limits the Gitaly requests' do
- expect { subject }.to change { Gitlab::GitalyClient.get_request_count }.by(3)
+ expect { get_pipelines_index_json }
+ .to change { Gitlab::GitalyClient.get_request_count }.by(2)
end
end
+
+ def get_pipelines_index_json
+ get :index, namespace_id: project.namespace,
+ project_id: project,
+ format: :json
+ end
+
+ def create_pipeline(status, sha)
+ pipeline = create(:ci_empty_pipeline, status: status,
+ project: project,
+ sha: sha)
+
+ create_build(pipeline, 'build', 1, 'build')
+ create_build(pipeline, 'test', 2, 'test')
+ create_build(pipeline, 'deploy', 3, 'deploy')
+ end
+
+ def create_build(pipeline, stage, stage_idx, name)
+ status = %w[created running pending success failed canceled].sample
+ create(:ci_build, pipeline: pipeline, stage: stage, stage_idx: stage_idx, name: name, status: status)
+ end
end
- describe 'GET show JSON' do
+ describe 'GET show.json' do
let(:pipeline) { create(:ci_pipeline_with_one_job, project: project) }
it 'returns the pipeline' do
@@ -67,6 +126,14 @@ describe Projects::PipelinesController do
end
context 'when the pipeline has multiple stages and groups', :request_store do
+ let(:project) { create(:project, :repository) }
+
+ let(:pipeline) do
+ create(:ci_empty_pipeline, project: project,
+ user: user,
+ sha: project.commit.id)
+ end
+
before do
create_build('build', 0, 'build')
create_build('test', 1, 'rspec 0')
@@ -74,11 +141,6 @@ describe Projects::PipelinesController do
create_build('post deploy', 3, 'pages 0')
end
- let(:project) { create(:project, :repository) }
- let(:pipeline) do
- create(:ci_empty_pipeline, project: project, user: user, sha: project.commit.id)
- end
-
it 'does not perform N + 1 queries' do
control_count = ActiveRecord::QueryRecorder.new { get_pipeline_json }.count
@@ -90,6 +152,7 @@ describe Projects::PipelinesController do
create_build('post deploy', 3, 'pages 2')
new_count = ActiveRecord::QueryRecorder.new { get_pipeline_json }.count
+
expect(new_count).to be_within(12).of(control_count)
end
end
diff --git a/spec/lib/gitlab/ci/pipeline/preloader_spec.rb b/spec/lib/gitlab/ci/pipeline/preloader_spec.rb
index 477c7477df0..40dfd893465 100644
--- a/spec/lib/gitlab/ci/pipeline/preloader_spec.rb
+++ b/spec/lib/gitlab/ci/pipeline/preloader_spec.rb
@@ -3,18 +3,47 @@
require 'spec_helper'
describe Gitlab::Ci::Pipeline::Preloader do
- describe '.preload' do
- it 'preloads the author of every pipeline commit' do
- commit = double(:commit)
- pipeline = double(:pipeline, commit: commit)
+ let(:stage) { double(:stage) }
+ let(:commit) { double(:commit) }
- expect(commit)
- .to receive(:lazy_author)
+ let(:pipeline) do
+ double(:pipeline, commit: commit, stages: [stage])
+ end
+
+ describe '.preload!' do
+ context 'when preloading multiple commits' do
+ let(:project) { create(:project, :repository) }
+
+ it 'preloads all commits once' do
+ expect(Commit).to receive(:decorate).once.and_call_original
+
+ pipelines = [build_pipeline(ref: 'HEAD'),
+ build_pipeline(ref: 'HEAD~1')]
+
+ described_class.preload!(pipelines)
+ end
+
+ def build_pipeline(ref:)
+ build_stubbed(:ci_pipeline, project: project, sha: project.commit(ref).id)
+ end
+ end
+
+ it 'preloads commit authors and number of warnings' do
+ expect(commit).to receive(:lazy_author)
+ expect(pipeline).to receive(:number_of_warnings)
+ expect(stage).to receive(:number_of_warnings)
+
+ described_class.preload!([pipeline])
+ end
+
+ it 'returns original collection' do
+ allow(commit).to receive(:lazy_author)
+ allow(pipeline).to receive(:number_of_warnings)
+ allow(stage).to receive(:number_of_warnings)
- expect(pipeline)
- .to receive(:number_of_warnings)
+ pipelines = [pipeline, pipeline]
- described_class.preload([pipeline])
+ expect(described_class.preload!(pipelines)).to eq pipelines
end
end
end
diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml
index a129855dbd8..2ea66479c1b 100644
--- a/spec/lib/gitlab/import_export/all_models.yml
+++ b/spec/lib/gitlab/import_export/all_models.yml
@@ -257,6 +257,7 @@ project:
- import_data
- commit_statuses
- pipelines
+- stages
- builds
- runner_projects
- runners
diff --git a/spec/models/ci/group_spec.rb b/spec/models/ci/group_spec.rb
index 51123e73fe6..838fa63cb1f 100644
--- a/spec/models/ci/group_spec.rb
+++ b/spec/models/ci/group_spec.rb
@@ -41,4 +41,55 @@ describe Ci::Group do
end
end
end
+
+ describe '.fabricate' do
+ let(:pipeline) { create(:ci_empty_pipeline) }
+ let(:stage) { create(:ci_stage_entity, pipeline: pipeline) }
+
+ before do
+ create_build(:ci_build, name: 'rspec 0 2')
+ create_build(:ci_build, name: 'rspec 0 1')
+ create_build(:ci_build, name: 'spinach 0 1')
+ create_build(:commit_status, name: 'aaaaa')
+ end
+
+ it 'returns an array of three groups' do
+ expect(stage.groups).to be_a Array
+ expect(stage.groups).to all(be_a described_class)
+ expect(stage.groups.size).to eq 3
+ end
+
+ it 'returns groups with correctly ordered statuses' do
+ expect(stage.groups.first.jobs.map(&:name))
+ .to eq ['aaaaa']
+ expect(stage.groups.second.jobs.map(&:name))
+ .to eq ['rspec 0 1', 'rspec 0 2']
+ expect(stage.groups.third.jobs.map(&:name))
+ .to eq ['spinach 0 1']
+ end
+
+ it 'returns groups with correct names' do
+ expect(stage.groups.map(&:name))
+ .to eq %w[aaaaa rspec spinach]
+ end
+
+ context 'when a name is nil on legacy pipelines' do
+ before do
+ pipeline.builds.first.update_attribute(:name, nil)
+ end
+
+ it 'returns an array of three groups' do
+ expect(stage.groups.map(&:name))
+ .to eq ['', 'aaaaa', 'rspec', 'spinach']
+ end
+ end
+
+ def create_build(type, status: 'success', **opts)
+ create(type, pipeline: pipeline,
+ stage: stage.name,
+ status: status,
+ stage_id: stage.id,
+ **opts)
+ end
+ end
end
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index 24692ebb9a3..2bae98dcbb8 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -537,6 +537,87 @@ describe Ci::Pipeline, :mailer do
end
end
end
+
+ describe '#stages' do
+ before do
+ create(:ci_stage_entity, project: project,
+ pipeline: pipeline,
+ name: 'build')
+ end
+
+ it 'returns persisted stages' do
+ expect(pipeline.stages).not_to be_empty
+ expect(pipeline.stages).to all(be_persisted)
+ end
+ end
+
+ describe '#ordered_stages' do
+ before do
+ create(:ci_stage_entity, project: project,
+ pipeline: pipeline,
+ position: 4,
+ name: 'deploy')
+
+ create(:ci_build, project: project,
+ pipeline: pipeline,
+ stage: 'test',
+ stage_idx: 3,
+ name: 'test')
+
+ create(:ci_build, project: project,
+ pipeline: pipeline,
+ stage: 'build',
+ stage_idx: 2,
+ name: 'build')
+
+ create(:ci_stage_entity, project: project,
+ pipeline: pipeline,
+ position: 1,
+ name: 'sanity')
+
+ create(:ci_stage_entity, project: project,
+ pipeline: pipeline,
+ position: 5,
+ name: 'cleanup')
+ end
+
+ subject { pipeline.ordered_stages }
+
+ context 'when using legacy stages' do
+ before do
+ stub_feature_flags(ci_pipeline_persisted_stages: false)
+ end
+
+ it 'returns legacy stages in valid order' do
+ expect(subject.map(&:name)).to eq %w[build test]
+ end
+ end
+
+ context 'when using persisted stages' do
+ before do
+ stub_feature_flags(ci_pipeline_persisted_stages: true)
+ end
+
+ context 'when pipelines is not complete' do
+ it 'still returns legacy stages' do
+ expect(subject).to all(be_a Ci::LegacyStage)
+ expect(subject.map(&:name)).to eq %w[build test]
+ end
+ end
+
+ context 'when pipeline is complete' do
+ before do
+ pipeline.succeed!
+ end
+
+ it 'returns stages in valid order' do
+ expect(subject).to all(be_a Ci::Stage)
+ expect(subject.map(&:name))
+ .to eq %w[sanity build test deploy cleanup]
+ end
+ end
+ end
+ end
end
describe 'state machine' do
@@ -1181,6 +1262,43 @@ describe Ci::Pipeline, :mailer do
end
end
+ describe '#update_status' do
+ context 'when pipeline is empty' do
+ it 'updates does not change pipeline status' do
+ expect(pipeline.statuses.latest.status).to be_nil
+
+ expect { pipeline.update_status }
+ .to change { pipeline.reload.status }.to 'skipped'
+ end
+ end
+
+ context 'when updating status to pending' do
+ before do
+ allow(pipeline)
+ .to receive_message_chain(:statuses, :latest, :status)
+ .and_return(:running)
+ end
+
+ it 'updates pipeline status to running' do
+ expect { pipeline.update_status }
+ .to change { pipeline.reload.status }.to 'running'
+ end
+ end
+
+ context 'when statuses status was not recognized' do
+ before do
+ allow(pipeline)
+ .to receive(:latest_builds_status)
+ .and_return(:unknown)
+ end
+
+ it 'raises an exception' do
+ expect { pipeline.update_status }
+ .to raise_error(HasStatus::UnknownStatusError)
+ end
+ end
+ end
+
describe '#detailed_status' do
subject { pipeline.detailed_status(user) }
diff --git a/spec/models/ci/stage_spec.rb b/spec/models/ci/stage_spec.rb
index a00db1d2bfc..22a4556c10c 100644
--- a/spec/models/ci/stage_spec.rb
+++ b/spec/models/ci/stage_spec.rb
@@ -65,7 +65,31 @@ describe Ci::Stage, :models do
end
end
- context 'when stage is skipped' do
+ context 'when stage has only created builds' do
+ let(:stage) { create(:ci_stage_entity, status: :created) }
+
+ before do
+ create(:ci_build, :created, stage_id: stage.id)
+ end
+
+ it 'updates status to skipped' do
+ expect(stage.reload.status).to eq 'created'
+ end
+ end
+
+ context 'when stage is skipped because of skipped builds' do
+ before do
+ create(:ci_build, :skipped, stage_id: stage.id)
+ end
+
+ it 'updates status to skipped' do
+ expect { stage.update_status }
+ .to change { stage.reload.status }
+ .to 'skipped'
+ end
+ end
+
+ context 'when stage is skipped because is empty' do
it 'updates status to skipped' do
expect { stage.update_status }
.to change { stage.reload.status }
@@ -86,9 +110,85 @@ describe Ci::Stage, :models do
expect(stage.reload).to be_failed
end
end
+
+ context 'when statuses status was not recognized' do
+ before do
+ allow(stage)
+ .to receive_message_chain(:statuses, :latest, :status)
+ .and_return(:unknown)
+ end
+
+ it 'raises an exception' do
+ expect { stage.update_status }
+ .to raise_error(HasStatus::UnknownStatusError)
+ end
+ end
+ end
+
+ describe '#detailed_status' do
+ using RSpec::Parameterized::TableSyntax
+
+ let(:user) { create(:user) }
+ let(:stage) { create(:ci_stage_entity, status: :created) }
+ subject { stage.detailed_status(user) }
+
+ where(:statuses, :label) do
+ %w[created] | :created
+ %w[success] | :passed
+ %w[pending] | :pending
+ %w[skipped] | :skipped
+ %w[canceled] | :canceled
+ %w[success failed] | :failed
+ %w[running pending] | :running
+ end
+
+ with_them do
+ before do
+ statuses.each do |status|
+ create(:commit_status, project: stage.project,
+ pipeline: stage.pipeline,
+ stage_id: stage.id,
+ status: status)
+
+ stage.update_status
+ end
+ end
+
+ it 'has a correct label' do
+ expect(subject.label).to eq label.to_s
+ end
+ end
+
+ context 'when stage has warnings' do
+ before do
+ create(:ci_build, project: stage.project,
+ pipeline: stage.pipeline,
+ stage_id: stage.id,
+ status: :failed,
+ allow_failure: true)
+
+ stage.update_status
+ end
+
+ it 'is passed with warnings' do
+ expect(subject.label).to eq 'passed with warnings'
+ end
+ end
end
- describe '#index' do
+ describe '#groups' do
+ before do
+ create(:ci_build, stage_id: stage.id, name: 'rspec 0 1')
+ create(:ci_build, stage_id: stage.id, name: 'rspec 0 2')
+ end
+
+ it 'groups stage builds by name' do
+ expect(stage.groups).to be_one
+ expect(stage.groups.first.name).to eq 'rspec'
+ end
+ end
+
+ describe '#position' do
context 'when stage has been imported and does not have position index set' do
before do
stage.update_column(:position, nil)
@@ -119,4 +219,42 @@ describe Ci::Stage, :models do
end
end
end
+
+ context 'when stage has warnings' do
+ before do
+ create(:ci_build, :failed, :allowed_to_fail, stage_id: stage.id)
+ end
+
+ describe '#has_warnings?' do
+ it 'returns true' do
+ expect(stage).to have_warnings
+ end
+ end
+
+ describe '#number_of_warnings' do
+ it 'returns a lazy stage warnings counter' do
+ lazy_queries = ActiveRecord::QueryRecorder.new do
+ stage.number_of_warnings
+ end
+
+ synced_queries = ActiveRecord::QueryRecorder.new do
+ stage.number_of_warnings.to_i
+ end
+
+ expect(lazy_queries.count).to eq 0
+ expect(synced_queries.count).to eq 1
+
+ expect(stage.number_of_warnings.inspect).to include 'BatchLoader'
+ expect(stage.number_of_warnings).to eq 1
+ end
+ end
+ end
+
+ context 'when stage does not have warnings' do
+ describe '#has_warnings?' do
+ it 'returns false' do
+ expect(stage).not_to have_warnings
+ end
+ end
+ end
end
diff --git a/spec/serializers/pipeline_serializer_spec.rb b/spec/serializers/pipeline_serializer_spec.rb
index b741308e2c5..e0e6eecb300 100644
--- a/spec/serializers/pipeline_serializer_spec.rb
+++ b/spec/serializers/pipeline_serializer_spec.rb
@@ -8,6 +8,10 @@ describe PipelineSerializer do
described_class.new(current_user: user)
end
+ before do
+ stub_feature_flags(ci_pipeline_persisted_stages: true)
+ end
+
subject { serializer.represent(resource) }
describe '#represent' do
@@ -99,7 +103,8 @@ describe PipelineSerializer do
end
end
- context 'number of queries' do
+ describe 'number of queries when preloaded' do
+ subject { serializer.represent(resource, preload: true) }
let(:resource) { Ci::Pipeline.all }
before do
@@ -107,7 +112,7 @@ describe PipelineSerializer do
# gitaly calls in this block
# Issue: https://gitlab.com/gitlab-org/gitlab-ce/issues/37772
Gitlab::GitalyClient.allow_n_plus_1_calls do
- Ci::Pipeline::AVAILABLE_STATUSES.each do |status|
+ Ci::Pipeline::COMPLETED_STATUSES.each do |status|
create_pipeline(status)
end
end
@@ -120,7 +125,7 @@ describe PipelineSerializer do
it 'verifies number of queries', :request_store do
recorded = ActiveRecord::QueryRecorder.new { subject }
- expect(recorded.count).to be_within(1).of(44)
+ expect(recorded.count).to be_within(2).of(27)
expect(recorded.cached_count).to eq(0)
end
end
@@ -139,7 +144,7 @@ describe PipelineSerializer do
# pipeline. With the same ref this check is cached but if refs are
# different then there is an extra query per ref
# https://gitlab.com/gitlab-org/gitlab-ce/issues/46368
- expect(recorded.count).to be_within(1).of(51)
+ expect(recorded.count).to be_within(2).of(30)
expect(recorded.cached_count).to eq(0)
end
end