diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2021-04-20 23:50:22 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2021-04-20 23:50:22 +0000 |
commit | 9dc93a4519d9d5d7be48ff274127136236a3adb3 (patch) | |
tree | 70467ae3692a0e35e5ea56bcb803eb512a10bedb /spec/services | |
parent | 4b0f34b6d759d6299322b3a54453e930c6121ff0 (diff) | |
download | gitlab-ce-9dc93a4519d9d5d7be48ff274127136236a3adb3.tar.gz |
Add latest changes from gitlab-org/gitlab@13-11-stable-eev13.11.0-rc43
Diffstat (limited to 'spec/services')
142 files changed, 3926 insertions, 1383 deletions
diff --git a/spec/services/authorized_project_update/find_records_due_for_refresh_service_spec.rb b/spec/services/authorized_project_update/find_records_due_for_refresh_service_spec.rb new file mode 100644 index 00000000000..8a53d9fbf7c --- /dev/null +++ b/spec/services/authorized_project_update/find_records_due_for_refresh_service_spec.rb @@ -0,0 +1,295 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe AuthorizedProjectUpdate::FindRecordsDueForRefreshService do + # We're using let! here so that any expectations for the service class are not + # triggered twice. + let!(:project) { create(:project) } + + let(:user) { project.namespace.owner } + let(:service) { described_class.new(user) } + + describe '#execute' do + context 'callbacks' do + let(:callback) { double('callback') } + + context 'incorrect_auth_found_callback callback' do + let(:user) { create(:user) } + let(:service) do + described_class.new(user, + incorrect_auth_found_callback: callback) + end + + it 'is called' do + access_level = Gitlab::Access::DEVELOPER + create(:project_authorization, user: user, project: project, access_level: access_level) + + expect(callback).to receive(:call).with(project.id, access_level).once + + service.execute + end + end + + context 'missing_auth_found_callback callback' do + let(:service) do + described_class.new(user, + missing_auth_found_callback: callback) + end + + it 'is called' do + ProjectAuthorization.delete_all + + expect(callback).to receive(:call).with(project.id, Gitlab::Access::MAINTAINER).once + + service.execute + end + end + end + + context 'finding project authorizations due for refresh' do + context 'when there are changes to be made' do + before do + user.project_authorizations.delete_all + end + + it 'finds projects authorizations that needs to be refreshed' do + project2 = create(:project) + user.project_authorizations + .create!(project: project2, access_level: Gitlab::Access::MAINTAINER) + + to_be_removed = [project2.id] + to_be_added = [[user.id, project.id, Gitlab::Access::MAINTAINER]] + + expect(service.execute).to eq([to_be_removed, to_be_added]) + end + + it 'finds duplicate entries that has to be removed' do + [Gitlab::Access::MAINTAINER, Gitlab::Access::REPORTER].each do |access_level| + user.project_authorizations.create!(project: project, access_level: access_level) + end + + to_be_removed = [project.id] + to_be_added = [[user.id, project.id, Gitlab::Access::MAINTAINER]] + + expect(service.execute).to eq([to_be_removed, to_be_added]) + end + + it 'finds entries with wrong access levels' do + user.project_authorizations + .create!(project: project, access_level: Gitlab::Access::DEVELOPER) + + to_be_removed = [project.id] + to_be_added = [[user.id, project.id, Gitlab::Access::MAINTAINER]] + + expect(service.execute).to eq([to_be_removed, to_be_added]) + end + end + + context 'when there are no changes to be made' do + it 'returns empty arrays' do + expect(service.execute).to eq([[], []]) + end + end + end + end + + describe '#needs_refresh?' do + subject { service.needs_refresh? } + + context 'when there are records due for either removal or addition' do + context 'when there are both removals and additions to be made' do + before do + user.project_authorizations.delete_all + create(:project_authorization, user: user) + end + + it { is_expected.to eq(true) } + end + + context 'when there are no removals, but there are additions to be made' do + before do + user.project_authorizations.delete_all + end + + it { is_expected.to eq(true) } + end + + context 'when there are no additions, but there are removals to be made' do + before do + create(:project_authorization, user: user) + end + + it { is_expected.to eq(true) } + end + end + + context 'when there are no additions or removals to be made' do + it { is_expected.to eq(false) } + end + end + + describe '#fresh_access_levels_per_project' do + let(:hash) { service.fresh_access_levels_per_project } + + it 'returns a Hash' do + expect(hash).to be_an_instance_of(Hash) + end + + it 'sets the keys to the project IDs' do + expect(hash.keys).to eq([project.id]) + end + + it 'sets the values to the access levels' do + expect(hash.values).to eq([Gitlab::Access::MAINTAINER]) + end + + context 'personal projects' do + it 'includes the project with the right access level' do + expect(hash[project.id]).to eq(Gitlab::Access::MAINTAINER) + end + end + + context 'projects the user is a member of' do + let!(:other_project) { create(:project) } + + before do + other_project.team.add_reporter(user) + end + + it 'includes the project with the right access level' do + expect(hash[other_project.id]).to eq(Gitlab::Access::REPORTER) + end + end + + context 'projects of groups the user is a member of' do + let(:group) { create(:group) } + let!(:other_project) { create(:project, group: group) } + + before do + group.add_owner(user) + end + + it 'includes the project with the right access level' do + expect(hash[other_project.id]).to eq(Gitlab::Access::OWNER) + end + end + + context 'projects of subgroups of groups the user is a member of' do + let(:group) { create(:group) } + let(:nested_group) { create(:group, parent: group) } + let!(:other_project) { create(:project, group: nested_group) } + + before do + group.add_maintainer(user) + end + + it 'includes the project with the right access level' do + expect(hash[other_project.id]).to eq(Gitlab::Access::MAINTAINER) + end + end + + context 'projects shared with groups the user is a member of' do + let(:group) { create(:group) } + let(:other_project) { create(:project) } + let!(:project_group_link) { create(:project_group_link, project: other_project, group: group, group_access: Gitlab::Access::GUEST) } + + before do + group.add_maintainer(user) + end + + it 'includes the project with the right access level' do + expect(hash[other_project.id]).to eq(Gitlab::Access::GUEST) + end + end + + context 'projects shared with subgroups of groups the user is a member of' do + let(:group) { create(:group) } + let(:nested_group) { create(:group, parent: group) } + let(:other_project) { create(:project) } + let!(:project_group_link) { create(:project_group_link, project: other_project, group: nested_group, group_access: Gitlab::Access::DEVELOPER) } + + before do + group.add_maintainer(user) + end + + it 'includes the project with the right access level' do + expect(hash[other_project.id]).to eq(Gitlab::Access::DEVELOPER) + end + end + end + + describe '#current_authorizations_per_project' do + let(:hash) { service.current_authorizations_per_project } + + it 'returns a Hash' do + expect(hash).to be_an_instance_of(Hash) + end + + it 'sets the keys to the project IDs' do + expect(hash.keys).to eq([project.id]) + end + + it 'sets the values to the project authorization rows' do + expect(hash.values.length).to eq(1) + + value = hash.values[0] + + expect(value.project_id).to eq(project.id) + expect(value.access_level).to eq(Gitlab::Access::MAINTAINER) + end + end + + describe '#current_authorizations' do + context 'without authorizations' do + it 'returns an empty list' do + user.project_authorizations.delete_all + + expect(service.current_authorizations.empty?).to eq(true) + end + end + + context 'with an authorization' do + let(:row) { service.current_authorizations.take } + + it 'returns the currently authorized projects' do + expect(service.current_authorizations.length).to eq(1) + end + + it 'includes the project ID for every row' do + expect(row.project_id).to eq(project.id) + end + + it 'includes the access level for every row' do + expect(row.access_level).to eq(Gitlab::Access::MAINTAINER) + end + end + end + + describe '#fresh_authorizations' do + it 'returns the new authorized projects' do + expect(service.fresh_authorizations.length).to eq(1) + end + + it 'returns the highest access level' do + project.team.add_guest(user) + + rows = service.fresh_authorizations.to_a + + expect(rows.length).to eq(1) + expect(rows.first.access_level).to eq(Gitlab::Access::MAINTAINER) + end + + context 'every returned row' do + let(:row) { service.fresh_authorizations.take } + + it 'includes the project ID' do + expect(row.project_id).to eq(project.id) + end + + it 'includes the access level' do + expect(row.access_level).to eq(Gitlab::Access::MAINTAINER) + end + end + end +end diff --git a/spec/services/authorized_project_update/recalculate_for_user_range_service_spec.rb b/spec/services/authorized_project_update/recalculate_for_user_range_service_spec.rb index 0c944cad40c..95e2c0380bf 100644 --- a/spec/services/authorized_project_update/recalculate_for_user_range_service_spec.rb +++ b/spec/services/authorized_project_update/recalculate_for_user_range_service_spec.rb @@ -7,12 +7,14 @@ RSpec.describe AuthorizedProjectUpdate::RecalculateForUserRangeService do let_it_be(:users) { create_list(:user, 2) } it 'calls Users::RefreshAuthorizedProjectsService' do - users.each do |user| + user_ids = users.map(&:id) + + User.where(id: user_ids).select(:id).each do |user| expect(Users::RefreshAuthorizedProjectsService).to( receive(:new).with(user, source: described_class.name).and_call_original) end - range = users.map(&:id).minmax + range = user_ids.minmax described_class.new(*range).execute end end diff --git a/spec/services/award_emojis/add_service_spec.rb b/spec/services/award_emojis/add_service_spec.rb index 85c39015614..0fbb785e2d6 100644 --- a/spec/services/award_emojis/add_service_spec.rb +++ b/spec/services/award_emojis/add_service_spec.rb @@ -6,6 +6,7 @@ RSpec.describe AwardEmojis::AddService do let_it_be(:user) { create(:user) } let_it_be(:project) { create(:project) } let_it_be(:awardable) { create(:note, project: project) } + let(:name) { 'thumbsup' } subject(:service) { described_class.new(awardable, name, user) } diff --git a/spec/services/award_emojis/destroy_service_spec.rb b/spec/services/award_emojis/destroy_service_spec.rb index 2aba078b638..f743de7c59e 100644 --- a/spec/services/award_emojis/destroy_service_spec.rb +++ b/spec/services/award_emojis/destroy_service_spec.rb @@ -6,6 +6,7 @@ RSpec.describe AwardEmojis::DestroyService do let_it_be(:user) { create(:user) } let_it_be(:awardable) { create(:note) } let_it_be(:project) { awardable.project } + let(:name) { 'thumbsup' } let!(:award_from_other_user) do create(:award_emoji, name: name, awardable: awardable, user: create(:user)) diff --git a/spec/services/award_emojis/toggle_service_spec.rb b/spec/services/award_emojis/toggle_service_spec.rb index a7feeed50c6..74e97c66193 100644 --- a/spec/services/award_emojis/toggle_service_spec.rb +++ b/spec/services/award_emojis/toggle_service_spec.rb @@ -6,6 +6,7 @@ RSpec.describe AwardEmojis::ToggleService do let_it_be(:user) { create(:user) } let_it_be(:project) { create(:project, :public) } let_it_be(:awardable) { create(:note, project: project) } + let(:name) { 'thumbsup' } subject(:service) { described_class.new(awardable, name, user) } diff --git a/spec/services/boards/destroy_service_spec.rb b/spec/services/boards/destroy_service_spec.rb new file mode 100644 index 00000000000..cd6df832547 --- /dev/null +++ b/spec/services/boards/destroy_service_spec.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Boards::DestroyService do + context 'with project board' do + let_it_be(:parent) { create(:project) } + + let(:boards) { parent.boards } + let(:board_factory) { :board } + + it_behaves_like 'board destroy service' + end + + context 'with group board' do + let_it_be(:parent) { create(:group) } + + let(:boards) { parent.boards } + let(:board_factory) { :board } + + it_behaves_like 'board destroy service' + end +end diff --git a/spec/services/boards/issues/move_service_spec.rb b/spec/services/boards/issues/move_service_spec.rb index 01a3ec72987..3a25f13762c 100644 --- a/spec/services/boards/issues/move_service_spec.rb +++ b/spec/services/boards/issues/move_service_spec.rb @@ -62,6 +62,7 @@ RSpec.describe Boards::Issues::MoveService do let_it_be(:testing) { create(:group_label, group: group, name: 'Testing') } let_it_be(:list1) { create(:list, board: board1, label: development, position: 0) } let_it_be(:list2) { create(:list, board: board1, label: testing, position: 1) } + let(:params) { { board_id: board1.id, from_list_id: list1.id, to_list_id: list2.id } } before do diff --git a/spec/services/boards/lists/list_service_spec.rb b/spec/services/boards/lists/list_service_spec.rb index 21619abf6aa..0c8a8dc7329 100644 --- a/spec/services/boards/lists/list_service_spec.rb +++ b/spec/services/boards/lists/list_service_spec.rb @@ -8,46 +8,32 @@ RSpec.describe Boards::Lists::ListService do describe '#execute' do let(:service) { described_class.new(parent, user) } - shared_examples 'hidden lists' do - let!(:list) { create(:list, board: board, label: label) } - - context 'when hide_backlog_list is true' do - it 'hides backlog list' do - board.update!(hide_backlog_list: true) - - expect(service.execute(board)).to match_array([board.closed_list, list]) - end - end - - context 'when hide_closed_list is true' do - it 'hides closed list' do - board.update!(hide_closed_list: true) - - expect(service.execute(board)).to match_array([board.backlog_list, list]) - end - end - end - context 'when board parent is a project' do - let(:project) { create(:project) } - let(:board) { create(:board, project: project) } - let(:label) { create(:label, project: project) } - let!(:list) { create(:list, board: board, label: label) } + let_it_be(:project) { create(:project) } + let_it_be_with_reload(:board) { create(:board, project: project) } + let_it_be(:label) { create(:label, project: project) } + let_it_be(:list) { create(:list, board: board, label: label) } + let_it_be(:unrelated_list) { create(:list) } + let(:parent) { project } it_behaves_like 'lists list service' - it_behaves_like 'hidden lists' end context 'when board parent is a group' do - let(:group) { create(:group) } - let(:board) { create(:board, group: group) } - let(:label) { create(:group_label, group: group) } - let!(:list) { create(:list, board: board, label: label) } + let_it_be(:group) { create(:group) } + let_it_be_with_reload(:board) { create(:board, group: group) } + let_it_be(:label) { create(:group_label, group: group) } + let_it_be(:list) { create(:list, board: board, label: label) } + let_it_be(:unrelated_list) { create(:list) } + let(:parent) { group } it_behaves_like 'lists list service' - it_behaves_like 'hidden lists' + end + + def create_backlog_list(board) + create(:backlog_list, board: board) end end end diff --git a/spec/services/boards/lists/update_service_spec.rb b/spec/services/boards/lists/update_service_spec.rb index cdc7784469a..10fed9b7aac 100644 --- a/spec/services/boards/lists/update_service_spec.rb +++ b/spec/services/boards/lists/update_service_spec.rb @@ -6,47 +6,6 @@ RSpec.describe Boards::Lists::UpdateService do let(:user) { create(:user) } let!(:list) { create(:list, board: board, position: 0) } - shared_examples 'moving list' do - context 'when user can admin list' do - it 'calls Lists::MoveService to update list position' do - board.resource_parent.add_developer(user) - - expect(Boards::Lists::MoveService).to receive(:new).with(board.resource_parent, user, params).and_call_original - expect_any_instance_of(Boards::Lists::MoveService).to receive(:execute).with(list) - - service.execute(list) - end - end - - context 'when user cannot admin list' do - it 'does not call Lists::MoveService to update list position' do - expect(Boards::Lists::MoveService).not_to receive(:new) - - service.execute(list) - end - end - end - - shared_examples 'updating list preferences' do - context 'when user can read list' do - it 'updates list preference for user' do - board.resource_parent.add_guest(user) - - service.execute(list) - - expect(list.preferences_for(user).collapsed).to eq(true) - end - end - - context 'when user cannot read list' do - it 'does not update list preference for user' do - service.execute(list) - - expect(list.preferences_for(user).collapsed).to be_nil - end - end - end - describe '#execute' do let(:service) { described_class.new(board.resource_parent, user, params) } diff --git a/spec/services/bulk_create_integration_service_spec.rb b/spec/services/bulk_create_integration_service_spec.rb index 3ac993972c6..479309572a5 100644 --- a/spec/services/bulk_create_integration_service_spec.rb +++ b/spec/services/bulk_create_integration_service_spec.rb @@ -30,7 +30,7 @@ RSpec.describe BulkCreateIntegrationService do described_class.new(integration, batch, association).execute expect(created_integration.reload.data_fields.attributes.except(*excluded_attributes)) - .to eq(integration.data_fields.attributes.except(*excluded_attributes)) + .to eq(integration.reload.data_fields.attributes.except(*excluded_attributes)) end end end diff --git a/spec/services/bulk_update_integration_service_spec.rb b/spec/services/bulk_update_integration_service_spec.rb index e7944f07bb7..e20bcd44923 100644 --- a/spec/services/bulk_update_integration_service_spec.rb +++ b/spec/services/bulk_update_integration_service_spec.rb @@ -68,8 +68,8 @@ RSpec.describe BulkUpdateIntegrationService do it 'updates the data fields from the integration', :aggregate_failures do described_class.new(subgroup_integration, batch).execute - expect(integration.data_fields.attributes.except(*excluded_attributes)) - .to eq(subgroup_integration.data_fields.attributes.except(*excluded_attributes)) + expect(integration.reload.data_fields.attributes.except(*excluded_attributes)) + .to eq(subgroup_integration.reload.data_fields.attributes.except(*excluded_attributes)) expect(integration.data_fields.attributes.except(*excluded_attributes)) .not_to eq(excluded_integration.data_fields.attributes.except(*excluded_attributes)) diff --git a/spec/services/ci/abort_pipelines_service_spec.rb b/spec/services/ci/abort_pipelines_service_spec.rb new file mode 100644 index 00000000000..e31a45cb123 --- /dev/null +++ b/spec/services/ci/abort_pipelines_service_spec.rb @@ -0,0 +1,94 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::AbortPipelinesService do + let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project, namespace: user.namespace) } + + let_it_be(:cancelable_pipeline, reload: true) { create(:ci_pipeline, :running, project: project, user: user) } + let_it_be(:manual_pipeline, reload: true) { create(:ci_pipeline, status: :manual, project: project, user: user) } # not cancelable + let_it_be(:other_users_pipeline, reload: true) { create(:ci_pipeline, :running, project: project, user: create(:user)) } # not this user's pipeline + let_it_be(:cancelable_build, reload: true) { create(:ci_build, :running, pipeline: cancelable_pipeline) } + let_it_be(:non_cancelable_build, reload: true) { create(:ci_build, :success, pipeline: cancelable_pipeline) } + let_it_be(:cancelable_stage, reload: true) { create(:ci_stage_entity, name: 'stageA', status: :running, pipeline: cancelable_pipeline, project: project) } + let_it_be(:non_cancelable_stage, reload: true) { create(:ci_stage_entity, name: 'stageB', status: :success, pipeline: cancelable_pipeline, project: project) } + + describe '#execute' do + def expect_correct_cancellations + expect(cancelable_pipeline.finished_at).not_to be_nil + expect(cancelable_pipeline.status).to eq('failed') + expect((cancelable_pipeline.stages - [non_cancelable_stage]).map(&:status)).to all(eq('failed')) + expect(cancelable_build.status).to eq('failed') + expect(cancelable_build.finished_at).not_to be_nil + + expect(manual_pipeline.status).not_to eq('failed') + expect(non_cancelable_stage.status).not_to eq('failed') + expect(non_cancelable_build.status).not_to eq('failed') + end + + context 'with project pipelines' do + def abort_project_pipelines + described_class.new.execute(project.all_pipelines, :project_deleted) + end + + it 'fails all running pipelines and related jobs' do + expect(abort_project_pipelines).to be_success + + expect_correct_cancellations + + expect(other_users_pipeline.status).to eq('failed') + expect(other_users_pipeline.failure_reason).to eq('project_deleted') + expect(other_users_pipeline.stages.map(&:status)).to all(eq('failed')) + end + + it 'avoids N+1 queries' do + control_count = ActiveRecord::QueryRecorder.new { abort_project_pipelines }.count + + pipelines = create_list(:ci_pipeline, 5, :running, project: project) + create_list(:ci_build, 5, :running, pipeline: pipelines.first) + + expect { abort_project_pipelines }.not_to exceed_query_limit(control_count) + end + + context 'with live build logs' do + before do + create(:ci_build_trace_chunk, build: cancelable_build) + end + + it 'makes failed builds with stale trace visible' do + expect(Ci::Build.with_stale_live_trace.count).to eq 0 + + travel_to(2.days.ago) do + abort_project_pipelines + end + + expect(Ci::Build.with_stale_live_trace.count).to eq 1 + end + end + end + + context 'with user pipelines' do + def abort_user_pipelines + described_class.new.execute(user.pipelines, :user_blocked) + end + + it 'fails all running pipelines and related jobs' do + expect(abort_user_pipelines).to be_success + + expect_correct_cancellations + + expect(other_users_pipeline.status).not_to eq('failed') + end + + it 'avoids N+1 queries' do + control_count = ActiveRecord::QueryRecorder.new { abort_user_pipelines }.count + + pipelines = create_list(:ci_pipeline, 5, :running, project: project, user: user) + create_list(:ci_build, 5, :running, pipeline: pipelines.first) + + expect { abort_user_pipelines }.not_to exceed_query_limit(control_count) + end + end + end +end diff --git a/spec/services/ci/abort_project_pipelines_service_spec.rb b/spec/services/ci/abort_project_pipelines_service_spec.rb deleted file mode 100644 index 9af909ac2ab..00000000000 --- a/spec/services/ci/abort_project_pipelines_service_spec.rb +++ /dev/null @@ -1,42 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Ci::AbortProjectPipelinesService do - let_it_be(:project) { create(:project) } - let_it_be(:pipeline) { create(:ci_pipeline, :running, project: project) } - let_it_be(:build) { create(:ci_build, :running, pipeline: pipeline) } - - describe '#execute' do - it 'cancels all running pipelines and related jobs' do - result = described_class.new.execute(project) - - expect(result).to be_success - expect(pipeline.reload).to be_canceled - expect(build.reload).to be_canceled - end - - it 'avoids N+1 queries' do - control_count = ActiveRecord::QueryRecorder.new { described_class.new.execute(project) }.count - - pipelines = create_list(:ci_pipeline, 5, :running, project: project) - create_list(:ci_build, 5, :running, pipeline: pipelines.first) - - expect { described_class.new.execute(project) }.not_to exceed_query_limit(control_count) - end - end - - context 'when feature disabled' do - before do - stub_feature_flags(abort_deleted_project_pipelines: false) - end - - it 'does not abort the pipeline' do - result = described_class.new.execute(project) - - expect(result).to be(nil) - expect(pipeline.reload).to be_running - expect(build.reload).to be_running - end - end -end diff --git a/spec/services/ci/after_requeue_job_service_spec.rb b/spec/services/ci/after_requeue_job_service_spec.rb new file mode 100644 index 00000000000..a2147759dba --- /dev/null +++ b/spec/services/ci/after_requeue_job_service_spec.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::AfterRequeueJobService do + let_it_be(:project) { create(:project) } + let_it_be(:user) { project.owner } + + let(:pipeline) { create(:ci_pipeline, project: project) } + + let!(:build) { create(:ci_build, pipeline: pipeline, stage_idx: 0) } + let!(:test1) { create(:ci_build, :success, pipeline: pipeline, stage_idx: 1) } + let!(:test2) { create(:ci_build, :skipped, pipeline: pipeline, stage_idx: 1) } + + subject(:execute_service) { described_class.new(project, user).execute(build) } + + it 'marks subsequent skipped jobs as processable' do + expect(test1.reload).to be_success + expect(test2.reload).to be_skipped + + execute_service + + expect(test1.reload).to be_success + expect(test2.reload).to be_created + end + + context 'when the pipeline is a downstream pipeline and the bridge is depended' do + let!(:trigger_job) { create(:ci_bridge, :strategy_depend, status: 'success') } + + before do + create(:ci_sources_pipeline, pipeline: pipeline, source_job: trigger_job) + end + + it 'marks source bridge as pending' do + expect { execute_service }.to change { trigger_job.reload.status }.from('success').to('pending') + end + end +end diff --git a/spec/services/ci/archive_trace_service_spec.rb b/spec/services/ci/archive_trace_service_spec.rb index 07ea314debc..a4f498f17c3 100644 --- a/spec/services/ci/archive_trace_service_spec.rb +++ b/spec/services/ci/archive_trace_service_spec.rb @@ -24,6 +24,52 @@ RSpec.describe Ci::ArchiveTraceService, '#execute' do it 'does not create an archived trace' do expect { subject }.not_to change { Ci::JobArtifact.trace.count } end + + context 'when live trace chunks still exist' do + before do + create(:ci_build_trace_chunk, build: job) + end + + context 'when the feature flag `erase_traces_from_already_archived_jobs_when_archiving_again` is enabled' do + before do + stub_feature_flags(erase_traces_from_already_archived_jobs_when_archiving_again: true) + end + + it 'removes the trace chunks' do + expect { subject }.to change { job.trace_chunks.count }.to(0) + end + + context 'when associated data does not exist' do + before do + job.job_artifacts_trace.file.remove! + end + + it 'removes the trace artifact' do + expect { subject }.to change { job.reload.job_artifacts_trace }.to(nil) + end + end + end + + context 'when the feature flag `erase_traces_from_already_archived_jobs_when_archiving_again` is disabled' do + before do + stub_feature_flags(erase_traces_from_already_archived_jobs_when_archiving_again: false) + end + + it 'does not remove the trace chunks' do + expect { subject }.not_to change { job.trace_chunks.count } + end + + context 'when associated data does not exist' do + before do + job.job_artifacts_trace.file.remove! + end + + it 'does not remove the trace artifact' do + expect { subject }.not_to change { job.reload.job_artifacts_trace } + end + end + end + end end context 'when job does not have trace' do diff --git a/spec/services/ci/cancel_user_pipelines_service_spec.rb b/spec/services/ci/cancel_user_pipelines_service_spec.rb deleted file mode 100644 index 8491242dfd5..00000000000 --- a/spec/services/ci/cancel_user_pipelines_service_spec.rb +++ /dev/null @@ -1,35 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Ci::CancelUserPipelinesService do - describe '#execute' do - let(:user) { create(:user) } - - subject { described_class.new.execute(user) } - - context 'when user has running CI pipelines' do - let(:pipeline) { create(:ci_pipeline, :running, user: user) } - let!(:build) { create(:ci_build, :running, pipeline: pipeline) } - - it 'cancels all running pipelines and related jobs', :sidekiq_might_not_need_inline do - subject - - expect(pipeline.reload).to be_canceled - expect(build.reload).to be_canceled - end - end - - context 'when an error ocurrs' do - it 'raises a service level error' do - service = double(execute: ServiceResponse.error(message: 'Error canceling pipeline')) - allow(::Ci::CancelUserPipelinesService).to receive(:new).and_return(service) - - result = subject - - expect(result).to be_a(ServiceResponse) - expect(result).to be_error - end - end - end -end diff --git a/spec/services/ci/create_downstream_pipeline_service_spec.rb b/spec/services/ci/create_downstream_pipeline_service_spec.rb index 860932d4fde..dd10fb017aa 100644 --- a/spec/services/ci/create_downstream_pipeline_service_spec.rb +++ b/spec/services/ci/create_downstream_pipeline_service_spec.rb @@ -112,7 +112,7 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do it 'updates bridge status when downstream pipeline gets processed' do pipeline = service.execute(bridge) - expect(pipeline.reload).to be_pending + expect(pipeline.reload).to be_created expect(bridge.reload).to be_success end @@ -227,7 +227,7 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do it 'updates bridge status when downstream pipeline gets processed' do pipeline = service.execute(bridge) - expect(pipeline.reload).to be_pending + expect(pipeline.reload).to be_created expect(bridge.reload).to be_success end diff --git a/spec/services/ci/create_pipeline_service/cross_project_pipeline_spec.rb b/spec/services/ci/create_pipeline_service/cross_project_pipeline_spec.rb index 9cf66dfceb0..d4e9946ac46 100644 --- a/spec/services/ci/create_pipeline_service/cross_project_pipeline_spec.rb +++ b/spec/services/ci/create_pipeline_service/cross_project_pipeline_spec.rb @@ -40,6 +40,7 @@ RSpec.describe Ci::CreatePipelineService, '#execute' do it 'creates bridge job with resource group' do pipeline = create_pipeline! + Ci::InitialPipelineProcessWorker.new.perform(pipeline.id) test = pipeline.statuses.find_by(name: 'instrumentation_test') expect(pipeline).to be_created_successfully diff --git a/spec/services/ci/create_pipeline_service/needs_spec.rb b/spec/services/ci/create_pipeline_service/needs_spec.rb index a6b0a9662c9..4521067cd52 100644 --- a/spec/services/ci/create_pipeline_service/needs_spec.rb +++ b/spec/services/ci/create_pipeline_service/needs_spec.rb @@ -202,7 +202,7 @@ RSpec.describe Ci::CreatePipelineService do YAML end - it 'creates a pipeline with build_a and test_b pending; deploy_b manual' do + it 'creates a pipeline with build_a and test_b pending; deploy_b manual', :sidekiq_inline do processables = pipeline.processables build_a = processables.find { |processable| processable.name == 'build_a' } diff --git a/spec/services/ci/create_pipeline_service/parent_child_pipeline_spec.rb b/spec/services/ci/create_pipeline_service/parent_child_pipeline_spec.rb index a3818937113..5ea75c2253b 100644 --- a/spec/services/ci/create_pipeline_service/parent_child_pipeline_spec.rb +++ b/spec/services/ci/create_pipeline_service/parent_child_pipeline_spec.rb @@ -91,6 +91,7 @@ RSpec.describe Ci::CreatePipelineService, '#execute' do it 'creates bridge job with resource group', :aggregate_failures do pipeline = create_pipeline! + Ci::InitialPipelineProcessWorker.new.perform(pipeline.id) test = pipeline.statuses.find_by(name: 'instrumentation_test') expect(pipeline).to be_created_successfully diff --git a/spec/services/ci/create_pipeline_service/rules_spec.rb b/spec/services/ci/create_pipeline_service/rules_spec.rb index e97e74c1515..33ec6aacc44 100644 --- a/spec/services/ci/create_pipeline_service/rules_spec.rb +++ b/spec/services/ci/create_pipeline_service/rules_spec.rb @@ -151,11 +151,29 @@ RSpec.describe Ci::CreatePipelineService do context 'variables:' do let(:config) do <<-EOY - job: + variables: + VAR4: workflow var 4 + VAR5: workflow var 5 + VAR7: workflow var 7 + + workflow: + rules: + - if: $CI_COMMIT_REF_NAME =~ /master/ + variables: + VAR4: overridden workflow var 4 + - if: $CI_COMMIT_REF_NAME =~ /feature/ + variables: + VAR5: overridden workflow var 5 + VAR6: new workflow var 6 + VAR7: overridden workflow var 7 + - when: always + + job1: script: "echo job1" variables: - VAR1: my var 1 - VAR2: my var 2 + VAR1: job var 1 + VAR2: job var 2 + VAR5: job var 5 rules: - if: $CI_COMMIT_REF_NAME =~ /master/ variables: @@ -164,45 +182,117 @@ RSpec.describe Ci::CreatePipelineService do variables: VAR2: overridden var 2 VAR3: new var 3 + VAR7: overridden var 7 + - when: on_success + + job2: + script: "echo job2" + inherit: + variables: [VAR4, VAR6, VAR7] + variables: + VAR4: job var 4 + rules: + - if: $CI_COMMIT_REF_NAME =~ /master/ + variables: + VAR7: overridden var 7 - when: on_success EOY end - let(:job) { pipeline.builds.find_by(name: 'job') } + let(:job1) { pipeline.builds.find_by(name: 'job1') } + let(:job2) { pipeline.builds.find_by(name: 'job2') } + + let(:variable_keys) { %w(VAR1 VAR2 VAR3 VAR4 VAR5 VAR6 VAR7) } + + context 'when no match' do + let(:ref) { 'refs/heads/wip' } + + it 'does not affect vars' do + expect(job1.scoped_variables.to_hash.values_at(*variable_keys)).to eq( + ['job var 1', 'job var 2', nil, 'workflow var 4', 'job var 5', nil, 'workflow var 7'] + ) + + expect(job2.scoped_variables.to_hash.values_at(*variable_keys)).to eq( + [nil, nil, nil, 'job var 4', nil, nil, 'workflow var 7'] + ) + end + end context 'when matching to the first rule' do let(:ref) { 'refs/heads/master' } - it 'overrides VAR1' do - variables = job.scoped_variables.to_hash + it 'overrides variables' do + expect(job1.scoped_variables.to_hash.values_at(*variable_keys)).to eq( + ['overridden var 1', 'job var 2', nil, 'overridden workflow var 4', 'job var 5', nil, 'workflow var 7'] + ) - expect(variables['VAR1']).to eq('overridden var 1') - expect(variables['VAR2']).to eq('my var 2') - expect(variables['VAR3']).to be_nil + expect(job2.scoped_variables.to_hash.values_at(*variable_keys)).to eq( + [nil, nil, nil, 'job var 4', nil, nil, 'overridden var 7'] + ) + end + + context 'when FF ci_workflow_rules_variables is disabled' do + before do + stub_feature_flags(ci_workflow_rules_variables: false) + end + + it 'does not affect workflow variables but job variables' do + expect(job1.scoped_variables.to_hash.values_at(*variable_keys)).to eq( + ['overridden var 1', 'job var 2', nil, 'workflow var 4', 'job var 5', nil, 'workflow var 7'] + ) + + expect(job2.scoped_variables.to_hash.values_at(*variable_keys)).to eq( + [nil, nil, nil, 'job var 4', nil, nil, 'overridden var 7'] + ) + end end end context 'when matching to the second rule' do let(:ref) { 'refs/heads/feature' } - it 'overrides VAR2 and adds VAR3' do - variables = job.scoped_variables.to_hash + it 'overrides variables' do + expect(job1.scoped_variables.to_hash.values_at(*variable_keys)).to eq( + ['job var 1', 'overridden var 2', 'new var 3', 'workflow var 4', 'job var 5', 'new workflow var 6', 'overridden var 7'] + ) - expect(variables['VAR1']).to eq('my var 1') - expect(variables['VAR2']).to eq('overridden var 2') - expect(variables['VAR3']).to eq('new var 3') + expect(job2.scoped_variables.to_hash.values_at(*variable_keys)).to eq( + [nil, nil, nil, 'job var 4', nil, 'new workflow var 6', 'overridden workflow var 7'] + ) end end - context 'when no match' do - let(:ref) { 'refs/heads/wip' } + context 'using calculated workflow var in job rules' do + let(:config) do + <<-EOY + variables: + VAR1: workflow var 4 + + workflow: + rules: + - if: $CI_COMMIT_REF_NAME =~ /master/ + variables: + VAR1: overridden workflow var 4 + - when: always + + job: + script: "echo job1" + rules: + - if: $VAR1 =~ "overridden workflow var 4" + variables: + VAR1: overridden var 1 + - when: on_success + EOY + end - it 'does not affect vars' do - variables = job.scoped_variables.to_hash + let(:job) { pipeline.builds.find_by(name: 'job') } + + context 'when matching the first workflow condition' do + let(:ref) { 'refs/heads/master' } - expect(variables['VAR1']).to eq('my var 1') - expect(variables['VAR2']).to eq('my var 2') - expect(variables['VAR3']).to be_nil + it 'uses VAR1 of job rules result' do + expect(job.scoped_variables.to_hash['VAR1']).to eq('overridden var 1') + end end end end @@ -230,8 +320,8 @@ RSpec.describe Ci::CreatePipelineService do end context 'matching the first rule in the list' do - it 'saves a pending pipeline' do - expect(pipeline).to be_pending + it 'saves a created pipeline' do + expect(pipeline).to be_created expect(pipeline).to be_persisted end end @@ -239,8 +329,8 @@ RSpec.describe Ci::CreatePipelineService do context 'matching the last rule in the list' do let(:ref) { 'refs/heads/feature' } - it 'saves a pending pipeline' do - expect(pipeline).to be_pending + it 'saves a created pipeline' do + expect(pipeline).to be_created expect(pipeline).to be_persisted end end @@ -280,8 +370,8 @@ RSpec.describe Ci::CreatePipelineService do end context 'matching the first rule in the list' do - it 'saves a pending pipeline' do - expect(pipeline).to be_pending + it 'saves a created pipeline' do + expect(pipeline).to be_created expect(pipeline).to be_persisted end end @@ -305,8 +395,8 @@ RSpec.describe Ci::CreatePipelineService do context 'with partial match' do let(:ref) { 'refs/heads/feature' } - it 'saves a pending pipeline' do - expect(pipeline).to be_pending + it 'saves a created pipeline' do + expect(pipeline).to be_created expect(pipeline).to be_persisted end end @@ -349,8 +439,8 @@ RSpec.describe Ci::CreatePipelineService do context 'where workflow passes and the job passes' do let(:ref) { 'refs/heads/feature' } - it 'saves a pending pipeline' do - expect(pipeline).to be_pending + it 'saves a created pipeline' do + expect(pipeline).to be_created expect(pipeline).to be_persisted end end diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index 9fafc57a770..98c85234fe7 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -63,7 +63,7 @@ RSpec.describe Ci::CreatePipelineService do expect(pipeline).to be_push expect(pipeline).to eq(project.ci_pipelines.last) expect(pipeline).to have_attributes(user: user) - expect(pipeline).to have_attributes(status: 'pending') + expect(pipeline).to have_attributes(status: 'created') expect(pipeline.iid).not_to be_nil expect(pipeline.repository_source?).to be true expect(pipeline.builds.first).to be_kind_of(Ci::Build) @@ -71,19 +71,21 @@ RSpec.describe Ci::CreatePipelineService do end it 'increments the prometheus counter' do - expect(Gitlab::Metrics).to receive(:counter) - .with(:pipelines_created_total, "Counter of pipelines created") - .and_call_original - allow(Gitlab::Metrics).to receive(:counter).and_call_original # allow other counters + counter = spy('pipeline created counter') + + allow(Gitlab::Ci::Pipeline::Metrics) + .to receive(:pipelines_created_counter).and_return(counter) pipeline + + expect(counter).to have_received(:increment) end it 'records pipeline size in a prometheus histogram' do histogram = spy('pipeline size histogram') allow(Gitlab::Ci::Pipeline::Metrics) - .to receive(:new).and_return(histogram) + .to receive(:pipeline_size_histogram).and_return(histogram) execute_service @@ -253,7 +255,7 @@ RSpec.describe Ci::CreatePipelineService do pipeline pipeline_on_previous_commit - expect(pipeline.reload).to have_attributes(status: 'pending', auto_canceled_by_id: nil) + expect(pipeline.reload).to have_attributes(status: 'created', auto_canceled_by_id: nil) end it 'auto cancel pending non-HEAD pipelines', :sidekiq_might_not_need_inline do @@ -263,8 +265,8 @@ RSpec.describe Ci::CreatePipelineService do expect(pipeline_on_previous_commit.reload).to have_attributes(status: 'canceled', auto_canceled_by_id: pipeline.id) end - it 'cancels running outdated pipelines', :sidekiq_might_not_need_inline do - pipeline_on_previous_commit.run + it 'cancels running outdated pipelines', :sidekiq_inline do + pipeline_on_previous_commit.reload.run head_pipeline = execute_service expect(pipeline_on_previous_commit.reload).to have_attributes(status: 'canceled', auto_canceled_by_id: head_pipeline.id) @@ -278,13 +280,13 @@ RSpec.describe Ci::CreatePipelineService do end it 'does not cancel pipelines from the other branches' do - pending_pipeline = execute_service( + new_pipeline = execute_service( ref: 'refs/heads/feature', after: previous_commit_sha_from_ref('feature') ) pipeline - expect(pending_pipeline.reload).to have_attributes(status: 'pending', auto_canceled_by_id: nil) + expect(new_pipeline.reload).to have_attributes(status: 'created', auto_canceled_by_id: nil) end context 'when the interruptible attribute is' do @@ -465,12 +467,12 @@ RSpec.describe Ci::CreatePipelineService do project.update!(auto_cancel_pending_pipelines: 'disabled') end - it 'does not auto cancel pending non-HEAD pipelines' do + it 'does not auto cancel created non-HEAD pipelines' do pipeline_on_previous_commit pipeline expect(pipeline_on_previous_commit.reload) - .to have_attributes(status: 'pending', auto_canceled_by_id: nil) + .to have_attributes(status: 'created', auto_canceled_by_id: nil) end end @@ -580,6 +582,13 @@ RSpec.describe Ci::CreatePipelineService do it_behaves_like 'a failed pipeline' + it 'increments the error metric' do + stub_ci_pipeline_yaml_file(ci_yaml) + + counter = Gitlab::Metrics.counter(:gitlab_ci_pipeline_failure_reasons, 'desc') + expect { execute_service }.to change { counter.get(reason: 'config_error') }.by(1) + end + context 'when receive git commit' do before do allow_any_instance_of(Ci::Pipeline).to receive(:git_commit_message) { message } @@ -770,7 +779,7 @@ RSpec.describe Ci::CreatePipelineService do stub_ci_pipeline_yaml_file(config) end - it 'does not create a new pipeline' do + it 'does not create a new pipeline', :sidekiq_inline do result = execute_service expect(result).to be_persisted diff --git a/spec/services/ci/create_web_ide_terminal_service_spec.rb b/spec/services/ci/create_web_ide_terminal_service_spec.rb index c1c94e30018..c1acf8fd60c 100644 --- a/spec/services/ci/create_web_ide_terminal_service_spec.rb +++ b/spec/services/ci/create_web_ide_terminal_service_spec.rb @@ -66,6 +66,25 @@ RSpec.describe Ci::CreateWebIdeTerminalService do it_behaves_like 'be successful' end + + context 'for configuration with variables' do + let(:config_content) do + <<-EOS + terminal: + script: rspec + variables: + KEY1: VAL1 + EOS + end + + it_behaves_like 'be successful' + + it 'saves the variables' do + expect(subject[:pipeline].builds[0].variables).to include( + key: 'KEY1', value: 'VAL1', public: true, masked: false + ) + end + end end end diff --git a/spec/services/ci/disable_user_pipeline_schedules_service_spec.rb b/spec/services/ci/disable_user_pipeline_schedules_service_spec.rb new file mode 100644 index 00000000000..4ff8dcf075b --- /dev/null +++ b/spec/services/ci/disable_user_pipeline_schedules_service_spec.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::DisableUserPipelineSchedulesService do + describe '#execute' do + let(:user) { create(:user) } + + subject(:service) { described_class.new.execute(user) } + + context 'when user has active pipeline schedules' do + let(:owned_pipeline_schedule) { create(:ci_pipeline_schedule, active: true, owner: user) } + + it 'disables all active pipeline schedules', :aggregate_failures do + expect { service }.to change { owned_pipeline_schedule.reload.active? } + end + end + end +end diff --git a/spec/services/ci/drop_pipeline_service_spec.rb b/spec/services/ci/drop_pipeline_service_spec.rb new file mode 100644 index 00000000000..4adbb99b9e2 --- /dev/null +++ b/spec/services/ci/drop_pipeline_service_spec.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::DropPipelineService do + let_it_be(:user) { create(:user) } + + let(:failure_reason) { :user_blocked } + + let!(:cancelable_pipeline) { create(:ci_pipeline, :running, user: user) } + let!(:running_build) { create(:ci_build, :running, pipeline: cancelable_pipeline) } + let!(:success_pipeline) { create(:ci_pipeline, :success, user: user) } + let!(:success_build) { create(:ci_build, :success, pipeline: success_pipeline) } + + describe '#execute_async_for_all' do + subject { described_class.new.execute_async_for_all(user.pipelines, failure_reason, user) } + + it 'drops only cancelable pipelines asynchronously', :sidekiq_inline do + subject + + expect(cancelable_pipeline.reload).to be_failed + expect(running_build.reload).to be_failed + + expect(success_pipeline.reload).to be_success + expect(success_build.reload).to be_success + end + end + + describe '#execute' do + subject { described_class.new.execute(cancelable_pipeline.id, failure_reason) } + + def drop_pipeline!(pipeline) + described_class.new.execute(pipeline, failure_reason) + end + + it 'drops each cancelable build in the pipeline', :aggregate_failures do + drop_pipeline!(cancelable_pipeline) + + expect(running_build.reload).to be_failed + expect(running_build.failure_reason).to eq(failure_reason.to_s) + + expect(success_build.reload).to be_success + end + + it 'avoids N+1 queries when reading data' do + control_count = ActiveRecord::QueryRecorder.new do + drop_pipeline!(cancelable_pipeline) + end.count + + writes_per_build = 2 + expected_reads_count = control_count - writes_per_build + + create_list(:ci_build, 5, :running, pipeline: cancelable_pipeline) + + expect do + drop_pipeline!(cancelable_pipeline) + end.not_to exceed_query_limit(expected_reads_count + (5 * writes_per_build)) + end + end +end diff --git a/spec/services/ci/external_pull_requests/create_pipeline_service_spec.rb b/spec/services/ci/external_pull_requests/create_pipeline_service_spec.rb index e2bdfae27f0..0cbeaa5446b 100644 --- a/spec/services/ci/external_pull_requests/create_pipeline_service_spec.rb +++ b/spec/services/ci/external_pull_requests/create_pipeline_service_spec.rb @@ -34,7 +34,7 @@ RSpec.describe Ci::ExternalPullRequests::CreatePipelineService do expect(subject).to eq(project.ci_pipelines.last) expect(subject.external_pull_request).to eq(pull_request) expect(subject.user).to eq(user) - expect(subject.status).to eq('pending') + expect(subject.status).to eq('created') expect(subject.ref).to eq(pull_request.source_branch) expect(subject.sha).to eq(pull_request.source_sha) expect(subject.source_sha).to eq(pull_request.source_sha) diff --git a/spec/services/ci/create_job_artifacts_service_spec.rb b/spec/services/ci/job_artifacts/create_service_spec.rb index 1efd1d390a2..22aa9e62c6f 100644 --- a/spec/services/ci/create_job_artifacts_service_spec.rb +++ b/spec/services/ci/job_artifacts/create_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Ci::CreateJobArtifactsService do +RSpec.describe Ci::JobArtifacts::CreateService do let_it_be(:project) { create(:project) } let(:service) { described_class.new(job) } let(:job) { create(:ci_build, project: project) } diff --git a/spec/services/ci/destroy_expired_job_artifacts_service_spec.rb b/spec/services/ci/job_artifacts/destroy_all_expired_service_spec.rb index d315dd35632..04fa55068f2 100644 --- a/spec/services/ci/destroy_expired_job_artifacts_service_spec.rb +++ b/spec/services/ci/job_artifacts/destroy_all_expired_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared_state do +RSpec.describe Ci::JobArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_shared_state do include ExclusiveLeaseHelpers let(:service) { described_class.new } @@ -24,7 +24,7 @@ RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared job = create(:ci_build, pipeline: artifact.job.pipeline) create(:ci_job_artifact, :archive, :expired, job: job) - stub_const('Ci::DestroyExpiredJobArtifactsService::LOOP_LIMIT', 1) + stub_const("#{described_class}::LOOP_LIMIT", 1) end it 'performs the smallest number of queries for job_artifacts' do @@ -113,7 +113,7 @@ RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared context 'when failed to destroy artifact' do before do - stub_const('Ci::DestroyExpiredJobArtifactsService::LOOP_LIMIT', 10) + stub_const("#{described_class}::LOOP_LIMIT", 10) end context 'when the import fails' do @@ -159,8 +159,8 @@ RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared let!(:second_artifact) { create(:ci_job_artifact, expire_at: 1.day.ago) } before do - stub_const('Ci::DestroyExpiredJobArtifactsService::LOOP_TIMEOUT', 0.seconds) - stub_const('Ci::DestroyExpiredJobArtifactsService::BATCH_SIZE', 1) + stub_const("#{described_class}::LOOP_TIMEOUT", 0.seconds) + stub_const("#{described_class}::BATCH_SIZE", 1) second_artifact.job.pipeline.unlocked! end @@ -176,8 +176,8 @@ RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared context 'when loop reached loop limit' do before do - stub_const('Ci::DestroyExpiredJobArtifactsService::LOOP_LIMIT', 1) - stub_const('Ci::DestroyExpiredJobArtifactsService::BATCH_SIZE', 1) + stub_const("#{described_class}::LOOP_LIMIT", 1) + stub_const("#{described_class}::BATCH_SIZE", 1) second_artifact.job.pipeline.unlocked! end @@ -209,7 +209,7 @@ RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared context 'when there are artifacts more than batch sizes' do before do - stub_const('Ci::DestroyExpiredJobArtifactsService::BATCH_SIZE', 1) + stub_const("#{described_class}::BATCH_SIZE", 1) second_artifact.job.pipeline.unlocked! end diff --git a/spec/services/ci/job_artifacts_destroy_batch_service_spec.rb b/spec/services/ci/job_artifacts/destroy_batch_service_spec.rb index 74fbbf28ef1..52aaf73d67e 100644 --- a/spec/services/ci/job_artifacts_destroy_batch_service_spec.rb +++ b/spec/services/ci/job_artifacts/destroy_batch_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Ci::JobArtifactsDestroyBatchService do +RSpec.describe Ci::JobArtifacts::DestroyBatchService do include ExclusiveLeaseHelpers let(:artifacts) { Ci::JobArtifact.all } diff --git a/spec/services/ci/pipeline_artifacts/destroy_expired_artifacts_service_spec.rb b/spec/services/ci/pipeline_artifacts/destroy_all_expired_service_spec.rb index ac1a590face..3dc4f35df22 100644 --- a/spec/services/ci/pipeline_artifacts/destroy_expired_artifacts_service_spec.rb +++ b/spec/services/ci/pipeline_artifacts/destroy_all_expired_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Ci::PipelineArtifacts::DestroyExpiredArtifactsService do +RSpec.describe Ci::PipelineArtifacts::DestroyAllExpiredService do let(:service) { described_class.new } describe '.execute' do @@ -10,7 +10,7 @@ RSpec.describe Ci::PipelineArtifacts::DestroyExpiredArtifactsService do context 'when timeout happens' do before do - stub_const('Ci::PipelineArtifacts::DestroyExpiredArtifactsService::LOOP_TIMEOUT', 0.1.seconds) + stub_const('Ci::PipelineArtifacts::DestroyAllExpiredService::LOOP_TIMEOUT', 0.1.seconds) allow(service).to receive(:destroy_artifacts_batch) { true } end @@ -27,8 +27,8 @@ RSpec.describe Ci::PipelineArtifacts::DestroyExpiredArtifactsService do context 'when the loop limit is reached' do before do - stub_const('::Ci::PipelineArtifacts::DestroyExpiredArtifactsService::LOOP_LIMIT', 1) - stub_const('::Ci::PipelineArtifacts::DestroyExpiredArtifactsService::BATCH_SIZE', 1) + stub_const('::Ci::PipelineArtifacts::DestroyAllExpiredService::LOOP_LIMIT', 1) + stub_const('::Ci::PipelineArtifacts::DestroyAllExpiredService::BATCH_SIZE', 1) create_list(:ci_pipeline_artifact, 2, expire_at: 1.week.ago) end @@ -44,7 +44,7 @@ RSpec.describe Ci::PipelineArtifacts::DestroyExpiredArtifactsService do context 'when there are artifacts more than batch sizes' do before do - stub_const('Ci::PipelineArtifacts::DestroyExpiredArtifactsService::BATCH_SIZE', 1) + stub_const('Ci::PipelineArtifacts::DestroyAllExpiredService::BATCH_SIZE', 1) create_list(:ci_pipeline_artifact, 2, expire_at: 1.week.ago) end diff --git a/spec/services/ci/pipeline_processing/shared_processing_service_tests_with_yaml.rb b/spec/services/ci/pipeline_processing/shared_processing_service_tests_with_yaml.rb index a9f9db8c689..572808cd2db 100644 --- a/spec/services/ci/pipeline_processing/shared_processing_service_tests_with_yaml.rb +++ b/spec/services/ci/pipeline_processing/shared_processing_service_tests_with_yaml.rb @@ -49,7 +49,13 @@ RSpec.shared_context 'Pipeline Processing Service Tests With Yaml' do statuses = pipeline.latest_statuses.by_name(job_names).to_a expect(statuses.count).to eq(job_names.count) # ensure that we have the same counts - statuses.each { |status| status.public_send("#{event}!") } + statuses.each do |status| + if event == 'play' + status.play(user) + else + status.public_send("#{event}!") + end + end end end end diff --git a/spec/services/ci/pipeline_processing/test_cases/dag_build_test_two_manual_review_test_staging_production.yml b/spec/services/ci/pipeline_processing/test_cases/dag_build_test_two_manual_review_test_staging_production.yml new file mode 100644 index 00000000000..ed009ee4f25 --- /dev/null +++ b/spec/services/ci/pipeline_processing/test_cases/dag_build_test_two_manual_review_test_staging_production.yml @@ -0,0 +1,171 @@ +config: + stages: [build, test, review, deploy, post_deploy] + + build: + stage: build + script: exit 0 + + test: + stage: test + script: exit 0 + + release_test1: + stage: test + when: manual + script: exit 0 + + release_test2: + stage: test + when: manual + script: exit 0 + + review: + stage: review + script: exit 0 + needs: [test, release_test1, release_test2] + + staging: + stage: deploy + script: exit 0 + needs: [release_test1] + + production: + stage: deploy + script: exit 0 + needs: [release_test2] + + after_deploy: + stage: post_deploy + script: exit 0 + needs: [production] + + handle_failure: + stage: post_deploy + when: on_failure + script: exit 0 + +init: + expect: + pipeline: pending + stages: + build: pending + test: created + review: created + deploy: created + post_deploy: created + jobs: + build: pending + test: created + release_test1: created + release_test2: created + review: created + staging: created + production: created + after_deploy: created + handle_failure: created + +transitions: + - event: success + jobs: [build] + expect: + pipeline: running + stages: + build: success + test: pending + review: skipped + deploy: skipped + post_deploy: pending + jobs: + build: success + test: pending + release_test1: manual + release_test2: manual + review: skipped + staging: skipped + production: skipped + after_deploy: skipped + handle_failure: created + + - event: success + jobs: [test] + expect: + pipeline: success + stages: + build: success + test: success + review: skipped + deploy: skipped + post_deploy: skipped + jobs: + build: success + test: success + release_test1: manual + release_test2: manual + review: skipped + staging: skipped + production: skipped + after_deploy: skipped + handle_failure: skipped + + - event: play + jobs: [release_test1] + expect: + pipeline: running + stages: + build: success + test: running + review: skipped + deploy: pending + post_deploy: pending + jobs: + build: success + test: success + release_test1: pending + release_test2: manual + review: skipped + staging: created + production: skipped + after_deploy: skipped + handle_failure: created + + - event: success + jobs: [release_test1] + expect: + pipeline: running + stages: + build: success + test: success + review: skipped + deploy: pending + post_deploy: pending + jobs: + build: success + test: success + release_test1: success + release_test2: manual + review: skipped + staging: pending + production: skipped + after_deploy: skipped + handle_failure: created + + - event: success + jobs: [staging] + expect: + pipeline: success + stages: + build: success + test: success + review: skipped + deploy: success + post_deploy: skipped + jobs: + build: success + test: success + release_test1: success + release_test2: manual + review: skipped + staging: success + production: skipped + after_deploy: skipped + handle_failure: skipped diff --git a/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_allow_failure_false.yml b/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_allow_failure_false.yml index 1d61cd24f8c..7987f4568a4 100644 --- a/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_allow_failure_false.yml +++ b/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_allow_failure_false.yml @@ -21,7 +21,7 @@ init: deploy: created transitions: - - event: enqueue + - event: play jobs: [test] expect: pipeline: pending diff --git a/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_allow_failure_false_deploy_always.yml b/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_allow_failure_false_deploy_always.yml index bb8723aa303..ea7f0f06c50 100644 --- a/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_allow_failure_false_deploy_always.yml +++ b/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_allow_failure_false_deploy_always.yml @@ -22,7 +22,7 @@ init: deploy: created transitions: - - event: enqueue + - event: play jobs: [test] expect: pipeline: pending diff --git a/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_allow_failure_false_deploy_on_failure.yml b/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_allow_failure_false_deploy_on_failure.yml index 3099a94befb..5c839ebc0e9 100644 --- a/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_allow_failure_false_deploy_on_failure.yml +++ b/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_allow_failure_false_deploy_on_failure.yml @@ -22,7 +22,7 @@ init: deploy: created transitions: - - event: enqueue + - event: play jobs: [test] expect: pipeline: pending diff --git a/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_allow_failure_true_deploy_on_failure.yml b/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_allow_failure_true_deploy_on_failure.yml index 81aad4940b6..2d379f2d7c5 100644 --- a/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_allow_failure_true_deploy_on_failure.yml +++ b/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_allow_failure_true_deploy_on_failure.yml @@ -22,7 +22,7 @@ init: deploy: skipped transitions: - - event: enqueue + - event: play jobs: [test] expect: pipeline: pending @@ -31,7 +31,7 @@ transitions: deploy: skipped jobs: test: pending - deploy: skipped + deploy: created - event: drop jobs: [test] diff --git a/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_allow_failure_true.yml b/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_allow_failure_true_fails.yml index b8fcdd1566a..fbe04c7e18e 100644 --- a/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_allow_failure_true.yml +++ b/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_allow_failure_true_fails.yml @@ -21,7 +21,7 @@ init: deploy: skipped transitions: - - event: enqueue + - event: play jobs: [test] expect: pipeline: pending @@ -30,7 +30,7 @@ transitions: deploy: skipped jobs: test: pending - deploy: skipped + deploy: created - event: run jobs: [test] @@ -41,15 +41,26 @@ transitions: deploy: skipped jobs: test: running - deploy: skipped + deploy: created - event: drop jobs: [test] expect: + pipeline: running + stages: + test: success + deploy: pending + jobs: + test: failed + deploy: pending + + - event: success + jobs: [deploy] + expect: pipeline: success stages: test: success - deploy: skipped + deploy: success jobs: test: failed - deploy: skipped + deploy: success diff --git a/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_allow_failure_true_succeeds.yml b/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_allow_failure_true_succeeds.yml new file mode 100644 index 00000000000..68ef057f62d --- /dev/null +++ b/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_allow_failure_true_succeeds.yml @@ -0,0 +1,66 @@ +config: + test: + stage: test + when: manual + allow_failure: true + script: exit 0 + + deploy: + stage: deploy + script: exit 0 + needs: [test] + +init: + expect: + pipeline: skipped + stages: + test: skipped + deploy: skipped + jobs: + test: manual + deploy: skipped + +transitions: + - event: play + jobs: [test] + expect: + pipeline: pending + stages: + test: pending + deploy: skipped + jobs: + test: pending + deploy: created + + - event: run + jobs: [test] + expect: + pipeline: running + stages: + test: running + deploy: skipped + jobs: + test: running + deploy: created + + - event: success + jobs: [test] + expect: + pipeline: running + stages: + test: success + deploy: pending + jobs: + test: success + deploy: pending + + - event: success + jobs: [deploy] + expect: + pipeline: success + stages: + test: success + deploy: success + jobs: + test: success + deploy: success diff --git a/spec/services/ci/pipeline_processing/test_cases/stage_test_manual_allow_failure_false.yml b/spec/services/ci/pipeline_processing/test_cases/stage_test_manual_allow_failure_false.yml index 2ffa35b56d7..759b4d0ae75 100644 --- a/spec/services/ci/pipeline_processing/test_cases/stage_test_manual_allow_failure_false.yml +++ b/spec/services/ci/pipeline_processing/test_cases/stage_test_manual_allow_failure_false.yml @@ -20,7 +20,7 @@ init: deploy: created transitions: - - event: enqueue + - event: play jobs: [test] expect: pipeline: pending diff --git a/spec/services/ci/pipeline_processing/test_cases/stage_test_manual_allow_failure_true.yml b/spec/services/ci/pipeline_processing/test_cases/stage_test_manual_allow_failure_true.yml index 088fab5ca09..93eecae8fcf 100644 --- a/spec/services/ci/pipeline_processing/test_cases/stage_test_manual_allow_failure_true.yml +++ b/spec/services/ci/pipeline_processing/test_cases/stage_test_manual_allow_failure_true.yml @@ -31,7 +31,7 @@ transitions: test: manual deploy: success - - event: enqueue + - event: play jobs: [test] expect: pipeline: running diff --git a/spec/services/ci/pipeline_processing/test_cases/stage_test_manual_allow_failure_true_deploy_on_failure.yml b/spec/services/ci/pipeline_processing/test_cases/stage_test_manual_allow_failure_true_deploy_on_failure.yml index 2b30316aef6..301f9631845 100644 --- a/spec/services/ci/pipeline_processing/test_cases/stage_test_manual_allow_failure_true_deploy_on_failure.yml +++ b/spec/services/ci/pipeline_processing/test_cases/stage_test_manual_allow_failure_true_deploy_on_failure.yml @@ -21,7 +21,7 @@ init: deploy: skipped transitions: - - event: enqueue + - event: play jobs: [test] expect: pipeline: pending @@ -30,7 +30,7 @@ transitions: deploy: skipped jobs: test: pending - deploy: skipped + deploy: created - event: drop jobs: [test] diff --git a/spec/services/ci/pipeline_trigger_service_spec.rb b/spec/services/ci/pipeline_trigger_service_spec.rb index 89d3da89011..36055779a2e 100644 --- a/spec/services/ci/pipeline_trigger_service_spec.rb +++ b/spec/services/ci/pipeline_trigger_service_spec.rb @@ -55,17 +55,6 @@ RSpec.describe Ci::PipelineTriggerService do expect(var.variable_type).to eq('file') end - context 'when FF ci_trigger_payload_into_pipeline is disabled' do - before do - stub_feature_flags(ci_trigger_payload_into_pipeline: false) - end - - it 'does not store the payload as a variable' do - expect { result }.not_to change { Ci::PipelineVariable.count } - expect(result[:pipeline].variables).to be_empty - end - end - context 'when commit message has [ci skip]' do before do allow_next(Ci::Pipeline).to receive(:git_commit_message) { '[ci skip]' } diff --git a/spec/services/ci/play_bridge_service_spec.rb b/spec/services/ci/play_bridge_service_spec.rb index 0482ad4d76f..d6130325b5a 100644 --- a/spec/services/ci/play_bridge_service_spec.rb +++ b/spec/services/ci/play_bridge_service_spec.rb @@ -35,6 +35,28 @@ RSpec.describe Ci::PlayBridgeService, '#execute' do expect(bridge.reload.user).to eq(user) end + context 'when a subsequent job is skipped' do + let!(:job) { create(:ci_build, :skipped, pipeline: pipeline, stage_idx: bridge.stage_idx + 1) } + + before do + create(:ci_build_need, build: job, name: bridge.name) + end + + it 'marks the subsequent job as processable' do + expect { execute_service }.to change { job.reload.status }.from('skipped').to('created') + end + + context 'when the FF ci_fix_pipeline_status_for_dag_needs_manual is disabled' do + before do + stub_feature_flags(ci_fix_pipeline_status_for_dag_needs_manual: false) + end + + it 'does not change the subsequent job' do + expect { execute_service }.not_to change { job.reload.status }.from('skipped') + end + end + end + context 'when bridge is not playable' do let(:bridge) { create(:ci_bridge, :failed, pipeline: pipeline, downstream: downstream_project) } diff --git a/spec/services/ci/play_build_service_spec.rb b/spec/services/ci/play_build_service_spec.rb index 00c6de7681d..78de91675f9 100644 --- a/spec/services/ci/play_build_service_spec.rb +++ b/spec/services/ci/play_build_service_spec.rb @@ -61,6 +61,28 @@ RSpec.describe Ci::PlayBuildService, '#execute' do expect(build.reload.user).to eq user end + context 'when a subsequent job is skipped' do + let!(:job) { create(:ci_build, :skipped, pipeline: pipeline, stage_idx: build.stage_idx + 1) } + + before do + create(:ci_build_need, build: job, name: build.name) + end + + it 'marks the subsequent job as processable' do + expect { service.execute(build) }.to change { job.reload.status }.from('skipped').to('created') + end + + context 'when the FF ci_fix_pipeline_status_for_dag_needs_manual is disabled' do + before do + stub_feature_flags(ci_fix_pipeline_status_for_dag_needs_manual: false) + end + + it 'does not change the subsequent job' do + expect { service.execute(build) }.not_to change { job.reload.status }.from('skipped') + end + end + end + context 'when variables are supplied' do let(:job_variables) do [{ key: 'first', secret_value: 'first' }, diff --git a/spec/services/ci/process_build_service_spec.rb b/spec/services/ci/process_build_service_spec.rb index 42a92504839..b54fc45d36a 100644 --- a/spec/services/ci/process_build_service_spec.rb +++ b/spec/services/ci/process_build_service_spec.rb @@ -145,28 +145,5 @@ RSpec.describe Ci::ProcessBuildService, '#execute' do expect { subject }.to change { build.status }.to(after_status) end end - - context 'when FF skip_dag_manual_and_delayed_jobs is disabled on the project' do - let_it_be(:other_project) { create(:project) } - - before do - stub_feature_flags(skip_dag_manual_and_delayed_jobs: other_project) - end - - where(:build_when, :current_status, :after_status) do - :on_success | 'success' | 'pending' - :on_success | 'skipped' | 'skipped' - :manual | 'success' | 'manual' - :manual | 'skipped' | 'manual' - :delayed | 'success' | 'manual' - :delayed | 'skipped' | 'manual' - end - - with_them do - it 'proceeds the build' do - expect { subject }.to change { build.status }.to(after_status) - end - end - end end end diff --git a/spec/services/ci/process_pipeline_service_spec.rb b/spec/services/ci/process_pipeline_service_spec.rb index e02536fd07f..254bd19c808 100644 --- a/spec/services/ci/process_pipeline_service_spec.rb +++ b/spec/services/ci/process_pipeline_service_spec.rb @@ -10,6 +10,14 @@ RSpec.describe Ci::ProcessPipelineService do create(:ci_empty_pipeline, ref: 'master', project: project) end + let(:pipeline_processing_events_counter) { double(increment: true) } + let(:legacy_update_jobs_counter) { double(increment: true) } + + let(:metrics) do + double(pipeline_processing_events_counter: pipeline_processing_events_counter, + legacy_update_jobs_counter: legacy_update_jobs_counter) + end + subject { described_class.new(pipeline) } before do @@ -17,22 +25,13 @@ RSpec.describe Ci::ProcessPipelineService do stub_not_protect_default_branch project.add_developer(user) + + allow(subject).to receive(:metrics).and_return(metrics) end describe 'processing events counter' do - let(:metrics) { double('pipeline metrics') } - let(:counter) { double('events counter') } - - before do - allow(subject) - .to receive(:metrics).and_return(metrics) - allow(metrics) - .to receive(:pipeline_processing_events_counter) - .and_return(counter) - end - it 'increments processing events counter' do - expect(counter).to receive(:increment) + expect(pipeline_processing_events_counter).to receive(:increment) subject.execute end @@ -64,33 +63,22 @@ RSpec.describe Ci::ProcessPipelineService do expect(all_builds.retried).to contain_exactly(build_retried) end - context 'counter ci_legacy_update_jobs_as_retried_total' do - let(:counter) { double(increment: true) } + it 'increments the counter' do + expect(legacy_update_jobs_counter).to receive(:increment) + subject.execute + end + + context 'when the previous build has already retried column true' do before do - allow(Gitlab::Metrics).to receive(:counter).and_call_original - allow(Gitlab::Metrics).to receive(:counter) - .with(:ci_legacy_update_jobs_as_retried_total, anything) - .and_return(counter) + build_retried.update_columns(retried: true) end - it 'increments the counter' do - expect(counter).to receive(:increment) + it 'does not increment the counter' do + expect(legacy_update_jobs_counter).not_to receive(:increment) subject.execute end - - context 'when the previous build has already retried column true' do - before do - build_retried.update_columns(retried: true) - end - - it 'does not increment the counter' do - expect(counter).not_to receive(:increment) - - subject.execute - end - end end end diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb index 9187dd4f300..02b48e8ba06 100644 --- a/spec/services/ci/register_job_service_spec.rb +++ b/spec/services/ci/register_job_service_spec.rb @@ -225,6 +225,28 @@ module Ci end end + context 'when the use_distinct_in_register_job_object_hierarchy feature flag is enabled' do + before do + stub_feature_flags(use_distinct_in_register_job_object_hierarchy: true) + stub_feature_flags(use_distinct_for_all_object_hierarchy: true) + end + + it 'calls DISTINCT' do + expect(described_class.new(group_runner).send(:builds_for_group_runner).to_sql).to include("DISTINCT") + end + end + + context 'when the use_distinct_in_register_job_object_hierarchy feature flag is disabled' do + before do + stub_feature_flags(use_distinct_in_register_job_object_hierarchy: false) + stub_feature_flags(use_distinct_for_all_object_hierarchy: false) + end + + it 'does not call DISTINCT' do + expect(described_class.new(group_runner).send(:builds_for_group_runner).to_sql).not_to include("DISTINCT") + end + end + context 'group runner' do let(:build) { execute(group_runner) } @@ -593,9 +615,22 @@ module Ci create(:ci_build, pipeline: pipeline, tag_list: %w[non-matching]) end - it "observes queue size of only matching jobs" do + it 'observes queue size of only matching jobs' do # pending_job + 2 x matching ones - expect(Gitlab::Ci::Queue::Metrics.queue_size_total).to receive(:observe).with({}, 3) + expect(Gitlab::Ci::Queue::Metrics.queue_size_total).to receive(:observe) + .with({ runner_type: specific_runner.runner_type }, 3) + + expect(execute(specific_runner)).to eq(pending_job) + end + + it 'observes queue processing time by the runner type' do + expect(Gitlab::Ci::Queue::Metrics.queue_iteration_duration_seconds) + .to receive(:observe) + .with({ runner_type: specific_runner.runner_type }, anything) + + expect(Gitlab::Ci::Queue::Metrics.queue_retrieval_duration_seconds) + .to receive(:observe) + .with({ runner_type: specific_runner.runner_type }, anything) expect(execute(specific_runner)).to eq(pending_job) end diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index bdf60bb3fdc..7dd3d963e56 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -181,7 +181,7 @@ RSpec.describe Ci::RetryBuildService do end it 'resolves todos for old build that failed' do - expect(MergeRequests::AddTodoWhenBuildFailsService) + expect(::MergeRequests::AddTodoWhenBuildFailsService) .to receive_message_chain(:new, :close) service.execute(build) diff --git a/spec/services/ci/retry_pipeline_service_spec.rb b/spec/services/ci/retry_pipeline_service_spec.rb index 3c6a99efbf8..3e2e9f07723 100644 --- a/spec/services/ci/retry_pipeline_service_spec.rb +++ b/spec/services/ci/retry_pipeline_service_spec.rb @@ -272,7 +272,7 @@ RSpec.describe Ci::RetryPipelineService, '#execute' do end it 'closes all todos about failed jobs for pipeline' do - expect(MergeRequests::AddTodoWhenBuildFailsService) + expect(::MergeRequests::AddTodoWhenBuildFailsService) .to receive_message_chain(:new, :close_all) service.execute(pipeline) diff --git a/spec/services/ci/test_failure_history_service_spec.rb b/spec/services/ci/test_failure_history_service_spec.rb index d9c1c8dc3fa..c19df6e217b 100644 --- a/spec/services/ci/test_failure_history_service_spec.rb +++ b/spec/services/ci/test_failure_history_service_spec.rb @@ -11,15 +11,15 @@ RSpec.describe Ci::TestFailureHistoryService, :aggregate_failures do context 'when pipeline has failed builds with test reports' do before do - # The test report has 2 test case failures + # The test report has 2 unit test failures create(:ci_build, :failed, :test_reports, pipeline: pipeline, project: project) end - it 'creates test case failures records' do + it 'creates unit test failures records' do execute_service - expect(Ci::TestCase.count).to eq(2) - expect(Ci::TestCaseFailure.count).to eq(2) + expect(Ci::UnitTest.count).to eq(2) + expect(Ci::UnitTestFailure.count).to eq(2) end context 'when pipeline is not for the default branch' do @@ -30,8 +30,8 @@ RSpec.describe Ci::TestFailureHistoryService, :aggregate_failures do it 'does not persist data' do execute_service - expect(Ci::TestCase.count).to eq(0) - expect(Ci::TestCaseFailure.count).to eq(0) + expect(Ci::UnitTest.count).to eq(0) + expect(Ci::UnitTestFailure.count).to eq(0) end end @@ -43,12 +43,12 @@ RSpec.describe Ci::TestFailureHistoryService, :aggregate_failures do it 'does not fail but does not persist new data' do expect { described_class.new(pipeline).execute }.not_to raise_error - expect(Ci::TestCase.count).to eq(2) - expect(Ci::TestCaseFailure.count).to eq(2) + expect(Ci::UnitTest.count).to eq(2) + expect(Ci::UnitTestFailure.count).to eq(2) end end - context 'when number of failed test cases exceed the limit' do + context 'when number of failed unit tests exceed the limit' do before do stub_const("#{described_class.name}::MAX_TRACKABLE_FAILURES", 1) end @@ -56,16 +56,16 @@ RSpec.describe Ci::TestFailureHistoryService, :aggregate_failures do it 'does not persist data' do execute_service - expect(Ci::TestCase.count).to eq(0) - expect(Ci::TestCaseFailure.count).to eq(0) + expect(Ci::UnitTest.count).to eq(0) + expect(Ci::UnitTestFailure.count).to eq(0) end end - context 'when number of failed test cases across multiple builds exceed the limit' do + context 'when number of failed unit tests across multiple builds exceed the limit' do before do stub_const("#{described_class.name}::MAX_TRACKABLE_FAILURES", 2) - # This other test report has 1 unique test case failure which brings us to 3 total failures across all builds + # This other test report has 1 unique unit test failure which brings us to 3 total failures across all builds # thus exceeding the limit of 2 for MAX_TRACKABLE_FAILURES create(:ci_build, :failed, :test_reports_with_duplicate_failed_test_names, pipeline: pipeline, project: project) end @@ -73,23 +73,23 @@ RSpec.describe Ci::TestFailureHistoryService, :aggregate_failures do it 'does not persist data' do execute_service - expect(Ci::TestCase.count).to eq(0) - expect(Ci::TestCaseFailure.count).to eq(0) + expect(Ci::UnitTest.count).to eq(0) + expect(Ci::UnitTestFailure.count).to eq(0) end end end - context 'when test failure data have duplicates within the same payload (happens when the JUnit report has duplicate test case names but have different failures)' do + context 'when test failure data have duplicates within the same payload (happens when the JUnit report has duplicate unit test names but have different failures)' do before do - # The test report has 2 test case failures but with the same test case keys + # The test report has 2 unit test failures but with the same unit test keys create(:ci_build, :failed, :test_reports_with_duplicate_failed_test_names, pipeline: pipeline, project: project) end it 'does not fail but does not persist duplicate data' do expect { execute_service }.not_to raise_error - expect(Ci::TestCase.count).to eq(1) - expect(Ci::TestCaseFailure.count).to eq(1) + expect(Ci::UnitTest.count).to eq(1) + expect(Ci::UnitTestFailure.count).to eq(1) end end @@ -102,8 +102,8 @@ RSpec.describe Ci::TestFailureHistoryService, :aggregate_failures do it 'does not persist data' do execute_service - expect(Ci::TestCase.count).to eq(0) - expect(Ci::TestCaseFailure.count).to eq(0) + expect(Ci::UnitTest.count).to eq(0) + expect(Ci::UnitTestFailure.count).to eq(0) end end end diff --git a/spec/services/clusters/integrations/create_service_spec.rb b/spec/services/clusters/integrations/create_service_spec.rb new file mode 100644 index 00000000000..cfc0943b6ad --- /dev/null +++ b/spec/services/clusters/integrations/create_service_spec.rb @@ -0,0 +1,90 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Clusters::Integrations::CreateService, '#execute' do + let_it_be(:project) { create(:project) } + let_it_be_with_reload(:cluster) { create(:cluster, :provided_by_gcp, projects: [project]) } + + let(:params) do + { application_type: 'prometheus', enabled: true } + end + + let(:service) do + described_class.new(container: project, cluster: cluster, current_user: project.owner, params: params) + end + + it 'creates a new Prometheus instance' do + expect(service.execute).to be_success + + expect(cluster.integration_prometheus).to be_present + expect(cluster.integration_prometheus).to be_persisted + expect(cluster.integration_prometheus).to be_enabled + end + + context 'enabled param is false' do + let(:params) do + { application_type: 'prometheus', enabled: false } + end + + it 'creates a new uninstalled Prometheus instance' do + expect(service.execute).to be_success + + expect(cluster.integration_prometheus).to be_present + expect(cluster.integration_prometheus).to be_persisted + expect(cluster.integration_prometheus).not_to be_enabled + end + end + + context 'unauthorized user' do + let(:service) do + unauthorized_user = create(:user) + + described_class.new(container: project, cluster: cluster, current_user: unauthorized_user, params: params) + end + + it 'does not create a new Prometheus instance' do + expect(service.execute).to be_error + + expect(cluster.integration_prometheus).to be_nil + end + end + + context 'prometheus record exists' do + before do + create(:clusters_integrations_prometheus, cluster: cluster) + end + + it 'updates the Prometheus instance' do + expect(service.execute).to be_success + + expect(cluster.integration_prometheus).to be_present + expect(cluster.integration_prometheus).to be_persisted + expect(cluster.integration_prometheus).to be_enabled + end + + context 'enabled param is false' do + let(:params) do + { application_type: 'prometheus', enabled: false } + end + + it 'updates the Prometheus instance as uninstalled' do + expect(service.execute).to be_success + + expect(cluster.integration_prometheus).to be_present + expect(cluster.integration_prometheus).to be_persisted + expect(cluster.integration_prometheus).not_to be_enabled + end + end + end + + context 'for an un-supported application type' do + let(:params) do + { application_type: 'something_else', enabled: true } + end + + it 'errors' do + expect { service.execute}.to raise_error(ArgumentError) + end + end +end diff --git a/spec/services/deployments/link_merge_requests_service_spec.rb b/spec/services/deployments/link_merge_requests_service_spec.rb index e2ac2273b8c..a5a13230d6f 100644 --- a/spec/services/deployments/link_merge_requests_service_spec.rb +++ b/spec/services/deployments/link_merge_requests_service_spec.rb @@ -32,6 +32,17 @@ RSpec.describe Deployments::LinkMergeRequestsService do end end + context 'when the deployment failed' do + it 'does nothing' do + environment = create(:environment, name: 'foo') + deploy = create(:deployment, :failed, environment: environment) + + expect(deploy).not_to receive(:link_merge_requests) + + described_class.new(deploy).execute + end + end + context 'when there is a previous deployment' do it 'links all merge requests merged since the previous deployment' do deploy1 = create( diff --git a/spec/services/design_management/copy_design_collection/copy_service_spec.rb b/spec/services/design_management/copy_design_collection/copy_service_spec.rb index ddbed91815f..03242487b53 100644 --- a/spec/services/design_management/copy_design_collection/copy_service_spec.rb +++ b/spec/services/design_management/copy_design_collection/copy_service_spec.rb @@ -7,6 +7,7 @@ RSpec.describe DesignManagement::CopyDesignCollection::CopyService, :clean_gitla let_it_be(:user) { create(:user) } let_it_be(:project) { create(:project) } let_it_be(:issue, refind: true) { create(:issue, project: project) } + let(:target_issue) { create(:issue) } subject { described_class.new(project, user, issue: issue, target_issue: target_issue).execute } diff --git a/spec/services/design_management/delete_designs_service_spec.rb b/spec/services/design_management/delete_designs_service_spec.rb index ed161b4c8ff..341f71fa62c 100644 --- a/spec/services/design_management/delete_designs_service_spec.rb +++ b/spec/services/design_management/delete_designs_service_spec.rb @@ -7,6 +7,7 @@ RSpec.describe DesignManagement::DeleteDesignsService do let_it_be(:project) { create(:project) } let_it_be(:issue) { create(:issue, project: project) } let_it_be(:user) { create(:user) } + let(:designs) { create_designs } subject(:service) { described_class.new(project, user, issue: issue, designs: designs) } diff --git a/spec/services/design_management/save_designs_service_spec.rb b/spec/services/design_management/save_designs_service_spec.rb index f36e68c8dbd..5bc763cc95e 100644 --- a/spec/services/design_management/save_designs_service_spec.rb +++ b/spec/services/design_management/save_designs_service_spec.rb @@ -7,6 +7,7 @@ RSpec.describe DesignManagement::SaveDesignsService do let_it_be_with_reload(:issue) { create(:issue) } let_it_be(:developer) { create(:user, developer_projects: [issue.project]) } + let(:project) { issue.project } let(:user) { developer } let(:files) { [rails_sample] } @@ -309,7 +310,7 @@ RSpec.describe DesignManagement::SaveDesignsService do end context 'when the user is not allowed to upload designs' do - let(:user) { build_stubbed(:user) } + let(:user) { build_stubbed(:user, id: non_existing_record_id) } it_behaves_like 'a service error' end diff --git a/spec/services/draft_notes/publish_service_spec.rb b/spec/services/draft_notes/publish_service_spec.rb index f83e91b683f..f93622dc25a 100644 --- a/spec/services/draft_notes/publish_service_spec.rb +++ b/spec/services/draft_notes/publish_service_spec.rb @@ -229,7 +229,7 @@ RSpec.describe DraftNotes::PublishService do expect(DraftNote.count).to eq(2) end - context 'with quick actions' do + context 'with quick actions', :sidekiq_inline do it 'performs quick actions' do other_user = create(:user) project.add_developer(other_user) diff --git a/spec/services/environments/auto_stop_service_spec.rb b/spec/services/environments/auto_stop_service_spec.rb index 8e56c7e642c..93b1596586f 100644 --- a/spec/services/environments/auto_stop_service_spec.rb +++ b/spec/services/environments/auto_stop_service_spec.rb @@ -8,6 +8,7 @@ RSpec.describe Environments::AutoStopService, :clean_gitlab_redis_shared_state d let_it_be(:project) { create(:project, :repository) } let_it_be(:user) { create(:user) } + let(:service) { described_class.new } before_all do @@ -19,6 +20,7 @@ RSpec.describe Environments::AutoStopService, :clean_gitlab_redis_shared_state d let_it_be(:project) { create(:project, :repository) } let_it_be(:user) { create(:user) } + let(:environments) { Environment.all } before_all do diff --git a/spec/services/environments/canary_ingress/update_service_spec.rb b/spec/services/environments/canary_ingress/update_service_spec.rb index 5ba62e7104c..0e72fff1ed2 100644 --- a/spec/services/environments/canary_ingress/update_service_spec.rb +++ b/spec/services/environments/canary_ingress/update_service_spec.rb @@ -8,6 +8,7 @@ RSpec.describe Environments::CanaryIngress::UpdateService, :clean_gitlab_redis_c let_it_be(:project, refind: true) { create(:project) } let_it_be(:maintainer) { create(:user) } let_it_be(:reporter) { create(:user) } + let(:user) { maintainer } let(:params) { {} } let(:service) { described_class.new(project, user, params) } diff --git a/spec/services/environments/reset_auto_stop_service_spec.rb b/spec/services/environments/reset_auto_stop_service_spec.rb index cab1bf2cc26..4a0b091c12d 100644 --- a/spec/services/environments/reset_auto_stop_service_spec.rb +++ b/spec/services/environments/reset_auto_stop_service_spec.rb @@ -6,6 +6,7 @@ RSpec.describe Environments::ResetAutoStopService do let_it_be(:project) { create(:project) } let_it_be(:developer) { create(:user).tap { |user| project.add_developer(user) } } let_it_be(:reporter) { create(:user).tap { |user| project.add_reporter(user) } } + let(:user) { developer } let(:service) { described_class.new(project, user) } diff --git a/spec/services/groups/auto_devops_service_spec.rb b/spec/services/groups/auto_devops_service_spec.rb index 3d89ee96823..486a99dd8df 100644 --- a/spec/services/groups/auto_devops_service_spec.rb +++ b/spec/services/groups/auto_devops_service_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' RSpec.describe Groups::AutoDevopsService, '#execute' do let_it_be(:group) { create(:group) } let_it_be(:user) { create(:user) } + let(:group_params) { { auto_devops_enabled: '0' } } let(:service) { described_class.new(group, user, group_params) } diff --git a/spec/services/groups/group_links/update_service_spec.rb b/spec/services/groups/group_links/update_service_spec.rb index 436cdf89a0f..82c4a10f15a 100644 --- a/spec/services/groups/group_links/update_service_spec.rb +++ b/spec/services/groups/group_links/update_service_spec.rb @@ -8,6 +8,7 @@ RSpec.describe Groups::GroupLinks::UpdateService, '#execute' do let_it_be(:group) { create(:group, :private) } let_it_be(:shared_group) { create(:group, :private) } let_it_be(:project) { create(:project, group: shared_group) } + let(:group_member_user) { create(:user) } let!(:link) { create(:group_group_link, shared_group: shared_group, shared_with_group: group) } diff --git a/spec/services/groups/merge_requests_count_service_spec.rb b/spec/services/groups/merge_requests_count_service_spec.rb new file mode 100644 index 00000000000..10c7ba5fca4 --- /dev/null +++ b/spec/services/groups/merge_requests_count_service_spec.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Groups::MergeRequestsCountService, :use_clean_rails_memory_store_caching do + let_it_be(:user) { create(:user) } + let_it_be(:group) { create(:group, :public)} + let_it_be(:project) { create(:project, :repository, namespace: group) } + let_it_be(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + + subject { described_class.new(group, user) } + + describe '#relation_for_count' do + before do + group.add_reporter(user) + allow(MergeRequestsFinder).to receive(:new).and_call_original + end + + it 'uses the MergeRequestsFinder to scope merge requests' do + expect(MergeRequestsFinder) + .to receive(:new) + .with(user, group_id: group.id, state: 'opened', non_archived: true, include_subgroups: true) + + subject.count + end + end + + it_behaves_like 'a counter caching service with threshold' +end diff --git a/spec/services/groups/open_issues_count_service_spec.rb b/spec/services/groups/open_issues_count_service_spec.rb index 8bbb1c90c6b..740e9846119 100644 --- a/spec/services/groups/open_issues_count_service_spec.rb +++ b/spec/services/groups/open_issues_count_service_spec.rb @@ -54,53 +54,7 @@ RSpec.describe Groups::OpenIssuesCountService, :use_clean_rails_memory_store_cac end end - context 'with different cache values' do - let(:public_count_key) { subject.cache_key(described_class::PUBLIC_COUNT_KEY) } - let(:under_threshold) { described_class::CACHED_COUNT_THRESHOLD - 1 } - let(:over_threshold) { described_class::CACHED_COUNT_THRESHOLD + 1 } - - context 'when cache is empty' do - before do - Rails.cache.delete(public_count_key) - end - - it 'refreshes cache if value over threshold' do - allow(subject).to receive(:uncached_count).and_return(over_threshold) - - expect(subject.count).to eq(over_threshold) - expect(Rails.cache.read(public_count_key)).to eq(over_threshold) - end - - it 'does not refresh cache if value under threshold' do - allow(subject).to receive(:uncached_count).and_return(under_threshold) - - expect(subject.count).to eq(under_threshold) - expect(Rails.cache.read(public_count_key)).to be_nil - end - end - - context 'when cached count is under the threshold value' do - before do - Rails.cache.write(public_count_key, under_threshold) - end - - it 'does not refresh cache' do - expect(Rails.cache).not_to receive(:write) - expect(subject.count).to eq(under_threshold) - end - end - - context 'when cached count is over the threshold value' do - before do - Rails.cache.write(public_count_key, over_threshold) - end - - it 'does not refresh cache' do - expect(Rails.cache).not_to receive(:write) - expect(subject.count).to eq(over_threshold) - end - end - end + it_behaves_like 'a counter caching service with threshold' end end end diff --git a/spec/services/groups/transfer_service_spec.rb b/spec/services/groups/transfer_service_spec.rb index 19b746ade34..3a1197970f4 100644 --- a/spec/services/groups/transfer_service_spec.rb +++ b/spec/services/groups/transfer_service_spec.rb @@ -5,12 +5,14 @@ require 'spec_helper' RSpec.describe Groups::TransferService do let_it_be(:user) { create(:user) } let_it_be(:new_parent_group) { create(:group, :public) } + let!(:group_member) { create(:group_member, :owner, group: group, user: user) } let(:transfer_service) { described_class.new(group, user) } context 'handling packages' do let_it_be(:group) { create(:group, :public) } let_it_be(:new_group) { create(:group, :public) } + let(:project) { create(:project, :public, namespace: group) } before do @@ -272,6 +274,7 @@ RSpec.describe Groups::TransferService do context 'with a group integration' do let_it_be(:instance_integration) { create(:slack_service, :instance, webhook: 'http://project.slack.com') } + let(:new_created_integration) { Service.find_by(group: group) } context 'with an inherited integration' do diff --git a/spec/services/groups/update_shared_runners_service_spec.rb b/spec/services/groups/update_shared_runners_service_spec.rb index e2838c4ce0b..e941958eb8c 100644 --- a/spec/services/groups/update_shared_runners_service_spec.rb +++ b/spec/services/groups/update_shared_runners_service_spec.rb @@ -59,6 +59,7 @@ RSpec.describe Groups::UpdateSharedRunnersService do context 'disable shared Runners' do let_it_be(:group) { create(:group) } + let(:params) { { shared_runners_setting: 'disabled_and_unoverridable' } } it 'receives correct method and succeeds' do diff --git a/spec/services/ide/base_config_service_spec.rb b/spec/services/ide/base_config_service_spec.rb index debdc6e5809..ee57f2c18ec 100644 --- a/spec/services/ide/base_config_service_spec.rb +++ b/spec/services/ide/base_config_service_spec.rb @@ -5,6 +5,7 @@ require 'spec_helper' RSpec.describe Ide::BaseConfigService do let_it_be(:project) { create(:project, :repository) } let_it_be(:user) { create(:user) } + let(:sha) { 'sha' } describe '#execute' do diff --git a/spec/services/ide/schemas_config_service_spec.rb b/spec/services/ide/schemas_config_service_spec.rb index 19e5ca9e87d..69ad9b5cbea 100644 --- a/spec/services/ide/schemas_config_service_spec.rb +++ b/spec/services/ide/schemas_config_service_spec.rb @@ -5,6 +5,7 @@ require 'spec_helper' RSpec.describe Ide::SchemasConfigService do let_it_be(:project) { create(:project, :repository) } let_it_be(:user) { create(:user) } + let(:filename) { 'sample.yml' } let(:schema_content) { double(body: '{"title":"Sample schema"}') } diff --git a/spec/services/ide/terminal_config_service_spec.rb b/spec/services/ide/terminal_config_service_spec.rb index d6c4f7a2a69..483b6413be3 100644 --- a/spec/services/ide/terminal_config_service_spec.rb +++ b/spec/services/ide/terminal_config_service_spec.rb @@ -5,6 +5,7 @@ require 'spec_helper' RSpec.describe Ide::TerminalConfigService do let_it_be(:project) { create(:project, :repository) } let_it_be(:user) { create(:user) } + let(:sha) { 'sha' } describe '#execute' do @@ -47,6 +48,7 @@ RSpec.describe Ide::TerminalConfigService do terminal: { tag_list: [], yaml_variables: [], + job_variables: [], options: { script: ["sleep 60"] } }) end @@ -61,6 +63,7 @@ RSpec.describe Ide::TerminalConfigService do terminal: { tag_list: [], yaml_variables: [], + job_variables: [], options: { before_script: ["ls"], script: ["sleep 60"] } }) end diff --git a/spec/services/issuable/destroy_service_spec.rb b/spec/services/issuable/destroy_service_spec.rb index 8d62932f986..fa4902e5237 100644 --- a/spec/services/issuable/destroy_service_spec.rb +++ b/spec/services/issuable/destroy_service_spec.rb @@ -4,7 +4,8 @@ require 'spec_helper' RSpec.describe Issuable::DestroyService do let(:user) { create(:user) } - let(:project) { create(:project, :public) } + let(:group) { create(:group, :public) } + let(:project) { create(:project, :public, group: group) } subject(:service) { described_class.new(project, user) } @@ -22,17 +23,14 @@ RSpec.describe Issuable::DestroyService do service.execute(issue) end - it 'updates the todo caches for users with todos on the issue' do - create(:todo, target: issue, user: user, author: user, project: project) - - expect { service.execute(issue) } - .to change { user.todos_pending_count }.from(1).to(0) - end - it 'invalidates the issues count cache for the assignees' do expect_any_instance_of(User).to receive(:invalidate_cache_counts).once service.execute(issue) end + + it_behaves_like 'service deleting todos' do + let(:issuable) { issue } + end end context 'when issuable is a merge request' do @@ -53,11 +51,8 @@ RSpec.describe Issuable::DestroyService do service.execute(merge_request) end - it 'updates the todo caches for users with todos on the merge request' do - create(:todo, target: merge_request, user: user, author: user, project: project) - - expect { service.execute(merge_request) } - .to change { user.todos_pending_count }.from(1).to(0) + it_behaves_like 'service deleting todos' do + let(:issuable) { merge_request } end end end diff --git a/spec/services/issuable/process_assignees_spec.rb b/spec/services/issuable/process_assignees_spec.rb index 876c84957cc..45d57a1772a 100644 --- a/spec/services/issuable/process_assignees_spec.rb +++ b/spec/services/issuable/process_assignees_spec.rb @@ -4,10 +4,10 @@ require 'spec_helper' RSpec.describe Issuable::ProcessAssignees do describe '#execute' do - it 'returns assignee_ids when assignee_ids are specified' do + it 'returns assignee_ids when add_assignee_ids and remove_assignee_ids are not specified' do process = Issuable::ProcessAssignees.new(assignee_ids: %w(5 7 9), - add_assignee_ids: %w(2 4 6), - remove_assignee_ids: %w(4 7 11), + add_assignee_ids: nil, + remove_assignee_ids: nil, existing_assignee_ids: %w(1 3 9), extra_assignee_ids: %w(2 5 12)) result = process.execute @@ -15,19 +15,19 @@ RSpec.describe Issuable::ProcessAssignees do expect(result.sort).to eq(%w(5 7 9).sort) end - it 'combines other ids when assignee_ids is empty' do - process = Issuable::ProcessAssignees.new(assignee_ids: [], - add_assignee_ids: %w(2 4 6), - remove_assignee_ids: %w(4 7 11), + it 'combines other ids when assignee_ids is nil' do + process = Issuable::ProcessAssignees.new(assignee_ids: nil, + add_assignee_ids: nil, + remove_assignee_ids: nil, existing_assignee_ids: %w(1 3 11), extra_assignee_ids: %w(2 5 12)) result = process.execute - expect(result.sort).to eq(%w(1 2 3 5 6 12).sort) + expect(result.sort).to eq(%w(1 2 3 5 11 12).sort) end - it 'combines other ids when assignee_ids is nil' do - process = Issuable::ProcessAssignees.new(assignee_ids: nil, + it 'combines other ids when both add_assignee_ids and remove_assignee_ids are not empty' do + process = Issuable::ProcessAssignees.new(assignee_ids: %w(5 7 9), add_assignee_ids: %w(2 4 6), remove_assignee_ids: %w(4 7 11), existing_assignee_ids: %w(1 3 11), @@ -37,8 +37,8 @@ RSpec.describe Issuable::ProcessAssignees do expect(result.sort).to eq(%w(1 2 3 5 6 12).sort) end - it 'combines other ids when assignee_ids and add_assignee_ids are nil' do - process = Issuable::ProcessAssignees.new(assignee_ids: nil, + it 'combines other ids when remove_assignee_ids is not empty' do + process = Issuable::ProcessAssignees.new(assignee_ids: %w(5 7 9), add_assignee_ids: nil, remove_assignee_ids: %w(4 7 11), existing_assignee_ids: %w(1 3 11), @@ -48,8 +48,8 @@ RSpec.describe Issuable::ProcessAssignees do expect(result.sort).to eq(%w(1 2 3 5 12).sort) end - it 'combines other ids when assignee_ids and remove_assignee_ids are nil' do - process = Issuable::ProcessAssignees.new(assignee_ids: nil, + it 'combines other ids when add_assignee_ids is not empty' do + process = Issuable::ProcessAssignees.new(assignee_ids: %w(5 7 9), add_assignee_ids: %w(2 4 6), remove_assignee_ids: nil, existing_assignee_ids: %w(1 3 11), @@ -59,8 +59,8 @@ RSpec.describe Issuable::ProcessAssignees do expect(result.sort).to eq(%w(1 2 4 3 5 6 11 12).sort) end - it 'combines ids when only add_assignee_ids and remove_assignee_ids are passed' do - process = Issuable::ProcessAssignees.new(assignee_ids: nil, + it 'combines ids when existing_assignee_ids and extra_assignee_ids are omitted' do + process = Issuable::ProcessAssignees.new(assignee_ids: %w(5 7 9), add_assignee_ids: %w(2 4 6), remove_assignee_ids: %w(4 7 11)) result = process.execute diff --git a/spec/services/issue_links/create_service_spec.rb b/spec/services/issue_links/create_service_spec.rb index 873890d25cf..1bca717acb7 100644 --- a/spec/services/issue_links/create_service_spec.rb +++ b/spec/services/issue_links/create_service_spec.rb @@ -24,7 +24,7 @@ RSpec.describe IssueLinks::CreateService do end it 'returns error' do - is_expected.to eq(message: 'No Issue found for given params', status: :error, http_status: 404) + is_expected.to eq(message: 'No matching issue found. Make sure that you are adding a valid issue URL.', status: :error, http_status: 404) end end @@ -34,7 +34,7 @@ RSpec.describe IssueLinks::CreateService do end it 'returns error' do - is_expected.to eq(message: 'No Issue found for given params', status: :error, http_status: 404) + is_expected.to eq(message: 'No matching issue found. Make sure that you are adding a valid issue URL.', status: :error, http_status: 404) end it 'no relationship is created' do @@ -52,7 +52,7 @@ RSpec.describe IssueLinks::CreateService do it 'returns error' do target_issuable.project.add_guest(user) - is_expected.to eq(message: 'No Issue found for given params', status: :error, http_status: 404) + is_expected.to eq(message: 'No matching issue found. Make sure that you are adding a valid issue URL.', status: :error, http_status: 404) end it 'no relationship is created' do diff --git a/spec/services/issues/after_create_service_spec.rb b/spec/services/issues/after_create_service_spec.rb new file mode 100644 index 00000000000..bc9be3211d3 --- /dev/null +++ b/spec/services/issues/after_create_service_spec.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Issues::AfterCreateService do + include AfterNextHelpers + + let_it_be(:project) { create(:project) } + let_it_be(:current_user) { create(:user) } + let_it_be(:assignee) { create(:user) } + let_it_be(:milestone) { create(:milestone, project: project) } + let_it_be(:issue) { create(:issue, project: project, author: current_user, milestone: milestone, assignee_ids: [assignee.id]) } + + subject(:after_create_service) { described_class.new(project, current_user) } + + describe '#execute' do + it 'creates a pending todo for new assignee' do + attributes = { + project: project, + author: current_user, + user: assignee, + target_id: issue.id, + target_type: issue.class.name, + action: Todo::ASSIGNED, + state: :pending + } + + expect { after_create_service.execute(issue) }.to change { Todo.where(attributes).count }.by(1) + end + + it 'deletes milestone issues count cache' do + expect_next(Milestones::IssuesCountService, milestone) + .to receive(:delete_cache).and_call_original + + after_create_service.execute(issue) + end + + context 'with a regular issue' do + it_behaves_like 'does not track incident management event', :incident_management_incident_created do + subject { after_create_service.execute(issue) } + end + end + + context 'with an incident issue' do + let(:issue) { create(:issue, :incident, project: project, author: current_user) } + + it_behaves_like 'an incident management tracked event', :incident_management_incident_created do + subject { after_create_service.execute(issue) } + end + end + end +end diff --git a/spec/services/issues/build_service_spec.rb b/spec/services/issues/build_service_spec.rb index 16433d49ca1..80fe2474ecd 100644 --- a/spec/services/issues/build_service_spec.rb +++ b/spec/services/issues/build_service_spec.rb @@ -6,6 +6,7 @@ RSpec.describe Issues::BuildService do let_it_be(:project) { create(:project, :repository) } let_it_be(:developer) { create(:user) } let_it_be(:guest) { create(:user) } + let(:user) { developer } before_all do diff --git a/spec/services/issues/clone_service_spec.rb b/spec/services/issues/clone_service_spec.rb index 9ceb4ffeec5..44180a322ca 100644 --- a/spec/services/issues/clone_service_spec.rb +++ b/spec/services/issues/clone_service_spec.rb @@ -242,6 +242,7 @@ RSpec.describe Issues::CloneService do context 'issue with a design', :clean_gitlab_redis_shared_state do let_it_be(:new_project) { create(:project) } + let!(:design) { create(:design, :with_lfs_file, issue: old_issue) } let!(:note) { create(:diff_note_on_design, noteable: design, issue: old_issue, project: old_issue.project) } let(:subject) { clone_service.execute(old_issue, new_project) } diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index d548e5ee74a..83c6373c335 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -3,12 +3,15 @@ require 'spec_helper' RSpec.describe Issues::CreateService do + include AfterNextHelpers + let_it_be_with_reload(:project) { create(:project) } let_it_be(:user) { create(:user) } describe '#execute' do let_it_be(:assignee) { create(:user) } let_it_be(:milestone) { create(:milestone, project: project) } + let(:issue) { described_class.new(project, user, opts).execute } context 'when params are valid' do @@ -64,7 +67,6 @@ RSpec.describe Issues::CreateService do it_behaves_like 'incident issue' it_behaves_like 'has incident label' - it_behaves_like 'an incident management tracked event', :incident_management_incident_created it 'does create an incident label' do expect { subject } @@ -112,20 +114,6 @@ RSpec.describe Issues::CreateService do end end - it 'creates a pending todo for new assignee' do - attributes = { - project: project, - author: user, - user: assignee, - target_id: issue.id, - target_type: issue.class.name, - action: Todo::ASSIGNED, - state: :pending - } - - expect(Todo.where(attributes).count).to eq 1 - end - it 'moves the issue to the end, in an asynchronous worker' do expect(IssuePlacementWorker).to receive(:perform_async).with(be_nil, Integer) @@ -279,14 +267,6 @@ RSpec.describe Issues::CreateService do end end - it 'deletes milestone issues count cache' do - expect_next_instance_of(Milestones::IssuesCountService, milestone) do |service| - expect(service).to receive(:delete_cache).and_call_original - end - - issue - end - it 'schedules a namespace onboarding create action worker' do expect(Namespaces::OnboardingIssueCreatedWorker).to receive(:perform_async).with(project.namespace.id) @@ -458,7 +438,7 @@ RSpec.describe Issues::CreateService do end context 'checking spam' do - let(:request) { double(:request) } + let(:request) { double(:request, headers: nil) } let(:api) { true } let(:captcha_response) { 'abc123' } let(:spam_log_id) { 1 } diff --git a/spec/services/issues/export_csv_service_spec.rb b/spec/services/issues/export_csv_service_spec.rb index d199f825276..d04480bec18 100644 --- a/spec/services/issues/export_csv_service_spec.rb +++ b/spec/services/issues/export_csv_service_spec.rb @@ -8,6 +8,7 @@ RSpec.describe Issues::ExportCsvService do let_it_be(:project) { create(:project, :public, group: group) } let_it_be(:issue) { create(:issue, project: project, author: user) } let_it_be(:bad_issue) { create(:issue, project: project, author: user) } + subject { described_class.new(Issue.all, project) } it 'renders csv to string' do diff --git a/spec/services/issues/move_service_spec.rb b/spec/services/issues/move_service_spec.rb index eb124f07900..2f29a2e2022 100644 --- a/spec/services/issues/move_service_spec.rb +++ b/spec/services/issues/move_service_spec.rb @@ -206,6 +206,7 @@ RSpec.describe Issues::MoveService do context 'issue with a design', :clean_gitlab_redis_shared_state do let_it_be(:new_project) { create(:project) } + let!(:design) { create(:design, :with_lfs_file, issue: old_issue) } let!(:note) { create(:diff_note_on_design, noteable: design, issue: old_issue, project: old_issue.project) } let(:subject) { move_service.execute(old_issue, new_project) } diff --git a/spec/services/issues/related_branches_service_spec.rb b/spec/services/issues/related_branches_service_spec.rb index a8a1f95e800..c9c029bca4f 100644 --- a/spec/services/issues/related_branches_service_spec.rb +++ b/spec/services/issues/related_branches_service_spec.rb @@ -5,6 +5,7 @@ require 'spec_helper' RSpec.describe Issues::RelatedBranchesService do let_it_be(:developer) { create(:user) } let_it_be(:issue) { create(:issue) } + let(:user) { developer } subject { described_class.new(issue.project, user) } diff --git a/spec/services/keys/expiry_notification_service_spec.rb b/spec/services/keys/expiry_notification_service_spec.rb new file mode 100644 index 00000000000..1d1da179cf7 --- /dev/null +++ b/spec/services/keys/expiry_notification_service_spec.rb @@ -0,0 +1,97 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Keys::ExpiryNotificationService do + let_it_be_with_reload(:user) { create(:user) } + + let(:params) { { keys: user.keys, expiring_soon: expiring_soon } } + + subject { described_class.new(user, params) } + + shared_examples 'sends a notification' do + it do + perform_enqueued_jobs do + subject.execute + end + should_email(user) + end + end + + shared_examples 'uses notification service to send email to the user' do |notification_method| + it do + expect_next_instance_of(NotificationService) do |notification_service| + expect(notification_service).to receive(notification_method).with(key.user, [key.fingerprint]) + end + + subject.execute + end + end + + shared_examples 'does not send notification' do + it do + perform_enqueued_jobs do + subject.execute + end + should_not_email(user) + end + end + + shared_context 'block user' do + before do + user.block! + end + end + + context 'with key expiring today', :mailer do + let_it_be_with_reload(:key) { create(:key, expires_at: Time.current, user: user) } + + let(:expiring_soon) { false } + + context 'when user has permission to receive notification' do + it_behaves_like 'sends a notification' + + it_behaves_like 'uses notification service to send email to the user', :ssh_key_expired + + it 'updates notified column' do + expect { subject.execute }.to change { key.reload.expiry_notification_delivered_at } + end + end + + context 'when user does NOT have permission to receive notification' do + include_context 'block user' + + it_behaves_like 'does not send notification' + + it 'does not update notified column' do + expect { subject.execute }.not_to change { key.reload.expiry_notification_delivered_at } + end + end + end + + context 'with key expiring soon', :mailer do + let_it_be_with_reload(:key) { create(:key, expires_at: 3.days.from_now, user: user) } + + let(:expiring_soon) { true } + + context 'when user has permission to receive notification' do + it_behaves_like 'sends a notification' + + it_behaves_like 'uses notification service to send email to the user', :ssh_key_expiring_soon + + it 'updates notified column' do + expect { subject.execute }.to change { key.reload.before_expiry_notification_delivered_at } + end + end + + context 'when user does NOT have permission to receive notification' do + include_context 'block user' + + it_behaves_like 'does not send notification' + + it 'does not update notified column' do + expect { subject.execute }.not_to change { key.reload.before_expiry_notification_delivered_at } + end + end + end +end diff --git a/spec/services/members/create_service_spec.rb b/spec/services/members/create_service_spec.rb index 50efee9f43c..916941e1111 100644 --- a/spec/services/members/create_service_spec.rb +++ b/spec/services/members/create_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Members::CreateService, :clean_gitlab_redis_shared_state, :sidekiq_inline do +RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_shared_state, :sidekiq_inline do let_it_be(:source) { create(:project) } let_it_be(:user) { create(:user) } let_it_be(:member) { create(:user) } @@ -10,7 +10,7 @@ RSpec.describe Members::CreateService, :clean_gitlab_redis_shared_state, :sideki let_it_be(:access_level) { Gitlab::Access::GUEST } let(:params) { { user_ids: user_ids, access_level: access_level } } - subject(:execute_service) { described_class.new(user, params).execute(source) } + subject(:execute_service) { described_class.new(user, params.merge({ source: source })).execute } before do if source.is_a?(Project) diff --git a/spec/services/members/destroy_service_spec.rb b/spec/services/members/destroy_service_spec.rb index 4f731ad5852..b9f382d3cd8 100644 --- a/spec/services/members/destroy_service_spec.rb +++ b/spec/services/members/destroy_service_spec.rb @@ -289,60 +289,137 @@ RSpec.describe Members::DestroyService do let(:group_project) { create(:project, :public, group: group) } let(:control_project) { create(:project, group: subsubgroup) } - before do - create(:group_member, :developer, group: subsubgroup, user: member_user) - create(:project_member, :invited, project: group_project, created_by: member_user) - create(:group_member, :invited, group: group, created_by: member_user) - create(:project_member, :invited, project: subsubproject, created_by: member_user) - create(:group_member, :invited, group: subgroup, created_by: member_user) + context 'with memberships' do + before do + subgroup.add_developer(member_user) + subsubgroup.add_developer(member_user) + subsubproject.add_developer(member_user) + group_project.add_developer(member_user) + control_project.add_maintainer(user) + group.add_owner(user) + + @group_member = create(:group_member, :developer, group: group, user: member_user) + end - subsubproject.add_developer(member_user) - control_project.add_maintainer(user) - group.add_owner(user) + context 'with skipping of subresources' do + before do + described_class.new(user).execute(@group_member, skip_subresources: true) + end - group_member = create(:group_member, :developer, group: group, user: member_user) + it 'removes the group membership' do + expect(group.members.map(&:user)).not_to include(member_user) + end - described_class.new(user).execute(group_member) - end + it 'does not remove the project membership' do + expect(group_project.members.map(&:user)).to include(member_user) + end - it 'removes the project membership' do - expect(group_project.members.map(&:user)).not_to include(member_user) - end + it 'does not remove the subgroup membership' do + expect(subgroup.members.map(&:user)).to include(member_user) + end - it 'removes the group membership' do - expect(group.members.map(&:user)).not_to include(member_user) - end + it 'does not remove the subsubgroup membership' do + expect(subsubgroup.members.map(&:user)).to include(member_user) + end - it 'removes the subgroup membership' do - expect(subgroup.members.map(&:user)).not_to include(member_user) - end + it 'does not remove the subsubproject membership' do + expect(subsubproject.members.map(&:user)).to include(member_user) + end - it 'removes the subsubgroup membership' do - expect(subsubgroup.members.map(&:user)).not_to include(member_user) - end + it 'does not remove the user from the control project' do + expect(control_project.members.map(&:user)).to include(user) + end + end - it 'removes the subsubproject membership' do - expect(subsubproject.members.map(&:user)).not_to include(member_user) - end + context 'without skipping of subresources' do + before do + described_class.new(user).execute(@group_member, skip_subresources: false) + end - it 'does not remove the user from the control project' do - expect(control_project.members.map(&:user)).to include(user) - end + it 'removes the project membership' do + expect(group_project.members.map(&:user)).not_to include(member_user) + end - it 'removes group members invited by deleted user' do - expect(group.members.not_accepted_invitations_by_user(member_user)).to be_empty - end + it 'removes the group membership' do + expect(group.members.map(&:user)).not_to include(member_user) + end - it 'removes project members invited by deleted user' do - expect(group_project.members.not_accepted_invitations_by_user(member_user)).to be_empty - end + it 'removes the subgroup membership' do + expect(subgroup.members.map(&:user)).not_to include(member_user) + end + + it 'removes the subsubgroup membership' do + expect(subsubgroup.members.map(&:user)).not_to include(member_user) + end + + it 'removes the subsubproject membership' do + expect(subsubproject.members.map(&:user)).not_to include(member_user) + end - it 'removes subgroup members invited by deleted user' do - expect(subgroup.members.not_accepted_invitations_by_user(member_user)).to be_empty + it 'does not remove the user from the control project' do + expect(control_project.members.map(&:user)).to include(user) + end + end end - it 'removes subproject members invited by deleted user' do - expect(subsubproject.members.not_accepted_invitations_by_user(member_user)).to be_empty + context 'with invites' do + before do + create(:group_member, :developer, group: subsubgroup, user: member_user) + create(:project_member, :invited, project: group_project, created_by: member_user) + create(:group_member, :invited, group: group, created_by: member_user) + create(:project_member, :invited, project: subsubproject, created_by: member_user) + create(:group_member, :invited, group: subgroup, created_by: member_user) + + subsubproject.add_developer(member_user) + control_project.add_maintainer(user) + group.add_owner(user) + + @group_member = create(:group_member, :developer, group: group, user: member_user) + end + + context 'with skipping of subresources' do + before do + described_class.new(user).execute(@group_member, skip_subresources: true) + end + + it 'does not remove group members invited by deleted user' do + expect(group.members.not_accepted_invitations_by_user(member_user)).not_to be_empty + end + + it 'does not remove project members invited by deleted user' do + expect(group_project.members.not_accepted_invitations_by_user(member_user)).not_to be_empty + end + + it 'does not remove subgroup members invited by deleted user' do + expect(subgroup.members.not_accepted_invitations_by_user(member_user)).not_to be_empty + end + + it 'does not remove subproject members invited by deleted user' do + expect(subsubproject.members.not_accepted_invitations_by_user(member_user)).not_to be_empty + end + end + + context 'without skipping of subresources' do + before do + described_class.new(user).execute(@group_member, skip_subresources: false) + end + + it 'removes group members invited by deleted user' do + expect(group.members.not_accepted_invitations_by_user(member_user)).to be_empty + end + + it 'removes project members invited by deleted user' do + expect(group_project.members.not_accepted_invitations_by_user(member_user)).to be_empty + end + + it 'removes subgroup members invited by deleted user' do + expect(subgroup.members.not_accepted_invitations_by_user(member_user)).to be_empty + end + + it 'removes subproject members invited by deleted user' do + expect(subsubproject.members.not_accepted_invitations_by_user(member_user)).to be_empty + end + end end end diff --git a/spec/services/members/invite_service_spec.rb b/spec/services/members/invite_service_spec.rb index cced93896a5..d7fd7d5b2ca 100644 --- a/spec/services/members/invite_service_spec.rb +++ b/spec/services/members/invite_service_spec.rb @@ -2,29 +2,43 @@ require 'spec_helper' -RSpec.describe Members::InviteService, :aggregate_failures do +RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_shared_state, :sidekiq_inline do let_it_be(:project) { create(:project) } let_it_be(:user) { project.owner } let_it_be(:project_user) { create(:user) } + let_it_be(:namespace) { project.namespace } let(:params) { {} } - let(:base_params) { { access_level: Gitlab::Access::GUEST } } + let(:base_params) { { access_level: Gitlab::Access::GUEST, source: project } } - subject(:result) { described_class.new(user, base_params.merge(params)).execute(project) } + subject(:result) { described_class.new(user, base_params.merge(params) ).execute } - context 'when email is previously unused by current members' do + context 'when there is a valid member invited' do let(:params) { { email: 'email@example.org' } } it 'successfully creates a member' do - expect { result }.to change(ProjectMember, :count).by(1) + expect_to_create_members(count: 1) expect(result[:status]).to eq(:success) end + + it_behaves_like 'records an onboarding progress action', :user_added + end + + context 'when email is not a valid email' do + let(:params) { { email: '_bogus_' } } + + it 'returns an error' do + expect_not_to_create_members + expect(result[:message]['_bogus_']).to eq("Invite email is invalid") + end + + it_behaves_like 'does not record an onboarding progress action' end context 'when emails are passed as an array' do let(:params) { { email: %w[email@example.org email2@example.org] } } it 'successfully creates members' do - expect { result }.to change(ProjectMember, :count).by(2) + expect_to_create_members(count: 2) expect(result[:status]).to eq(:success) end end @@ -33,25 +47,25 @@ RSpec.describe Members::InviteService, :aggregate_failures do let(:params) { { email: '' } } it 'returns an error' do - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('Email cannot be blank') + expect_not_to_create_members + expect(result[:message]).to eq('Emails cannot be blank') end end context 'when email param is not included' do it 'returns an error' do - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('Email cannot be blank') + expect_not_to_create_members + expect(result[:message]).to eq('Emails cannot be blank') end end - context 'when email is not a valid email' do + context 'when email is not a valid email format' do let(:params) { { email: '_bogus_' } } it 'returns an error' do expect { result }.not_to change(ProjectMember, :count) expect(result[:status]).to eq(:error) - expect(result[:message]['_bogus_']).to eq("Invite email is invalid") + expect(result[:message][params[:email]]).to eq("Invite email is invalid") end end @@ -59,7 +73,7 @@ RSpec.describe Members::InviteService, :aggregate_failures do let(:params) { { email: 'email@example.org,email@example.org' } } it 'only creates one member per unique address' do - expect { result }.to change(ProjectMember, :count).by(1) + expect_to_create_members(count: 1) expect(result[:status]).to eq(:success) end end @@ -71,8 +85,7 @@ RSpec.describe Members::InviteService, :aggregate_failures do let(:params) { { email: emails } } it 'limits the number of emails to 100' do - expect { result }.not_to change(ProjectMember, :count) - expect(result[:status]).to eq(:error) + expect_not_to_create_members expect(result[:message]).to eq('Too many users specified (limit is 100)') end end @@ -81,8 +94,7 @@ RSpec.describe Members::InviteService, :aggregate_failures do let(:params) { { email: 'email@example.org,email2@example.org', limit: 1 } } it 'limits the number of emails to the limit supplied' do - expect { result }.not_to change(ProjectMember, :count) - expect(result[:status]).to eq(:error) + expect_not_to_create_members expect(result[:message]).to eq('Too many users specified (limit is 1)') end end @@ -91,7 +103,7 @@ RSpec.describe Members::InviteService, :aggregate_failures do let(:params) { { email: emails, limit: -1 } } it 'does not limit number of emails' do - expect { result }.to change(ProjectMember, :count).by(101) + expect_to_create_members(count: 101) expect(result[:status]).to eq(:success) end end @@ -101,7 +113,7 @@ RSpec.describe Members::InviteService, :aggregate_failures do let(:params) { { email: project_user.email } } it 'adds an existing user to members' do - expect { result }.to change(ProjectMember, :count).by(1) + expect_to_create_members(count: 1) expect(result[:status]).to eq(:success) expect(project.users).to include project_user end @@ -111,9 +123,9 @@ RSpec.describe Members::InviteService, :aggregate_failures do let(:params) { { email: project_user.email, access_level: -1 } } it 'returns an error' do - expect { result }.not_to change(ProjectMember, :count) - expect(result[:status]).to eq(:error) - expect(result[:message][project_user.email]).to eq("Access level is not included in the list") + expect_not_to_create_members + expect(result[:message][project_user.email]) + .to eq("Access level is not included in the list") end end @@ -122,9 +134,10 @@ RSpec.describe Members::InviteService, :aggregate_failures do let(:params) { { email: "#{invited_member.invite_email},#{project_user.email}" } } it 'adds new email and returns an error for the already invited email' do - expect { result }.to change(ProjectMember, :count).by(1) + expect_to_create_members(count: 1) expect(result[:status]).to eq(:error) - expect(result[:message][invited_member.invite_email]).to eq("Member already invited to #{project.name}") + expect(result[:message][invited_member.invite_email]) + .to eq("Invite email has already been taken") expect(project.users).to include project_user end end @@ -134,10 +147,10 @@ RSpec.describe Members::InviteService, :aggregate_failures do let(:params) { { email: "#{requested_member.user.email},#{project_user.email}" } } it 'adds new email and returns an error for the already invited email' do - expect { result }.to change(ProjectMember, :count).by(1) + expect_to_create_members(count: 1) expect(result[:status]).to eq(:error) expect(result[:message][requested_member.user.email]) - .to eq("Member cannot be invited because they already requested to join #{project.name}") + .to eq("User already exists in source") expect(project.users).to include project_user end end @@ -147,10 +160,20 @@ RSpec.describe Members::InviteService, :aggregate_failures do let(:params) { { email: "#{existing_member.user.email},#{project_user.email}" } } it 'adds new email and returns an error for the already invited email' do - expect { result }.to change(ProjectMember, :count).by(1) + expect_to_create_members(count: 1) expect(result[:status]).to eq(:error) - expect(result[:message][existing_member.user.email]).to eq("Already a member of #{project.name}") + expect(result[:message][existing_member.user.email]) + .to eq("User already exists in source") expect(project.users).to include project_user end end + + def expect_to_create_members(count:) + expect { result }.to change(ProjectMember, :count).by(count) + end + + def expect_not_to_create_members + expect { result }.not_to change(ProjectMember, :count) + expect(result[:status]).to eq(:error) + end end diff --git a/spec/services/merge_requests/add_todo_when_build_fails_service_spec.rb b/spec/services/merge_requests/add_todo_when_build_fails_service_spec.rb index 3c81ad6722d..6edaa91b8b2 100644 --- a/spec/services/merge_requests/add_todo_when_build_fails_service_spec.rb +++ b/spec/services/merge_requests/add_todo_when_build_fails_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe MergeRequests::AddTodoWhenBuildFailsService do +RSpec.describe ::MergeRequests::AddTodoWhenBuildFailsService do let(:user) { create(:user) } let(:project) { create(:project, :repository) } let(:sha) { '1234567890abcdef1234567890abcdef12345678' } @@ -24,8 +24,8 @@ RSpec.describe MergeRequests::AddTodoWhenBuildFailsService do before do allow_any_instance_of(MergeRequest) - .to receive(:head_pipeline) - .and_return(pipeline) + .to receive(:head_pipeline_id) + .and_return(pipeline.id) allow(service).to receive(:todo_service).and_return(todo_service) end diff --git a/spec/services/merge_requests/after_create_service_spec.rb b/spec/services/merge_requests/after_create_service_spec.rb index dce351d8a31..e1f28e32164 100644 --- a/spec/services/merge_requests/after_create_service_spec.rb +++ b/spec/services/merge_requests/after_create_service_spec.rb @@ -93,5 +93,109 @@ RSpec.describe MergeRequests::AfterCreateService do expect(merge_request.reload).to be_unchecked end end + + it 'increments the usage data counter of create event' do + counter = Gitlab::UsageDataCounters::MergeRequestCounter + + expect { execute_service }.to change { counter.read(:create) }.by(1) + end + + context 'with a milestone' do + let(:milestone) { create(:milestone, project: merge_request.target_project) } + + before do + merge_request.update!(milestone_id: milestone.id) + end + + it 'deletes the cache key for milestone merge request counter', :use_clean_rails_memory_store_caching do + expect_next_instance_of(Milestones::MergeRequestsCountService, milestone) do |service| + expect(service).to receive(:delete_cache).and_call_original + end + + execute_service + end + end + + context 'todos' do + it 'does not creates todos' do + attributes = { + project: merge_request.target_project, + target_id: merge_request.id, + target_type: merge_request.class.name + } + + expect { execute_service }.not_to change { Todo.where(attributes).count } + end + + context 'when merge request is assigned to someone' do + let_it_be(:assignee) { create(:user) } + let_it_be(:merge_request) { create(:merge_request, assignees: [assignee]) } + + it 'creates a todo for new assignee' do + attributes = { + project: merge_request.target_project, + author: merge_request.author, + user: assignee, + target_id: merge_request.id, + target_type: merge_request.class.name, + action: Todo::ASSIGNED, + state: :pending + } + + expect { execute_service }.to change { Todo.where(attributes).count }.by(1) + end + end + + context 'when reviewer is assigned' do + let_it_be(:reviewer) { create(:user) } + let_it_be(:merge_request) { create(:merge_request, reviewers: [reviewer]) } + + it 'creates a todo for new reviewer' do + attributes = { + project: merge_request.target_project, + author: merge_request.author, + user: reviewer, + target_id: merge_request.id, + target_type: merge_request.class.name, + action: Todo::REVIEW_REQUESTED, + state: :pending + } + + expect { execute_service }.to change { Todo.where(attributes).count }.by(1) + end + end + end + + context 'when saving references to issues that the created merge request closes' do + let_it_be(:first_issue) { create(:issue, project: merge_request.target_project) } + let_it_be(:second_issue) { create(:issue, project: merge_request.target_project) } + + it 'creates a `MergeRequestsClosingIssues` record for each issue' do + merge_request.description = "Closes #{first_issue.to_reference} and #{second_issue.to_reference}" + merge_request.source_branch = "feature" + merge_request.target_branch = merge_request.target_project.default_branch + merge_request.save! + + execute_service + + issue_ids = MergeRequestsClosingIssues.where(merge_request: merge_request).pluck(:issue_id) + expect(issue_ids).to match_array([first_issue.id, second_issue.id]) + end + end + + it 'tracks merge request creation in usage data' do + expect(Gitlab::UsageDataCounters::MergeRequestCounter).to receive(:count).with(:create) + + execute_service + end + + it 'calls MergeRequests::LinkLfsObjectsService#execute' do + service = instance_spy(MergeRequests::LinkLfsObjectsService) + allow(MergeRequests::LinkLfsObjectsService).to receive(:new).with(merge_request.target_project).and_return(service) + + execute_service + + expect(service).to have_received(:execute).with(merge_request) + end end end diff --git a/spec/services/merge_requests/base_service_spec.rb b/spec/services/merge_requests/base_service_spec.rb index 83431105545..d8ba2bc43fb 100644 --- a/spec/services/merge_requests/base_service_spec.rb +++ b/spec/services/merge_requests/base_service_spec.rb @@ -6,6 +6,7 @@ RSpec.describe MergeRequests::BaseService do include ProjectForksHelper let_it_be(:project) { create(:project, :repository) } + let(:title) { 'Awesome merge_request' } let(:params) do { diff --git a/spec/services/merge_requests/create_pipeline_service_spec.rb b/spec/services/merge_requests/create_pipeline_service_spec.rb index 4dd70627977..3e2e940dc24 100644 --- a/spec/services/merge_requests/create_pipeline_service_spec.rb +++ b/spec/services/merge_requests/create_pipeline_service_spec.rb @@ -7,6 +7,7 @@ RSpec.describe MergeRequests::CreatePipelineService do let_it_be(:project, reload: true) { create(:project, :repository) } let_it_be(:user) { create(:user) } + let(:service) { described_class.new(project, actor, params) } let(:actor) { user } let(:params) { {} } @@ -50,6 +51,7 @@ RSpec.describe MergeRequests::CreatePipelineService do context 'with fork merge request' do let_it_be(:forked_project) { fork_project(project, nil, repository: true, target_project: create(:project, :private, :repository)) } + let(:source_project) { forked_project } context 'when actor has permission to create pipelines in target project' do diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb index 4f47a22b07c..f2bc55103f0 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -47,16 +47,6 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do .to change { project.open_merge_requests_count }.from(0).to(1) end - it 'does not creates todos' do - attributes = { - project: project, - target_id: merge_request.id, - target_type: merge_request.class.name - } - - expect(Todo.where(attributes).count).to be_zero - end - it 'creates exactly 1 create MR event', :sidekiq_might_not_need_inline do attributes = { action: :created, @@ -67,6 +57,10 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do expect(Event.where(attributes).count).to eq(1) end + it 'sets the merge_status to preparing' do + expect(merge_request.reload).to be_preparing + end + describe 'when marked with /wip' do context 'in title and in description' do let(:opts) do @@ -113,20 +107,6 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do end it { expect(merge_request.assignees).to eq([user2]) } - - it 'creates a todo for new assignee' do - attributes = { - project: project, - author: user, - user: user2, - target_id: merge_request.id, - target_type: merge_request.class.name, - action: Todo::ASSIGNED, - state: :pending - } - - expect(Todo.where(attributes).count).to eq 1 - end end context 'when reviewer is assigned' do @@ -142,20 +122,6 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do it { expect(merge_request.reviewers).to eq([user2]) } - it 'creates a todo for new reviewer' do - attributes = { - project: project, - author: user, - user: user2, - target_id: merge_request.id, - target_type: merge_request.class.name, - action: Todo::REVIEW_REQUESTED, - state: :pending - } - - expect(Todo.where(attributes).count).to eq 1 - end - it 'invalidates counter cache for reviewers', :use_clean_rails_memory_store_caching do expect { merge_request } .to change { user2.review_requested_open_merge_requests_count } @@ -328,12 +294,6 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do end end - it 'increments the usage data counter of create event' do - counter = Gitlab::UsageDataCounters::MergeRequestCounter - - expect { service.execute }.to change { counter.read(:create) }.by(1) - end - context 'after_save callback to store_mentions' do let(:labels) { create_pair(:label, project: project) } let(:milestone) { create(:milestone, project: project) } @@ -494,35 +454,6 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do end end - context 'while saving references to issues that the created merge request closes' do - let(:first_issue) { create(:issue, project: project) } - let(:second_issue) { create(:issue, project: project) } - - let(:opts) do - { - title: 'Awesome merge_request', - source_branch: 'feature', - target_branch: 'master', - force_remove_source_branch: '1' - } - end - - before do - project.add_maintainer(user) - project.add_developer(user2) - end - - it 'creates a `MergeRequestsClosingIssues` record for each issue' do - issue_closing_opts = opts.merge(description: "Closes #{first_issue.to_reference} and #{second_issue.to_reference}") - service = described_class.new(project, user, issue_closing_opts) - allow(service).to receive(:execute_hooks) - merge_request = service.execute - - issue_ids = MergeRequestsClosingIssues.where(merge_request: merge_request).pluck(:issue_id) - expect(issue_ids).to match_array([first_issue.id, second_issue.id]) - end - end - context 'when source and target projects are different' do let(:target_project) { fork_project(project, nil, repository: true) } @@ -571,14 +502,6 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do expect(merge_request).to be_persisted end - it 'calls MergeRequests::LinkLfsObjectsService#execute', :sidekiq_might_not_need_inline do - expect_next_instance_of(MergeRequests::LinkLfsObjectsService) do |service| - expect(service).to receive(:execute).with(instance_of(MergeRequest)) - end - - described_class.new(project, user, opts).execute - end - it 'does not create the merge request when the target project is archived' do target_project.update!(archived: true) diff --git a/spec/services/merge_requests/export_csv_service_spec.rb b/spec/services/merge_requests/export_csv_service_spec.rb index 4ce032c396e..97217e979a5 100644 --- a/spec/services/merge_requests/export_csv_service_spec.rb +++ b/spec/services/merge_requests/export_csv_service_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' RSpec.describe MergeRequests::ExportCsvService do let_it_be(:merge_request) { create(:merge_request) } + let(:csv) { CSV.parse(subject.csv_data, headers: true).first } subject { described_class.new(MergeRequest.where(id: merge_request.id), merge_request.project) } @@ -46,6 +47,7 @@ RSpec.describe MergeRequests::ExportCsvService do describe 'approvers' do context 'when approved' do let_it_be(:merge_request) { create(:merge_request) } + let(:approvers) { create_list(:user, 2) } before do diff --git a/spec/services/merge_requests/handle_assignees_change_service_spec.rb b/spec/services/merge_requests/handle_assignees_change_service_spec.rb new file mode 100644 index 00000000000..cc595aab04b --- /dev/null +++ b/spec/services/merge_requests/handle_assignees_change_service_spec.rb @@ -0,0 +1,114 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::HandleAssigneesChangeService do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { create(:user) } + let_it_be(:assignee) { create(:user) } + let_it_be(:merge_request) { create(:merge_request, author: user, source_project: project, assignees: [assignee]) } + let_it_be(:old_assignees) { create_list(:user, 3) } + + let(:options) { {} } + let(:service) { described_class.new(project, user) } + + before_all do + project.add_maintainer(user) + project.add_developer(assignee) + + old_assignees.each do |old_assignee| + project.add_developer(old_assignee) + end + end + + describe '#async_execute' do + def async_execute + service.async_execute(merge_request, old_assignees, options) + end + + it 'performs MergeRequests::HandleAssigneesChangeWorker asynchronously' do + expect(MergeRequests::HandleAssigneesChangeWorker) + .to receive(:perform_async) + .with( + merge_request.id, + user.id, + old_assignees.map(&:id), + options + ) + + async_execute + end + + context 'when async_handle_merge_request_assignees_change feature is disabled' do + before do + stub_feature_flags(async_handle_merge_request_assignees_change: false) + end + + it 'calls #execute' do + expect(service).to receive(:execute).with(merge_request, old_assignees, options) + + async_execute + end + end + end + + describe '#execute' do + def execute + service.execute(merge_request, old_assignees, options) + end + + it 'creates assignee note' do + execute + + note = merge_request.notes.last + + expect(note).not_to be_nil + expect(note.note).to include "assigned to #{assignee.to_reference} and unassigned #{old_assignees.map(&:to_reference).to_sentence}" + end + + it 'sends email notifications to old and new assignees', :mailer, :sidekiq_inline do + perform_enqueued_jobs do + execute + end + + should_email(assignee) + old_assignees.each do |old_assignee| + should_email(old_assignee) + end + end + + it 'creates pending todo for assignee' do + execute + + todo = assignee.todos.last + + expect(todo).to be_pending + end + + it 'tracks users assigned event' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .to receive(:track_users_assigned_to_mr).once.with(users: [assignee]) + + execute + end + + it 'tracks assignees changed event' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .to receive(:track_assignees_changed_action).once.with(user: user) + + execute + end + + context 'when execute_hooks option is set to true' do + let(:options) { { execute_hooks: true } } + + it 'execute hooks and services' do + expect(merge_request.project).to receive(:execute_hooks).with(anything, :merge_request_hooks) + expect(merge_request.project).to receive(:execute_services).with(anything, :merge_request_hooks) + expect(service).to receive(:enqueue_jira_connect_messages_for).with(merge_request) + + execute + end + end + end +end diff --git a/spec/services/merge_requests/merge_orchestration_service_spec.rb b/spec/services/merge_requests/merge_orchestration_service_spec.rb index 67dbb5a1a01..da37cc97857 100644 --- a/spec/services/merge_requests/merge_orchestration_service_spec.rb +++ b/spec/services/merge_requests/merge_orchestration_service_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' RSpec.describe MergeRequests::MergeOrchestrationService do let_it_be(:maintainer) { create(:user) } + let(:merge_params) { { sha: merge_request.diff_head_sha } } let(:user) { maintainer } let(:service) { described_class.new(project, user, merge_params) } diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index 87e5750ce6e..c73cbad9d2f 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -5,6 +5,7 @@ require 'spec_helper' RSpec.describe MergeRequests::MergeService do let_it_be(:user) { create(:user) } let_it_be(:user2) { create(:user) } + let(:merge_request) { create(:merge_request, :simple, author: user2, assignees: [user2]) } let(:project) { merge_request.project } @@ -166,20 +167,6 @@ RSpec.describe MergeRequests::MergeService do service.execute(merge_request) end - context 'when jira_issue_transition_id is not present' do - before do - allow_any_instance_of(JIRA::Resource::Issue).to receive(:resolution).and_return(nil) - end - - it 'does not close issue' do - jira_tracker.update!(jira_issue_transition_id: nil) - - expect_any_instance_of(JiraService).not_to receive(:transition_issue) - - service.execute(merge_request) - end - end - context 'wrong issue markdown' do it 'does not close issues on Jira issue tracker' do jira_issue = ExternalIssue.new('#JIRA-123', project) diff --git a/spec/services/merge_requests/merge_to_ref_service_spec.rb b/spec/services/merge_requests/merge_to_ref_service_spec.rb index 14ef5b0b772..938165a807c 100644 --- a/spec/services/merge_requests/merge_to_ref_service_spec.rb +++ b/spec/services/merge_requests/merge_to_ref_service_spec.rb @@ -68,6 +68,7 @@ RSpec.describe MergeRequests::MergeToRefService do end let_it_be(:user) { create(:user) } + let(:merge_request) { create(:merge_request, :simple) } let(:project) { merge_request.project } @@ -226,6 +227,7 @@ RSpec.describe MergeRequests::MergeToRefService do describe 'cascading merge refs' do let_it_be(:project) { create(:project, :repository) } + let(:params) { { commit_message: 'Cascading merge', first_parent_ref: first_parent_ref, target_ref: target_ref, sha: merge_request.diff_head_sha } } context 'when first merge happens' do @@ -257,8 +259,9 @@ RSpec.describe MergeRequests::MergeToRefService do let(:params) { { allow_conflicts: true } } it 'calls merge_to_ref with allow_conflicts param' do - expect(project.repository).to receive(:merge_to_ref) - .with(anything, anything, anything, anything, anything, anything, true) + expect(project.repository).to receive(:merge_to_ref) do |user, **kwargs| + expect(kwargs[:allow_conflicts]).to eq(true) + end.and_call_original service.execute(merge_request) end diff --git a/spec/services/merge_requests/push_options_handler_service_spec.rb b/spec/services/merge_requests/push_options_handler_service_spec.rb index c2769d4fa88..b5086ea3a82 100644 --- a/spec/services/merge_requests/push_options_handler_service_spec.rb +++ b/spec/services/merge_requests/push_options_handler_service_spec.rb @@ -6,10 +6,12 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do include ProjectForksHelper let_it_be(:project) { create(:project, :public, :repository) } - let_it_be(:user) { create(:user, developer_projects: [project]) } - let_it_be(:forked_project) { fork_project(project, user, repository: true) } + let_it_be(:user1) { create(:user, developer_projects: [project]) } + let_it_be(:user2) { create(:user, developer_projects: [project]) } + let_it_be(:user3) { create(:user, developer_projects: [project]) } + let_it_be(:forked_project) { fork_project(project, user1, repository: true) } - let(:service) { described_class.new(project, user, changes, push_options) } + let(:service) { described_class.new(project, user1, changes, push_options) } let(:source_branch) { 'fix' } let(:target_branch) { 'feature' } let(:title) { 'my title' } @@ -23,32 +25,8 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do let(:default_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{project.default_branch}" } let(:error_mr_required) { "A merge_request.create push option is required to create a merge request for branch #{source_branch}" } - shared_examples_for 'a service that can create a merge request' do - subject(:last_mr) { MergeRequest.last } - - it 'creates a merge request with the correct target branch and assigned user' do - branch = push_options[:target] || project.default_branch - - expect { service.execute }.to change { MergeRequest.count }.by(1) - expect(last_mr.target_branch).to eq(branch) - expect(last_mr.assignees).to contain_exactly(user) - end - - context 'when project has been forked', :sidekiq_might_not_need_inline do - let(:forked_project) { fork_project(project, user, repository: true) } - let(:service) { described_class.new(forked_project, user, changes, push_options) } - - before do - allow(forked_project).to receive(:empty_repo?).and_return(false) - end - - it 'sets the correct source and target project' do - service.execute - - expect(last_mr.source_project).to eq(forked_project) - expect(last_mr.target_project).to eq(project) - end - end + before do + stub_licensed_features(multiple_merge_request_assignees: false) end shared_examples_for 'a service that can set the target of a merge request' do @@ -91,7 +69,7 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do expect(last_mr.auto_merge_enabled).to eq(true) expect(last_mr.auto_merge_strategy).to eq(AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS) - expect(last_mr.merge_user).to eq(user) + expect(last_mr.merge_user).to eq(user1) expect(last_mr.merge_params['sha']).to eq(change[:newrev]) end end @@ -116,12 +94,6 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do end end - shared_examples_for 'a service that does not create a merge request' do - it do - expect { service.execute }.not_to change { MergeRequest.count } - end - end - shared_examples_for 'a service that does not update a merge request' do it do expect { service.execute }.not_to change { MergeRequest.maximum(:updated_at) } @@ -133,6 +105,18 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do include_examples 'a service that does not update a merge request' end + shared_examples 'with a deleted branch' do + let(:changes) { deleted_branch_changes } + + it_behaves_like 'a service that does nothing' + end + + shared_examples 'with the project default branch' do + let(:changes) { default_branch_changes } + + it_behaves_like 'a service that does nothing' + end + describe '`create` push option' do let(:push_options) { { create: true } } @@ -155,17 +139,8 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that does not create a merge request' end - context 'with a deleted branch' do - let(:changes) { deleted_branch_changes } - - it_behaves_like 'a service that does nothing' - end - - context 'with the project default branch' do - let(:changes) { default_branch_changes } - - it_behaves_like 'a service that does nothing' - end + it_behaves_like 'with a deleted branch' + it_behaves_like 'with the project default branch' end describe '`merge_when_pipeline_succeeds` push option' do @@ -217,17 +192,8 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that can set the merge request to merge when pipeline succeeds' end - context 'with a deleted branch' do - let(:changes) { deleted_branch_changes } - - it_behaves_like 'a service that does nothing' - end - - context 'with the project default branch' do - let(:changes) { default_branch_changes } - - it_behaves_like 'a service that does nothing' - end + it_behaves_like 'with a deleted branch' + it_behaves_like 'with the project default branch' end describe '`remove_source_branch` push option' do @@ -239,11 +205,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that does not create a merge request' it 'adds an error to the service' do - error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" - service.execute - expect(service.errors).to include(error) + expect(service.errors).to include(error_mr_required) end context 'when coupled with the `create` push option' do @@ -281,17 +245,8 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that can remove the source branch when it is merged' end - context 'with a deleted branch' do - let(:changes) { deleted_branch_changes } - - it_behaves_like 'a service that does nothing' - end - - context 'with the project default branch' do - let(:changes) { default_branch_changes } - - it_behaves_like 'a service that does nothing' - end + it_behaves_like 'with a deleted branch' + it_behaves_like 'with the project default branch' end describe '`target` push option' do @@ -343,17 +298,8 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that can set the target of a merge request' end - context 'with a deleted branch' do - let(:changes) { deleted_branch_changes } - - it_behaves_like 'a service that does nothing' - end - - context 'with the project default branch' do - let(:changes) { default_branch_changes } - - it_behaves_like 'a service that does nothing' - end + it_behaves_like 'with a deleted branch' + it_behaves_like 'with the project default branch' end describe '`title` push option' do @@ -405,17 +351,8 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that can set the title of a merge request' end - context 'with a deleted branch' do - let(:changes) { deleted_branch_changes } - - it_behaves_like 'a service that does nothing' - end - - context 'with the project default branch' do - let(:changes) { default_branch_changes } - - it_behaves_like 'a service that does nothing' - end + it_behaves_like 'with a deleted branch' + it_behaves_like 'with the project default branch' end describe '`description` push option' do @@ -467,17 +404,8 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that can set the description of a merge request' end - context 'with a deleted branch' do - let(:changes) { deleted_branch_changes } - - it_behaves_like 'a service that does nothing' - end - - context 'with the project default branch' do - let(:changes) { default_branch_changes } - - it_behaves_like 'a service that does nothing' - end + it_behaves_like 'with a deleted branch' + it_behaves_like 'with the project default branch' end describe '`label` push option' do @@ -529,17 +457,8 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that can change labels of a merge request', 2 end - context 'with a deleted branch' do - let(:changes) { deleted_branch_changes } - - it_behaves_like 'a service that does nothing' - end - - context 'with the project default branch' do - let(:changes) { default_branch_changes } - - it_behaves_like 'a service that does nothing' - end + it_behaves_like 'with a deleted branch' + it_behaves_like 'with the project default branch' end describe '`unlabel` push option' do @@ -551,11 +470,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that does not create a merge request' it 'adds an error to the service' do - error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" - service.execute - expect(service.errors).to include(error) + expect(service.errors).to include(error_mr_required) end context 'when coupled with the `create` push option' do @@ -572,11 +489,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that does not create a merge request' it 'adds an error to the service' do - error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" - service.execute - expect(service.errors).to include(error) + expect(service.errors).to include(error_mr_required) end context 'when coupled with the `create` push option' do @@ -595,17 +510,42 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that can change labels of a merge request', 1 end - context 'with a deleted branch' do - let(:changes) { deleted_branch_changes } + it_behaves_like 'with a deleted branch' + it_behaves_like 'with the project default branch' + end + + shared_examples 'with an existing branch that has a merge request open in foss' do + let(:changes) { existing_branch_changes } + let!(:merge_request) { create(:merge_request, source_project: project, source_branch: source_branch)} - it_behaves_like 'a service that does nothing' - end + it_behaves_like 'a service that does not create a merge request' + it_behaves_like 'a service that can change assignees of a merge request', 1 + end - context 'with the project default branch' do - let(:changes) { default_branch_changes } + describe '`assign` push option' do + let(:assigned) { { user2.id => 1, user3.id => 1 } } + let(:unassigned) { nil } + let(:push_options) { { assign: assigned, unassign: unassigned } } - it_behaves_like 'a service that does nothing' - end + it_behaves_like 'with a new branch', 1 + it_behaves_like 'with an existing branch but no open MR', 1 + it_behaves_like 'with an existing branch that has a merge request open in foss' + + it_behaves_like 'with a deleted branch' + it_behaves_like 'with the project default branch' + end + + describe '`unassign` push option' do + let(:assigned) { { user2.id => 1, user3.id => 1 } } + let(:unassigned) { { user1.id => 1, user3.id => 1 } } + let(:push_options) { { assign: assigned, unassign: unassigned } } + + it_behaves_like 'with a new branch', 1 + it_behaves_like 'with an existing branch but no open MR', 1 + it_behaves_like 'with an existing branch that has a merge request open in foss' + + it_behaves_like 'with a deleted branch' + it_behaves_like 'with the project default branch' end describe 'multiple pushed branches' do @@ -645,7 +585,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do end describe 'no user' do - let(:user) { nil } + let(:user1) { nil } + let(:user2) { nil } + let(:user3) { nil } let(:push_options) { { create: true } } let(:changes) { new_branch_changes } @@ -661,7 +603,7 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do let(:changes) { new_branch_changes } it 'records an error' do - Members::DestroyService.new(user).execute(ProjectMember.find_by!(user_id: user.id)) + Members::DestroyService.new(user1).execute(ProjectMember.find_by!(user_id: user1.id)) service.execute @@ -707,7 +649,7 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do end describe 'when MRs are not enabled' do - let(:project) { create(:project, :public, :repository).tap { |pr| pr.add_developer(user) } } + let(:project) { create(:project, :public, :repository).tap { |pr| pr.add_developer(user1) } } let(:push_options) { { create: true } } let(:changes) { new_branch_changes } diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index 2abe7a23bfe..f9b76db877b 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -198,7 +198,7 @@ RSpec.describe MergeRequests::RefreshService do end end - describe 'Pipelines for merge requests' do + shared_examples 'Pipelines for merge requests' do before do stub_ci_pipeline_yaml_file(config) end @@ -256,7 +256,7 @@ RSpec.describe MergeRequests::RefreshService do stub_feature_flags(ci_disallow_to_create_merge_request_pipelines_in_target_project: false) end - it 'creates detached merge request pipeline for fork merge request', :sidekiq_inline do + it 'creates detached merge request pipeline for fork merge request' do expect { subject } .to change { @fork_merge_request.pipelines_for_merge_request.count }.by(1) @@ -364,6 +364,18 @@ RSpec.describe MergeRequests::RefreshService do end end + context 'when the code_review_async_pipeline_creation feature flag is on', :sidekiq_inline do + it_behaves_like 'Pipelines for merge requests' + end + + context 'when the code_review_async_pipeline_creation feature flag is off', :sidekiq_inline do + before do + stub_feature_flags(code_review_async_pipeline_creation: false) + end + + it_behaves_like 'Pipelines for merge requests' + end + context 'push to origin repo source branch' do let(:refresh_service) { service.new(@project, @user) } let(:notification_service) { spy('notification_service') } diff --git a/spec/services/merge_requests/resolve_todos_service_spec.rb b/spec/services/merge_requests/resolve_todos_service_spec.rb new file mode 100644 index 00000000000..3e6f2ea3f5d --- /dev/null +++ b/spec/services/merge_requests/resolve_todos_service_spec.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::ResolveTodosService do + let_it_be(:merge_request) { create(:merge_request) } + let_it_be(:user) { create(:user) } + + let(:service) { described_class.new(merge_request, user) } + + describe '#async_execute' do + def async_execute + service.async_execute + end + + it 'performs MergeRequests::ResolveTodosWorker asynchronously' do + expect(MergeRequests::ResolveTodosWorker) + .to receive(:perform_async) + .with( + merge_request.id, + user.id + ) + + async_execute + end + + context 'when resolve_merge_request_todos_async feature is disabled' do + before do + stub_feature_flags(resolve_merge_request_todos_async: false) + end + + it 'calls #execute' do + expect(service).to receive(:execute) + + async_execute + end + end + end + + describe '#execute' do + it 'marks pending todo as done' do + pending_todo = create(:todo, :pending, user: user, project: merge_request.project, target: merge_request) + + service.execute + + expect(pending_todo.reload).to be_done + end + end +end diff --git a/spec/services/merge_requests/update_assignees_service_spec.rb b/spec/services/merge_requests/update_assignees_service_spec.rb new file mode 100644 index 00000000000..de03aab5418 --- /dev/null +++ b/spec/services/merge_requests/update_assignees_service_spec.rb @@ -0,0 +1,79 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::UpdateAssigneesService do + include AfterNextHelpers + + let_it_be(:group) { create(:group, :public) } + let_it_be(:project) { create(:project, :private, :repository, group: group) } + let_it_be(:user) { create(:user) } + let_it_be(:user2) { create(:user) } + let_it_be(:user3) { create(:user) } + + let_it_be_with_reload(:merge_request) do + create(:merge_request, :simple, :unique_branches, + title: 'Old title', + description: "FYI #{user2.to_reference}", + assignee_ids: [user3.id], + source_project: project, + author: create(:user)) + end + + before do + project.add_maintainer(user) + project.add_developer(user2) + project.add_developer(user3) + end + + let(:service) { described_class.new(project, user, opts) } + let(:opts) { { assignee_ids: [user2.id] } } + + describe 'execute' do + def update_merge_request + service.execute(merge_request) + merge_request.reload + end + + context 'when the parameters are valid' do + it 'updates the MR, and queues the more expensive work for later' do + expect_next(MergeRequests::HandleAssigneesChangeService, project, user) do |service| + expect(service) + .to receive(:async_execute) + .with(merge_request, [user3], execute_hooks: true) + end + + expect { update_merge_request } + .to change(merge_request, :assignees).to([user2]) + .and change(merge_request, :updated_at) + .and change(merge_request, :updated_by).to(user) + end + + it 'does not update the assignees if they do not have access' do + opts[:assignee_ids] = [create(:user).id] + + expect { update_merge_request }.not_to change(merge_request, :assignee_ids) + end + + it 'is more efficient than using the full update-service' do + allow_next(MergeRequests::HandleAssigneesChangeService, project, user) do |service| + expect(service) + .to receive(:async_execute) + .with(merge_request, [user3], execute_hooks: true) + end + + other_mr = create(:merge_request, :simple, :unique_branches, + title: merge_request.title, + description: merge_request.description, + assignee_ids: merge_request.assignee_ids, + source_project: merge_request.project, + author: merge_request.author) + + update_service = ::MergeRequests::UpdateService.new(project, user, opts) + + expect { service.execute(merge_request) } + .to issue_fewer_queries_than { update_service.execute(other_mr) } + end + end + end +end diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 7a7f684c6d0..8c010855eb2 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -205,30 +205,6 @@ RSpec.describe MergeRequests::UpdateService, :mailer do MergeRequests::UpdateService.new(project, user, opts).execute(merge_request) end - context 'assignees' do - context 'when assignees changed' do - it 'tracks assignees changed event' do - expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) - .to receive(:track_assignees_changed_action).once.with(user: user) - - opts[:assignees] = [user2] - - MergeRequests::UpdateService.new(project, user, opts).execute(merge_request) - end - end - - context 'when assignees did not change' do - it 'does not track assignees changed event' do - expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) - .not_to receive(:track_assignees_changed_action) - - opts[:assignees] = merge_request.assignees - - MergeRequests::UpdateService.new(project, user, opts).execute(merge_request) - end - end - end - context 'reviewers' do context 'when reviewers changed' do it 'tracks reviewers changed event' do @@ -272,6 +248,41 @@ RSpec.describe MergeRequests::UpdateService, :mailer do it_behaves_like 'updates milestone' end + + context 'milestone counters cache reset' do + let(:milestone_old) { create(:milestone, project: project) } + let(:opts) { { milestone: milestone_old } } + + it 'deletes milestone counters' do + expect_next_instance_of(Milestones::MergeRequestsCountService, milestone_old) do |service| + expect(service).to receive(:delete_cache).and_call_original + end + + expect_next_instance_of(Milestones::MergeRequestsCountService, milestone) do |service| + expect(service).to receive(:delete_cache).and_call_original + end + + update_merge_request(milestone: milestone) + end + + it 'deletes milestone counters when the milestone is removed' do + expect_next_instance_of(Milestones::MergeRequestsCountService, milestone_old) do |service| + expect(service).to receive(:delete_cache).and_call_original + end + + update_merge_request(milestone: nil) + end + + it 'deletes milestone counters when the milestone was not set' do + update_merge_request(milestone: nil) + + expect_next_instance_of(Milestones::MergeRequestsCountService, milestone) do |service| + expect(service).to receive(:delete_cache).and_call_original + end + + update_merge_request(milestone: milestone) + end + end end it 'executes hooks with update action' do @@ -291,21 +302,6 @@ RSpec.describe MergeRequests::UpdateService, :mailer do ) end - it 'sends email to user2 about assign of new merge request and email to user3 about merge request unassignment', :sidekiq_might_not_need_inline do - deliveries = ActionMailer::Base.deliveries - email = deliveries.last - recipients = deliveries.last(2).flat_map(&:to) - expect(recipients).to include(user2.email, user3.email) - expect(email.subject).to include(merge_request.title) - end - - it 'creates system note about merge_request reassign' do - note = find_note('assigned to') - - expect(note).not_to be_nil - expect(note.note).to include "assigned to #{user.to_reference} and unassigned #{user3.to_reference}" - end - context 'with reviewers' do let(:opts) { { reviewer_ids: [user2.id] } } @@ -594,62 +590,54 @@ RSpec.describe MergeRequests::UpdateService, :mailer do let!(:pending_todo) { create(:todo, :assigned, user: user, project: project, target: merge_request, author: user2) } context 'when the title change' do - before do - update_merge_request({ title: 'New title' }) - end + it 'calls MergeRequest::ResolveTodosService#async_execute' do + expect_next_instance_of(MergeRequests::ResolveTodosService, merge_request, user) do |service| + expect(service).to receive(:async_execute) + end - it 'marks pending todos as done' do - expect(pending_todo.reload).to be_done + update_merge_request({ title: 'New title' }) end it 'does not create any new todos' do + update_merge_request({ title: 'New title' }) + expect(Todo.count).to eq(1) end end context 'when the description change' do - before do - update_merge_request({ description: "Also please fix #{user2.to_reference} #{user3.to_reference}" }) - end + it 'calls MergeRequest::ResolveTodosService#async_execute' do + expect_next_instance_of(MergeRequests::ResolveTodosService, merge_request, user) do |service| + expect(service).to receive(:async_execute) + end - it 'marks pending todos as done' do - expect(pending_todo.reload).to be_done + update_merge_request({ description: "Also please fix #{user2.to_reference} #{user3.to_reference}" }) end it 'creates only 1 new todo' do + update_merge_request({ description: "Also please fix #{user2.to_reference} #{user3.to_reference}" }) + expect(Todo.count).to eq(2) end end context 'when is reassigned' do - before do - update_merge_request({ assignee_ids: [user2.id] }) - end - - it 'marks previous assignee pending todos as done' do - expect(pending_todo.reload).to be_done - end - - it 'creates a pending todo for new assignee' do - attributes = { - project: project, - author: user, - user: user2, - target_id: merge_request.id, - target_type: merge_request.class.name, - action: Todo::ASSIGNED, - state: :pending - } + it 'calls MergeRequest::ResolveTodosService#async_execute' do + expect_next_instance_of(MergeRequests::ResolveTodosService, merge_request, user) do |service| + expect(service).to receive(:async_execute) + end - expect(Todo.where(attributes).count).to eq 1 + update_merge_request({ assignee_ids: [user2.id] }) end end context 'when reviewers gets changed' do - it 'marks pending todo as done' do - update_merge_request({ reviewer_ids: [user2.id] }) + it 'calls MergeRequest::ResolveTodosService#async_execute' do + expect_next_instance_of(MergeRequests::ResolveTodosService, merge_request, user) do |service| + expect(service).to receive(:async_execute) + end - expect(pending_todo.reload).to be_done + update_merge_request({ reviewer_ids: [user2.id] }) end it 'creates a pending todo for new review request' do @@ -727,10 +715,12 @@ RSpec.describe MergeRequests::UpdateService, :mailer do end end - it 'marks pending todos as done' do - update_merge_request({ milestone: create(:milestone, project: project) }) + it 'calls MergeRequests::ResolveTodosService#async_execute' do + expect_next_instance_of(MergeRequests::ResolveTodosService, merge_request, user) do |service| + expect(service).to receive(:async_execute) + end - expect(pending_todo.reload).to be_done + update_merge_request({ milestone: create(:milestone, project: project) }) end it 'sends notifications for subscribers of changed milestone', :sidekiq_might_not_need_inline do @@ -744,17 +734,19 @@ RSpec.describe MergeRequests::UpdateService, :mailer do end context 'when the labels change' do - before do - travel_to(1.minute.from_now) do - update_merge_request({ label_ids: [label.id] }) + it 'calls MergeRequests::ResolveTodosService#async_execute' do + expect_next_instance_of(MergeRequests::ResolveTodosService, merge_request, user) do |service| + expect(service).to receive(:async_execute) end - end - it 'marks pending todos as done' do - expect(pending_todo.reload).to be_done + update_merge_request({ label_ids: [label.id] }) end it 'updates updated_at' do + travel_to(1.minute.from_now) do + update_merge_request({ label_ids: [label.id] }) + end + expect(merge_request.reload.updated_at).to be > Time.current end end @@ -769,24 +761,26 @@ RSpec.describe MergeRequests::UpdateService, :mailer do end context 'when the target branch change' do - before do - update_merge_request({ target_branch: 'target' }) - end + it 'calls MergeRequests::ResolveTodosService#async_execute' do + expect_next_instance_of(MergeRequests::ResolveTodosService, merge_request, user) do |service| + expect(service).to receive(:async_execute) + end - it 'marks pending todos as done' do - expect(pending_todo.reload).to be_done + update_merge_request({ target_branch: 'target' }) end end context 'when auto merge is enabled and target branch changed' do before do AutoMergeService.new(project, user, { sha: merge_request.diff_head_sha }).execute(merge_request, AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS) - - update_merge_request({ target_branch: 'target' }) end - it 'marks pending todos as done' do - expect(pending_todo.reload).to be_done + it 'calls MergeRequests::ResolveTodosService#async_execute' do + expect_next_instance_of(MergeRequests::ResolveTodosService, merge_request, user) do |service| + expect(service).to receive(:async_execute) + end + + update_merge_request({ target_branch: 'target' }) end end end @@ -948,18 +942,8 @@ RSpec.describe MergeRequests::UpdateService, :mailer do end it 'removes `MergeRequestsClosingIssues` records when issues are not closed anymore' do - opts = { - title: 'Awesome merge_request', - description: "Closes #{first_issue.to_reference} and #{second_issue.to_reference}", - source_branch: 'feature', - target_branch: 'master', - force_remove_source_branch: '1' - } - - merge_request = MergeRequests::CreateService.new(project, user, opts).execute - - issue_ids = MergeRequestsClosingIssues.where(merge_request: merge_request).pluck(:issue_id) - expect(issue_ids).to match_array([first_issue.id, second_issue.id]) + create(:merge_requests_closing_issues, issue: first_issue, merge_request: merge_request) + create(:merge_requests_closing_issues, issue: second_issue, merge_request: merge_request) service = described_class.new(project, user, description: "not closing any issues") allow(service).to receive(:execute_hooks) @@ -971,9 +955,45 @@ RSpec.describe MergeRequests::UpdateService, :mailer do end context 'updating asssignee_ids' do + context ':use_specialized_service' do + context 'when true' do + it 'passes the update action to ::MergeRequests::UpdateAssigneesService' do + expect(::MergeRequests::UpdateAssigneesService) + .to receive(:new).and_call_original + + update_merge_request({ + assignee_ids: [user2.id], + use_specialized_service: true + }) + end + end + + context 'when false or nil' do + before do + expect(::MergeRequests::UpdateAssigneesService).not_to receive(:new) + end + + it 'does not pass the update action to ::MergeRequests::UpdateAssigneesService when false' do + update_merge_request({ + assignee_ids: [user2.id], + use_specialized_service: false + }) + end + + it 'does not pass the update action to ::MergeRequests::UpdateAssigneesService when nil' do + update_merge_request({ + assignee_ids: [user2.id], + use_specialized_service: nil + }) + end + end + end + it 'does not update assignee when assignee_id is invalid' do merge_request.update!(assignee_ids: [user.id]) + expect(MergeRequests::HandleAssigneesChangeService).not_to receive(:new) + update_merge_request(assignee_ids: [-1]) expect(merge_request.reload.assignees).to eq([user]) @@ -982,29 +1002,35 @@ RSpec.describe MergeRequests::UpdateService, :mailer do it 'unassigns assignee when user id is 0' do merge_request.update!(assignee_ids: [user.id]) + expect_next_instance_of(MergeRequests::HandleAssigneesChangeService, project, user) do |service| + expect(service) + .to receive(:async_execute) + .with(merge_request, [user]) + end + update_merge_request(assignee_ids: [0]) expect(merge_request.assignee_ids).to be_empty end it 'saves assignee when user id is valid' do + expect_next_instance_of(MergeRequests::HandleAssigneesChangeService, project, user) do |service| + expect(service) + .to receive(:async_execute) + .with(merge_request, [user3]) + end + update_merge_request(assignee_ids: [user.id]) expect(merge_request.assignee_ids).to eq([user.id]) end - it 'updates the tracking when user ids are valid' do - expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) - .to receive(:track_users_assigned_to_mr) - .with(users: [user]) - - update_merge_request(assignee_ids: [user.id]) - end - it 'does not update assignee_id when user cannot read issue' do non_member = create(:user) original_assignees = merge_request.assignees + expect(MergeRequests::HandleAssigneesChangeService).not_to receive(:new) + update_merge_request(assignee_ids: [non_member.id]) expect(merge_request.reload.assignees).to eq(original_assignees) diff --git a/spec/services/milestones/destroy_service_spec.rb b/spec/services/milestones/destroy_service_spec.rb index dd68471d927..6c08b7db43a 100644 --- a/spec/services/milestones/destroy_service_spec.rb +++ b/spec/services/milestones/destroy_service_spec.rb @@ -22,14 +22,16 @@ RSpec.describe Milestones::DestroyService do expect { milestone.reload }.to raise_error ActiveRecord::RecordNotFound end - it 'deletes milestone id from issuables' do - issue = create(:issue, project: project, milestone: milestone) - merge_request = create(:merge_request, source_project: project, milestone: milestone) + context 'with an existing merge request' do + let!(:issue) { create(:issue, project: project, milestone: milestone) } + let!(:merge_request) { create(:merge_request, source_project: project, milestone: milestone) } - service.execute(milestone) + it 'deletes milestone id from issuables' do + service.execute(milestone) - expect(issue.reload.milestone).to be_nil - expect(merge_request.reload.milestone).to be_nil + expect(issue.reload.milestone).to be_nil + expect(merge_request.reload.milestone).to be_nil + end end it 'logs destroy event' do diff --git a/spec/services/milestones/merge_requests_count_service_spec.rb b/spec/services/milestones/merge_requests_count_service_spec.rb new file mode 100644 index 00000000000..aecc7d5ef52 --- /dev/null +++ b/spec/services/milestones/merge_requests_count_service_spec.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Milestones::MergeRequestsCountService, :use_clean_rails_memory_store_caching do + let_it_be(:project) { create(:project, :empty_repo) } + let_it_be(:milestone) { create(:milestone, project: project) } + + before_all do + create(:merge_request, milestone: milestone, source_project: project) + create(:merge_request, :closed, milestone: milestone, source_project: project) + end + + subject { described_class.new(milestone) } + + it_behaves_like 'a counter caching service' + + it 'counts all merge requests' do + expect(subject.count).to eq(2) + end +end diff --git a/spec/services/milestones/transfer_service_spec.rb b/spec/services/milestones/transfer_service_spec.rb index 6f4f55b2bd0..afbc9c7dca2 100644 --- a/spec/services/milestones/transfer_service_spec.rb +++ b/spec/services/milestones/transfer_service_spec.rb @@ -50,7 +50,7 @@ RSpec.describe Milestones::TransferService do end end - it 'deletes milestone issue counters cache for both milestones' do + it 'deletes milestone counters cache for both milestones' do new_milestone = create(:milestone, project: project, title: group_milestone.title) expect_next_instance_of(Milestones::IssuesCountService, group_milestone) do |service| @@ -59,12 +59,18 @@ RSpec.describe Milestones::TransferService do expect_next_instance_of(Milestones::ClosedIssuesCountService, group_milestone) do |service| expect(service).to receive(:delete_cache).and_call_original end + expect_next_instance_of(Milestones::MergeRequestsCountService, group_milestone) do |service| + expect(service).to receive(:delete_cache).and_call_original + end expect_next_instance_of(Milestones::IssuesCountService, new_milestone) do |service| expect(service).to receive(:delete_cache).and_call_original end expect_next_instance_of(Milestones::ClosedIssuesCountService, new_milestone) do |service| expect(service).to receive(:delete_cache).and_call_original end + expect_next_instance_of(Milestones::MergeRequestsCountService, new_milestone) do |service| + expect(service).to receive(:delete_cache).and_call_original + end service.execute end diff --git a/spec/services/namespace_settings/update_service_spec.rb b/spec/services/namespace_settings/update_service_spec.rb index b588bf2034d..887d56df099 100644 --- a/spec/services/namespace_settings/update_service_spec.rb +++ b/spec/services/namespace_settings/update_service_spec.rb @@ -44,5 +44,36 @@ RSpec.describe NamespaceSettings::UpdateService do .from(nil).to(example_branch_name) end end + + context "updating :resource_access_token_creation_allowed" do + let(:settings) { { resource_access_token_creation_allowed: false } } + + context 'when user is a group owner' do + before do + group.add_owner(user) + end + + it "changes settings" do + expect { service.execute } + .to change { group.namespace_settings.resource_access_token_creation_allowed } + .from(true).to(false) + end + end + + context 'when user is not a group owner' do + before do + group.add_developer(user) + end + + it "does not change settings" do + expect { service.execute }.not_to change { group.namespace_settings.resource_access_token_creation_allowed } + end + + it 'returns the group owner error' do + service.execute + expect(group.namespace_settings.errors.messages[:resource_access_token_creation_allowed]).to include('can only be changed by a group admin.') + end + end + end end end diff --git a/spec/services/namespaces/in_product_marketing_emails_service_spec.rb b/spec/services/namespaces/in_product_marketing_emails_service_spec.rb index 28b2e699e5e..3094f574184 100644 --- a/spec/services/namespaces/in_product_marketing_emails_service_spec.rb +++ b/spec/services/namespaces/in_product_marketing_emails_service_spec.rb @@ -3,14 +3,12 @@ require 'spec_helper' RSpec.describe Namespaces::InProductMarketingEmailsService, '#execute' do - subject(:execute_service) do - travel_to(frozen_time) { described_class.new(track, interval).execute } - end + subject(:execute_service) { described_class.new(track, interval).execute } let(:track) { :create } let(:interval) { 1 } - let(:frozen_time) { Time.current } + let(:frozen_time) { Time.zone.parse('23 Mar 2021 10:14:40 UTC') } let(:previous_action_completed_at) { frozen_time - 2.days } let(:current_action_completed_at) { nil } let(:experiment_enabled) { true } @@ -21,6 +19,7 @@ RSpec.describe Namespaces::InProductMarketingEmailsService, '#execute' do let_it_be(:user) { create(:user, email_opted_in: true) } before do + travel_to(frozen_time) create(:onboarding_progress, namespace: group, **actions_completed) group.add_developer(user) stub_experiment_for_subject(in_product_marketing_emails: experiment_enabled) @@ -86,26 +85,46 @@ RSpec.describe Namespaces::InProductMarketingEmailsService, '#execute' do end describe 'experimentation' do - context 'when the experiment is enabled' do - it 'adds the group as an experiment subject in the experimental group' do - expect(Experiment).to receive(:add_group) - .with(:in_product_marketing_emails, variant: :experimental, group: group) + context 'when on dotcom' do + before do + allow(::Gitlab).to receive(:com?).and_return(true) + end + + context 'when the experiment is enabled' do + it 'adds the group as an experiment subject in the experimental group' do + expect(Experiment).to receive(:add_group) + .with(:in_product_marketing_emails, variant: :experimental, group: group) - execute_service + execute_service + end end - end - context 'when the experiment is disabled' do - let(:experiment_enabled) { false } + context 'when the experiment is disabled' do + let(:experiment_enabled) { false } + + it 'adds the group as an experiment subject in the control group' do + expect(Experiment).to receive(:add_group) + .with(:in_product_marketing_emails, variant: :control, group: group) - it 'adds the group as an experiment subject in the control group' do - expect(Experiment).to receive(:add_group) - .with(:in_product_marketing_emails, variant: :control, group: group) + execute_service + end - execute_service + it { is_expected.not_to send_in_product_marketing_email } end - it { is_expected.not_to send_in_product_marketing_email } + context 'when not on dotcom' do + before do + allow(::Gitlab).to receive(:com?).and_return(false) + end + + it 'does not add the group as an experiment subject' do + expect(Experiment).not_to receive(:add_group) + + execute_service + end + + it { is_expected.to send_in_product_marketing_email(user.id, group.id, :create, 0) } + end end end @@ -139,25 +158,87 @@ RSpec.describe Namespaces::InProductMarketingEmailsService, '#execute' do it { is_expected.not_to send_in_product_marketing_email } end - context 'when the user has already received a marketing email as part of another group' do - before do - other_group = create(:group) - other_group.add_developer(user) - create(:onboarding_progress, namespace: other_group, created_at: previous_action_completed_at, git_write_at: current_action_completed_at) + describe 'do not send emails twice' do + subject { described_class.send_for_all_tracks_and_intervals } + + let(:user) { create(:user, email_opted_in: true) } + + context 'when user already got a specific email' do + before do + create(:in_product_marketing_email, user: user, track: track, series: 0) + end + + it { is_expected.not_to send_in_product_marketing_email(user.id, anything, track, 0) } end - # For any group Notify is called exactly once - it { is_expected.to send_in_product_marketing_email(user.id, anything, :create, 0) } + context 'when user already got sent the whole track' do + before do + 0.upto(2) do |series| + create(:in_product_marketing_email, user: user, track: track, series: series) + end + end + + it 'does not send any of the emails anymore', :aggregate_failures do + 0.upto(2) do |series| + expect(subject).not_to send_in_product_marketing_email(user.id, anything, track, series) + end + end + end + + context 'when user is in two groups' do + let(:other_group) { create(:group) } + + before do + other_group.add_developer(user) + end + + context 'when both groups would get the same email' do + before do + create(:onboarding_progress, namespace: other_group, **actions_completed) + end + + it 'does not send the same email twice' do + subject + + expect(Notify).to have_received(:in_product_marketing_email).with(user.id, anything, :create, 0).once + end + end + + context 'when other group gets a different email' do + before do + create(:onboarding_progress, namespace: other_group, created_at: previous_action_completed_at, git_write_at: frozen_time - 2.days) + end + + it 'sends both emails' do + subject + + expect(Notify).to have_received(:in_product_marketing_email).with(user.id, group.id, :create, 0) + expect(Notify).to have_received(:in_product_marketing_email).with(user.id, other_group.id, :verify, 0) + end + end + end + end + + it 'records sent emails' do + expect { subject }.to change { Users::InProductMarketingEmail.count }.by(1) + + expect( + Users::InProductMarketingEmail.where( + user: user, + track: Users::InProductMarketingEmail.tracks[:create], + series: 0 + ) + ).to exist end context 'when invoked with a non existing track' do let(:track) { :foo } before do - stub_const("#{described_class}::TRACKS", { foo: :git_write }) + stub_const("#{described_class}::TRACKS", { bar: :git_write }) end - it { expect { subject }.to raise_error(NotImplementedError, 'No ability defined for track foo') } + it { expect { subject }.to raise_error(ArgumentError, 'Track foo not defined') } end context 'when group is a sub-group' do diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb index f59749f0b63..d28cb118529 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -345,6 +345,24 @@ RSpec.describe Notes::CreateService do expect(note.errors[:commands_only]).to be_present end + + it 'adds commands failed message to note errors' do + note_text = %(/reopen) + note = described_class.new(project, user, opts.merge(note: note_text)).execute + + expect(note.errors[:commands_only]).to contain_exactly('Could not apply reopen command.') + end + + it 'generates success and failed error messages' do + note_text = %(/close\n/reopen) + service = double(:service) + allow(Issues::UpdateService).to receive(:new).and_return(service) + expect(service).to receive(:execute) + + note = described_class.new(project, user, opts.merge(note: note_text)).execute + + expect(note.errors[:commands_only]).to contain_exactly('Closed this issue. Could not apply reopen command.') + end end end diff --git a/spec/services/notification_recipients/builder/default_spec.rb b/spec/services/notification_recipients/builder/default_spec.rb index d25410235c2..994138ea828 100644 --- a/spec/services/notification_recipients/builder/default_spec.rb +++ b/spec/services/notification_recipients/builder/default_spec.rb @@ -6,7 +6,7 @@ RSpec.describe NotificationRecipients::Builder::Default do describe '#build!' do let_it_be(:group) { create(:group, :public) } let_it_be(:project) { create(:project, :public, group: group).tap { |p| p.add_developer(project_watcher) } } - let_it_be(:issue) { create(:issue, project: project) } + let_it_be(:target) { create(:issue, project: project) } let_it_be(:current_user) { create(:user) } let_it_be(:other_user) { create(:user) } @@ -17,11 +17,11 @@ RSpec.describe NotificationRecipients::Builder::Default do let_it_be(:notification_setting_project_w) { create(:notification_setting, source: project, user: project_watcher, level: 2) } let_it_be(:notification_setting_group_w) { create(:notification_setting, source: group, user: group_watcher, level: 2) } - subject { described_class.new(issue, current_user, action: :new).tap { |s| s.build! } } + subject { described_class.new(target, current_user, action: :new).tap { |s| s.build! } } context 'participants and project watchers' do before do - expect(issue).to receive(:participants).and_return([participant, current_user]) + expect(target).to receive(:participants).and_return([participant, current_user]) end it 'adds all participants and watchers' do @@ -34,11 +34,147 @@ RSpec.describe NotificationRecipients::Builder::Default do it 'adds all subscribers' do subscriber = create(:user) non_subscriber = create(:user) - create(:subscription, project: project, user: subscriber, subscribable: issue, subscribed: true) - create(:subscription, project: project, user: non_subscriber, subscribable: issue, subscribed: false) + create(:subscription, project: project, user: subscriber, subscribable: target, subscribed: true) + create(:subscription, project: project, user: non_subscriber, subscribable: target, subscribed: false) expect(subject.recipients.map(&:user)).to include(subscriber) end end + + context 'custom notifications' do + shared_examples 'custom notification recipients' do + let_it_be(:custom_notification_user) { create(:user) } + let_it_be(:another_group) { create(:group) } + let_it_be(:another_project) { create(:project, namespace: another_group) } + + context 'with project custom notification setting' do + before do + create(:notification_setting, source: project, user: custom_notification_user, level: :custom) + end + + it 'adds the user to the recipients' do + expect(subject.recipients.map(&:user)).to include(custom_notification_user) + end + end + + context 'with the project custom notification setting in another project' do + before do + create(:notification_setting, source: another_project, user: custom_notification_user, level: :custom) + end + + it 'does not add the user to the recipients' do + expect(subject.recipients.map(&:user)).not_to include(custom_notification_user) + end + end + + context 'with group custom notification setting' do + before do + create(:notification_setting, source: group, user: custom_notification_user, level: :custom) + end + + it 'adds the user to the recipients' do + expect(subject.recipients.map(&:user)).to include(custom_notification_user) + end + end + + context 'with the group custom notification setting in another group' do + before do + create(:notification_setting, source: another_group, user: custom_notification_user, level: :custom) + end + + it 'does not add the user to the recipients' do + expect(subject.recipients.map(&:user)).not_to include(custom_notification_user) + end + end + + context 'with project global custom notification setting' do + before do + create(:notification_setting, source: project, user: custom_notification_user, level: :global) + end + + context 'with global custom notification setting' do + before do + create(:notification_setting, source: nil, user: custom_notification_user, level: :custom) + end + + it 'adds the user to the recipients' do + expect(subject.recipients.map(&:user)).to include(custom_notification_user) + end + end + + context 'without global custom notification setting' do + it 'does not add the user to the recipients' do + expect(subject.recipients.map(&:user)).not_to include(custom_notification_user) + end + end + end + + context 'with group global custom notification setting' do + before do + create(:notification_setting, source: group, user: custom_notification_user, level: :global) + end + + context 'with global custom notification setting' do + before do + create(:notification_setting, source: nil, user: custom_notification_user, level: :custom) + end + + it 'adds the user to the recipients' do + expect(subject.recipients.map(&:user)).to include(custom_notification_user) + end + end + + context 'without global custom notification setting' do + it 'does not add the user to the recipients' do + expect(subject.recipients.map(&:user)).not_to include(custom_notification_user) + end + end + end + + context 'with group custom notification setting in deeply nested parent group' do + let(:grand_parent_group) { create(:group, :public) } + let(:parent_group) { create(:group, :public, parent: grand_parent_group) } + let(:group) { create(:group, :public, parent: parent_group) } + let(:project) { create(:project, :public, group: group).tap { |p| p.add_developer(project_watcher) } } + let(:target) { create(:issue, project: project) } + + before do + create(:notification_setting, source: grand_parent_group, user: custom_notification_user, level: :custom) + end + + it 'adds the user to the recipients' do + expect(subject.recipients.map(&:user)).to include(custom_notification_user) + end + end + + context 'without a project or group' do + let(:target) { create(:snippet) } + + before do + create(:notification_setting, source: nil, user: custom_notification_user, level: :custom) + end + + it 'does not add the user to the recipients' do + expect(subject.recipients.map(&:user)).not_to include(custom_notification_user) + end + end + end + + before do + stub_feature_flags(notification_setting_recipient_refactor: enabled) + end + + context 'with notification_setting_recipient_refactor enabled' do + let(:enabled) { true } + + it_behaves_like 'custom notification recipients' + end + + context 'with notification_setting_recipient_refactor disabled' do + let(:enabled) { false } + + it_behaves_like 'custom notification recipients' + end + end end end diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index f3cd2776ce7..6eff768eac2 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -105,7 +105,7 @@ RSpec.describe NotificationService, :mailer do recipient_1 = NotificationRecipient.new(user_1, :custom, custom_action: :new_release) allow(NotificationRecipients::BuildService).to receive(:build_new_release_recipients).and_return([recipient_1]) - expect(Gitlab::AppLogger).to receive(:warn).with(message: 'Skipping sending notifications', user: current_user.id, klass: object.class, object_id: object.id) + expect(Gitlab::AppLogger).to receive(:warn).with(message: 'Skipping sending notifications', user: current_user.id, klass: object.class.to_s, object_id: object.id) action @@ -290,6 +290,49 @@ RSpec.describe NotificationService, :mailer do end end + describe 'SSH Keys' do + let_it_be_with_reload(:user) { create(:user) } + let_it_be(:fingerprints) { ["aa:bb:cc:dd:ee:zz"] } + + shared_context 'block user' do + before do + user.block! + end + end + + describe '#ssh_key_expired' do + subject { notification.ssh_key_expired(user, fingerprints) } + + it 'sends email to the token owner' do + expect { subject }.to have_enqueued_email(user, fingerprints, mail: "ssh_key_expired_email") + end + + context 'when user is not allowed to receive notifications' do + include_context 'block user' + + it 'does not send email to the token owner' do + expect { subject }.not_to have_enqueued_email(user, fingerprints, mail: "ssh_key_expired_email") + end + end + end + + describe '#ssh_key_expiring_soon' do + subject { notification.ssh_key_expiring_soon(user, fingerprints) } + + it 'sends email to the token owner' do + expect { subject }.to have_enqueued_email(user, fingerprints, mail: "ssh_key_expiring_soon_email") + end + + context 'when user is not allowed to receive notifications' do + include_context 'block user' + + it 'does not send email to the token owner' do + expect { subject }.not_to have_enqueued_email(user, fingerprints, mail: "ssh_key_expiring_soon_email") + end + end + end + end + describe '#unknown_sign_in' do let_it_be(:user) { create(:user) } let_it_be(:ip) { '127.0.0.1' } @@ -1662,7 +1705,7 @@ RSpec.describe NotificationService, :mailer do notification.issue_due(issue) email = find_email_for(@subscriber) - expect(email.header[:from].display_names).to eq([issue.author.name]) + expect(email.header[:from].display_names).to eq(["#{issue.author.name} (@#{issue.author.username})"]) end it_behaves_like 'participating notifications' do diff --git a/spec/services/packages/create_dependency_service_spec.rb b/spec/services/packages/create_dependency_service_spec.rb index 3ad59b31b2c..3eae9f099f7 100644 --- a/spec/services/packages/create_dependency_service_spec.rb +++ b/spec/services/packages/create_dependency_service_spec.rb @@ -5,7 +5,7 @@ RSpec.describe Packages::CreateDependencyService do describe '#execute' do let_it_be(:namespace) {create(:namespace)} let_it_be(:version) { '1.0.1' } - let_it_be(:package_name) { "@#{namespace.path}/my-app".freeze } + let_it_be(:package_name) { "@#{namespace.path}/my-app" } context 'when packages are published' do let(:json_file) { 'packages/npm/payload.json' } diff --git a/spec/services/packages/debian/extract_changes_metadata_service_spec.rb b/spec/services/packages/debian/extract_changes_metadata_service_spec.rb new file mode 100644 index 00000000000..2a92b8ed26e --- /dev/null +++ b/spec/services/packages/debian/extract_changes_metadata_service_spec.rb @@ -0,0 +1,160 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe Packages::Debian::ExtractChangesMetadataService do + describe '#execute' do + let_it_be(:distribution) { create(:debian_project_distribution, codename: 'unstable') } + let_it_be(:incoming) { create(:debian_incoming, project: distribution.project) } + + let(:package_file) { incoming.package_files.last } + let(:service) { described_class.new(package_file) } + + subject { service.execute } + + context 'with valid package file' do + it 'extract metadata', :aggregate_failures do + expected_fields = { 'Architecture' => 'source amd64', 'Binary' => 'libsample0 sample-dev sample-udeb' } + + expect(subject[:file_type]).to eq(:changes) + expect(subject[:architecture]).to be_nil + expect(subject[:fields]).to include(expected_fields) + expect(subject[:files].count).to eq(6) + end + end + + context 'with invalid package file' do + let(:package_file) { incoming.package_files.first } + + it 'raise ArgumentError', :aggregate_failures do + expect { subject }.to raise_error(described_class::ExtractionError, "is not a changes file") + end + end + + context 'with invalid metadata' do + let(:md5_dsc) { '3b0817804f669e16cdefac583ad88f0e 671 libs optional sample_1.2.3~alpha2.dsc' } + let(:md5_source) { 'd79b34f58f61ff4ad696d9bd0b8daa68 864 libs optional sample_1.2.3~alpha2.tar.xz' } + let(:md5s) { "#{md5_dsc}\n#{md5_source}" } + let(:sha1_dsc) { '32ecbd674f0bfd310df68484d87752490685a8d6 671 sample_1.2.3~alpha2.dsc' } + let(:sha1_source) { '5f8bba5574eb01ac3b1f5e2988e8c29307788236 864 sample_1.2.3~alpha2.tar.xz' } + let(:sha1s) { "#{sha1_dsc}\n#{sha1_source}" } + let(:sha256_dsc) { '844f79825b7e8aaa191e514b58a81f9ac1e58e2180134b0c9512fa66d896d7ba 671 sample_1.2.3~alpha2.dsc' } + let(:sha256_source) { 'b5a599e88e7cbdda3bde808160a21ba1dd1ec76b2ec8d4912aae769648d68362 864 sample_1.2.3~alpha2.tar.xz' } + let(:sha256s) { "#{sha256_dsc}\n#{sha256_source}" } + let(:fields) { { 'Files' => md5s, 'Checksums-Sha1' => sha1s, 'Checksums-Sha256' => sha256s } } + let(:metadata) { { file_type: :changes, architecture: 'amd64', fields: fields } } + + before do + allow_next_instance_of(::Packages::Debian::ExtractMetadataService) do |extract_metadata_service| + allow(extract_metadata_service).to receive(:execute).and_return(metadata) + end + end + + context 'without Files field' do + let(:md5s) { nil } + + it 'raise ArgumentError', :aggregate_failures do + expect { subject }.to raise_error(described_class::ExtractionError, "Files field is missing") + end + end + + context 'without Checksums-Sha1 field' do + let(:sha1s) { nil } + + it 'raise ArgumentError', :aggregate_failures do + expect { subject }.to raise_error(described_class::ExtractionError, "Checksums-Sha1 field is missing") + end + end + + context 'without Checksums-Sha256 field' do + let(:sha256s) { nil } + + it 'raise ArgumentError', :aggregate_failures do + expect { subject }.to raise_error(described_class::ExtractionError, "Checksums-Sha256 field is missing") + end + end + + context 'with file in Checksums-Sha1 but not in Files' do + let(:md5_dsc) { '' } + + it 'raise ArgumentError', :aggregate_failures do + expect { subject }.to raise_error(described_class::ExtractionError, "sample_1.2.3~alpha2.dsc is listed in Checksums-Sha1 but not in Files") + end + end + + context 'with different size in Checksums-Sha1' do + let(:sha1_dsc) { '32ecbd674f0bfd310df68484d87752490685a8d6 42 sample_1.2.3~alpha2.dsc' } + + it 'raise ArgumentError', :aggregate_failures do + expect { subject }.to raise_error(described_class::ExtractionError, "Size for sample_1.2.3~alpha2.dsc in Files and Checksums-Sha1 differ") + end + end + + context 'with file in Checksums-Sha256 but not in Files' do + let(:md5_dsc) { '' } + let(:sha1_dsc) { '' } + + it 'raise ArgumentError', :aggregate_failures do + expect { subject }.to raise_error(described_class::ExtractionError, "sample_1.2.3~alpha2.dsc is listed in Checksums-Sha256 but not in Files") + end + end + + context 'with different size in Checksums-Sha256' do + let(:sha256_dsc) { '844f79825b7e8aaa191e514b58a81f9ac1e58e2180134b0c9512fa66d896d7ba 42 sample_1.2.3~alpha2.dsc' } + + it 'raise ArgumentError', :aggregate_failures do + expect { subject }.to raise_error(described_class::ExtractionError, "Size for sample_1.2.3~alpha2.dsc in Files and Checksums-Sha256 differ") + end + end + + context 'with file in Files but not in Checksums-Sha1' do + let(:sha1_dsc) { '' } + + it 'raise ArgumentError', :aggregate_failures do + expect { subject }.to raise_error(described_class::ExtractionError, "Validation failed: Sha1sum can't be blank") + end + end + + context 'with file in Files but not in Checksums-Sha256' do + let(:sha256_dsc) { '' } + + it 'raise ArgumentError', :aggregate_failures do + expect { subject }.to raise_error(described_class::ExtractionError, "Validation failed: Sha256sum can't be blank") + end + end + + context 'with invalid MD5' do + let(:md5_dsc) { '1234567890123456789012345678012 671 libs optional sample_1.2.3~alpha2.dsc' } + + it 'raise ArgumentError', :aggregate_failures do + expect { subject }.to raise_error(described_class::ExtractionError, "Validation failed: Md5sum mismatch for sample_1.2.3~alpha2.dsc: 3b0817804f669e16cdefac583ad88f0e != 1234567890123456789012345678012") + end + end + + context 'with invalid SHA1' do + let(:sha1_dsc) { '1234567890123456789012345678901234567890 671 sample_1.2.3~alpha2.dsc' } + + it 'raise ArgumentError', :aggregate_failures do + expect { subject }.to raise_error(described_class::ExtractionError, "Validation failed: Sha1sum mismatch for sample_1.2.3~alpha2.dsc: 32ecbd674f0bfd310df68484d87752490685a8d6 != 1234567890123456789012345678901234567890") + end + end + + context 'with invalid SHA256' do + let(:sha256_dsc) { '1234567890123456789012345678901234567890123456789012345678901234 671 sample_1.2.3~alpha2.dsc' } + + it 'raise ArgumentError', :aggregate_failures do + expect { subject }.to raise_error(described_class::ExtractionError, "Validation failed: Sha256sum mismatch for sample_1.2.3~alpha2.dsc: 844f79825b7e8aaa191e514b58a81f9ac1e58e2180134b0c9512fa66d896d7ba != 1234567890123456789012345678901234567890123456789012345678901234") + end + end + end + + context 'with missing package file' do + before do + incoming.package_files.first.destroy! + end + + it 'raise ArgumentError' do + expect { subject }.to raise_error(described_class::ExtractionError, "sample_1.2.3~alpha2.tar.xz is listed in Files but was not uploaded") + end + end + end +end diff --git a/spec/services/packages/debian/extract_deb_metadata_service_spec.rb b/spec/services/packages/debian/extract_deb_metadata_service_spec.rb index 33059adf8a2..ee3f3d179dc 100644 --- a/spec/services/packages/debian/extract_deb_metadata_service_spec.rb +++ b/spec/services/packages/debian/extract_deb_metadata_service_spec.rb @@ -10,17 +10,17 @@ RSpec.describe Packages::Debian::ExtractDebMetadataService do context 'with correct file' do it 'return as expected' do expected = { - 'Package': 'libsample0', - 'Source': 'sample', - 'Version': '1.2.3~alpha2', - 'Architecture': 'amd64', - 'Maintainer': 'John Doe <john.doe@example.com>', - 'Installed-Size': '7', - 'Section': 'libs', - 'Priority': 'optional', - 'Multi-Arch': 'same', - 'Homepage': 'https://gitlab.com/', - 'Description': "Some mostly empty lib\nUsed in GitLab tests.\n\nTesting another paragraph." + 'Package' => 'libsample0', + 'Source' => 'sample', + 'Version' => '1.2.3~alpha2', + 'Architecture' => 'amd64', + 'Maintainer' => 'John Doe <john.doe@example.com>', + 'Installed-Size' => '7', + 'Section' => 'libs', + 'Priority' => 'optional', + 'Multi-Arch' => 'same', + 'Homepage' => 'https://gitlab.com/', + 'Description' => "Some mostly empty lib\nUsed in GitLab tests.\n\nTesting another paragraph." } expect(subject.execute).to eq expected diff --git a/spec/services/packages/debian/extract_metadata_service_spec.rb b/spec/services/packages/debian/extract_metadata_service_spec.rb index 0aa9a67b263..e3911dbbfe0 100644 --- a/spec/services/packages/debian/extract_metadata_service_spec.rb +++ b/spec/services/packages/debian/extract_metadata_service_spec.rb @@ -33,11 +33,11 @@ RSpec.describe Packages::Debian::ExtractMetadataService do where(:case_name, :trait, :expected_file_type, :expected_architecture, :expected_fields) do 'with invalid' | :invalid | :unknown | nil | nil 'with source' | :source | :source | nil | nil - 'with dsc' | :dsc | :dsc | nil | { 'Binary': 'sample-dev, libsample0, sample-udeb' } - 'with deb' | :deb | :deb | 'amd64' | { 'Multi-Arch': 'same' } - 'with udeb' | :udeb | :udeb | 'amd64' | { 'Package': 'sample-udeb' } - 'with buildinfo' | :buildinfo | :buildinfo | nil | { 'Architecture': 'amd64 source', 'Build-Architecture': 'amd64' } - 'with changes' | :changes | :changes | nil | { 'Architecture': 'source amd64', 'Binary': 'libsample0 sample-dev sample-udeb' } + 'with dsc' | :dsc | :dsc | nil | { 'Binary' => 'sample-dev, libsample0, sample-udeb' } + 'with deb' | :deb | :deb | 'amd64' | { 'Multi-Arch' => 'same' } + 'with udeb' | :udeb | :udeb | 'amd64' | { 'Package' => 'sample-udeb' } + 'with buildinfo' | :buildinfo | :buildinfo | nil | { 'Architecture' => 'amd64 source', 'Build-Architecture' => 'amd64' } + 'with changes' | :changes | :changes | nil | { 'Architecture' => 'source amd64', 'Binary' => 'libsample0 sample-dev sample-udeb' } end with_them do diff --git a/spec/services/packages/debian/parse_debian822_service_spec.rb b/spec/services/packages/debian/parse_debian822_service_spec.rb index b67daca89c4..f43e38991ce 100644 --- a/spec/services/packages/debian/parse_debian822_service_spec.rb +++ b/spec/services/packages/debian/parse_debian822_service_spec.rb @@ -27,17 +27,17 @@ RSpec.describe Packages::Debian::ParseDebian822Service do it 'return as expected, preserving order' do expected = { 'Package: libsample0' => { - 'Package': 'libsample0', - 'Source': 'sample', - 'Version': '1.2.3~alpha2', - 'Architecture': 'amd64', - 'Maintainer': 'John Doe <john.doe@example.com>', - 'Installed-Size': '9', - 'Section': 'libs', - 'Priority': 'optional', - 'Multi-Arch': 'same', - 'Homepage': 'https://gitlab.com/', - 'Description': "Some mostly empty lib\nUsed in GitLab tests.\n\nTesting another paragraph." + 'Package' => 'libsample0', + 'Source' => 'sample', + 'Version' => '1.2.3~alpha2', + 'Architecture' => 'amd64', + 'Maintainer' => 'John Doe <john.doe@example.com>', + 'Installed-Size' => '9', + 'Section' => 'libs', + 'Priority' => 'optional', + 'Multi-Arch' => 'same', + 'Homepage' => 'https://gitlab.com/', + 'Description' => "Some mostly empty lib\nUsed in GitLab tests.\n\nTesting another paragraph." } } @@ -51,38 +51,38 @@ RSpec.describe Packages::Debian::ParseDebian822Service do it 'return as expected, preserving order' do expected = { 'Source: sample' => { - 'Source': 'sample', - 'Priority': 'optional', - 'Maintainer': 'John Doe <john.doe@example.com>', - 'Build-Depends': 'debhelper-compat (= 13)', - 'Standards-Version': '4.5.0', - 'Section': 'libs', - 'Homepage': 'https://gitlab.com/', - # 'Vcs-Browser': 'https://salsa.debian.org/debian/sample-1.2.3', - # '#Vcs-Git': 'https://salsa.debian.org/debian/sample-1.2.3.git', - 'Rules-Requires-Root': 'no' + 'Source' => 'sample', + 'Priority' => 'optional', + 'Maintainer' => 'John Doe <john.doe@example.com>', + 'Build-Depends' => 'debhelper-compat (= 13)', + 'Standards-Version' => '4.5.0', + 'Section' => 'libs', + 'Homepage' => 'https://gitlab.com/', + # 'Vcs-Browser' => 'https://salsa.debian.org/debian/sample-1.2.3', + # '#Vcs-Git' => 'https://salsa.debian.org/debian/sample-1.2.3.git', + 'Rules-Requires-Root' => 'no' }, 'Package: sample-dev' => { - 'Package': 'sample-dev', - 'Section': 'libdevel', - 'Architecture': 'any', - 'Multi-Arch': 'same', - 'Depends': 'libsample0 (= ${binary:Version}), ${misc:Depends}', - 'Description': "Some mostly empty developpement files\nUsed in GitLab tests.\n\nTesting another paragraph." + 'Package' => 'sample-dev', + 'Section' => 'libdevel', + 'Architecture' => 'any', + 'Multi-Arch' => 'same', + 'Depends' => 'libsample0 (= ${binary:Version}), ${misc:Depends}', + 'Description' => "Some mostly empty developpement files\nUsed in GitLab tests.\n\nTesting another paragraph." }, 'Package: libsample0' => { - 'Package': 'libsample0', - 'Architecture': 'any', - 'Multi-Arch': 'same', - 'Depends': '${shlibs:Depends}, ${misc:Depends}', - 'Description': "Some mostly empty lib\nUsed in GitLab tests.\n\nTesting another paragraph." + 'Package' => 'libsample0', + 'Architecture' => 'any', + 'Multi-Arch' => 'same', + 'Depends' => '${shlibs:Depends}, ${misc:Depends}', + 'Description' => "Some mostly empty lib\nUsed in GitLab tests.\n\nTesting another paragraph." }, 'Package: sample-udeb' => { - 'Package': 'sample-udeb', - 'Package-Type': 'udeb', - 'Architecture': 'any', - 'Depends': 'installed-base', - 'Description': 'Some mostly empty udeb' + 'Package' => 'sample-udeb', + 'Package-Type' => 'udeb', + 'Architecture' => 'any', + 'Depends' => 'installed-base', + 'Description' => 'Some mostly empty udeb' } } diff --git a/spec/services/packages/debian/process_changes_service_spec.rb b/spec/services/packages/debian/process_changes_service_spec.rb new file mode 100644 index 00000000000..98b531bde10 --- /dev/null +++ b/spec/services/packages/debian/process_changes_service_spec.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe Packages::Debian::ProcessChangesService do + describe '#execute' do + let_it_be(:user) { create(:user) } + let_it_be_with_reload(:distribution) { create(:debian_project_distribution, :with_file, codename: 'unstable') } + let_it_be(:incoming) { create(:debian_incoming, project: distribution.project) } + + let(:package_file) { incoming.package_files.last } + + subject { described_class.new(package_file, user) } + + context 'with valid package file' do + it 'updates package and package file', :aggregate_failures do + expect { subject.execute } + .to change { Packages::Package.count }.from(1).to(2) + .and not_change { Packages::PackageFile.count } + .and change { incoming.package_files.count }.from(7).to(0) + + created_package = Packages::Package.last + expect(created_package.name).to eq 'sample' + expect(created_package.version).to eq '1.2.3~alpha2' + expect(created_package.creator).to eq user + end + end + + context 'with invalid package file' do + let(:package_file) { incoming.package_files.first } + + it 'raise ExtractionError', :aggregate_failures do + expect { subject.execute } + .to not_change { Packages::Package.count } + .and not_change { Packages::PackageFile.count } + .and not_change { incoming.package_files.count } + .and not_change { distribution.reload.needs_update? } + .and raise_error(Packages::Debian::ExtractChangesMetadataService::ExtractionError, 'is not a changes file') + end + end + + context 'when creating package fails' do + before do + allow_next_instance_of(::Packages::Debian::FindOrCreatePackageService) do |find_or_create_package_service| + expect(find_or_create_package_service).to receive(:execute).and_raise(ActiveRecord::ConnectionTimeoutError, 'connect timeout') + end + end + + it 'remove the package file', :aggregate_failures do + expect { subject.execute } + .to not_change { Packages::Package.count } + .and not_change { Packages::PackageFile.count } + .and not_change { incoming.package_files.count } + .and not_change { distribution.reload.needs_update? } + .and raise_error(ActiveRecord::ConnectionTimeoutError, 'connect timeout') + end + end + end +end diff --git a/spec/services/packages/go/create_package_service_spec.rb b/spec/services/packages/go/create_package_service_spec.rb new file mode 100644 index 00000000000..5c5fec0aa3a --- /dev/null +++ b/spec/services/packages/go/create_package_service_spec.rb @@ -0,0 +1,73 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Packages::Go::CreatePackageService do + let_it_be(:project) { create :project_empty_repo, path: 'my-go-lib' } + let_it_be(:mod) { create :go_module, project: project } + + before :all do + create :go_module_commit, :module, project: project, tag: 'v1.0.0' + end + + shared_examples 'creates a package' do |files:| + it "returns a valid package with #{files ? files.to_s : 'no'} file(s)" do + expect(subject).to be_valid + expect(subject.name).to eq(version.mod.name) + expect(subject.version).to eq(version.name) + expect(subject.package_type).to eq('golang') + expect(subject.created_at).to eq(version.commit.committed_date) + expect(subject.package_files.count).to eq(files) + end + end + + shared_examples 'creates a package file' do |type| + it "returns a package with a #{type} file" do + file_name = "#{version.name}.#{type}" + expect(subject.package_files.map { |f| f.file_name }).to include(file_name) + + file = subject.package_files.with_file_name(file_name).first + expect(file).not_to be_nil + expect(file.file).not_to be_nil + expect(file.size).to eq(file.file.size) + expect(file.file_name).to eq(file_name) + expect(file.file_md5).not_to be_nil + expect(file.file_sha1).not_to be_nil + expect(file.file_sha256).not_to be_nil + end + end + + describe '#execute' do + subject { described_class.new(project, nil, version: version).execute } + + let(:version) { create :go_module_version, :tagged, mod: mod, name: 'v1.0.0' } + + context 'with no existing package' do + it_behaves_like 'creates a package', files: 2 + it_behaves_like 'creates a package file', :mod + it_behaves_like 'creates a package file', :zip + + it 'creates a new package' do + expect { subject } + .to change { project.packages.count }.by(1) + .and change { Packages::PackageFile.count }.by(2) + end + end + + context 'with an existing package' do + before do + described_class.new(project, version: version).execute + end + + it_behaves_like 'creates a package', files: 2 + it_behaves_like 'creates a package file', :mod + it_behaves_like 'creates a package file', :zip + + it 'does not create a package or files' do + expect { subject } + .to not_change { project.packages.count } + .and not_change { Packages::PackageFile.count } + end + end + end +end diff --git a/spec/services/packages/go/sync_packages_service_spec.rb b/spec/services/packages/go/sync_packages_service_spec.rb new file mode 100644 index 00000000000..565b0f252ce --- /dev/null +++ b/spec/services/packages/go/sync_packages_service_spec.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Packages::Go::SyncPackagesService do + include_context 'basic Go module' + + let(:params) { { info: true, mod: true, zip: true } } + + describe '#execute_async' do + it 'schedules a package refresh' do + expect(::Packages::Go::SyncPackagesWorker).to receive(:perform_async).once + + described_class.new(project, 'master').execute_async + end + end + + describe '#initialize' do + context 'without a project' do + it 'raises an error' do + expect { described_class.new(nil, 'master') } + .to raise_error(ArgumentError, 'project is required') + end + end + + context 'without a ref' do + it 'raises an error' do + expect { described_class.new(project, nil) } + .to raise_error(ArgumentError, 'ref is required') + end + end + + context 'with an invalid ref' do + it 'raises an error' do + expect { described_class.new(project, 'not-a-ref') } + .to raise_error(ArgumentError) + end + end + end +end diff --git a/spec/services/packages/maven/metadata/sync_service_spec.rb b/spec/services/packages/maven/metadata/sync_service_spec.rb index f5634159e6d..30ddb48207a 100644 --- a/spec/services/packages/maven/metadata/sync_service_spec.rb +++ b/spec/services/packages/maven/metadata/sync_service_spec.rb @@ -131,7 +131,7 @@ RSpec.describe ::Packages::Maven::Metadata::SyncService do expect(::Packages::Maven::Metadata::CreateVersionsXmlService).not_to receive(:new) end - it_behaves_like 'returning an error service response', message: 'Non existing versionless package' + it_behaves_like 'returning a success service response', message: 'Non existing versionless package(s). Nothing to do.' end context 'without a metadata package file for versions' do @@ -141,7 +141,7 @@ RSpec.describe ::Packages::Maven::Metadata::SyncService do expect(::Packages::Maven::Metadata::CreateVersionsXmlService).not_to receive(:new) end - it_behaves_like 'returning an error service response', message: 'Non existing metadata file for versions' + it_behaves_like 'returning a success service response', message: 'Non existing versionless package(s). Nothing to do.' end context 'without a project' do @@ -205,7 +205,7 @@ RSpec.describe ::Packages::Maven::Metadata::SyncService do it_behaves_like 'returning a success service response', message: 'Versionless package for versions destroyed' end - context 'with a too big maven metadata file for versions' do + context 'with a too big maven metadata file for plugins' do before do metadata_file_for_plugins.update!(size: 100.megabytes) end @@ -244,6 +244,15 @@ RSpec.describe ::Packages::Maven::Metadata::SyncService do it_behaves_like 'returning a success service response', message: 'New metadata package files created' end + context 'without a versionless package for versions' do + before do + versionless_package_for_versions.package_files.update_all(file_name: 'test.txt') + expect(::Packages::Maven::Metadata::CreateVersionsXmlService).not_to receive(:new) + end + + it_behaves_like 'returning a success service response', message: 'No changes for plugins xml' + end + context 'without a metadata package file for plugins' do before do versionless_package_for_plugins.package_files.update_all(file_name: 'test.txt') diff --git a/spec/services/packages/rubygems/create_dependencies_service_spec.rb b/spec/services/packages/rubygems/create_dependencies_service_spec.rb new file mode 100644 index 00000000000..b6e12b1cc61 --- /dev/null +++ b/spec/services/packages/rubygems/create_dependencies_service_spec.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe Packages::Rubygems::CreateDependenciesService do + include RubygemsHelpers + + let_it_be(:package) { create(:rubygems_package) } + let_it_be(:package_file) { create(:package_file, :gem) } + let_it_be(:gem) { gem_from_file(package_file.file) } + let_it_be(:gemspec) { gem.spec } + + let(:service) { described_class.new(package, gemspec) } + + describe '#execute' do + subject { service.execute } + + it 'creates dependencies', :aggregate_failures do + expect { subject }.to change { Packages::Dependency.count }.by(4) + + gemspec.dependencies.each do |dependency| + persisted_dependency = Packages::Dependency.find_by(name: dependency.name) + + expect(persisted_dependency.version_pattern).to eq dependency.requirement.to_s + end + end + + it 'links dependencies to the package' do + expect { subject }.to change { package.dependency_links.count }.by(4) + + expect(package.dependency_links.first).to be_dependencies + end + end +end diff --git a/spec/services/packages/rubygems/create_gemspec_service_spec.rb b/spec/services/packages/rubygems/create_gemspec_service_spec.rb new file mode 100644 index 00000000000..198e978a47e --- /dev/null +++ b/spec/services/packages/rubygems/create_gemspec_service_spec.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe Packages::Rubygems::CreateGemspecService do + include RubygemsHelpers + + let_it_be(:package_file) { create(:package_file, :gem) } + let_it_be(:gem) { gem_from_file(package_file.file) } + let_it_be(:gemspec) { gem.spec } + let_it_be(:package) { package_file.package } + + let(:service) { described_class.new(package, gemspec) } + + describe '#execute' do + subject { service.execute } + + it 'creates a new package file', :aggregate_failures do + expect { subject }.to change { package.package_files.count }.by(1) + + gemspec_file = package.package_files.find_by(file_name: "#{gemspec.name}.gemspec") + expect(gemspec_file.file).not_to be_nil + expect(gemspec_file.size).not_to be_nil + expect(gemspec_file.file_md5).not_to be_nil + expect(gemspec_file.file_sha1).not_to be_nil + expect(gemspec_file.file_sha256).not_to be_nil + end + end +end diff --git a/spec/services/packages/rubygems/metadata_extraction_service_spec.rb b/spec/services/packages/rubygems/metadata_extraction_service_spec.rb new file mode 100644 index 00000000000..b308daad8f5 --- /dev/null +++ b/spec/services/packages/rubygems/metadata_extraction_service_spec.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true +require 'spec_helper' +require 'rubygems/package' + +RSpec.describe Packages::Rubygems::MetadataExtractionService do + include RubygemsHelpers + + let_it_be(:package) { create(:rubygems_package) } + let_it_be(:package_file) { create(:package_file, :gem) } + let_it_be(:gem) { gem_from_file(package_file.file) } + let_it_be(:gemspec) { gem.spec } + + let(:service) { described_class.new(package, gemspec) } + + describe '#execute' do + subject { service.execute } + + it 'creates the metadata' do + expect { subject }.to change { Packages::Rubygems::Metadatum.count }.by(1) + end + + it 'stores the metadata', :aggregate_failures do + subject + + metadata = package.rubygems_metadatum + + expect(metadata.authors).to eq(gemspec.authors.to_json) + expect(metadata.files).to eq(gemspec.files.to_json) + expect(metadata.summary).to eq(gemspec.summary) + expect(metadata.description).to eq(gemspec.description) + expect(metadata.email).to eq(gemspec.email) + expect(metadata.homepage).to eq(gemspec.homepage) + expect(metadata.licenses).to eq(gemspec.licenses.to_json) + expect(metadata.metadata).to eq(gemspec.metadata.to_json) + expect(metadata.author).to eq(gemspec.author) + expect(metadata.bindir).to eq(gemspec.bindir) + expect(metadata.executables).to eq(gemspec.executables.to_json) + expect(metadata.extensions).to eq(gemspec.extensions.to_json) + expect(metadata.extra_rdoc_files).to eq(gemspec.extra_rdoc_files.to_json) + expect(metadata.platform).to eq(gemspec.platform) + expect(metadata.post_install_message).to eq(gemspec.post_install_message) + expect(metadata.rdoc_options).to eq(gemspec.rdoc_options.to_json) + expect(metadata.require_paths).to eq(gemspec.require_paths.to_json) + expect(metadata.required_ruby_version).to eq(gemspec.required_ruby_version.to_s) + expect(metadata.required_rubygems_version).to eq(gemspec.required_rubygems_version.to_s) + expect(metadata.requirements).to eq(gemspec.requirements.to_json) + expect(metadata.rubygems_version).to eq(gemspec.rubygems_version) + end + end +end diff --git a/spec/services/packages/rubygems/process_gem_service_spec.rb b/spec/services/packages/rubygems/process_gem_service_spec.rb new file mode 100644 index 00000000000..83e868d9579 --- /dev/null +++ b/spec/services/packages/rubygems/process_gem_service_spec.rb @@ -0,0 +1,134 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Packages::Rubygems::ProcessGemService do + include ExclusiveLeaseHelpers + include RubygemsHelpers + + let_it_be_with_reload(:package) { create(:rubygems_package, :processing, name: 'temp_name', version: '0.0.0') } + + let(:package_file) { create(:package_file, :unprocessed_gem, package: package) } + let(:gem) { gem_from_file(package_file.file) } + let(:gemspec) { gem.spec } + let(:service) { described_class.new(package_file) } + + describe '#execute' do + subject { service.execute } + + context 'no gem file', :aggregate_failures do + let(:package_file) { nil } + + it 'returns an error' do + expect(subject.error?).to be(true) + expect(subject.message).to eq('Gem was not processed') + end + end + + context 'success' do + let(:sub_service) { double } + + before do + expect(Packages::Rubygems::MetadataExtractionService).to receive(:new).with(package, gemspec).and_return(sub_service) + expect(Packages::Rubygems::CreateGemspecService).to receive(:new).with(package, gemspec).and_return(sub_service) + expect(Packages::Rubygems::CreateDependenciesService).to receive(:new).with(package, gemspec).and_return(sub_service) + + expect(sub_service).to receive(:execute).exactly(3).times.and_return(true) + end + + it 'returns successfully', :aggregate_failures do + result = subject + + expect(result.success?).to be true + expect(result.payload[:package]).to eq(package) + end + + it 'updates the package name and version', :aggregate_failures do + expect(package.name).to eq('temp_name') + expect(package.version).to eq('0.0.0') + expect(package).to be_processing + + subject + + expect(package.reload.name).to eq('package') + expect(package.version).to eq('0.0.1') + expect(package).to be_default + end + + it 'updates the package file name', :aggregate_failures do + expect(package_file.file_name).to eq('package.gem') + + subject + + expect(package_file.reload.file_name).to eq('package-0.0.1.gem') + end + end + + context 'when the package already exists' do + let_it_be(:existing_package) { create(:rubygems_package, name: 'package', version: '0.0.1', project: package.project) } + + let(:sub_service) { double } + + before do + expect(Packages::Rubygems::MetadataExtractionService).to receive(:new).with(existing_package, gemspec).and_return(sub_service) + expect(Packages::Rubygems::CreateGemspecService).to receive(:new).with(existing_package, gemspec).and_return(sub_service) + expect(Packages::Rubygems::CreateDependenciesService).to receive(:new).with(existing_package, gemspec).and_return(sub_service) + + expect(sub_service).to receive(:execute).exactly(3).times.and_return(true) + end + + it 'assigns the package_file to the existing package and deletes the temporary package', :aggregate_failures do + expect(package).to receive(:destroy) + + expect { subject }.to change { existing_package.package_files.count }.by(1) + + expect(package_file.reload.package).to eq(existing_package) + end + end + + context 'sub-service failure' do + before do + expect(Packages::Rubygems::MetadataExtractionService).to receive(:new).with(package, gemspec).and_raise(::Packages::Rubygems::ProcessGemService::ExtractionError.new('failure')) + end + + it 'returns an error' do + expect { subject }.to raise_error(::Packages::Rubygems::ProcessGemService::ExtractionError, 'failure') + end + end + + context 'bad gem file' do + before do + expect(Gem::Package).to receive(:new).and_raise(ArgumentError) + end + + it 'returns an error' do + expect { subject }.to raise_error(::Packages::Rubygems::ProcessGemService::ExtractionError, 'Unable to read gem file') + end + end + + context 'without obtaining an exclusive lease' do + let(:lease_key) { "packages:rubygems:process_gem_service:package:#{package.id}" } + + before do + stub_exclusive_lease_taken(lease_key, timeout: 1.hour) + end + + it 'does not perform the services', :aggregate_failures do + # The #use_file call triggers a separate lease on the package file being opened + # for use with the gem. We don't want to test that here, so we allow the call to proceed + expect(Gitlab::ExclusiveLease).to receive(:new).with("object_storage_migrate:Packages::PackageFile:#{package_file.id}", anything).and_call_original + + expect(Packages::Rubygems::MetadataExtractionService).not_to receive(:new) + expect(Packages::Rubygems::CreateGemspecService).not_to receive(:new) + expect(Packages::Rubygems::CreateDependenciesService).not_to receive(:new) + + subject + + expect(package.reload.name).to eq('temp_name') + expect(package.version).to eq('0.0.0') + expect(package).to be_processing + expect(package_file.reload.file_name).to eq('package.gem') + end + end + end +end diff --git a/spec/services/pages/delete_services_spec.rb b/spec/services/pages/delete_service_spec.rb index f1edf93b0c1..a79c89a1c35 100644 --- a/spec/services/pages/delete_services_spec.rb +++ b/spec/services/pages/delete_service_spec.rb @@ -16,7 +16,10 @@ RSpec.describe Pages::DeleteService do it 'deletes published pages', :sidekiq_inline do expect(project.pages_deployed?).to be(true) - expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project).and_return true + expect_next_instance_of(Gitlab::PagesTransfer) do |pages_transfer| + expect(pages_transfer).to receive(:rename_project).and_return true + end + expect(PagesWorker).to receive(:perform_in).with(5.minutes, :remove, project.namespace.full_path, anything) service.execute @@ -24,11 +27,10 @@ RSpec.describe Pages::DeleteService do expect(project.pages_deployed?).to be(false) end - it "doesn't remove anything from the legacy storage if updates on it are disabled", :sidekiq_inline do - stub_feature_flags(pages_update_legacy_storage: false) + it "doesn't remove anything from the legacy storage", :sidekiq_inline do + allow(Settings.pages.local_store).to receive(:enabled).and_return(false) expect(project.pages_deployed?).to be(true) - expect(PagesWorker).not_to receive(:perform_in) service.execute @@ -69,7 +71,9 @@ RSpec.describe Pages::DeleteService do expect(project.pages_deployed?).to eq(false) expect(project.pages_domains.count).to eq(0) - expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project).and_return true + expect_next_instance_of(Gitlab::PagesTransfer) do |pages_transfer| + expect(pages_transfer).to receive(:rename_project).and_return true + end Sidekiq::Worker.drain_all end diff --git a/spec/services/pages/migrate_from_legacy_storage_service_spec.rb b/spec/services/pages/migrate_from_legacy_storage_service_spec.rb index 4ec57044912..d058324f3bb 100644 --- a/spec/services/pages/migrate_from_legacy_storage_service_spec.rb +++ b/spec/services/pages/migrate_from_legacy_storage_service_spec.rb @@ -3,90 +3,135 @@ require 'spec_helper' RSpec.describe Pages::MigrateFromLegacyStorageService do - let(:service) { described_class.new(Rails.logger, migration_threads: 3, batch_size: 10, ignore_invalid_entries: false) } + let(:batch_size) { 10 } + let(:mark_projects_as_not_deployed) { false } + let(:service) { described_class.new(Rails.logger, ignore_invalid_entries: false, mark_projects_as_not_deployed: mark_projects_as_not_deployed) } - it 'does not try to migrate pages if pages are not deployed' do - expect(::Pages::MigrateLegacyStorageToDeploymentService).not_to receive(:new) + shared_examples "migrates projects properly" do + it 'does not try to migrate pages if pages are not deployed' do + expect(::Pages::MigrateLegacyStorageToDeploymentService).not_to receive(:new) - expect(service.execute).to eq(migrated: 0, errored: 0) - end + is_expected.to eq(migrated: 0, errored: 0) + end - it 'uses multiple threads' do - projects = create_list(:project, 20) - projects.each do |project| - project.mark_pages_as_deployed + context 'when pages are marked as deployed' do + let(:project) { create(:project) } - FileUtils.mkdir_p File.join(project.pages_path, "public") - File.open(File.join(project.pages_path, "public/index.html"), "w") do |f| - f.write("Hello!") + before do + project.mark_pages_as_deployed end - end - - service = described_class.new(Rails.logger, migration_threads: 3, batch_size: 2, ignore_invalid_entries: false) - threads = Concurrent::Set.new + context 'when pages directory does not exist' do + context 'when mark_projects_as_not_deployed is set' do + let(:mark_projects_as_not_deployed) { true } - expect(service).to receive(:migrate_project).exactly(20).times.and_wrap_original do |m, *args| - threads.add(Thread.current) + it 'counts project as migrated' do + expect_next_instance_of(::Pages::MigrateLegacyStorageToDeploymentService, project, ignore_invalid_entries: false, mark_projects_as_not_deployed: true) do |service| + expect(service).to receive(:execute).and_call_original + end - # sleep to be 100% certain that once thread can't consume all the queue - # it works without it, but I want to avoid making this test flaky - sleep(0.01) + is_expected.to eq(migrated: 1, errored: 0) + end + end - m.call(*args) - end + it 'counts project as errored' do + expect_next_instance_of(::Pages::MigrateLegacyStorageToDeploymentService, project, ignore_invalid_entries: false, mark_projects_as_not_deployed: false) do |service| + expect(service).to receive(:execute).and_call_original + end - expect(service.execute).to eq(migrated: 20, errored: 0) - expect(threads.length).to eq(3) - end + is_expected.to eq(migrated: 0, errored: 1) + end + end - context 'when pages are marked as deployed' do - let(:project) { create(:project) } + context 'when pages directory exists on disk' do + before do + FileUtils.mkdir_p File.join(project.pages_path, "public") + File.open(File.join(project.pages_path, "public/index.html"), "w") do |f| + f.write("Hello!") + end + end - before do - project.mark_pages_as_deployed - end + it 'migrates pages projects without deployments' do + expect_next_instance_of(::Pages::MigrateLegacyStorageToDeploymentService, project, ignore_invalid_entries: false, mark_projects_as_not_deployed: false) do |service| + expect(service).to receive(:execute).and_call_original + end - context 'when pages directory does not exist' do - it 'tries to migrate the project, but does not crash' do - expect_next_instance_of(::Pages::MigrateLegacyStorageToDeploymentService, project, ignore_invalid_entries: false) do |service| - expect(service).to receive(:execute).and_call_original + expect(project.pages_metadatum.reload.pages_deployment).to eq(nil) + expect(subject).to eq(migrated: 1, errored: 0) + expect(project.pages_metadatum.reload.pages_deployment).to be end - expect(service.execute).to eq(migrated: 0, errored: 1) + context 'when deployed already exists for the project' do + before do + deployment = create(:pages_deployment, project: project) + project.set_first_pages_deployment!(deployment) + end + + it 'does not try to migrate project' do + expect(::Pages::MigrateLegacyStorageToDeploymentService).not_to receive(:new) + + is_expected.to eq(migrated: 0, errored: 0) + end + end end end + end - context 'when pages directory exists on disk' do - before do - FileUtils.mkdir_p File.join(project.pages_path, "public") - File.open(File.join(project.pages_path, "public/index.html"), "w") do |f| - f.write("Hello!") + describe '#execute_with_threads' do + subject { service.execute_with_threads(threads: 3, batch_size: batch_size) } + + include_examples "migrates projects properly" + + context 'when there is work for multiple threads' do + let(:batch_size) { 2 } # override to force usage of multiple threads + + it 'uses multiple threads' do + projects = create_list(:project, 20) + projects.each do |project| + project.mark_pages_as_deployed + + FileUtils.mkdir_p File.join(project.pages_path, "public") + File.open(File.join(project.pages_path, "public/index.html"), "w") do |f| + f.write("Hello!") + end end - end - it 'migrates pages projects without deployments' do - expect_next_instance_of(::Pages::MigrateLegacyStorageToDeploymentService, project, ignore_invalid_entries: false) do |service| - expect(service).to receive(:execute).and_call_original + threads = Concurrent::Set.new + + expect(service).to receive(:migrate_project).exactly(20).times.and_wrap_original do |m, *args| + threads.add(Thread.current) + + # sleep to be 100% certain that once thread can't consume all the queue + # it works without it, but I want to avoid making this test flaky + sleep(0.01) + + m.call(*args) end - expect do - expect(service.execute).to eq(migrated: 1, errored: 0) - end.to change { project.pages_metadatum.reload.pages_deployment }.from(nil) + is_expected.to eq(migrated: 20, errored: 0) + expect(threads.length).to eq(3) end + end + end - context 'when deployed already exists for the project' do - before do - deployment = create(:pages_deployment, project: project) - project.set_first_pages_deployment!(deployment) - end + describe "#execute_for_batch" do + subject { service.execute_for_batch(Project.ids) } - it 'does not try to migrate project' do - expect(::Pages::MigrateLegacyStorageToDeploymentService).not_to receive(:new) + include_examples "migrates projects properly" - expect(service.execute).to eq(migrated: 0, errored: 0) + it 'only tries to migrate projects with passed ids' do + projects = create_list(:project, 5) + + projects.each(&:mark_pages_as_deployed) + projects_to_migrate = projects.first(3) + + projects_to_migrate.each do |project| + expect_next_instance_of(::Pages::MigrateLegacyStorageToDeploymentService, project, ignore_invalid_entries: false, mark_projects_as_not_deployed: false) do |service| + expect(service).to receive(:execute).and_call_original end end + + expect(service.execute_for_batch(projects_to_migrate.pluck(:id))).to eq(migrated: 0, errored: 3) end end end diff --git a/spec/services/pages/migrate_legacy_storage_to_deployment_service_spec.rb b/spec/services/pages/migrate_legacy_storage_to_deployment_service_spec.rb index d95303c3e85..25f571a73d1 100644 --- a/spec/services/pages/migrate_legacy_storage_to_deployment_service_spec.rb +++ b/spec/services/pages/migrate_legacy_storage_to_deployment_service_spec.rb @@ -14,44 +14,44 @@ RSpec.describe Pages::MigrateLegacyStorageToDeploymentService do expect(described_class.new(project, ignore_invalid_entries: true).execute[:status]).to eq(:error) end - it 'marks pages as not deployed if public directory is absent' do - project.mark_pages_as_deployed - - expect(project.pages_metadatum.reload.deployed).to eq(true) + context 'when mark_projects_as_not_deployed is passed' do + let(:service) { described_class.new(project, mark_projects_as_not_deployed: true) } - expect(service.execute).to( - eq(status: :error, - message: "Can't create zip archive: Can not find valid public dir in #{project.pages_path}") - ) + it 'marks pages as not deployed if public directory is absent and invalid entries are ignored' do + project.mark_pages_as_deployed + expect(project.pages_metadatum.reload.deployed).to eq(true) - expect(project.pages_metadatum.reload.deployed).to eq(false) - end + expect(service.execute).to( + eq(status: :success, + message: "Archive not created. Missing public directory in #{project.pages_path}? Marked project as not deployed") + ) - it 'does not mark pages as not deployed if public directory is absent but pages_deployment exists' do - deployment = create(:pages_deployment, project: project) - project.update_pages_deployment!(deployment) - project.mark_pages_as_deployed + expect(project.pages_metadatum.reload.deployed).to eq(false) + end - expect(project.pages_metadatum.reload.deployed).to eq(true) + it 'does not mark pages as not deployed if public directory is absent but pages_deployment exists' do + deployment = create(:pages_deployment, project: project) + project.update_pages_deployment!(deployment) + project.mark_pages_as_deployed + expect(project.pages_metadatum.reload.deployed).to eq(true) - expect(service.execute).to( - eq(status: :error, - message: "Can't create zip archive: Can not find valid public dir in #{project.pages_path}") - ) + expect(service.execute).to( + eq(status: :success, + message: "Archive not created. Missing public directory in #{project.pages_path}? Marked project as not deployed") + ) - expect(project.pages_metadatum.reload.deployed).to eq(true) + expect(project.pages_metadatum.reload.deployed).to eq(true) + end end - it 'does not mark pages as not deployed if public directory is absent but feature is disabled' do - stub_feature_flags(pages_migration_mark_as_not_deployed: false) - + it 'does not mark pages as not deployed if public directory is absent but invalid entries are not ignored' do project.mark_pages_as_deployed expect(project.pages_metadatum.reload.deployed).to eq(true) expect(service.execute).to( eq(status: :error, - message: "Can't create zip archive: Can not find valid public dir in #{project.pages_path}") + message: "Archive not created. Missing public directory in #{project.pages_path}") ) expect(project.pages_metadatum.reload.deployed).to eq(true) diff --git a/spec/services/pages/zip_directory_service_spec.rb b/spec/services/pages/zip_directory_service_spec.rb index 9de68dd62bb..9cce90c6c0d 100644 --- a/spec/services/pages/zip_directory_service_spec.rb +++ b/spec/services/pages/zip_directory_service_spec.rb @@ -12,8 +12,10 @@ RSpec.describe Pages::ZipDirectoryService do let(:ignore_invalid_entries) { false } + let(:service_directory) { @work_dir } + let(:service) do - described_class.new(@work_dir, ignore_invalid_entries: ignore_invalid_entries) + described_class.new(service_directory, ignore_invalid_entries: ignore_invalid_entries) end let(:result) do @@ -25,32 +27,32 @@ RSpec.describe Pages::ZipDirectoryService do let(:archive) { result[:archive_path] } let(:entries_count) { result[:entries_count] } - it 'returns error if project pages dir does not exist' do - expect(Gitlab::ErrorTracking).not_to receive(:track_exception) - - expect( - described_class.new("/tmp/not/existing/dir").execute - ).to eq(status: :error, message: "Can not find valid public dir in /tmp/not/existing/dir") + shared_examples 'handles invalid public directory' do + it 'returns success' do + expect(status).to eq(:success) + expect(archive).to be_nil + expect(entries_count).to be_nil + end end - it 'returns nils if there is no public directory and does not leave archive' do - expect(status).to eq(:error) - expect(message).to eq("Can not find valid public dir in #{@work_dir}") - expect(archive).to eq(nil) - expect(entries_count).to eq(nil) + context "when work direcotry doesn't exist" do + let(:service_directory) { "/tmp/not/existing/dir" } - expect(File.exist?(File.join(@work_dir, '@migrated.zip'))).to eq(false) + include_examples 'handles invalid public directory' end - it 'returns nils if public directory is a symlink' do - create_dir('target') - create_file('./target/index.html', 'hello') - create_link("public", "./target") + context 'when public directory is absent' do + include_examples 'handles invalid public directory' + end + + context 'when public directory is a symlink' do + before do + create_dir('target') + create_file('./target/index.html', 'hello') + create_link("public", "./target") + end - expect(status).to eq(:error) - expect(message).to eq("Can not find valid public dir in #{@work_dir}") - expect(archive).to eq(nil) - expect(entries_count).to eq(nil) + include_examples 'handles invalid public directory' end context 'when there is a public directory' do diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index 306d87eefb8..e0d6b9afcff 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -673,7 +673,17 @@ RSpec.describe Projects::CreateService, '#execute' do expect(rugged.config['gitlab.fullpath']).to eq project.full_path end + it 'triggers PostCreationWorker' do + expect(Projects::PostCreationWorker).to receive(:perform_async).with(a_kind_of(Integer)) + + create_project(user, opts) + end + context 'when project has access to shared service' do + before do + stub_feature_flags(projects_post_creation_worker: false) + end + context 'Prometheus application is shared via group cluster' do let(:cluster) { create(:cluster, :group, groups: [group]) } let(:group) do @@ -714,9 +724,7 @@ RSpec.describe Projects::CreateService, '#execute' do it 'cleans invalid record and logs warning', :aggregate_failures do invalid_service_record = build(:prometheus_service, properties: { api_url: nil, manual_configuration: true }.to_json) - allow_next_instance_of(Project) do |instance| - allow(instance).to receive(:build_prometheus_service).and_return(invalid_service_record) - end + allow(PrometheusService).to receive(:new).and_return(invalid_service_record) expect(Gitlab::ErrorTracking).to receive(:track_exception).with(an_instance_of(ActiveRecord::RecordInvalid), include(extra: { project_id: a_kind_of(Integer) })) project = create_project(user, opts) diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb index 5410e784cc0..b2a68bbd0aa 100644 --- a/spec/services/projects/destroy_service_spec.rb +++ b/spec/services/projects/destroy_service_spec.rb @@ -93,10 +93,26 @@ RSpec.describe Projects::DestroyService, :aggregate_failures do destroy_project(project, user, {}) end - it 'performs cancel for project ci pipelines' do - expect(::Ci::AbortProjectPipelinesService).to receive_message_chain(:new, :execute).with(project) + context 'with abort_deleted_project_pipelines feature disabled' do + it 'does not cancel project ci pipelines' do + stub_feature_flags(abort_deleted_project_pipelines: false) - destroy_project(project, user, {}) + expect(::Ci::AbortPipelinesService).not_to receive(:new) + + destroy_project(project, user, {}) + end + end + + context 'with abort_deleted_project_pipelines feature enabled' do + it 'performs cancel for project ci pipelines' do + stub_feature_flags(abort_deleted_project_pipelines: true) + pipelines = build_list(:ci_pipeline, 3, :running) + allow(project).to receive(:all_pipelines).and_return(pipelines) + + expect(::Ci::AbortPipelinesService).to receive_message_chain(:new, :execute).with(pipelines, :project_deleted) + + destroy_project(project, user, {}) + end end context 'when project has remote mirrors' do diff --git a/spec/services/projects/update_pages_configuration_service_spec.rb b/spec/services/projects/update_pages_configuration_service_spec.rb index 9ef66a10f0d..58939ef4ada 100644 --- a/spec/services/projects/update_pages_configuration_service_spec.rb +++ b/spec/services/projects/update_pages_configuration_service_spec.rb @@ -35,7 +35,7 @@ RSpec.describe Projects::UpdatePagesConfigurationService do end it "doesn't update configuration files if updates on legacy storage are disabled" do - stub_feature_flags(pages_update_legacy_storage: false) + allow(Settings.pages.local_store).to receive(:enabled).and_return(false) expect(service).not_to receive(:update_file) diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb index b735f4b6bc2..b11607bc213 100644 --- a/spec/services/projects/update_pages_service_spec.rb +++ b/spec/services/projects/update_pages_service_spec.rb @@ -16,6 +16,7 @@ RSpec.describe Projects::UpdatePagesService do subject { described_class.new(project, build) } before do + stub_feature_flags(skip_pages_deploy_to_legacy_storage: false) project.legacy_remove_pages end @@ -55,8 +56,24 @@ RSpec.describe Projects::UpdatePagesService do end end + it 'creates a temporary directory with the project and build ID' do + expect(Dir).to receive(:mktmpdir).with("project-#{project.id}-build-#{build.id}-", anything).and_call_original + + subject.execute + end + it "doesn't deploy to legacy storage if it's disabled" do - stub_feature_flags(pages_update_legacy_storage: false) + allow(Settings.pages.local_store).to receive(:enabled).and_return(false) + + expect(execute).to eq(:success) + expect(project.pages_deployed?).to be_truthy + + expect(File.exist?(File.join(project.pages_path, 'public', 'index.html'))).to eq(false) + end + + it "doesn't deploy to legacy storage if skip_pages_deploy_to_legacy_storage is enabled" do + allow(Settings.pages.local_store).to receive(:enabled).and_return(true) + stub_feature_flags(skip_pages_deploy_to_legacy_storage: true) expect(execute).to eq(:success) expect(project.pages_deployed?).to be_truthy diff --git a/spec/services/projects/update_remote_mirror_service_spec.rb b/spec/services/projects/update_remote_mirror_service_spec.rb index 30530da8013..96dbfe8e0b7 100644 --- a/spec/services/projects/update_remote_mirror_service_spec.rb +++ b/spec/services/projects/update_remote_mirror_service_spec.rb @@ -12,7 +12,9 @@ RSpec.describe Projects::UpdateRemoteMirrorService do subject(:service) { described_class.new(project, project.creator) } describe '#execute' do - subject(:execute!) { service.execute(remote_mirror, 0) } + let(:retries) { 0 } + + subject(:execute!) { service.execute(remote_mirror, retries) } before do project.repository.add_branch(project.owner, 'existing-branch', 'master') @@ -62,8 +64,18 @@ RSpec.describe Projects::UpdateRemoteMirrorService do allow(Gitlab::UrlBlocker).to receive(:blocked_url?).and_return(true) end - it 'fails and returns error status' do + it 'hard retries and returns error status' do expect(execute!).to eq(status: :error, message: 'The remote mirror URL is invalid.') + expect(remote_mirror).to be_to_retry + end + + context 'when retries are exceeded' do + let(:retries) { 4 } + + it 'hard fails and returns error status' do + expect(execute!).to eq(status: :error, message: 'The remote mirror URL is invalid.') + expect(remote_mirror).to be_failed + end end end diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index bf35e72a037..9df238c6dac 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -368,24 +368,24 @@ RSpec.describe QuickActions::InterpretService do spent_at: DateTime.current.to_date }) end - - it 'returns the spend_time message including the formatted duration and verb' do - _, _, message = service.execute('/spend -120m', issuable) - - expect(message).to eq('Subtracted 2h spent time.') - end end shared_examples 'spend command with negative time' do - it 'populates spend_time: -1800 if content contains /spend -30m' do + it 'populates spend_time: -7200 if content contains -120m' do _, updates, _ = service.execute(content, issuable) expect(updates).to eq(spend_time: { - duration: -1800, + duration: -7200, user_id: developer.id, spent_at: DateTime.current.to_date }) end + + it 'returns the spend_time message including the formatted duration and verb' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq('Subtracted 2h spent time.') + end end shared_examples 'spend command with valid date' do @@ -478,7 +478,7 @@ RSpec.describe QuickActions::InterpretService do end end - shared_examples 'empty command' do |error_msg| + shared_examples 'failed command' do |error_msg| it 'populates {} if content contains an unsupported command' do _, updates, _ = service.execute(content, issuable) @@ -607,10 +607,10 @@ RSpec.describe QuickActions::InterpretService do issuable.update!(confidential: true) end - it 'does not return the success message' do + it 'returns an error message' do _, _, message = service.execute(content, issuable) - expect(message).to be_empty + expect(message).to eq('Could not apply confidential command.') end it 'is not part of the available commands' do @@ -728,7 +728,7 @@ RSpec.describe QuickActions::InterpretService do context 'can not be merged when logged user does not have permissions' do let(:service) { described_class.new(project, create(:user)) } - it_behaves_like 'empty command' do + it_behaves_like 'failed command', 'Could not apply merge command.' do let(:content) { "/merge" } let(:issuable) { merge_request } end @@ -737,7 +737,7 @@ RSpec.describe QuickActions::InterpretService do context 'can not be merged when sha does not match' do let(:service) { described_class.new(project, developer, { merge_request_diff_head_sha: 'othersha' }) } - it_behaves_like 'empty command' do + it_behaves_like 'failed command', 'Could not apply merge command.' do let(:content) { "/merge" } let(:issuable) { merge_request } end @@ -755,21 +755,21 @@ RSpec.describe QuickActions::InterpretService do end context 'issue can not be merged' do - it_behaves_like 'empty command' do + it_behaves_like 'failed command', 'Could not apply merge command.' do let(:content) { "/merge" } let(:issuable) { issue } end end context 'non persisted merge request cant be merged' do - it_behaves_like 'empty command' do + it_behaves_like 'failed command', 'Could not apply merge command.' do let(:content) { "/merge" } let(:issuable) { build(:merge_request) } end end context 'not persisted merge request can not be merged' do - it_behaves_like 'empty command' do + it_behaves_like 'failed command', 'Could not apply merge command.' do let(:content) { "/merge" } let(:issuable) { build(:merge_request, source_project: project) } end @@ -786,7 +786,7 @@ RSpec.describe QuickActions::InterpretService do let(:issuable) { merge_request } end - it_behaves_like 'empty command' do + it_behaves_like 'failed command' do let(:content) { '/title' } let(:issuable) { issue } end @@ -869,12 +869,12 @@ RSpec.describe QuickActions::InterpretService do end end - it_behaves_like 'empty command', "Failed to assign a user because no user was found." do + it_behaves_like 'failed command', "Failed to assign a user because no user was found." do let(:content) { '/assign @abcd1234' } let(:issuable) { issue } end - it_behaves_like 'empty command', "Failed to assign a user because no user was found." do + it_behaves_like 'failed command', "Failed to assign a user because no user was found." do let(:content) { '/assign' } let(:issuable) { issue } end @@ -890,7 +890,7 @@ RSpec.describe QuickActions::InterpretService do context 'with an issue instead of a merge request' do let(:issuable) { issue } - it_behaves_like 'empty command' + it_behaves_like 'failed command', 'Could not apply assign_reviewer command.' end # CE does not have multiple reviewers @@ -935,7 +935,7 @@ RSpec.describe QuickActions::InterpretService do context 'with an incorrect user' do let(:content) { '/assign_reviewer @abcd1234' } - it_behaves_like 'empty command', "Failed to assign a reviewer because no user was found." + it_behaves_like 'failed command', "Failed to assign a reviewer because no user was found." end context 'with the "reviewer" alias' do @@ -953,7 +953,7 @@ RSpec.describe QuickActions::InterpretService do context 'with no user' do let(:content) { '/assign_reviewer' } - it_behaves_like 'empty command', "Failed to assign a reviewer because no user was found." + it_behaves_like 'failed command', "Failed to assign a reviewer because no user was found." end context 'includes only the user reference with extra text' do @@ -977,7 +977,7 @@ RSpec.describe QuickActions::InterpretService do context 'with an issue instead of a merge request' do let(:issuable) { issue } - it_behaves_like 'empty command' + it_behaves_like 'failed command', 'Could not apply unassign_reviewer command.' end context 'with anything after the command' do @@ -1035,14 +1035,20 @@ RSpec.describe QuickActions::InterpretService do end end - it_behaves_like 'milestone command' do - let(:content) { "/milestone %#{milestone.title}" } - let(:issuable) { issue } - end + context 'project milestones' do + before do + milestone + end - it_behaves_like 'milestone command' do - let(:content) { "/milestone %#{milestone.title}" } - let(:issuable) { merge_request } + it_behaves_like 'milestone command' do + let(:content) { "/milestone %#{milestone.title}" } + let(:issuable) { issue } + end + + it_behaves_like 'milestone command' do + let(:content) { "/milestone %#{milestone.title}" } + let(:issuable) { merge_request } + end end context 'only group milestones available' do @@ -1181,7 +1187,7 @@ RSpec.describe QuickActions::InterpretService do let(:issuable) { merge_request } end - it_behaves_like 'empty command' do + it_behaves_like 'failed command', 'Could not apply due command.' do let(:content) { '/due 2016-08-28' } let(:issuable) { merge_request } end @@ -1211,7 +1217,7 @@ RSpec.describe QuickActions::InterpretService do let(:issuable) { merge_request } end - it_behaves_like 'empty command' do + it_behaves_like 'failed command', 'Could not apply remove_due_date command.' do let(:content) { '/remove_due_date' } let(:issuable) { merge_request } end @@ -1221,12 +1227,12 @@ RSpec.describe QuickActions::InterpretService do let(:issuable) { issue } end - it_behaves_like 'empty command' do + it_behaves_like 'failed command' do let(:content) { '/estimate' } let(:issuable) { issue } end - it_behaves_like 'empty command' do + it_behaves_like 'failed command' do let(:content) { '/estimate abc' } let(:issuable) { issue } end @@ -1236,8 +1242,18 @@ RSpec.describe QuickActions::InterpretService do let(:issuable) { issue } end + it_behaves_like 'spend command' do + let(:content) { '/spent 1h' } + let(:issuable) { issue } + end + it_behaves_like 'spend command with negative time' do - let(:content) { '/spend -30m' } + let(:content) { '/spend -120m' } + let(:issuable) { issue } + end + + it_behaves_like 'spend command with negative time' do + let(:content) { '/spent -120m' } let(:issuable) { issue } end @@ -1247,26 +1263,52 @@ RSpec.describe QuickActions::InterpretService do let(:issuable) { issue } end + it_behaves_like 'spend command with valid date' do + let(:date) { '2016-02-02' } + let(:content) { "/spent 30m #{date}" } + let(:issuable) { issue } + end + it_behaves_like 'spend command with invalid date' do let(:content) { '/spend 30m 17-99-99' } let(:issuable) { issue } end + it_behaves_like 'spend command with invalid date' do + let(:content) { '/spent 30m 17-99-99' } + let(:issuable) { issue } + end + it_behaves_like 'spend command with future date' do let(:content) { '/spend 30m 6017-10-10' } let(:issuable) { issue } end - it_behaves_like 'empty command' do + it_behaves_like 'spend command with future date' do + let(:content) { '/spent 30m 6017-10-10' } + let(:issuable) { issue } + end + + it_behaves_like 'failed command' do let(:content) { '/spend' } let(:issuable) { issue } end - it_behaves_like 'empty command' do + it_behaves_like 'failed command' do + let(:content) { '/spent' } + let(:issuable) { issue } + end + + it_behaves_like 'failed command' do let(:content) { '/spend abc' } let(:issuable) { issue } end + it_behaves_like 'failed command' do + let(:content) { '/spent abc' } + let(:issuable) { issue } + end + it_behaves_like 'remove_estimate command' do let(:content) { '/remove_estimate' } let(:issuable) { issue } @@ -1323,7 +1365,7 @@ RSpec.describe QuickActions::InterpretService do end context 'if issuable is a Commit' do - it_behaves_like 'empty command' do + it_behaves_like 'failed command', 'Could not apply todo command.' do let(:issuable) { commit } end end @@ -1379,7 +1421,7 @@ RSpec.describe QuickActions::InterpretService do end end - it_behaves_like 'empty command' do + it_behaves_like 'failed command' do let(:content) { '/copy_metadata' } let(:issuable) { issue } end @@ -1419,19 +1461,19 @@ RSpec.describe QuickActions::InterpretService do end context 'cross project references' do - it_behaves_like 'empty command' do + it_behaves_like 'failed command' do let(:other_project) { create(:project, :public) } let(:source_issuable) { create(:labeled_issue, project: other_project, labels: [todo_label, inreview_label]) } let(:content) { "/copy_metadata #{source_issuable.to_reference(project)}" } let(:issuable) { issue } end - it_behaves_like 'empty command' do + it_behaves_like 'failed command' do let(:content) { "/copy_metadata imaginary##{non_existing_record_iid}" } let(:issuable) { issue } end - it_behaves_like 'empty command' do + it_behaves_like 'failed command' do let(:other_project) { create(:project, :private) } let(:source_issuable) { create(:issue, project: other_project) } @@ -1448,7 +1490,7 @@ RSpec.describe QuickActions::InterpretService do let(:issuable) { issue } end - it_behaves_like 'empty command' do + it_behaves_like 'failed command' do let(:content) { '/duplicate' } let(:issuable) { issue } end @@ -1461,12 +1503,12 @@ RSpec.describe QuickActions::InterpretService do let(:issuable) { issue } end - it_behaves_like 'empty command', _('Failed to mark this issue as a duplicate because referenced issue was not found.') do + it_behaves_like 'failed command', _('Failed to mark this issue as a duplicate because referenced issue was not found.') do let(:content) { "/duplicate imaginary##{non_existing_record_iid}" } let(:issuable) { issue } end - it_behaves_like 'empty command', _('Failed to mark this issue as a duplicate because referenced issue was not found.') do + it_behaves_like 'failed command', _('Failed to mark this issue as a duplicate because referenced issue was not found.') do let(:other_project) { create(:project, :private) } let(:issue_duplicate) { create(:issue, project: other_project) } @@ -1481,62 +1523,62 @@ RSpec.describe QuickActions::InterpretService do let(:issue) { create(:issue, project: project, author: visitor) } let(:service) { described_class.new(project, visitor) } - it_behaves_like 'empty command' do + it_behaves_like 'failed command', 'Could not apply assign command.' do let(:content) { "/assign @#{developer.username}" } let(:issuable) { issue } end - it_behaves_like 'empty command' do + it_behaves_like 'failed command', 'Could not apply unassign command.' do let(:content) { '/unassign' } let(:issuable) { issue } end - it_behaves_like 'empty command' do + it_behaves_like 'failed command', 'Could not apply milestone command.' do let(:content) { "/milestone %#{milestone.title}" } let(:issuable) { issue } end - it_behaves_like 'empty command' do + it_behaves_like 'failed command', 'Could not apply remove_milestone command.' do let(:content) { '/remove_milestone' } let(:issuable) { issue } end - it_behaves_like 'empty command' do + it_behaves_like 'failed command', 'Could not apply label command.' do let(:content) { %(/label ~"#{inprogress.title}" ~#{bug.title} ~unknown) } let(:issuable) { issue } end - it_behaves_like 'empty command' do + it_behaves_like 'failed command', 'Could not apply unlabel command.' do let(:content) { %(/unlabel ~"#{inprogress.title}") } let(:issuable) { issue } end - it_behaves_like 'empty command' do + it_behaves_like 'failed command', 'Could not apply relabel command.' do let(:content) { %(/relabel ~"#{inprogress.title}") } let(:issuable) { issue } end - it_behaves_like 'empty command' do + it_behaves_like 'failed command', 'Could not apply due command.' do let(:content) { '/due tomorrow' } let(:issuable) { issue } end - it_behaves_like 'empty command' do + it_behaves_like 'failed command', 'Could not apply remove_due_date command.' do let(:content) { '/remove_due_date' } let(:issuable) { issue } end - it_behaves_like 'empty command' do + it_behaves_like 'failed command', 'Could not apply confidential command.' do let(:content) { '/confidential' } let(:issuable) { issue } end - it_behaves_like 'empty command' do + it_behaves_like 'failed command', 'Could not apply lock command.' do let(:content) { '/lock' } let(:issuable) { issue } end - it_behaves_like 'empty command' do + it_behaves_like 'failed command', 'Could not apply unlock command.' do let(:content) { '/unlock' } let(:issuable) { issue } end @@ -1554,19 +1596,19 @@ RSpec.describe QuickActions::InterpretService do end context 'ignores command with no argument' do - it_behaves_like 'empty command' do + it_behaves_like 'failed command' do let(:content) { '/award' } let(:issuable) { issue } end end context 'ignores non-existing / invalid emojis' do - it_behaves_like 'empty command' do + it_behaves_like 'failed command' do let(:content) { '/award noop' } let(:issuable) { issue } end - it_behaves_like 'empty command' do + it_behaves_like 'failed command' do let(:content) { '/award :lorem_ipsum:' } let(:issuable) { issue } end @@ -1576,7 +1618,7 @@ RSpec.describe QuickActions::InterpretService do let(:content) { '/award :100:' } let(:issuable) { commit } - it_behaves_like 'empty command' + it_behaves_like 'failed command', 'Could not apply award command.' end end @@ -1622,14 +1664,14 @@ RSpec.describe QuickActions::InterpretService do end context 'ignores command with no argument' do - it_behaves_like 'empty command' do + it_behaves_like 'failed command', 'Could not apply target_branch command.' do let(:content) { '/target_branch' } let(:issuable) { another_merge_request } end end context 'ignores non-existing target branch' do - it_behaves_like 'empty command' do + it_behaves_like 'failed command', 'Could not apply target_branch command.' do let(:content) { '/target_branch totally_non_existing_branch' } let(:issuable) { another_merge_request } end @@ -1697,34 +1739,34 @@ RSpec.describe QuickActions::InterpretService do create(:board, project: project) end - it_behaves_like 'empty command' + it_behaves_like 'failed command', 'Could not apply board_move command.' end context 'if the given label does not exist' do let(:issuable) { issue } let(:content) { '/board_move ~"Fake Label"' } - it_behaves_like 'empty command', 'Failed to move this issue because label was not found.' + it_behaves_like 'failed command', 'Failed to move this issue because label was not found.' end context 'if multiple labels are given' do let(:issuable) { issue } let(:content) { %{/board_move ~"#{inreview.title}" ~"#{todo.title}"} } - it_behaves_like 'empty command', 'Failed to move this issue because only a single label can be provided.' + it_behaves_like 'failed command', 'Failed to move this issue because only a single label can be provided.' end context 'if the given label is not a list on the board' do let(:issuable) { issue } let(:content) { %{/board_move ~"#{bug.title}"} } - it_behaves_like 'empty command', 'Failed to move this issue because label was not found.' + it_behaves_like 'failed command', 'Failed to move this issue because label was not found.' end context 'if issuable is not an Issue' do let(:issuable) { merge_request } - it_behaves_like 'empty command' + it_behaves_like 'failed command', 'Could not apply board_move command.' end end @@ -1732,7 +1774,7 @@ RSpec.describe QuickActions::InterpretService do let(:issuable) { commit } context 'ignores command with no argument' do - it_behaves_like 'empty command' do + it_behaves_like 'failed command' do let(:content) { '/tag' } end end @@ -1797,7 +1839,7 @@ RSpec.describe QuickActions::InterpretService do context 'if issuable is not an Issue' do let(:issuable) { merge_request } - it_behaves_like 'empty command' + it_behaves_like 'failed command', 'Could not apply create_merge_request command.' end context "when logged user cannot create_merge_requests in the project" do @@ -1807,14 +1849,14 @@ RSpec.describe QuickActions::InterpretService do project.add_developer(developer) end - it_behaves_like 'empty command' + it_behaves_like 'failed command', 'Could not apply create_merge_request command.' end context 'when logged user cannot push code to the project' do let(:project) { create(:project, :private) } let(:service) { described_class.new(project, create(:user)) } - it_behaves_like 'empty command' + it_behaves_like 'failed command', 'Could not apply create_merge_request command.' end it 'populates create_merge_request with branch_name and issue iid' do @@ -1953,7 +1995,7 @@ RSpec.describe QuickActions::InterpretService do context 'invite_email command' do let_it_be(:issuable) { issue } - it_behaves_like 'empty command', "No email participants were added. Either none were provided, or they already exist." do + it_behaves_like 'failed command', "No email participants were added. Either none were provided, or they already exist." do let(:content) { '/invite_email' } end @@ -1964,7 +2006,7 @@ RSpec.describe QuickActions::InterpretService do issuable.issue_email_participants.create!(email: "a@gitlab.com") end - it_behaves_like 'empty command', "No email participants were added. Either none were provided, or they already exist." + it_behaves_like 'failed command', "No email participants were added. Either none were provided, or they already exist." end context 'with new email participants' do @@ -2241,10 +2283,14 @@ RSpec.describe QuickActions::InterpretService do end describe 'spend command' do - let(:content) { '/spend -120m' } + it 'includes the formatted duration and proper verb when using /spend' do + _, explanations = service.explain('/spend -120m', issue) - it 'includes the formatted duration and proper verb' do - _, explanations = service.explain(content, issue) + expect(explanations).to eq(['Subtracts 2h spent time.']) + end + + it 'includes the formatted duration and proper verb when using /spent' do + _, explanations = service.explain('/spent -120m', issue) expect(explanations).to eq(['Subtracts 2h spent time.']) end diff --git a/spec/services/repositories/changelog_service_spec.rb b/spec/services/repositories/changelog_service_spec.rb index dab38445ccf..02d60f076ca 100644 --- a/spec/services/repositories/changelog_service_spec.rb +++ b/spec/services/repositories/changelog_service_spec.rb @@ -130,13 +130,14 @@ RSpec.describe Repositories::ChangelogService do describe '#start_of_commit_range' do let(:project) { build_stubbed(:project) } let(:user) { build_stubbed(:user) } + let(:config) { Gitlab::Changelog::Config.new(project) } context 'when the "from" argument is specified' do it 'returns the value of the argument' do service = described_class .new(project, user, version: '1.0.0', from: 'foo', to: 'bar') - expect(service.start_of_commit_range).to eq('foo') + expect(service.start_of_commit_range(config)).to eq('foo') end end @@ -145,12 +146,12 @@ RSpec.describe Repositories::ChangelogService do service = described_class .new(project, user, version: '1.0.0', to: 'bar') - finder_spy = instance_spy(Repositories::PreviousTagFinder) + finder_spy = instance_spy(Repositories::ChangelogTagFinder) tag = double(:tag, target_commit: double(:commit, id: '123')) - allow(Repositories::PreviousTagFinder) + allow(Repositories::ChangelogTagFinder) .to receive(:new) - .with(project) + .with(project, regex: an_instance_of(String)) .and_return(finder_spy) allow(finder_spy) @@ -158,18 +159,18 @@ RSpec.describe Repositories::ChangelogService do .with('1.0.0') .and_return(tag) - expect(service.start_of_commit_range).to eq('123') + expect(service.start_of_commit_range(config)).to eq('123') end it 'raises an error when no tag is found' do service = described_class .new(project, user, version: '1.0.0', to: 'bar') - finder_spy = instance_spy(Repositories::PreviousTagFinder) + finder_spy = instance_spy(Repositories::ChangelogTagFinder) - allow(Repositories::PreviousTagFinder) + allow(Repositories::ChangelogTagFinder) .to receive(:new) - .with(project) + .with(project, regex: an_instance_of(String)) .and_return(finder_spy) allow(finder_spy) @@ -177,7 +178,7 @@ RSpec.describe Repositories::ChangelogService do .with('1.0.0') .and_return(nil) - expect { service.start_of_commit_range } + expect { service.start_of_commit_range(config) } .to raise_error(Gitlab::Changelog::Error) end end diff --git a/spec/services/security/ci_configuration/sast_parser_service_spec.rb b/spec/services/security/ci_configuration/sast_parser_service_spec.rb index 21490f993c7..4ebaddcfa4e 100644 --- a/spec/services/security/ci_configuration/sast_parser_service_spec.rb +++ b/spec/services/security/ci_configuration/sast_parser_service_spec.rb @@ -12,8 +12,8 @@ RSpec.describe Security::CiConfiguration::SastParserService do let(:sast_analyzer_image_tag) { configuration['global'][2] } let(:sast_pipeline_stage) { configuration['pipeline'][0] } let(:sast_search_max_depth) { configuration['pipeline'][1] } - let(:brakeman) { configuration['analyzers'][0] } - let(:bandit) { configuration['analyzers'][1] } + let(:bandit) { configuration['analyzers'][0] } + let(:brakeman) { configuration['analyzers'][1] } let(:sast_brakeman_level) { brakeman['variables'][0] } it 'parses the configuration for SAST' do diff --git a/spec/services/spam/spam_action_service_spec.rb b/spec/services/spam/spam_action_service_spec.rb index 371923f1518..e8ac826df1c 100644 --- a/spec/services/spam/spam_action_service_spec.rb +++ b/spec/services/spam/spam_action_service_spec.rb @@ -5,6 +5,8 @@ require 'spec_helper' RSpec.describe Spam::SpamActionService do include_context 'includes Spam constants' + let(:request) { double(:request, env: env, headers: {}) } + let(:issue) { create(:issue, project: project, author: user) } let(:fake_ip) { '1.2.3.4' } let(:fake_user_agent) { 'fake-user-agent' } let(:fake_referrer) { 'fake-http-referrer' } @@ -14,11 +16,8 @@ RSpec.describe Spam::SpamActionService do 'HTTP_REFERRER' => fake_referrer } end - let(:request) { double(:request, env: env) } - let_it_be(:project) { create(:project, :public) } let_it_be(:user) { create(:user) } - let(:issue) { create(:issue, project: project, author: user) } before do issue.spam = false @@ -48,7 +47,7 @@ RSpec.describe Spam::SpamActionService do shared_examples 'creates a spam log' do it do - expect { subject }.to change { SpamLog.count }.by(1) + expect { subject }.to change(SpamLog, :count).by(1) new_spam_log = SpamLog.last expect(new_spam_log.user_id).to eq(user.id) @@ -62,7 +61,7 @@ RSpec.describe Spam::SpamActionService do end describe '#execute' do - let(:request) { double(:request, env: env) } + let(:request) { double(:request, env: env, headers: nil) } let(:fake_captcha_verification_service) { double(:captcha_verification_service) } let(:fake_verdict_service) { double(:spam_verdict_service) } let(:allowlisted) { false } @@ -70,7 +69,7 @@ RSpec.describe Spam::SpamActionService do let(:captcha_response) { 'abc123' } let(:spam_log_id) { existing_spam_log.id } let(:spam_params) do - Spam::SpamActionService.filter_spam_params!( + ::Spam::SpamParams.new( api: api, captcha_response: captcha_response, spam_log_id: spam_log_id @@ -111,10 +110,30 @@ RSpec.describe Spam::SpamActionService do allow(Spam::SpamVerdictService).to receive(:new).with(verdict_service_args).and_return(fake_verdict_service) end + context 'when the captcha params are passed in the headers' do + let(:request) { double(:request, env: env, headers: headers) } + let(:spam_params) { Spam::SpamActionService.filter_spam_params!({ api: api }, request) } + let(:headers) do + { + 'X-GitLab-Captcha-Response' => captcha_response, + 'X-GitLab-Spam-Log-Id' => spam_log_id + } + end + + it 'extracts the headers correctly' do + expect(fake_captcha_verification_service) + .to receive(:execute).with(captcha_response: captcha_response, request: request).and_return(true) + expect(SpamLog) + .to receive(:verify_recaptcha!).with(user_id: user.id, id: spam_log_id) + + subject + end + end + context 'when captcha response verification returns true' do before do - expect(fake_captcha_verification_service) - .to receive(:execute).with(captcha_response: captcha_response, request: request) { true } + allow(fake_captcha_verification_service) + .to receive(:execute).with(captcha_response: captcha_response, request: request).and_return(true) end it "doesn't check with the SpamVerdictService" do @@ -136,8 +155,8 @@ RSpec.describe Spam::SpamActionService do context 'when captcha response verification returns false' do before do - expect(fake_captcha_verification_service) - .to receive(:execute).with(captcha_response: captcha_response, request: request) { false } + allow(fake_captcha_verification_service) + .to receive(:execute).with(captcha_response: captcha_response, request: request).and_return(false) end context 'when spammable attributes have not changed' do @@ -146,21 +165,20 @@ RSpec.describe Spam::SpamActionService do end it 'does not create a spam log' do - expect { subject } - .not_to change { SpamLog.count } + expect { subject }.not_to change(SpamLog, :count) end end context 'when spammable attributes have changed' do let(:expected_service_check_response_message) do - /check Issue spammable model for any errors or captcha requirement/ + /Check Issue spammable model for any errors or CAPTCHA requirement/ end before do - issue.description = 'SPAM!' + issue.description = 'Lovely Spam! Wonderful Spam!' end - context 'if allowlisted' do + context 'when allowlisted' do let(:allowlisted) { true } it 'does not perform spam check' do @@ -229,7 +247,7 @@ RSpec.describe Spam::SpamActionService do response = subject expect(response.message).to match(expected_service_check_response_message) - expect(issue.needs_recaptcha?).to be_truthy + expect(issue).to be_needs_recaptcha end end @@ -253,8 +271,7 @@ RSpec.describe Spam::SpamActionService do end it 'does not create a spam log' do - expect { subject } - .not_to change { SpamLog.count } + expect { subject }.not_to change(SpamLog, :count) end it 'clears spam flags' do @@ -264,9 +281,9 @@ RSpec.describe Spam::SpamActionService do end end - context 'spam verdict service options' do + context 'with spam verdict service options' do before do - allow(fake_verdict_service).to receive(:execute) { ALLOW } + allow(fake_verdict_service).to receive(:execute).and_return(ALLOW) end context 'when the request is nil' do diff --git a/spec/services/submit_usage_ping_service_spec.rb b/spec/services/submit_usage_ping_service_spec.rb index 24afa83ef2c..53cc33afcff 100644 --- a/spec/services/submit_usage_ping_service_spec.rb +++ b/spec/services/submit_usage_ping_service_spec.rb @@ -6,6 +6,7 @@ RSpec.describe SubmitUsagePingService do include StubRequests include UsageDataHelpers + let(:usage_data_id) { 31643 } let(:score_params) do { score: { @@ -40,6 +41,8 @@ RSpec.describe SubmitUsagePingService do leader_service_desk_issues: 15.8, instance_service_desk_issues: 15.1, + usage_data_id: usage_data_id, + non_existing_column: 'value' } } @@ -47,7 +50,6 @@ RSpec.describe SubmitUsagePingService do let(:with_dev_ops_score_params) { { dev_ops_score: score_params[:score] } } let(:with_conv_index_params) { { conv_index: score_params[:score] } } - let(:without_dev_ops_score_params) { { dev_ops_score: {} } } shared_examples 'does not run' do it do @@ -103,7 +105,7 @@ RSpec.describe SubmitUsagePingService do end it 'sends a POST request' do - response = stub_response(body: without_dev_ops_score_params) + response = stub_response(body: with_dev_ops_score_params) subject.execute @@ -111,7 +113,7 @@ RSpec.describe SubmitUsagePingService do end it 'forces a refresh of usage data statistics before submitting' do - stub_response(body: without_dev_ops_score_params) + stub_response(body: with_dev_ops_score_params) expect(Gitlab::UsageData).to receive(:data).with(force_refresh: true).and_call_original @@ -124,6 +126,33 @@ RSpec.describe SubmitUsagePingService do end it_behaves_like 'saves DevOps report data from the response' + + it 'saves usage_data_id to version_usage_data_id_value' do + recorded_at = Time.current + usage_data = { uuid: 'uuid', recorded_at: recorded_at } + + expect(Gitlab::UsageData).to receive(:data).with(force_refresh: true).and_return(usage_data) + + subject.execute + + raw_usage_data = RawUsageData.find_by(recorded_at: recorded_at) + + expect(raw_usage_data.version_usage_data_id_value).to eq(31643) + end + end + + context 'when version app usage_data_id is invalid' do + let(:usage_data_id) { -1000 } + + before do + stub_response(body: with_conv_index_params) + end + + it 'raises an exception' do + expect { subject.execute }.to raise_error(described_class::SubmissionError) do |error| + expect(error.message).to include('Invalid usage_data_id in response: -1000') + end + end end context 'when DevOps report data is passed' do diff --git a/spec/services/submodules/update_service_spec.rb b/spec/services/submodules/update_service_spec.rb index e7f92d5ba28..1a53da7b9fe 100644 --- a/spec/services/submodules/update_service_spec.rb +++ b/spec/services/submodules/update_service_spec.rb @@ -90,7 +90,7 @@ RSpec.describe Submodules::UpdateService do let(:submodule) { '../six' } it_behaves_like 'returns error result' do - let(:error_message) { 'Invalid parameters' } + let(:error_message) { 'Invalid submodule path' } end end end diff --git a/spec/services/system_hooks_service_spec.rb b/spec/services/system_hooks_service_spec.rb index 446325e5f71..d8435c72896 100644 --- a/spec/services/system_hooks_service_spec.rb +++ b/spec/services/system_hooks_service_spec.rb @@ -113,37 +113,9 @@ RSpec.describe SystemHooksService do expect(data[:old_path]).to eq('old-path') end end - - context 'user_rename' do - it 'contains old and new username' do - allow(user).to receive(:username_before_last_save).and_return('old-username') - - data = event_data(user, :rename) - - expect(data).to include(:event_name, :name, :created_at, :updated_at, :email, :user_id, :username, :old_username) - expect(data[:username]).to eq(user.username) - expect(data[:old_username]).to eq(user.username_before_last_save) - end - end - - context 'user_failed_login' do - it 'contains state of user' do - user.ldap_block! - - data = event_data(user, :failed_login) - - expect(data).to include(:event_name, :name, :created_at, :updated_at, :email, :user_id, :username, :state) - expect(data[:username]).to eq(user.username) - expect(data[:state]).to eq('ldap_blocked') - end - end end context 'event names' do - it { expect(event_name(user, :create)).to eq "user_create" } - it { expect(event_name(user, :destroy)).to eq "user_destroy" } - it { expect(event_name(user, :rename)).to eq 'user_rename' } - it { expect(event_name(user, :failed_login)).to eq 'user_failed_login' } it { expect(event_name(project, :create)).to eq "project_create" } it { expect(event_name(project, :destroy)).to eq "project_destroy" } it { expect(event_name(project, :rename)).to eq "project_rename" } diff --git a/spec/services/system_notes/alert_management_service_spec.rb b/spec/services/system_notes/alert_management_service_spec.rb index fc71799d8c5..1c36a4036cc 100644 --- a/spec/services/system_notes/alert_management_service_spec.rb +++ b/spec/services/system_notes/alert_management_service_spec.rb @@ -69,7 +69,7 @@ RSpec.describe ::SystemNotes::AlertManagementService do end it 'has the appropriate message' do - expect(subject.note).to eq('logged a resolving alert from **Some Service**') + expect(subject.note).to eq('logged a recovery alert from **Some Service**') end end end diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb index 743dc080b06..59f936509df 100644 --- a/spec/services/todo_service_spec.rb +++ b/spec/services/todo_service_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe TodoService do + include AfterNextHelpers + let_it_be(:project) { create(:project, :repository) } let_it_be(:author) { create(:user) } let_it_be(:assignee) { create(:user) } @@ -343,19 +345,19 @@ RSpec.describe TodoService do describe '#destroy_target' do it 'refreshes the todos count cache for users with todos on the target' do - create(:todo, target: issue, user: john_doe, author: john_doe, project: issue.project) + create(:todo, state: :pending, target: issue, user: john_doe, author: john_doe, project: issue.project) - expect_any_instance_of(User).to receive(:update_todos_count_cache).and_call_original + expect_next(Users::UpdateTodoCountCacheService, [john_doe]).to receive(:execute) - service.destroy_target(issue) { } + service.destroy_target(issue) { issue.destroy! } end it 'does not refresh the todos count cache for users with only done todos on the target' do create(:todo, :done, target: issue, user: john_doe, author: john_doe, project: issue.project) - expect_any_instance_of(User).not_to receive(:update_todos_count_cache) + expect(Users::UpdateTodoCountCacheService).not_to receive(:new) - service.destroy_target(issue) { } + service.destroy_target(issue) { issue.destroy! } end it 'yields the target to the caller' do @@ -1007,7 +1009,8 @@ RSpec.describe TodoService do end describe '#update_note' do - let(:noteable) { create(:issue, project: project) } + let_it_be(:noteable) { create(:issue, project: project) } + let(:note) { create(:note, project: project, note: mentions, noteable: noteable) } let(:addressed_note) { create(:note, project: project, note: "#{directly_addressed}", noteable: noteable) } @@ -1044,12 +1047,34 @@ RSpec.describe TodoService do should_not_create_todo(user: skipped, target: noteable, action: Todo::DIRECTLY_ADDRESSED) end - it 'does not create a todo if user was already mentioned and todo is pending' do - stub_feature_flags(multiple_todos: false) + context 'users already have pending todos and the multiple_todos feature is off' do + before do + stub_feature_flags(multiple_todos: false) + end + + let_it_be(:pending_todo_for_member) { create(:todo, :mentioned, user: member, project: project, target: noteable) } + let_it_be(:pending_todo_for_guest) { create(:todo, :mentioned, user: guest, project: project, target: noteable) } + let_it_be(:pending_todo_for_admin) { create(:todo, :mentioned, user: admin, project: project, target: noteable) } + let_it_be(:note_mentioning_1_user) do + create(:note, project: project, note: "FYI #{member.to_reference}", noteable: noteable) + end - create(:todo, :mentioned, user: member, project: project, target: noteable, author: author) + let_it_be(:note_mentioning_3_users) do + create(:note, project: project, note: 'FYI: ' + [member, guest, admin].map(&:to_reference).join(' '), noteable: noteable) + end + + it 'does not create a todo if user was already mentioned and todo is pending' do + expect { service.update_note(note_mentioning_1_user, author, skip_users) }.not_to change(member.todos, :count) + end - expect { service.update_note(note, author, skip_users) }.not_to change(member.todos, :count) + it 'does not create N+1 queries for pending todos' do + # Excluding queries for user permissions because those do execute N+1 queries + allow_any_instance_of(User).to receive(:can?).and_return(true) + + control_count = ActiveRecord::QueryRecorder.new { service.update_note(note_mentioning_1_user, author, skip_users) }.count + + expect { service.update_note(note_mentioning_3_users, author, skip_users) }.not_to exceed_query_limit(control_count) + end end it 'does not create a todo if user was already mentioned and todo is done' do @@ -1076,13 +1101,9 @@ RSpec.describe TodoService do it 'updates cached counts when a todo is created' do issue = create(:issue, project: project, assignees: [john_doe], author: author) - expect(john_doe.todos_pending_count).to eq(0) - expect(john_doe).to receive(:update_todos_count_cache).and_call_original + expect_next(Users::UpdateTodoCountCacheService, [john_doe]).to receive(:execute) service.new_issue(issue, author) - - expect(Todo.where(user_id: john_doe.id, state: :pending).count).to eq 1 - expect(john_doe.todos_pending_count).to eq(1) end shared_examples 'updating todos state' do |state, new_state, new_resolved_by = nil| diff --git a/spec/services/todos/destroy/destroyed_issuable_service_spec.rb b/spec/services/todos/destroy/destroyed_issuable_service_spec.rb new file mode 100644 index 00000000000..24f74bae7c8 --- /dev/null +++ b/spec/services/todos/destroy/destroyed_issuable_service_spec.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Todos::Destroy::DestroyedIssuableService do + describe '#execute' do + let_it_be(:target) { create(:merge_request) } + let_it_be(:pending_todo) { create(:todo, :pending, project: target.project, target: target, user: create(:user)) } + let_it_be(:done_todo) { create(:todo, :done, project: target.project, target: target, user: create(:user)) } + + def execute + described_class.new(target.id, target.class.name).execute + end + + it 'deletes todos for specified target ID and type' do + control_count = ActiveRecord::QueryRecorder.new { execute }.count + + # Create more todos for the target + create(:todo, :pending, project: target.project, target: target, user: create(:user)) + create(:todo, :pending, project: target.project, target: target, user: create(:user)) + create(:todo, :done, project: target.project, target: target, user: create(:user)) + create(:todo, :done, project: target.project, target: target, user: create(:user)) + + expect { execute }.not_to exceed_query_limit(control_count) + expect(target.reload.todos.count).to eq(0) + end + + it 'invalidates todos cache counts of todo users', :use_clean_rails_redis_caching do + expect { execute } + .to change { pending_todo.user.todos_pending_count }.from(1).to(0) + .and change { done_todo.user.todos_done_count }.from(1).to(0) + end + end +end diff --git a/spec/services/todos/destroy/entity_leave_service_spec.rb b/spec/services/todos/destroy/entity_leave_service_spec.rb index 4126eb88b0b..03fa2482bbf 100644 --- a/spec/services/todos/destroy/entity_leave_service_spec.rb +++ b/spec/services/todos/destroy/entity_leave_service_spec.rb @@ -224,6 +224,8 @@ RSpec.describe Todos::Destroy::EntityLeaveService do end context 'with nested groups' do + let(:parent_group) { create(:group, :public) } + let(:parent_subgroup) { create(:group)} let(:subgroup) { create(:group, :private, parent: group) } let(:subgroup2) { create(:group, :private, parent: group) } let(:subproject) { create(:project, group: subgroup) } @@ -235,12 +237,17 @@ RSpec.describe Todos::Destroy::EntityLeaveService do let!(:todo_subgroup2_user) { create(:todo, user: user, group: subgroup2) } let!(:todo_subproject_user2) { create(:todo, user: user2, project: subproject) } let!(:todo_subpgroup_user2) { create(:todo, user: user2, group: subgroup) } + let!(:todo_parent_group_user) { create(:todo, user: user, group: parent_group) } + + before do + group.update!(parent: parent_group) + end context 'when the user is not a member of any groups/projects' do it 'removes todos for the user including subprojects todos' do - expect { subject }.to change { Todo.count }.from(12).to(4) + expect { subject }.to change { Todo.count }.from(13).to(5) - expect(user.todos).to be_empty + expect(user.todos).to eq([todo_parent_group_user]) expect(user2.todos) .to match_array( [todo_issue_c_user2, todo_group_user2, todo_subproject_user2, todo_subpgroup_user2] @@ -250,8 +257,6 @@ RSpec.describe Todos::Destroy::EntityLeaveService do context 'when the user is member of a parent group' do before do - parent_group = create(:group) - group.update!(parent: parent_group) parent_group.add_developer(user) end @@ -264,9 +269,12 @@ RSpec.describe Todos::Destroy::EntityLeaveService do end it 'does not remove group and subproject todos' do - expect { subject }.to change { Todo.count }.from(12).to(7) + expect { subject }.to change { Todo.count }.from(13).to(8) - expect(user.todos).to match_array([todo_group_user, todo_subgroup_user, todo_subproject_user]) + expect(user.todos) + .to match_array( + [todo_group_user, todo_subgroup_user, todo_subproject_user, todo_parent_group_user] + ) expect(user2.todos) .to match_array( [todo_issue_c_user2, todo_group_user2, todo_subproject_user2, todo_subpgroup_user2] @@ -280,9 +288,12 @@ RSpec.describe Todos::Destroy::EntityLeaveService do end it 'does not remove subproject and group todos' do - expect { subject }.to change { Todo.count }.from(12).to(7) + expect { subject }.to change { Todo.count }.from(13).to(8) - expect(user.todos).to match_array([todo_subgroup_user, todo_group_user, todo_subproject_user]) + expect(user.todos) + .to match_array( + [todo_subgroup_user, todo_group_user, todo_subproject_user, todo_parent_group_user] + ) expect(user2.todos) .to match_array( [todo_issue_c_user2, todo_group_user2, todo_subproject_user2, todo_subpgroup_user2] diff --git a/spec/services/upload_service_spec.rb b/spec/services/upload_service_spec.rb index 89a28e6a098..48aa65451f3 100644 --- a/spec/services/upload_service_spec.rb +++ b/spec/services/upload_service_spec.rb @@ -67,6 +67,29 @@ RSpec.describe UploadService do it { expect(@link_to_file).to eq({}) } end + + describe '#override_max_attachment_size' do + let(:txt) { fixture_file_upload('spec/fixtures/doc_sample.txt', 'text/plain') } + let(:service) { described_class.new(@project, txt, FileUploader) } + + subject { service.execute.to_h } + + before do + allow(txt).to receive(:size) { 100.megabytes.to_i } + end + + it 'allows the upload' do + service.override_max_attachment_size = 101.megabytes + + expect(subject.keys).to eq(%i(alt url markdown)) + end + + it 'disallows the upload' do + service.override_max_attachment_size = 99.megabytes + + expect(subject).to eq({}) + end + end end def upload_file(project, file) diff --git a/spec/services/user_preferences/update_service_spec.rb b/spec/services/user_preferences/update_service_spec.rb new file mode 100644 index 00000000000..59089a4a7af --- /dev/null +++ b/spec/services/user_preferences/update_service_spec.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe UserPreferences::UpdateService do + let(:user) { create(:user) } + let(:params) { { view_diffs_file_by_file: false } } + + describe '#execute' do + subject(:service) { described_class.new(user, params) } + + context 'successfully updating the record' do + it 'updates the preference and returns a success' do + result = service.execute + + expect(result.status).to eq(:success) + expect(result.payload[:preferences].view_diffs_file_by_file).to eq(params[:view_diffs_file_by_file]) + end + end + + context 'unsuccessfully updating the record' do + before do + allow(user.user_preference).to receive(:update).and_return(false) + end + + it 'returns an error' do + result = service.execute + + expect(result.status).to eq(:error) + end + end + end +end diff --git a/spec/services/users/refresh_authorized_projects_service_spec.rb b/spec/services/users/refresh_authorized_projects_service_spec.rb index 1e74ff3d9eb..a8ad0d02f60 100644 --- a/spec/services/users/refresh_authorized_projects_service_spec.rb +++ b/spec/services/users/refresh_authorized_projects_service_spec.rb @@ -163,168 +163,4 @@ RSpec.describe Users::RefreshAuthorizedProjectsService do service.update_authorizations([], [[user.id, project.id, Gitlab::Access::MAINTAINER]]) end end - - describe '#fresh_access_levels_per_project' do - let(:hash) { service.fresh_access_levels_per_project } - - it 'returns a Hash' do - expect(hash).to be_an_instance_of(Hash) - end - - it 'sets the keys to the project IDs' do - expect(hash.keys).to eq([project.id]) - end - - it 'sets the values to the access levels' do - expect(hash.values).to eq([Gitlab::Access::MAINTAINER]) - end - - context 'personal projects' do - it 'includes the project with the right access level' do - expect(hash[project.id]).to eq(Gitlab::Access::MAINTAINER) - end - end - - context 'projects the user is a member of' do - let!(:other_project) { create(:project) } - - before do - other_project.team.add_reporter(user) - end - - it 'includes the project with the right access level' do - expect(hash[other_project.id]).to eq(Gitlab::Access::REPORTER) - end - end - - context 'projects of groups the user is a member of' do - let(:group) { create(:group) } - let!(:other_project) { create(:project, group: group) } - - before do - group.add_owner(user) - end - - it 'includes the project with the right access level' do - expect(hash[other_project.id]).to eq(Gitlab::Access::OWNER) - end - end - - context 'projects of subgroups of groups the user is a member of' do - let(:group) { create(:group) } - let(:nested_group) { create(:group, parent: group) } - let!(:other_project) { create(:project, group: nested_group) } - - before do - group.add_maintainer(user) - end - - it 'includes the project with the right access level' do - expect(hash[other_project.id]).to eq(Gitlab::Access::MAINTAINER) - end - end - - context 'projects shared with groups the user is a member of' do - let(:group) { create(:group) } - let(:other_project) { create(:project) } - let!(:project_group_link) { create(:project_group_link, project: other_project, group: group, group_access: Gitlab::Access::GUEST) } - - before do - group.add_maintainer(user) - end - - it 'includes the project with the right access level' do - expect(hash[other_project.id]).to eq(Gitlab::Access::GUEST) - end - end - - context 'projects shared with subgroups of groups the user is a member of' do - let(:group) { create(:group) } - let(:nested_group) { create(:group, parent: group) } - let(:other_project) { create(:project) } - let!(:project_group_link) { create(:project_group_link, project: other_project, group: nested_group, group_access: Gitlab::Access::DEVELOPER) } - - before do - group.add_maintainer(user) - end - - it 'includes the project with the right access level' do - expect(hash[other_project.id]).to eq(Gitlab::Access::DEVELOPER) - end - end - end - - describe '#current_authorizations_per_project' do - let(:hash) { service.current_authorizations_per_project } - - it 'returns a Hash' do - expect(hash).to be_an_instance_of(Hash) - end - - it 'sets the keys to the project IDs' do - expect(hash.keys).to eq([project.id]) - end - - it 'sets the values to the project authorization rows' do - expect(hash.values.length).to eq(1) - - value = hash.values[0] - - expect(value.project_id).to eq(project.id) - expect(value.access_level).to eq(Gitlab::Access::MAINTAINER) - end - end - - describe '#current_authorizations' do - context 'without authorizations' do - it 'returns an empty list' do - user.project_authorizations.delete_all - - expect(service.current_authorizations.empty?).to eq(true) - end - end - - context 'with an authorization' do - let(:row) { service.current_authorizations.take } - - it 'returns the currently authorized projects' do - expect(service.current_authorizations.length).to eq(1) - end - - it 'includes the project ID for every row' do - expect(row.project_id).to eq(project.id) - end - - it 'includes the access level for every row' do - expect(row.access_level).to eq(Gitlab::Access::MAINTAINER) - end - end - end - - describe '#fresh_authorizations' do - it 'returns the new authorized projects' do - expect(service.fresh_authorizations.length).to eq(1) - end - - it 'returns the highest access level' do - project.team.add_guest(user) - - rows = service.fresh_authorizations.to_a - - expect(rows.length).to eq(1) - expect(rows.first.access_level).to eq(Gitlab::Access::MAINTAINER) - end - - context 'every returned row' do - let(:row) { service.fresh_authorizations.take } - - it 'includes the project ID' do - expect(row.project_id).to eq(project.id) - end - - it 'includes the access level' do - expect(row.access_level).to eq(Gitlab::Access::MAINTAINER) - end - end - end end diff --git a/spec/services/users/update_todo_count_cache_service_spec.rb b/spec/services/users/update_todo_count_cache_service_spec.rb new file mode 100644 index 00000000000..3e3618b1291 --- /dev/null +++ b/spec/services/users/update_todo_count_cache_service_spec.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Users::UpdateTodoCountCacheService do + describe '#execute' do + let_it_be(:user1) { create(:user) } + let_it_be(:user2) { create(:user) } + + let_it_be(:todo1) { create(:todo, user: user1, state: :done) } + let_it_be(:todo2) { create(:todo, user: user1, state: :done) } + let_it_be(:todo3) { create(:todo, user: user1, state: :pending) } + let_it_be(:todo4) { create(:todo, user: user2, state: :done) } + let_it_be(:todo5) { create(:todo, user: user2, state: :pending) } + let_it_be(:todo6) { create(:todo, user: user2, state: :pending) } + + it 'updates the todos_counts for users', :use_clean_rails_memory_store_caching do + Rails.cache.write(['users', user1.id, 'todos_done_count'], 0) + Rails.cache.write(['users', user1.id, 'todos_pending_count'], 0) + Rails.cache.write(['users', user2.id, 'todos_done_count'], 0) + Rails.cache.write(['users', user2.id, 'todos_pending_count'], 0) + + expect { described_class.new([user1, user2]).execute } + .to change(user1, :todos_done_count).from(0).to(2) + .and change(user1, :todos_pending_count).from(0).to(1) + .and change(user2, :todos_done_count).from(0).to(1) + .and change(user2, :todos_pending_count).from(0).to(2) + + Todo.delete_all + + expect { described_class.new([user1, user2]).execute } + .to change(user1, :todos_done_count).from(2).to(0) + .and change(user1, :todos_pending_count).from(1).to(0) + .and change(user2, :todos_done_count).from(1).to(0) + .and change(user2, :todos_pending_count).from(2).to(0) + end + + it 'avoids N+1 queries' do + control_count = ActiveRecord::QueryRecorder.new { described_class.new([user1]).execute }.count + + expect { described_class.new([user1, user2]).execute }.not_to exceed_query_limit(control_count) + end + + it 'executes one query per batch of users' do + stub_const("#{described_class}::QUERY_BATCH_SIZE", 1) + + expect(ActiveRecord::QueryRecorder.new { described_class.new([user1]).execute }.count).to eq(1) + expect(ActiveRecord::QueryRecorder.new { described_class.new([user1, user2]).execute }.count).to eq(2) + end + + it 'sets the cache expire time to the users count_cache_validity_period' do + allow(user1).to receive(:count_cache_validity_period).and_return(1.minute) + allow(user2).to receive(:count_cache_validity_period).and_return(1.hour) + + expect(Rails.cache).to receive(:write).with(['users', user1.id, anything], anything, expires_in: 1.minute).twice + expect(Rails.cache).to receive(:write).with(['users', user2.id, anything], anything, expires_in: 1.hour).twice + + described_class.new([user1, user2]).execute + end + end +end |